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 <nuomi2...@gmail.com> wrote: > > > On Sun, Jun 16, 2024 at 11:26 PM Mark Thompson <s...@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.gisq...@gmail.com> wrote: >> >> >> >>> Le ven. 14 juin 2024, 11:39, Frank Plowman <p...@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@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". >> > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
