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

[Nuo Mi]

On Sun, Jun 16, 2024 at 12:38 AM Frank Plowman <post at frankplowman.com>
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).
>
Pretty solid analysis, thank you, Frank

>
> All the best,
> --
> Frank
> _______________________________________________
> 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".
>