iter5b Phase 5: review — CRIT-1 mechanical pseudocode fix, 3 IMP amendments

Sonnet-architect found one Critical pseudocode error and three
Important amendments. All mechanical; no structural plan change.

CRIT-1: Phase 4 C2 pseudocode used non-existent
`struct object_heap_iterator`. Actual API at object_heap.h:67-68 uses
`int *iterator`. Author re-verified vs request.c:411-418 canonical
usage. Verbatim paste would have compile-failed.

IMP-1: gate comment at surface.c:178-195 should mention codec/profile
change alongside resolution change.

IMP-2: dead `object_config->pixelformat` field at config.h:46 — accept
option (a): wire up at CreateConfig, return directly from heap walk.
Saves one pixelformat_for_profile() call in surface.c path.

IMP-3: characterize hantro mechanism precisely — substitution to
default MPEG2_DECODER codec_mode, not rejection. Explains why MPEG-2
worked but VP8 didn't pre-fix.

10 contract clauses scorecard: 1 FAIL (C2), 2 CONDITIONAL (C3, C10),
7 PASS. Phase 6 cleared conditionally pending all 4 amendments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

phase5_iter5b_review.md

@@ -0,0 +1,213 @@
+# Iteration 5b — Phase 5 (review)
+
+Phase 5 architect review of `phase4_iter5b_plan.md` by sonnet-architect, 2026-05-12. Empirical-over-theoretical and trace-fix-mechanism-to-consumer disciplines applied. Source files read: `surface.c` (135–380), `config.c` (1–194), `context.c` (49–227), `object_heap.h`, `object_heap.c` (150–200), `request.h` (50–107), `request.c` (354–424), `config.h`, `meson.build`. Remote source read of `hantro_v4l2.c` on boltzmann's linux-rockchip v7.0 tree.
+
+**Overall plan health**: Option α' choice over option β is defensible. 10 contract clauses are individually well-scoped. One Critical pseudocode error (compile failure if pasted verbatim). Three Important amendments improve accuracy without changing structure. Seven Non-Amendment findings confirm the plan is otherwise correct.
+
+## Critical findings (BLOCKING Phase 6)
+
+### CRIT-1 — Iterator type mismatch: pseudocode uses non-existent `struct object_heap_iterator`
+
+**Plan's C2 pseudocode** declares:
+
+```c
+struct object_heap_iterator iter;
+struct object_base *iter_obj;
+iter_obj = object_heap_first(&driver_data->config_heap, &iter);
+```
+
+**Actual API** at `object_heap.h:67–68`:
+
+```c
+struct object_base *object_heap_first(struct object_heap *heap, int *iterator);
+struct object_base *object_heap_next(struct object_heap *heap, int *iterator);
+```
+
+The iterator parameter is `int *`, not a struct pointer. `struct object_heap_iterator` does not exist anywhere in the codebase. The C2 pseudocode will fail to compile.
+
+**Reference usage** at `request.c:411–418` (the canonical pattern):
+
+```c
+int iterator;
+config_object = (struct object_config *)
+    object_heap_first(&driver_data->config_heap, &iterator);
+while (config_object != NULL) {
+    ...
+    config_object = (struct object_config *)
+        object_heap_next(&driver_data->config_heap, &iterator);
+}
+```
+
+**Required amendment to C2**:
+
+```c
+static VAProfile find_sole_active_profile(struct request_data *driver_data)
+{
+    VAProfile found = VAProfileNone;
+    int seen = 0;
+    int iter;
+    struct object_config *config_object;
+
+    config_object = (struct object_config *)
+        object_heap_first(&driver_data->config_heap, &iter);
+    while (config_object != NULL) {
+        if (seen == 0)
+            found = config_object->profile;
+        seen++;
+        config_object = (struct object_config *)
+            object_heap_next(&driver_data->config_heap, &iter);
+    }
+
+    if (seen == 1)
+        return found;
+
+    if (seen > 1)
+        request_log("CreateSurfaces2: %d active configs, profile-pick ambiguous; falling back to default OUTPUT format\n",
+                    seen);
+    return VAProfileNone;
+}
+```
+
+**Verdict**: Mechanical fix, but it would have been a Phase 6 compile failure that wasted a `meson compile` cycle. Phase 5 caught it. Phase 6 implementer pastes the corrected version above.
+
+## Important findings (incorporate into plan before Phase 6)
+
+### IMP-1 — Gate comment should mention codec/profile change alongside resolution change
+
+The existing comment at `surface.c:178–195` explains the teardown branch in terms of resolution change ("If we've previously allocated buffers at a different resolution"). C5 extends the gate predicate to fire on pixelformat change too, but the comment is now incomplete.
+
+**Resolution**: Phase 6 amends the comment to say "If we've previously allocated buffers at a different resolution or codec, tear them down on BOTH queues before re-setting the OUTPUT format." Two-line change, mechanical.
+
+### IMP-2 — Dead `object_config->pixelformat` field at config.h:46 creates a maintenance trap
+
+`config.h:46` declares `unsigned int pixelformat;` in `struct object_config`. `grep -n "pixelformat" config.c` returns no assignment hits. The field has been dead scaffolding since some prior iteration.
+
+The plan's C5 adds `last_output_pixelformat` to `struct request_data` instead. That's defensible — driver_data tracks "current V4L2 device state"; config tracks "what config wants." Distinct semantics.
+
+**Recommended amendment**: Either (a) wire up the dead field at CreateConfig (`config_object->pixelformat = pixelformat_for_profile(profile)`) — costs zero LOC if done alongside C1, makes the field meaningful, and the heap walk can use `config_object->pixelformat` directly instead of calling `pixelformat_for_profile()` again; OR (b) add `/* DEAD — see request_data::last_output_pixelformat */` comment at config.h:46.
+
+Option (a) is the cleaner fix. Recommended.
+
+### IMP-3 — MPEG-2/VP8 hantro mechanism: substitution, not rejection
+
+The plan's commit-message language ("kernel silently rejected the format/control mismatch") is imprecise. Empirical read of `hantro_v4l2.c` (boltzmann's v7.0 tree, `drivers/media/platform/verisilicon/hantro_v4l2.c::hantro_try_fmt`):
+
+- `S_FMT(OUTPUT, H264_SLICE)` on hantro → `hantro_find_format` returns NULL (H264_SLICE not in `rk3399_vpu_dec_fmts`) → `hantro_try_fmt` substitutes the default = `MPEG2_SLICE` → `ctx->vpu_src_fmt->codec_mode = MPEG2_DECODER`.
+- For MPEG-2 fixture: bitstream is MPEG-2 + controls dispatched to MPEG2 ops → decode succeeds (modulo timing).
+- For VP8 fixture: bitstream is VP8 but `codec_mode = MPEG2_DECODER` → VP8 controls dispatched to MPEG2 ops → undefined behavior → all-zero output.
+
+This is **more precise** than "format mismatch rejected" — the kernel doesn't reject, it silently substitutes a default codec_mode and dispatches into the wrong codec ops path. The fix (correct OUTPUT format per profile) makes hantro pick the right codec_mode.
+
+**Recommended amendment**: update commit message body for the Phase 6 commit to use the precise framing. Update `phase0_iter5_loopback.md` taxonomy note in passing.
+
+## Non-amendment findings (plan is correct)
+
+### NC-1 — Helper completeness (concern 1): PASS
+
+`pixelformat_for_profile()` in C1 covers every profile enumerated at `config.c:131–194`:
+
+| Profile (config.c enumerated) | C1 mapping |
+|---|---|
+| VAProfileMPEG2{Simple,Main} → MPEG2_SLICE | ✓ |
+| VAProfileH264{Main,High,ConstrainedBaseline,MultiviewHigh,StereoHigh} → H264_SLICE | ✓ |
+| VAProfileHEVCMain → HEVC_SLICE | ✓ |
+| VAProfileVP8Version0_3 → VP8_FRAME | ✓ |
+| VAProfileVP9Profile0 → VP9_FRAME | ✓ |
+
+`default: return 0` correctly signals unknown.
+
+### NC-2 — Heap iterator API (concern 2): SEE CRIT-1
+
+API exists; type is wrong in pseudocode. Already covered above.
+
+### NC-3 — `request_log` symbol available in surface.c: PASS
+
+`utils.h` already included at `surface.c:46`. No new include needed.
+
+### NC-4 — `VAProfileNone = -1` fallback: PASS
+
+`va.h:495` defines it. `pixelformat_for_profile(VAProfileNone)` hits `default: return 0`. Caller's `if (pixelformat == 0)` branch correctly fires. Fallback to H264_SLICE preserves pre-iter5b behavior for the pathological case.
+
+### NC-5 — `meson.build` update (C6): PASS
+
+`meson.build:30–53` (sources) and `:55–78` (headers) both omit codec.c / codec.h. C6 correctly identifies both list edits.
+
+### NC-6 — Option α' vs β choice (concern 7): DEFENSIBLE
+
+The surface.c:164–171 TODO comment specifically targets "consumers that legitimately stream multiple resolutions in sequence" — multi-resolution mid-stream, not iter5b's initial-CreateSurfaces case. Option α' addresses iter5b's bug without invoking the deferred-redesign flag.
+
+### NC-7 — mpv probe-then-real flow (concern 5): SAFE
+
+Pre-CreateConfig probe surfaces (128×128) hit VAProfileNone → fallback H264_SLICE → probe CAPTURE allocated. CreateConfig(HEVC) → real CreateSurfaces2 → find_sole_active_profile returns VAProfileHEVCMain → gate fires on both width/height AND pixelformat change → teardown destroys probe buffers → S_FMT(HEVC_SLICE) → correct allocation. Empirically: same teardown path as the mpv resolution-change case the existing code already exercises (surface.c:196–223).
+
+### NC-8 — Multi-config fallback safety (concern 8): SAFE
+
+Gate fires on pixelformat change at the eventual CreateContext → re-S_FMT corrects the format before the consumer actually decodes. The brief window between probe and real CreateSurfaces uses H264_SLICE buffers that are torn down before any decode. No silent corruption path.
+
+### NC-9 — Phase 7 hash stability (concern 10): SOUND
+
+Phase 3 baseline verified `kdirect == sw` byte-identity. The hwdownload pipeline (`ffmpeg -f rawvideo -pix_fmt yuv420p`) is deterministic for stateless decode. Re-running the sweep on unchanged kernel + fixtures produces identical hashes. Target hashes `9340b832…` / `4f1565e8…` / `136ce5cb…` stable.
+
+### NC-10 — Memory rule placement (concern 6): RECOMMENDATION
+
+Best path: extend `feedback_unconditional_codec_state.md` with the pixel format instance as a second case (start codes + pixel formats are the same bug class), AND add a new `feedback_va_lifecycle_profile_threading.md` for the specific VA-API-CreateSurfaces-has-no-config_id observation. The lessons are sibling but distinct.
+
+## Summary scorecard
+
+| Clause | Status | Finding |
+|---|---|---|
+| C1 (codec.h/codec.c helper) | PASS | Complete profile coverage |
+| C2 (find_sole_active_profile) | **FAIL** | CRIT-1: iterator type wrong; fix per amendment above |
+| C3 (use helper at surface.c:173) | CONDITIONAL | Depends on C2 fix |
+| C4 (#include "codec.h") | PASS | utils.h already provides request_log |
+| C5 (driver_data field + gate extension) | PASS | Gate semantics correct; comment needs IMP-1 amendment |
+| C6 (meson.build) | PASS | Both sources + headers lists need edit |
+| C7 (Phase 7 verification matrix) | PASS | Hashes stable; methodology sound |
+| C8 (build + install) | PASS | Backend-only; commands correct |
+| C9 (control-payload anchor regression) | PASS | Optional but cheap |
+| C10 (Phase 8 close) | CONDITIONAL | Depends on C2 fix; memory rule update per NC-10 |
+
+**Phase 6 cleared, conditional on incorporating CRIT-1 fix + IMP-1 comment update + IMP-2 dead-field resolution + IMP-3 commit-message precision.** All amendments are mechanical; no structural plan changes.
+
+## Author re-verification (post-review, 2026-05-12)
+
+Per `feedback_review_empirical_over_theoretical.md` Direction 2:
+
+### CRIT-1 — CONFIRMED
+
+`object_heap.h:67-68` empirically shows iterator parameter is `int *`. `request.c:411-418` empirically shows the canonical usage pattern. My Phase 4 pseudocode invented `struct object_heap_iterator` — classic theoretical-code-without-empirical-grounding error. Phase 5 caught what Phase 4 would have wasted a `meson compile` cycle on.
+
+### IMP-1 — CONFIRMED
+
+Comment update is trivially correct.
+
+### IMP-2 — CONFIRMED + ACCEPTING OPTION (a)
+
+`grep -n "pixelformat" config.c` empirically returns no hits. The dead field needs either wiring up or commenting. Option (a) is cleaner — wire up at CreateConfig. Phase 6 incorporates: `config_object->pixelformat = pixelformat_for_profile(profile)` at CreateConfig, and `find_sole_active_profile` returns `config_object->pixelformat` directly (saves one `pixelformat_for_profile()` call in the surface.c path).
+
+### IMP-3 — CONFIRMED
+
+The reviewer's mechanism description (hantro substitutes default = MPEG2_SLICE, dispatches to MPEG2 codec_mode ops) is empirically correct per `hantro_v4l2.c::hantro_find_format` + `hantro_try_fmt` + `rk3399_vpu_dec_fmts` ordering. Phase 6 commit message uses the precise framing.
+
+## Phase 5 review track-record acknowledgment
+
+| ID | Finding | Author verification | Status |
+|---|---|---|---|
+| CRIT-1 | `struct object_heap_iterator` doesn't exist; type is `int` | Empirical: object_heap.h:67-68 + request.c:411-418 confirm | **CORRECT, BLOCKING** |
+| IMP-1 | Gate comment should mention codec/profile change | Trivial; correct | **CORRECT** |
+| IMP-2 | Dead `object_config->pixelformat` field at config.h:46 | Empirical grep confirms no assignment; accept option (a) | **CORRECT** |
+| IMP-3 | Hantro substitutes (doesn't reject); dispatches wrong codec_mode | Reviewer cited hantro_v4l2.c flow; consistent with my Phase 0 loopback observation that MPEG-2 worked but VP8 didn't | **CORRECT** |
+| NC-1..NC-10 | Plan correct on the 10 non-amendment fronts | Spot-checked; all confirmed | **CORRECT** |
+
+All findings empirically correct. The reviewer caught a compile-failing pseudocode error before Phase 6 burned a build cycle on it. Estimated savings: 1 failed `meson compile` cycle (~30 seconds) + 1 mental-context-switch correction. Cheaper than iter5 Phase 5's CRIT-1 (which would have wasted a kernel build), but the same shape of catch.
+
+## Phase 4 amendments to land before Phase 6
+
+1. **C2 pseudocode**: replace iterator-type-and-loop with the corrected snippet above. Use `int iter;` and cast-on-return pattern from request.c:411-418.
+2. **C5 comment**: amend the surface.c:178-195 explanatory block to say "If we've previously allocated buffers at a different resolution **or codec**, tear them down…"
+3. **C1 wiring**: at `RequestCreateConfig` (config.c), add `config_object->pixelformat = pixelformat_for_profile(profile);` after the existing config_object allocation. This wires up the dead field at config.h:46.
+4. **C2 simplification**: `find_sole_active_profile` returns `config_object->pixelformat` directly (FOURCC), not the VAProfile. Rename to `find_sole_active_pixelformat` for accuracy. Surface.c caller skips the second `pixelformat_for_profile()` call.
+5. **Phase 6 commit message** (when the actual commit lands): use IMP-3's precise hantro-substitution framing instead of "format/control mismatch rejected."
+6. **Memory rules**: extend `feedback_unconditional_codec_state.md` + add `feedback_va_lifecycle_profile_threading.md`.
+
+LOC delta estimate unchanged at ~50 LOC. Amendments 1–4 simplify rather than expand.