Files
fresnel-fourier/phase5b_iter8_review.md
marfrit 64b370d863 iter8 Phase 5b review: CRIT-1 kills α-1 (rkvdec ignores constraint_set_flags)
Sonnet-architect Phase 5b read rkvdec-h264.c end-to-end and confirmed:
constraint_set_flags is NEVER accessed by the driver. assemble_hw_pps()
reads only chroma_format_idc, bit_depth_*, log2_max_frame_num_minus4,
max_num_ref_frames, pic_order_cnt_type, log2_max_pic_order_cnt_lsb_minus4,
and dimension fields. rkvdec_h264_validate_sps() doesn't validate it.

CONSTRAINT_SET3_FLAG and PROFILE_IDC in the hardware PPS packet are
hardcoded constants (1 and 0xFF respectively), not propagated from the
incoming SPS.

α-1 will not unblock Bug 4. Plan-killer.

CRIT-2: ConstrainedBaseline 0x42 mapping is wrong (bit 6 reserved);
correct value 0x12 (bit 1 | bit 4) per H.264 §A.2.1.1.

IMP-1 redirects: DPB entry flags + POC fields are the next candidate.
rkvdec config_registers() reads dpb[i].flags ACTIVE/FIELD bits and
dpb[i].fields TOP/BOT bits. lookup_ref_buf_idx() substitutes destination
buffer as reference when ACTIVE missing — silent corruption matching
observed symptom.

IMP-2/3: full PPS byte comparison + close-criteria framing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-13 12:33:40 +00:00

12 KiB
Raw Permalink Blame History

Iteration 8 — Phase 5b Review (α-1 SPS constraint_set_flags fix)

Reviewed 2026-05-13. Reviewer: claude-sonnet-4-6.

Plan under review: /home/mfritsche/src/fresnel-fourier/phase4b_iter8_plan.md Empirical anchor: /home/mfritsche/src/fresnel-fourier/phase7_iter8_verification.md

Sources read (empirical, not theoretical):

  • rkvdec-h264.c at /home/mfritsche/src/linux-rfc/drivers/staging/media/rkvdec/rkvdec-h264.c
  • v4l2_request_h264.c at /home/mfritsche/src/libva-multiplanar/references/ffmpeg-kwiboo/libavcodec/v4l2_request_h264.c
  • h264.c (libva backend) at /home/mfritsche/src/libva-multiplanar/libva-v4l2-request-fourier/src/h264.c
  • v4l2.c (libva backend) at /home/mfritsche/src/libva-multiplanar/libva-v4l2-request-fourier/src/v4l2.c
  • /usr/include/linux/v4l2-controls.h — V4L2 UAPI struct v4l2_ctrl_h264_sps

CRIT findings

CRIT-1: rkvdec does NOT read constraint_set_flags — the hypothesis is empirically falsified

The plan's core claim is that constraint_set_flags may trigger a "decoder-config table dispatch" or "profile-detection hint" inside rkvdec. The driver source falsifies this.

In assemble_hw_pps() (rkvdec-h264.c:633), the function translates the entire v4l2_ctrl_h264_sps struct into the hardware PPS packet. Crucially:

  • sps->constraint_set_flags is never read anywhere in rkvdec-h264.c. Zero references. Confirmed by grep -n "constraint_set" /home/mfritsche/src/linux-rfc/drivers/staging/media/rkvdec/rkvdec-h264.c returning no output.
  • The hardware PPS packet defines CONSTRAINT_SET3_FLAG at bit offset 12 (line 45), but that slot is hardcoded to 1 unconditionally (line 660: WRITE_PPS(1, CONSTRAINT_SET3_FLAG)). The incoming sps->constraint_set_flags does not feed it.
  • PROFILE_IDC in the hardware packet is also hardcoded to 0xFF (line 659). The incoming sps->profile_idc does not feed it.

The kernel's rkvdec_h264_validate_sps() (line 10291065) is the only SPS validation that can abort decoding. It checks:

  • chroma_format_idc <= 1
  • bit_depth_luma_minus8 == bit_depth_chroma_minus8
  • bit_depth_luma_minus8 == 0
  • Frame dimensions within coded_fmt

constraint_set_flags is not checked. The field is fully ignored by the rkvdec driver — it does not branch on it, does not write it to hardware, and does not validate it.

Verdict: The α-1 fix will compile and land cleanly, but cannot unblock decoding. constraint_set_flags = 0x02 versus 0x00 makes zero difference to rkvdec's hardware dispatch or validation path. R-1 in the plan ("constraint_set_flags hypothesis is wrong — probability: low-to-medium") should be rated near-certain based on source evidence.

This is a plan-killer in the strict sense: α-1 will not fix Bug 4. It is low-risk (no regression hazard), but it will consume a build/install/test cycle without advancing the criterion-1 hash match.

Recommendation: Either (a) implement α-1 as a correctness hygiene patch knowing it will not fix Bug 4 and move immediately to the next hypothesis, or (b) skip α-1 and triage the next candidate directly. If α-1 is implemented, the Phase 7b γ dump will show the same 16×32 partial fill, providing one more confirmed-excluded hypothesis, which has diagnostic value.


CRIT-2: The ConstrainedBaseline mapping 0x42 is wrong — bit 6 does not exist

The plan's mapping table assigns VAProfileH264ConstrainedBaseline → 0x42. The plan's own SPS bit table lists:

  • bit 0: constraint_set0_flag
  • bit 1: constraint_set1_flag
  • bit 2: constraint_set2_flag
  • bit 3: constraint_set3_flag
  • bit 4: constraint_set4_flag
  • bit 5: constraint_set5_flag
  • bits 6-7: reserved

0x42 = 0b01000010 = bit 1 + bit 6. But bit 6 is a reserved bit — there is no constraint_set6_flag. The v4l2_ctrl_h264_sps UAPI struct confirms bits 6-7 are unspecified/reserved.

Per H.264 §A.2.1.1 (Constrained Baseline profile, Amendment 2 and later), Constrained Baseline is signalled as profile_idc=66, constraint_set1_flag=1, constraint_set4_flag=1. That yields bit 1 | bit 4 = 0x02 | 0x10 = 0x12, not 0x42.

The value 0x40 that the plan mentions as an alternative ("or 0x40") is bit 6 only — also reserved, also wrong.

Since CRIT-1 already establishes that rkvdec ignores constraint_set_flags entirely, a wrong value in the mapping table causes no runtime damage. However, if α-1 is landed as a hygiene patch, the ConstrainedBaseline mapping is incorrect and would produce a non-conformant SPS for any ConstrainedBaseline stream sent to a future driver that does read this field.

Amendment required: Change VAProfileH264ConstrainedBaseline → 0x12 (bit 1 + bit 4).


IMP findings

IMP-1: DECODE_PARAMS DPB flags diff is the strongest remaining candidate — plan under-invests in it

Phase 3 established a second diff beyond constraint_set_flags:

libva  frame-2 DPB flags: 0x00000003 (VALID | ACTIVE)
kdirect frame-2 DPB flags: 0x00030001 (contains bit 16+17 beyond the defined flags)

The plan classifies this as "alternative α-2" and defers it. But after CRIT-1 confirms constraint_set_flags is inert, the DPB flags diff is now the leading candidate. The plan should be amended to make α-2 the primary Phase 6b/7b investigation, not a fallback after α-1 fails.

The rkvdec driver reads dpb[i].flags in config_registers() at lines 965-973:

if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
    refer_addr |= RKVDEC_COLMV_USED_FLAG_REF;
if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)
    refer_addr |= RKVDEC_FIELD_REF;

And dpb[i].fields at lines 970-973:

if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF)
    refer_addr |= RKVDEC_TOPFIELD_USED_REF;
if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
    refer_addr |= RKVDEC_BOTFIELD_USED_REF;

The driver also reads DPB flags in lookup_ref_buf_idx() at line 747: if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE). If ACTIVE is not set correctly, run->ref_buf[i] remains NULL, and the driver substitutes the destination buffer as reference — producing corrupted motion compensation without emitting FLAG_ERROR.

The Phase 3 diff for frame 2 (first P-frame) showed libva DPB flags=0x3 while kdirect had flags=0x00030001. The undefined upper bits in kdirect's value are suspicious but may be a strace read artifact (the reference_ts u64 and flags u32 are adjacent in the struct; a misaligned read could merge them). Before building on this finding, it needs re-examination. The Phase 3 note already flagged this uncertainty (line 107: "may be a kernel-aligned padding artifact").

Required for Phase 6b plan: Trace the libva backend's fill_dpb_entry analog — in h264.c, the DPB entry construction — and compare it field-by-field against the ffmpeg fill_dpb_entry (v4l2_request_h264.c:71). Specifically: does libva correctly set V4L2_H264_DPB_ENTRY_FLAG_ACTIVE for short-ref frames? Does it set top_field_order_cnt / bottom_field_order_cnt?

IMP-2: The plan does not address PPS diff

Phase 3 noted PPS first bytes were identical. But Phase 3 only compared the first 12 bytes. The PPS struct has 28+ fields; a diff in num_ref_idx_l0_default_active_minus1 or pic_init_qp_minus26 or CABAC flag would also cause silent decode errors. This was acknowledged as "PPS identical" but with a shallow check.

Before the Phase 6b plan finalises its α-2 candidate, a full PPS byte comparison between libva and kdirect should be on the checklist. This is a strace re-run (no code change needed), not a code audit. Low effort, eliminates a variable.

IMP-3: The success/partial-close criteria need tightening

The plan states: "If α-1 doesn't fix → iter8 = PARTIAL — Bug 4 narrowed; iter9 candidate."

With CRIT-1 established before any build, we already know α-1 will not fix Bug 4. The partial-close framing should be revised:

  • iter8 is not partial because α-1 failed — iter8 is partial because Bug 4's root cause is not yet identified. The empirical narrowing achieved (kernel-side partial decode, constraint_set_flags inert, rkvdec validate_sps not triggering) is meaningful progress and should be committed to the memory log regardless of whether α-1 is implemented.
  • The Phase 8 close should enumerate what is definitively excluded: (a) libva readback bug — excluded by γ. (b) slot binding / CAPTURE buffer lifecycle — excluded by IMP-1 memset. (c) constraint_set_flags hypothesis — excluded by source read. (d) SPS validate_sps rejection — excluded by source read (none of the checked fields differ between libva and kdirect).

Naming what's excluded is more useful than noting α-1 failed, because it constrains iter9's search space.


MIN findings

MIN-1: The plan conflates "informational per spec" with "not inspected by drivers"

The plan argues that because H.264 §7.4.2.1.1 marks constraint_set_flags as "informational," the kernel may still use it. This reasoning is correct in principle — drivers often rely on informational SPS fields for hardware quirk routing. However, the plan then asserts that "kernel decoders have been observed to use them as profile-detection hints" without citing a specific driver. This is generic background, not evidence. Having now confirmed rkvdec does not use it, the language in the plan should not be reused as a template for future hypotheses without grounding.

MIN-2: default: fallthrough returns 0x00 for unrecognised profiles

The helper in the plan code:

default:
    return 0x00;

This is correct fallback behaviour — unknown profiles get no constraint bits. No change needed; note it for hygiene.

MIN-3: The h264_derive_level_idc parallel in the plan is valid motivation

The plan correctly observes that level_idc was previously derived from frame dimensions because VAAPI doesn't forward it, making constraint_set_flags derivation the same class of problem. This framing is accurate and worth keeping as code documentation if α-1 lands as a hygiene patch.


Amendments to Phase 6b

  1. Drop α-1 as the primary fix hypothesis. The kernel driver does not read constraint_set_flags. α-1 may be landed as a conformance/hygiene one-liner if desired, but Phase 6b should not treat it as the iter8 criterion-1 candidate.

  2. Fix the ConstrainedBaseline mapping before landing α-1. Change the value from 0x42 to 0x12 (constraint_set1_flag | constraint_set4_flag). Value: correct conformance for any future driver that reads this field.

  3. Elevate α-2 (DECODE_PARAMS / DPB diff) to primary Phase 6b candidate. The next empirical step is a full field-by-field comparison of the libva DPB construction against the ffmpeg DPB construction for frame 2 (first P-frame). Specifically inspect:

    • top_field_order_cnt, bottom_field_order_cnt (libva may be zero-ing these for progressive frames where ffmpeg sends real values).
    • flags field — does libva set VALID|ACTIVE correctly for short-ref frames?
    • fields field — does libva set V4L2_H264_TOP_FIELD_REF for progressive frame references?
  4. Add a PPS full-byte comparison to the Phase 7b checklist (strace re-run, no build required) to exclude PPS as a diff source.

  5. Revise the iter8 close criteria. Phase 8 should log the definitively excluded hypotheses regardless of whether a criterion-1 PASS is achieved.


Summary table

ID Severity Finding Impact
CRIT-1 CRIT rkvdec ignores constraint_set_flags entirely — driver source confirmed α-1 will not fix Bug 4
CRIT-2 CRIT ConstrainedBaseline mapping 0x42 is wrong (bit 6 = reserved); correct value is 0x12 Wrong SPS if α-1 landed as-is
IMP-1 IMP DPB flags diff is now primary candidate; Phase 6b plan should triage it before building Redirection required
IMP-2 IMP PPS was only shallowly compared in Phase 3 Add full-byte PPS check to Phase 7b
IMP-3 IMP Partial-close framing incomplete; excluded hypotheses not enumerated Phase 8 close quality
MIN-1 MIN "Drivers have been observed" claim lacks citation Language quality
MIN-2 MIN default: return 0x00 is correct; no change needed Hygiene confirmation
MIN-3 MIN level_idc derivation parallel is valid motivation for code comment Code documentation