fresnel-fourier/phase4_iter5b_plan_v2.md

# Iteration 5b — Phase 4 v2 (plan, option β)

Captured 2026-05-12 after Phase 7 → Phase 4 loopback on the failed α' attempt. Re-plan with **option β: defer all OUTPUT-side V4L2 device state from `RequestCreateSurfaces2` to `RequestCreateContext`** — the lifecycle redesign the `surface.c:164-171` TODO comment anticipated and the Phase 5 reviewer-of-Phase-2 originally recommended.

## Why β, this time

Phase 7 empirically demonstrated α' fails because ffmpeg-vaapi calls `vaCreateSurfaces2` multiple times per session, with `vaCreateContext + STREAMON + frame decode` interleaved. α''s pixfmt-change-triggers-teardown gate corrupts state when called mid-stream. β eliminates the problem at the architectural level:

- **`CreateSurfaces2` no longer touches V4L2 device state.** No S_FMT, no CREATE_BUFS, no cap_pool. It allocates surface_object IDs and stores width/height. That's it.
- **`CreateContext` owns the full lifecycle.** It looks up `config_object->profile` (already does at context.c:71), derives the right pixelformat via `pixelformat_for_profile()`, does `S_FMT(OUTPUT, …)`, queries CAPTURE format, runs `cap_pool_init`, walks the bound `surface_ids[]` to fill per-surface `destination_*`. Then proceeds to existing `request_pool_init` + STREAMON + device-wide controls.
- **`DestroyContext` already handles teardown correctly.** STREAMOFF both queues, REQBUFS(0), cap_pool_destroy at context.c:276-307. No change needed.

The α' gate's destructive failure mode disappears because there's no gate — each `CreateContext` is a fresh setup, and the corresponding `DestroyContext` cleans up. mpv probe-then-real pattern: probe context creates with H264_SLICE (or whatever the probe profile), then DestroyContext, then real CreateContext with the real profile — STREAMOFF + REQBUFS(0) happens in DestroyContext between them.

## Reusable from failed iter5b

Of the three reverted commits, two are reusable without modification for β:

| iter5b commit (reverted) | Reusable for β? | Notes |
|---|---|---|
| `ce304ef` Commit A — `codec.h/codec.c` helper | **YES, identical** | `pixelformat_for_profile()` is profile→FOURCC map. Used at CreateConfig (commit B). |
| `f8256e6` Commit B — `last_output_pixelformat` field + `object_config->pixelformat` wire-up | **PARTIAL** | `object_config->pixelformat` wire-up at CreateConfig is reusable; the `last_output_pixelformat` field in driver_data is unnecessary in β (gate semantics move to per-CreateContext). |
| `4b2288f` Commit C — `find_sole_active_pixelformat` + surface.c gate change | **NO** | Wrong approach. β has no heap walk (CreateContext has config_id directly) and no surface.c gate (lifecycle is CreateContext-centric). |

## Contract clauses

### C1 — `pixelformat_for_profile()` helper (re-introduce from reverted commit A)

**New files**: `src/codec.h`, `src/codec.c` (identical to reverted commit A).

Helper:
```c
unsigned int pixelformat_for_profile(VAProfile profile);
```

Maps `VAProfile*` → `V4L2_PIX_FMT_*_SLICE/FRAME`. Returns 0 for unhandled.

Register in `meson.build`: add `'codec.c'` to sources, `'codec.h'` to headers.

### C2 — Wire `object_config->pixelformat` at CreateConfig (re-introduce from reverted commit B)

**Edit**: `src/config.c::RequestCreateConfig` at line ~98.

```diff
+ #include "codec.h"
...
  config_object->profile = profile;
  config_object->entrypoint = entrypoint;
+ config_object->pixelformat = pixelformat_for_profile(profile);
```

Populates the previously-dead `object_config->pixelformat` field (config.h:46) with the right FOURCC for each profile. CreateContext reads this field.

### C3 — Strip OUTPUT-side lifecycle from `RequestCreateSurfaces2`

**Edit**: `src/surface.c::RequestCreateSurfaces2`.

Remove:
- Lines 173-232 (OUTPUT format S_FMT + teardown branch + last_output_*).
- Lines 237-274 (`driver_data->video_format` probe — moves to CreateContext).
- Lines 276-280 (G_FMT CAPTURE — moves to CreateContext).
- Lines 282-326 (destination_* computation — moves to CreateContext).
- Lines 305-313 (cap_pool_init — moves to CreateContext).
- Inner per-surface destination_* fill at lines 340-359 (deferred to CreateContext walk).

Keep:
- Line 135-153: function entry, declarations.
- Line 234-235: `VA_RT_FORMAT_YUV420` validation.
- Lines 328-377: per-surface object_heap_allocate + minimal field init. Specifically: `width`, `height`, `status = VASurfaceReady`, `current_slot = NULL`, etc. Skip the destination_* fill (do it at CreateContext).

Result: `RequestCreateSurfaces2` becomes ~60 LOC (was ~250 LOC). Pure ID allocation + per-surface bookkeeping.

### C4 — Move OUTPUT-side lifecycle into `RequestCreateContext`

**Edit**: `src/context.c::RequestCreateContext`.

Current flow (post-revert):

```c
config_object = CONFIG(driver_data, config_id);          // line 71
...
request_pool_init(&driver_data->output_pool, ...);       // line 103
...
v4l2_set_controls(... H264_DECODE_MODE/START_CODE ...);  // line 142+
v4l2_set_controls(... HEVC_DECODE_MODE/START_CODE ...);  // line 164+
switch (config_object->profile) {                        // line 205
   case VAProfileH264*/HEVCMain: h264_start_code = true;
   default: h264_start_code = false;
}
v4l2_set_stream(... output_type, true);                  // line 219 STREAMON
v4l2_set_stream(... capture_type, true);                 // line 225 STREAMON
```

New flow:

```c
config_object = CONFIG(driver_data, config_id);

// β: derive OUTPUT pixelformat from the bound config's profile.
unsigned int pixelformat = config_object->pixelformat;
if (pixelformat == 0) {
    // Defensive: shouldn't happen because CreateConfig rejects unhandled profiles,
    // but Phase 5 review insists on never deref-ing 0 from helper return.
    status = VA_STATUS_ERROR_UNSUPPORTED_PROFILE;
    goto error;
}

unsigned int output_type_pre = v4l2_type_video_output(true);  // assume mplane; CAPTURE probe sets true/false below
rc = v4l2_set_format(driver_data->video_fd, output_type_pre, pixelformat,
                     picture_width, picture_height);
if (rc < 0) { status = VA_STATUS_ERROR_OPERATION_FAILED; goto error; }

// β: probe CAPTURE format (moved from surface.c:237-274).
if (driver_data->video_format == NULL) {
    /* SUNXI_TILED_NV12 / NV12 probe identical to surface.c logic */
    ...
    driver_data->video_format = ...;
}
video_format = driver_data->video_format;
output_type = v4l2_type_video_output(video_format->v4l2_mplane);
capture_type = v4l2_type_video_capture(video_format->v4l2_mplane);

// β: G_FMT CAPTURE to get sizes (moved from surface.c:276-280).
rc = v4l2_get_format(driver_data->video_fd, capture_type,
                     &format_width, &format_height,
                     destination_bytesperlines, destination_sizes, NULL);
if (rc < 0) { ... }

// β: cap_pool_init (moved from surface.c:305-313).
if (!driver_data->capture_pool.initialized) {
    unsigned int pool_count = surfaces_count > MIN_CAP_POOL ?
                              surfaces_count : MIN_CAP_POOL;
    rc = cap_pool_init(&driver_data->capture_pool,
                       driver_data->video_fd, capture_type,
                       pool_count, video_format->v4l2_buffers_count);
    if (rc < 0) { ... }
}

// β: walk surface_ids and fill destination_* (moved from surface.c).
unsigned int destination_planes_count = video_format->planes_count;
if (video_format->v4l2_buffers_count == 1) {
    destination_sizes[0] = destination_bytesperlines[0] * format_height;
    for (j = 1; j < destination_planes_count; j++)
        destination_sizes[j] = destination_sizes[0] / 2;
}
for (i = 0; i < surfaces_count; i++) {
    struct object_surface *surface_object = SURFACE(driver_data, surfaces_ids[i]);
    if (surface_object == NULL) { ... }
    surface_object->destination_planes_count = destination_planes_count;
    surface_object->destination_buffers_count = video_format->v4l2_buffers_count;
    /* Fill destination_offsets[], destination_sizes[], destination_bytesperlines[]
       per video_format->v4l2_buffers_count branch (1 vs n). Same logic as surface.c
       lines 340-359. */
    ...
}

// existing flow continues:
request_pool_init(...);
v4l2_set_controls(... H264_DECODE_MODE/START_CODE ...);
v4l2_set_controls(... HEVC_DECODE_MODE/START_CODE ...);
switch (config_object->profile) { ... h264_start_code ... }
v4l2_set_stream(... output_type, true);
v4l2_set_stream(... capture_type, true);
```

Predicted LOC: ~120 LOC added to context.c (mostly transplanted from surface.c).

### C5 — Resolution-change handling moves to DestroyContext

The existing DestroyContext at context.c:276-307 already does:
- STREAMOFF both queues
- REQBUFS(0) on OUTPUT
- cap_pool_destroy

No change needed — that path is already a complete teardown. mpv's probe-then-real pattern works because each context cycle does its own setup + teardown.

### C6 — `surface.h` per-surface fields stay the same

The `object_surface` struct still has `destination_planes_count`, `destination_buffers_count`, `destination_sizes`, `destination_offsets`, `destination_bytesperlines`, `destination_data`, etc. These are now filled at CreateContext time instead of CreateSurfaces2 time, but the field layout doesn't change. No header edit needed.

### C7 — No `last_output_*` fields needed in driver_data

β doesn't need the `last_output_width/height/pixelformat` gate because the lifecycle is CreateContext-centric. The existing `last_output_width/height` fields in driver_data become dead.

**Two sub-options** for what to do with the dead fields:

a) Leave them in `request_data` with a `/* DEAD post-iter5b-β; was used by pre-β CreateSurfaces2 gate */` comment.
b) Delete them entirely.

Recommend **(b) delete**: cleaner, the dev-process discipline favors removing dead code. Also delete `surface_reset_format_cache` at surface.c:75-79 which assigns to these dead fields.

### C8 — Phase 7 verification matrix

Re-run iter5 Phase 3 sweep. Expected hash matrix:

| Codec | Pre-β libva | Expected post-β libva | kdirect (anchor) |
|---|---|---|---|
| H.264 | `71ac099b…` keyframe partial | unchanged (Bug 4 deferred) | `1e7a0bc9…` |
| HEVC | `06b2c5a0…` all-zero | **`9340b832…`** | `9340b832…` |
| VP9 | `06b2c5a0…` all-zero | **`4f1565e8…`** | `4f1565e8…` |
| MPEG-2 | `19eefbf4…` worked | `19eefbf4…` (unchanged) | `19eefbf4…` |
| VP8 | `06b2c5a0…` all-zero | **`136ce5cb…`** | `136ce5cb…` |

PASS criteria same as Phase 4 v1: 3 codecs unblocked (HEVC + VP9 + VP8) + MPEG-2 unchanged + H.264 unchanged.

Plus an extra robustness check: **re-run the sweep within the same backend process** (or back-to-back ffmpeg invocations) to confirm no SIGSEGV. The α' failure mode was multi-CreateSurfaces2 mid-stream; β should be robust because each session is a self-contained CreateContext lifecycle.

### C9 — Phase 8 close criteria

- C8 verification 3/3 (HEVC/VP9/VP8) libva == kdirect.
- No regression on H.264 keyframe + MPEG-2.
- `phase8_iteration5b_close.md` documenting commits + verification.
- Memory updates:
  - Extend `feedback_unconditional_codec_state.md` with the pixelformat instance.
  - Add `feedback_va_lifecycle_profile_threading.md` describing the VA-API CreateSurfaces-has-no-config_id observation and why CreateContext-centric lifecycle (β) is the right architecture for v4l2-request style backends.
  - Add (or extend) a rule about **trust the Phase 2 reviewer's architectural recommendation when the smaller alternative carries lifecycle ordering risk** — the iter5b α' failure cost half a day's wallclock that β would have avoided.
- Campaign scoreboard: "5/5 with 4 direct + 1 (H.264 inter) partial" — same goal as iter5b-α' tried to reach.

## Risk register

| Risk | Probability | Impact | Mitigation |
|---|---|---|---|
| `video_format` probe at CreateContext fails because CAPTURE format isn't queryable until OUTPUT format is set first | LOW | MEDIUM | The S_FMT(OUTPUT) call happens BEFORE the CAPTURE probe in C4's flow; hantro's CAPTURE-derivation happens at OUTPUT S_FMT time. Order is correct. |
| Stripping CreateSurfaces2's logic accidentally drops state that consumers expect | LOW | MEDIUM | Per VA-API spec, surfaces between CreateSurfaces and CreateContext are inert. Consumers should not query `destination_*` before CreateContext binds them. Phase 5 reviewer verifies. |
| `cap_pool_init` at CreateContext when surfaces_count==0 (ffmpeg vaapi-copy might pass 0) | LOW | LOW | Existing code at surface.c:306 already handles this via `surfaces_count > MIN_CAP_POOL ?` clamp. Transplanted as-is. |
| Multiple CreateContext calls in the same session (e.g., ffmpeg-vaapi re-init with different config) | MEDIUM | LOW | First CreateContext: clean setup. Subsequent CreateContext: skips `video_format` and `cap_pool_init` if already initialized (existing `if (!driver_data->video_format)` and `if (!driver_data->capture_pool.initialized)` checks transplant). For multi-context resolution OR profile change, mpv/ffmpeg patterns call DestroyContext between contexts, which already STREAMOFF+REQBUFS(0)+cap_pool_destroy. Phase 7 verifies. |
| Resolution-change within a single context (no DestroyContext) | LOW | LOW | Not in scope for iter5b-β; consumers that do this would need a separate iteration (the TODO comment's "for the next iteration after this one" target). |
| Failing CreateContext leaves V4L2 state half-set (S_FMT done but cap_pool_init failed) | LOW | MEDIUM | Error paths goto `error:` which frees the context. Need to also REQBUFS(0) + STREAMOFF if partial state was created. Phase 5 reviewer verifies the error-recovery branch. |
| `picture_width`/`picture_height` from VA-API differ from surface width/height | LOW | LOW | VA-API spec: surfaces and context picture dimensions should match for hardware decode. If they differ, S_FMT uses picture_*, which is the right kernel-facing dimension. |

## Phase 5 review concerns to invite

When Phase 4 v2 lands and Phase 5 reviewer engages, attack:

1. **Error-recovery branches in CreateContext** — if S_FMT succeeds but cap_pool_init fails, do we correctly REQBUFS(0) and STREAMOFF? Walk the `goto error:` flow.
2. **Multi-context with shared driver_data** — `video_format` and `cap_pool` are driver_data-level (shared across contexts). First CreateContext sets them up; subsequent CreateContexts skip the init. Is the skip-logic correct? What if the second context has a different resolution?
3. **Surface ID lookup at CreateContext** — what if a `surfaces_ids[i]` doesn't resolve via `SURFACE()` macro (already-destroyed)? Existing code path handles NULL via error return. Phase 5 verifies.
4. **picture_width vs surface->width/height** — `surface_object` stores width/height from CreateSurfaces2 (the surface's own dimensions). CreateContext uses picture_width/picture_height (the context's picture dimensions). What if they differ? Use picture_* for S_FMT (kernel state); fill surface_object->{width,height} with picture_*.
5. **`destination_data` fill ordering** — currently filled at surface_bind_slot (BeginPicture time, with the cap_pool_slot's actual mmap pointer). β's CreateContext walks should fill ONLY format-uniform fields (sizes, offsets, bytesperlines, planes_count, buffers_count); destination_data stays NULL until BeginPicture binds a slot. Phase 5 verifies the fields are correctly partitioned: per-format-uniform (CreateContext) vs per-slot (BeginPicture).
6. **Backwards compat** — the libva-multiplanar (ohm) campaign uses the same fork. ohm tests H.264 + MPEG-2 only. Both worked pre-iter5b (with the H264_SLICE hardcode happenstance-correct for H.264, and hantro's tolerance for MPEG-2). Post-β: H.264 path on ohm hantro gets explicit H264_SLICE; MPEG-2 path gets explicit MPEG2_SLICE. Both should still work or improve. Phase 5 verifies via ohm's Phase 3 anchors if available.

## Predicted iter5b-β cadence

- Phase 5 review: ~30 min (medium surface).
- Phase 6 implementation:
  - Re-introduce commit A + commit B (mostly identical to iter5b commits A + B): ~15 min.
  - Commit C: strip surface.c — ~30 min careful editing.
  - Commit D: build out context.c — ~45 min careful editing.
  - Build + install on fresnel — ~10 min.
- Phase 7 verification:
  - Re-run sweep — ~5 min.
  - Multi-codec back-to-back stress test — ~10 min.
- Phase 8 close — ~30 min.

Total: 2-3 hours wallclock.

## Phase 4 v2 → Phase 5 handoff

This document supersedes `phase4_iter5b_plan.md` (the rejected α' plan). Phase 5 reviewer reads this fresh; the prior review (`phase5_iter5b_review.md`) is historical context only.

Most relevant context for the reviewer:
- `phase7_iter5b_verification.md` — the empirical failure mode that mandates β.
- `phase2_iter5b_situation.md` — original lifecycle analysis. Recommended β; this Phase 4 v2 implements that recommendation.
- `phase0_iter5_loopback.md` — root cause of Bug 2 (surface.c:173 hardcode).
- `phase3_iter5_baseline.md` — pre-fix per-codec hash matrix (anchors for Phase 7).

Reviewer should specifically verify the C4 logic transplant from surface.c to context.c — the per-surface destination_* walk has tricky single-buffer vs multi-buffer-per-frame logic that mustn't break.