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

[Nuo Mi]

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".
>