Thanks for the LGTM.
> When you say it "desynchronises the parser", what downstream issues
> were you observing: mismatches or crashes?
Hard parse failure, neither crashes nor silent mismatches.
The PH parser ends up reading two extra ue(v) syntax elements (or
skipping two it should have read), depending on whether SPS and PH
disagree on chroma MTT depth being zero. By the time
picture_header_structure() reaches its trailing byte_alignment_bit,
the bit position is off, the sanity check
"byte_alignment_bit_equal_to_one out of range: 0, but must be in
[1,1]" trips, and the PH NAL unit returns AVERROR_INVALIDDATA.
cbs_h266 is wired into vvc_parser.c via "#include cbs_h266.h" and
is invoked on every VVC_PH_NUT, so the failure is not limited to bsf
consumers; it surfaces in the regular VVC decode path as
"Failed to parse picture unit", and from there every following slice
fails with "PPS id 0 not available" / "Failed to read packet".
Concrete numbers from the 4136-frame compliant 720p sample attached
to issue 22958:
pre-fix: 4127 packets read, 0 frames decoded, 4127 decode errors;
ffmpeg exits 69 ("Conversion failed!")
post-fix: 4136 packets read, 4136 frames decoded, 0 decode errors;
ffmpeg exits 0.
So the user-visible effect is "an entirely undecodable stream with a
clean error message", not partial corruption. A parser without the
trailing alignment-bit sanity check could in principle desync further
into the slice headers and produce silent garbage, but in our case
that check stops the slide cleanly.
> Do we need to backport this?
I would say yes, to every active release branch.
The buggy line was introduced in dfc62fd1c6da6429bbd0eb3cbb6f3804e8fcb8ae
("avcodec/cbs: add cbs implementation for H266/VVC", 2023-03-21) and
has not been touched since, so 6.x / 7.x / 8.0 / 8.1 release branches
all carry it identically. The trigger is admittedly narrow
(sps_qtbtt_dual_tree_intra_flag = 1 plus a partition_constraints
override that flips chroma MTT depth across zero), but when it does
trigger the stream is wholly undecodable on stock ffmpeg, which for
downstream OTT / broadcast users is effectively a hard "compliant
stream X cannot be decoded" rather than a quality regression.
Happy to send rebased one-liners to the active release branches if
that is the preferred path; just let me know which branches you want
covered.
At 2026-04-29 16:06:28, "Frank Plowman via ffmpeg-devel"
<[email protected]> wrote:
>On 29/04/2026 08:17, Mou Fan via ffmpeg-devel wrote:
>> In the picture header parser, the chroma branch incorrectly tested
>> sps_max_mtt_hierarchy_depth_intra_slice_chroma to decide whether to
>> parse ph_log2_diff_max_{bt,tt}_min_qt_intra_slice_chroma.
>>
>> Per ITU-T H.266 (V4, 01/2026) section 7.3.2.8 "Picture header
>> structure syntax", the condition is on the just-parsed
>> ph_max_mtt_hierarchy_depth_intra_slice_chroma, exactly mirroring the
>> luma branch a few lines above and the inter-slice branch below.
>> sps_partition_constraints_override_enabled_flag allows the picture
>> header to override the SPS values, so testing the SPS field is
>> incorrect and desynchronises the parser whenever the PH override
>> changes the chroma MTT depth from/to zero.
>>
>> Signed-off-by: Mou Fan <[email protected]>
>> ---
>> libavcodec/cbs_h266_syntax_template.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_h266_syntax_template.c
>> b/libavcodec/cbs_h266_syntax_template.c
>> index 98a8954943..fc79ba46c7 100644
>> --- a/libavcodec/cbs_h266_syntax_template.c
>> +++ b/libavcodec/cbs_h266_syntax_template.c
>> @@ -2819,7 +2819,7 @@ static int FUNC(picture_header) (CodedBitstreamContext
>> *ctx, RWContext *rw,
>> 0, FFMIN(6, ctb_log2_size_y) - min_cb_log2_size_y);
>> ue(ph_max_mtt_hierarchy_depth_intra_slice_chroma,
>> 0, 2 * (ctb_log2_size_y - min_cb_log2_size_y));
>> - if (sps->sps_max_mtt_hierarchy_depth_intra_slice_chroma !=
>> 0) {
>> + if (current->ph_max_mtt_hierarchy_depth_intra_slice_chroma
>> != 0) {
>> unsigned int min_qt_log2_size_intra_c =
>>
>> current->ph_log2_diff_min_qt_min_cb_intra_slice_chroma +
>> min_cb_log2_size_y;
>
>LGTM.
>
>When you say it "desynchronises the parser", what downstream issues were
>you observing: mismatches or crashes? Do we need to backport this?
>
>--
>Frank
_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
