From 7d1c44bd90496c474912feface8f028188b47714 Mon Sep 17 00:00:00 2001 From: Markus Fritsche Date: Tue, 12 May 2026 08:04:38 +0000 Subject: [PATCH] =?UTF-8?q?iter5b=20Phase=205:=20review=20=E2=80=94=20CRIT?= =?UTF-8?q?-1=20mechanical=20pseudocode=20fix,=203=20IMP=20amendments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- phase5_iter5b_review.md | 213 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 phase5_iter5b_review.md diff --git a/phase5_iter5b_review.md b/phase5_iter5b_review.md new file mode 100644 index 0000000..f91e8a2 --- /dev/null +++ b/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.