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

[Nuo Mi]

Hi all,
I will merge this tomorrow if there are no objections.

Thank you.

On Mon, Jun 17, 2024 at 9:23 PM Nuo Mi <nuomi2021 at gmail.com> wrote:

>
>
> On Sun, Jun 16, 2024 at 11:26 PM Mark Thompson <sw at jkqxz.net> wrote:
>
>> 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?
>>
> No, the spec only states that when decoding the picture header, we need
> the corresponding PPS and SPS.
> If we strictly follow the spec, we should delay the PPS-derived process
> when decoding the picture header, but it may be very complex.
>
> 7.4.3.4 Sequence parameter set RBSP semantics
> An SPS RBSP shall be available to the decoding process, by inclusion in at
> least one AU with TemporalId equal to 0 or
> provided through external means, prior to it being referenced by either of
> the following:
> – a PH NAL unit having a ph_pic_parameter_set_id that refers to a PPS with
> pps_seq_parameter_set_id equal to the
> value of sps_seq_parameter_set_id in the SPS RBSP,
> – a coded slice NAL unit having sh_picture_header_in_slice_header_flag
> equal to 1 with a ph_pic_parameter_set_id
> that refers to a PPS with pps_seq_parameter_set_id equal to the value of
> sps_seq_parameter_set_id in the SPS RBSP
>
>>
>> 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
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>