On 3/23/2020 2:36 PM, Anton Khirnov wrote: > Quoting James Almer (2020-03-18 17:05:38) >> On 3/13/2020 7:28 AM, Anton Khirnov wrote: >>> It represents the relationship between them more naturally and will be >>> useful in the following commits. >>> >>> Allows significantly more frames in fate-h264-attachment-631 to be >>> decoded. >> >> That sample is odd. It has an SPS/PPS pair in extradata that's wrong for >> the Buffering Period and Picture Timing SEI messages in-band, but >> seemingly correct to actually decode all these frames revealed by this >> patch. >> The SPS that makes the aforementioned SEI messages parseable, but the >> frames seemingly undecodeable, is in-band in the second RAP (Which fwiw >> reports a different bitstream level). So this patch prevents the >> extradata PPS from using the in-band SPS after it replaced the extradata >> one in sps_list. There's no PPS in-band. >> >> Is this really intended? Is the active PPS' sps_id meant to reference >> the SPS with that id that's currently "valid" and in sps_list, or the >> whichever one was valid at the time the PPS was first parsed?. If the >> latter, then why replace the SPS in-band but never the PPS? >> The in-band SPS is virtually unused after this change. > > My guess would be that someone (or some/thing/, dun dun dun) did some > botched/misunderstood processing on that sample, so trying to figure out > what was intended is futile.
I just find it bizarre that the extradata SPS makes the frames decode and the SEI unparseable, but the in-band SPS makes the SEI parseable and the frames undecodeable. > > According to the spec, SPS contents must not change within a sequence > (they can change on sequence boundaries), so this sample is invalid. > From a more practical viewpoint, since a PPS is parsed for a given SPS > I'd say it is clearer, cleaner and safer to tie them together like I'm > doing in this patch. There's commented out code in remove_sps() that used to drop all PPS that depended on a given SPS once a new one with the same id was found. I'm going to guess it was done to keep this mess of a sample working. Since now you're making this behavior official, you could remove that dead code while at it. > >> >>> --- >>> libavcodec/h264_ps.c | 29 +++++- >>> libavcodec/h264_ps.h | 3 + >>> libavcodec/h264_slice.c | 14 +-- >> >> Missing changes to h264_parser.c, and one "h->ps.sps == (const >> SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data" check in h264dec.c > > Good catch, fixed locally. LGTM then. _______________________________________________ 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".
