master
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 claude-noether
|
656596aa6b |
iter3 Phase 5: sonnet review — 4 Critical findings, 4 amendments
Second-model review by sonnet-architect found 4 Critical bugs in
Phase 4 plan, all verified empirically by author before incorporation
per memory feedback_review_empirical_over_theoretical Direction 2.
Amendments applied in-place to phase4_iter3_plan.md +
phase2_iter3_situation.md.
Critical findings:
C1 first_part_header_bits = 0 was claimed cosmetic; actually
UNSAFE. hantro_g1_vp8_dec.c:260 + rockchip_vpu2_hw_vp8_dec.c:372
both read this field unconditionally to compute the macroblock
DMA offset. Setting 0 would place hardware at wrong DMA offset
for ALL macroblock data → garbage decode.
Fix: frame.first_part_header_bits = slice->macroblock_offset
(verified by source identity — vaapi_vp8.c:204 and
v4l2_request_vp8.c:83 use byte-identical formulas).
C2 first_part_size = slice->partition_size[0] was wrong; VAAPI's
partition_size[0] is the REMAINING bytes after parsing
(vaapi_vp8.c:209 confirms; va_dec_vp8.h:193-196 spec confirms).
Kernel needs the TOTAL control partition size.
Fix: frame.first_part_size = slice->partition_size[0] +
((macroblock_offset + 7) / 8)
Phase 3 keyframe numerics confirm: 21923 + 819 = 22742 ✓.
C3 VAProbabilityDataBufferType does not exist as a buffer-type
enum; it's the struct name. The actual enum constant is
VAProbabilityBufferType (= 13 per va.h:2058). Switch case
using the wrong identifier would have failed Phase 6 compile.
Fix: replace globally in phase2 + phase4 docs.
C4 (s8) cast undefined in userspace. Kernel has 's8' typedef in
linux/types.h (kernel-internal). UAPI exposes '__s8' (double-
underscore). Userspace portable cast is int8_t from <stdint.h>.
Fix: replace (s8) with (int8_t) in Clauses 6+7.
Suggested:
S3 Clause 8 comment was factually wrong: hantro_vp8.c::
hantro_vp8_prob_update reads coeff_probs unconditionally;
there is NO default-table fallback. If probability_set==false,
decode produces garbage. Practical risk low (FFmpeg vaapi_vp8.c
always sends VAProbabilityBufferType per frame), but corrected
comment + added assert(probability_set) runtime guard for
immediate Phase 6 surfacing.
Plus 5 minor S/Q items documented; non-blocking for iter3.
Author's 7 review questions all answered directly in the review:
Q1 quantization derivation: correct for typical content
Q2 first_part_header_bits=0 safety: UNSAFE → C1
Q3 num_dct_parts off-by-one: confirmed correct
Q4 field availability: 2 compile failures found (C3 + C4)
Q5 quant_update[s] semantics: signed delta confirmed
Q6 SHOW_FRAME unconditional: safe for BBB scope
Q7 buffer order independence: confirmed
Estimated saving: 1 Phase 6 → Phase 4 loopback + 2 Phase 6 fix-
forward commits. Review pass is the right path forward per memory
rule "Reviews are never skippable" — empty-review value =
empirical-verification value, regardless of finding count.
Refs:
phase4_iter3_plan.md (amended in-place; Phase 5 amendments
section appended)
phase2_iter3_situation.md (amended C3 globally)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
claude-noether
|
898544a29c |
iter3 Phase 2: situation analysis — VP8 backend gaps + contract surface
Source-read of every file the iter3 patch series will touch, plus the
kernel UAPI + VAAPI + downstream FFmpeg + kernel hantro reference
sources. Conducted on noether against fork tip 8d71e20 (iter2 Phase 6
commit B); fresnel.vpn was unreachable so Phase 3 baseline empirical
capture defers until laptop reachable.
Bug enumeration (10 sites the patch series must touch):
B1 config.c::RequestQueryConfigProfiles enumeration block missing
B2 config.c::RequestCreateConfig VP8 case label missing
B3 config.c::RequestQueryConfigEntrypoints VP8 case missing
B4 src/vp8.c new file ~160-220 LOC
B5 src/vp8.h new file ~35-45 LOC
B6 picture.c::codec_set_controls VP8 dispatch missing
B7 picture.c::codec_store_buffer 4 buffer-type cases +
VAProbabilityDataBufferType
outer case missing
B8 picture.c::RequestBeginPicture per-frame reset additions
B9 surface.h::object_surface::params union vp8 member missing
B10 meson.build vp8.c/vp8.h not in lists
Non-bugs (intentionally untouched):
- context.c (no DECODE_MODE/START_CODE menus for VP8)
- video.c (CAPTURE-side format list; VP8 is OUTPUT-side)
- v4l2.c (fourcc-agnostic helpers)
- buffer.c (buffer registry is type-agnostic)
- include/hevc-ctrls.h (already includes <linux/v4l2-controls.h>
which holds V4L2_CID_STATELESS_VP8_FRAME)
Contract surface cited verbatim:
- V4L2_CID_STATELESS_VP8_FRAME = V4L2_CID_CODEC_STATELESS_BASE+200
= 0x00a409c8 (matches Phase 0 V4L2 inventory)
- struct v4l2_ctrl_vp8_frame at <linux/v4l2-controls.h>:1929-1958
+ 5 sub-structs (segment, lf, quant, entropy, coder_state) at
1785-1888
- VAAPI VAPictureParameterBufferVP8 + VASliceParameterBufferVP8 +
VAProbabilityDataBufferVP8 + VAIQMatrixBufferVP8 at
references/libva/va/va_dec_vp8.h
- FFmpeg v4l2_request_vp8.c reference: single batched S_EXT_CTRLS
at end_frame, count=1, no init-time menus
- Kernel hantro_vp8.c::hantro_vp8_prob_update reads 9 fields from
hdr (skip/intra/last/gf probs, segment_probs, entropy.{y,uv,mv,
coeff}_probs)
VAAPI → V4L2 mapping table: 30 fields enumerated. Open questions for
Phase 3 baseline (6 items: first_part_header_bits derivation, num_
dct_parts off-by-one, DPB timestamp 0-sentinel handling, show_frame
default, lf.flags FILTER_TYPE_SIMPLE bit, first-frame DPB sentinel).
Patch-shape prediction: ~260-340 LOC across 6 modified + 2 new
files. Medium-sized iter — between iter1's 120 LOC (3 modified +
1 deleted) and iter2's 470 LOC (5 modified). The new file dominates.
Phase 3 baseline targets queued: cross-validator strace verbatim
S_EXT_CTRLS payload capture, VAAPI consumer trace, mpv-SW reference
JPEG capture for criterion 4 byte-compare anchor.
Phase 4 plan structure anticipated: 10-clause template per iter2.
Refs:
phase0_findings_iter3.md (Phase 1 lock)
phase8_iteration2_close.md (predecessor close)
src/mpeg2.c (iter1 single-codec template; iter3 will mirror shape)
src/h265.c (iter2 dispatcher pattern; iter3 takes structure cues)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|