[FFmpeg-devel] [PATCH 14/18] h264_ps: make the PPS hold a reference to its SPS

[Anton Khirnov]

Quoting Andreas Rheinhardt (2020-03-23 23:08:14)
> Anton Khirnov:
> > 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.
> > 
> > According to the spec, SPS contents must not change within a sequence
> > (they can change on sequence boundaries), so this sample is invalid.
> 
> While this sample is invalid, isn't it possible to have a sample as
> follows: First a SPS and a PPS referencing the SPS, then a coded video
> sequence for which these SPS and PPS are active, then another IDR access
> unit with a new SPS that overwrites the old one, but with no new PPS.
> This is valid if I am not mistaken. With your patch, the second SPS
> would never be used.

Hmm, this sounds pretty crazy, but I don't see anything in the spec
forbidding this.

But then again, we currently parse the PPS using values from the SPS. If
the SPS changed, we'd have to re-parse the PPS again using new values.
Since we are not doing that, we do not support this use case anyway.

-- 
Anton Khirnov