fresnel-fourier/phase5_iter5b_review.md

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:

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:

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):

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:

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.