libva-multiplanar/diff_against_ffmpeg.md

# Phase 4 input — `src/h264.c` diff vs FFmpeg + hantro kernel-source ground truth

Reference sources:
- **FFmpeg** (proven working on hantro): `references/ffmpeg-kwiboo/libavcodec/v4l2_request_h264.c` (Kwiboo's downstream `v4l2-request-n8.1`, head `b57fbbe`)
- **Linux mainline kernel** (the actual hardware-register-write code): `references/linux-mainline/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c`

Acquired 2026-05-04. Both gitignored from the campaign repo (separate-repo discipline; pull recipes in `references/README` if/when added).

## The smoking gun — DECODE_PARAMS bit-size fields are MMIO-bound on hantro G1

`hantro_g1_h264_dec.c::set_params()` writes these dec_param fields **directly into hardware registers**:

| dec_param field | Register | Our value | FFmpeg's source |
|---|---|---|---|
| `dec_ref_pic_marking_bit_size` | `G1_REG_DEC_CTRL5_REFPIC_MK_LEN` | **0** (uninitialized) | `sl->ref_pic_marking_bit_size` (bit-parsed from slice header) |
| `idr_pic_id` | `G1_REG_DEC_CTRL5_IDR_PIC_ID` | **0** (uninitialized) | `sl->idr_pic_id` (bit-parsed from slice header) |
| `pic_order_cnt_bit_size` | `G1_REG_DEC_CTRL6_POC_LENGTH` | **0** (uninitialized) | `sl->pic_order_cnt_bit_size` (bit-parsed from slice header) |

When all three are zero, hantro's hardware bitstream parser thinks the slice header has zero ref_pic_marking bits and zero POC syntax bits, advances the bitstream pointer by zero past `slice_type`, lands on garbage, decodes zero pixels into the CAPTURE buffer.

**Patch 0008's open question** — "does hantro tolerate the bit_size fields being zero, or do we need a slice_header() bit-level parser?" — is **empirically answered: NO, hantro does not tolerate zeros**. The patch left this unresolved; the kernel source resolves it definitively (registers read, hardware enforces). The "contract before code" discipline was violated in the predecessor work — the contract (these fields drive hardware MMIO writes) was not actually verified against kernel source, and "Empirical question" was left as an unaudited TODO.

This explains the all-zero CAPTURE output we see across mpv and Firefox.

## All hantro G1 dec_param reads

From `hantro_g1_h264_dec.c::set_params()`:

| Field | Our coverage | Status |
|---|---|---|
| `dec_param->nal_ref_idc` | ✓ set from slice byte (line 378) | ok |
| `dec_param->frame_num` | ✓ set from `VAPicture->frame_num` (line 379) | ok |
| `dec_param->top_field_order_cnt` | ✓ set, with patch-0015 POC sentinel strip | ok (verify strip works) |
| `dec_param->bottom_field_order_cnt` | ✓ set, with patch-0015 POC sentinel strip | ok (verify strip works) |
| `dec_param->dec_ref_pic_marking_bit_size` | ✗ **zero — written to MMIO REFPIC_MK_LEN** | **FIX REQUIRED** |
| `dec_param->idr_pic_id` | ✗ **zero — written to MMIO IDR_PIC_ID** | **FIX REQUIRED** |
| `dec_param->pic_order_cnt_bit_size` | ✗ **zero — written to MMIO POC_LENGTH** | **FIX REQUIRED** |
| `dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC` | ✓ set from VAAPI field_pic_flag | ok |
| `dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD` | ✓ set from VAAPI flags | ok |
| `dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC` | ✓ set when nal_unit_type == 5 | ok |
| `dec_param->dpb[].reference_ts` | populated by patch 0017 | verify against FFmpeg's `ff_v4l2_request_get_capture_timestamp` semantics |
| `dec_param->dpb[].frame_num` | populated by patch 0017 | ok |

## All hantro G1 sps reads

| sps field | Our coverage | Status |
|---|---|---|
| `sps->profile_idc` | ✓ set from VAProfile via `h264_profile_to_idc()` | verify h264_profile_to_idc returns reasonable values |
| `sps->max_num_ref_frames` | ✓ set from `VAPicture->num_ref_frames` | ok |
| `sps->chroma_format_idc` | ✓ set from `VAPicture->seq_fields` | ok |
| `sps->log2_max_frame_num_minus4` | ✓ set from `VAPicture->seq_fields` | ok |
| `sps->flags` (bits: `MB_ADAPTIVE_FRAME_FIELD`, `FRAME_MBS_ONLY`, `DIRECT_8X8_INFERENCE`, etc.) | ✓ set from VAAPI seq_fields | ok |

## All hantro G1 pps reads

| pps field | Our coverage | Status |
|---|---|---|
| `pps->chroma_qp_index_offset` | ✓ | ok |
| `pps->second_chroma_qp_index_offset` | ✓ | ok |
| `pps->pic_init_qp_minus26` | ✓ | ok |
| `pps->weighted_bipred_idc` | ✓ | ok |
| `pps->pic_parameter_set_id` | ✗ **zero (uninitialized)** — written to MMIO `G1_REG_DEC_CTRL6_PPS_ID` | For BBB single-PPS streams, ID=0 is accidentally correct; **FIX (set explicitly to 0 or bit-parse from slice header) for general correctness** |
| `pps->num_ref_idx_l0_default_active_minus1` | ✗ **zero (uninitialized)** — written to MMIO `G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(... + 1)` so kernel computes `0 + 1 = 1` | **FIX REQUIRED** — VAAPI `VAPictureParameterBufferH264.num_ref_idx_l0_default_active_minus1` exists, just copy |
| `pps->num_ref_idx_l1_default_active_minus1` | ✗ **zero (uninitialized)** | **FIX REQUIRED** — same as above |
| `pps->flags & V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT` | ✗ Patch 0012 sets only when `matrix_set` is true; **FFmpeg always sets it** (with a "FFmpeg always provide a scaling matrix" comment) | **FIX REQUIRED** — set unconditionally, and unconditionally send SCALING_MATRIX in the request (with default flat 16/16 matrix when no VAIQMatrixBuffer arrives) |
| `pps->flags` other bits | ✓ set from VAAPI pic_fields | ok |

## All FFmpeg per-request controls vs ours

FFmpeg's `v4l2_request_h264_queue_decode` for FRAME_BASED mode sends 4 controls:
1. `V4L2_CID_STATELESS_H264_SPS`
2. `V4L2_CID_STATELESS_H264_PPS`
3. `V4L2_CID_STATELESS_H264_SCALING_MATRIX` ← **always**
4. `V4L2_CID_STATELESS_H264_DECODE_PARAMS`

Our `src/h264.c` for FRAME_BASED sends 3 controls (when `matrix_set == false`):
1. SPS
2. PPS
3. DECODE_PARAMS
(SCALING_MATRIX omitted by patch 0012)

This matters because hantro reads `pps->flags & V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT` (line 64 of `hantro_g1_h264_dec.c`) and sets `G1_REG_DEC_CTRL2_TYPE1_QUANT_E` based on it. If we *don't* set the flag and *don't* send the matrix, hantro's `TYPE1_QUANT_E` register bit stays clear and the hardware skips the per-matrix scaling. That's probably *correct* behavior actually — but for safety + parity with FFmpeg, set it always.

The bigger ambiguity: does hantro's hardware tolerate `SCALING_MATRIX_PRESENT=0` + missing matrix control, OR does it expect the matrix to always be present? FFmpeg's "always provide a scaling matrix" comment suggests the latter is the safer assumption.

## Fix priority

**TIER 1 — easy, highest leverage, do first**:

A. **Set `dec_param->idr_pic_id`, `dec_ref_pic_marking_bit_size`, `pic_order_cnt_bit_size` correctly.** Requires bit-parsing the slice header. ~50–100 lines of careful C in a new helper, e.g. `h264_slice_header_extract_bitsizes()`. Inputs: slice_data buffer, `sps_id`, sps fields (`log2_max_frame_num_minus4`, `pic_order_cnt_type`, `log2_max_pic_order_cnt_lsb_minus4`), `pps_id`, pps fields (`bottom_field_pic_order_in_frame_present_flag`), `nal_unit_type`, `nal_ref_idc`. Outputs: `idr_pic_id`, `pic_order_cnt_lsb`, `delta_pic_order_cnt_bottom`, `delta_pic_order_cnt[0..1]`, `pic_order_cnt_bit_size`, `dec_ref_pic_marking_bit_size`. **This is the load-bearing fix.**

B. **Copy `pps->num_ref_idx_l0_default_active_minus1` and `num_ref_idx_l1_default_active_minus1` from VAAPI.** 2-line fix. Cheap insurance.

**TIER 2 — easy, parity-with-FFmpeg, safe**:

C. **Set `pps->flags |= V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT` always.** And unconditionally send `V4L2_CID_STATELESS_H264_SCALING_MATRIX` in the request, with a default flat matrix (all 16/16/...) when no VAIQMatrixBuffer arrived from the consumer.

D. **Verify patch 0015's POC sentinel strip is actually applied** to the values that reach VIDIOC_S_EXT_CTRLS. Add a post-strip dump or VIDIOC_G_EXT_CTRLS readback — patch 0014's DEBUG dump runs *before* the strip in the code path, so we never confirmed the strip works.

**TIER 3 — diagnostic plumbing (can run in parallel with the above)**:

E. **Fix patch 0011's cache coherency bug** (msync MS_INVALIDATE or DMA-BUF cache sync ioctl) so future probes are reliable.

F. **Add a VIDIOC_G_EXT_CTRLS readback** right before `MEDIA_REQUEST_IOC_QUEUE` to dump everything actually being submitted. Cross-validates fixes A-D land at the V4L2 layer.

## Order of attack

1. **Tier 2C first** (1-line fix to patch 0012 + small change to control-set composition) — cheapest possible test that disambiguates "scaling matrix is the only thing missing" from "we have multiple bugs."
2. **Tier 1B** (2-line copy of num_ref_idx fields).
3. Rebuild, reinstall on ohm, real-VO test. Check whether output is still all-zero or starts producing something.
4. **Tier 1A** (the slice header bit parser) — the load-bearing fix. Requires care; cross-test against ffmpeg's `H264SliceContext::ref_pic_marking_bit_size` etc. computations.
5. Rebuild, real-VO test. Bunny should appear.
6. **Tier 3E + 3F** as observability infrastructure for future probes.

## Why the predecessor work missed this

The Step 1 patch series was authored with cross-references to FFmpeg's `v4l2_request_h264.c` per the `STUDY.md`, but the **kernel source-side audit was not performed**. Patch 0008's commit message says "Empirical question: does hantro tolerate the bit_size fields being zero?" — the empirical answer was supposedly to be obtained "by running this patch and inspecting the CAPTURE buffer" (per `picture.c` comment). That inspection was performed via patch 0011's sentinel test, which had a cache-coherency bug (`phase0_evidence/2026-05-04-kernel-trace/findings.md`), so the empirical answer never landed. Plus "kernel source side audit" wasn't a step in the loop at that point.

The lesson: when the kernel code reads V4L2 control fields directly into MMIO registers, those fields are **load-bearing** at the contract level. Their values aren't merely "informational" — the hardware parses bitstream based on them. Setting them to zero isn't "passing through unspecified data," it's "lying to the hardware about how many bits to skip."

Adding to memory under `feedback_kernel_source_audit_for_uapi_contract.md` for the broader Phase 6 "contract before code" discipline.