Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Frank Plowman <post@frankplowman.com>]

n 15/06/2024 13:24, Nuo Mi wrote:
> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
> christophe.gisquet@gmail.com> wrote:
> 
>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
>> écrit :
>>
>>> When the SPS associated with a particular SPS ID changes, invalidate all
>>> the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
>>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
>>> ID reuse validation, as parts of the CBS parsing process for PPSs
>>> depend on the SPS being referred to.
>>>
>>
>> I am uncertain about this. I have no definite knowledge nor proof, but I
>> would have thought these are persistent, IE it's legal to update some of
>> them, their validity depending on something else.
>>
> 
>> Wondering if the tested streams are thus conformant.
>>
>> But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related to
>> some particular shape of a clean random access point, that would require
>> retransmitting VPS/SPS/PPS/APS/... ?
>>
>> Asking Benjamin Bross might be a better option here.
>>
> Hi Chris,
> spec said sps should not change in a CVS.  Frank has some patches to fix a
> similar issue.
> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
> 
> 
> Hi Frank,
> Did it crash before your error hand code in ps.c?
> Could you send me the clip?
> 
> Thank you
> 

Hi both,

Thank you for your reviews.

An example of a crashing bitstream which is fixed by this patch is ID
295 available here: https://github.com/ffvvc/tests/pull/43.  The
relevant part of the bitstream is a sequence of NAL units

AU (decode_order=5)
18. SPS
    sps_seq_parameter_set_id = 0
    sps_ctb_log2_size_y = 5
19. PPS
    pps_pic_parameter_set_id = 0
    pps_seq_parameter_set_id = 0
20. IDR_N_LP
    ph_pic_order_cnt_lsb = 0
    NoOutputBeforeRecoveryFlag = 1
    ph_pic_parameter_set_id = 0

AU (decode_order=6)
21. AUD
22. VPS
23. SPS
    sps_seq_parameter_set_id = 0
    sps_ctb_log2_size_y = 7
24. PREFIX_APS
25. IDR_N_LP
    ph_pic_order_cnt_lsb = 0
    NoOutputBeforeRecoveryFlag = 1
    ph_pic_parameter_set_id = 0

The layout of SPSs alone is legal (not covered by the checks introduced
in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
AU.  As a result, the bitstream crashes both before and after
2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
produce an error when the VCL NAL unit in the second AU (25.) tries to
use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
(18.) is no longer available.

Christophe, is my interpretation of your point correct when I say you
are suggesting that the above sequence may be legal, so long as the PPS
still satisfies the new bounds etc. derived from the second SPS?  I did
consider this, and I think it may be possible to implement by delaying
CBS element validation and inference until libavcodec/vvc/ps.c.
However, there are no bitstreams in the conformance suite which contain
such a structure and this is different to how the native HEVC decoder
behaves (see libavcodec/hevc/ps.c:72).

All the best,
-- 
Frank
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".