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

[Mark Thompson]

On 15/06/2024 17:37, Frank Plowman wrote:
> n 15/06/2024 13:24, Nuo Mi wrote:
>> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
>> christophe.gisquet at gmail.com> wrote:
>>
>>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post at 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).

Is there some requirement in H.266 that in a single stream the PPS precedes the SPS?

Currently we effectively require that for a single stream because we use the SPS to enforce constraints on the PPS in both H.265 and H.266, but I'm not seeing a hard dependency and it looks like it will currently work on later stream starts as long as the parameters don't change too much.

In H.264 there is an explicit dependency because you need chroma_format_idc to parse scaling lists, but again this will usually work because it's unlikely to change inline.

If that is not required then this will discard the PPS of a stream where the SPS follows the PPS.  (Though I admit that before this it was only dubiously working because the bounds checking might be wrong.)

Thanks,

- Mark