[FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI

[Josh Allmann]

On Sat, 6 Jul 2024 at 09:37, Michael Niedermayer <michael at niedermayer.cc> wrote:
>
> On Wed, Jul 03, 2024 at 02:05:06PM -0700, Josh Allmann wrote:
> > Encoders may emit a buffering period SEI without a corresponding
> > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
> >
> > During Annex B conversion, this may result in the SPS/PPS being
> > inserted *after* the buffering period SEI but before the IDR NAL.
> >
> > Since the buffering period SEI references the SPS, the SPS/PPS
> > needs to come first.
> > ---
> >  libavcodec/bsf/h264_mp4toannexb.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
>
> breaks fate
>
> TEST    h264-bsf-mp4toannexb
> --- ./tests/ref/fate/h264-bsf-mp4toannexb       2024-07-01 23:30:40.656213791 +0200
> +++ tests/data/fate/h264-bsf-mp4toannexb        2024-07-06 12:13:56.491072296 +0200
> @@ -1 +1 @@
> -5f04c27cc6ee8625fe2405fb0f7da9a3
> +ff2551123909f54c382294baa1bb4364
> Test h264-bsf-mp4toannexb failed. Look at tests/data/fate/h264-bsf-mp4toannexb.err for details.
> make: *** [tests/Makefile:311: fate-h264-bsf-mp4toannexb] Error 1
>

Thanks for the heads up. Took a look into each of the failing fate
tests from [0] and I think these changes are expected and OK.

Each of the failing tests shows the problem that this patch fixes,
which is that the SPS/PPS is written after the buffering period SEI.
An easy way to eyeball the issue is that probing the Annex B output
logs an error which says "non-existing SPS 0 referenced in buffering
period" - in fact this is why I wrote this patch, to get to the bottom
of that message. The NAL ordering can also be inspected with `bsf:v
trace_headers`. There also seems to be a side benefit that makes
segmenting more robust - details below.

Some notes on each failing test:

fate-segment-mp4-to-ts : Before this patch, the segments produced
after 000.ts are not independently decodable, because only the first
segment has any extradata [1]. After the patch, the segments can be
decoded independently. Unless the intent of the test is to actually
produce broken segments, this patch is probably correct in fixing
that. Also see the `fate-h264-bsf-mp4toannexb` testcase.

fate-h264-bsf-mp4toannexb : In the original version, the SPS/PPS is
only written once - maybe because there are no true IDRs after the
first frame, only recovery point SEIs. In the patched version, the
SPS/PPS is written before each buffering period SEI, 6 times in total.
I can see how this might be strictly unnecessary, but probably
harmless from a spec standpoint. Also it seems to make segmented
muxing more robust, since this testcase shares the same input as
`fate-segment-mp4-to-ts` which produces broken segments without the
patch.

fate-h264_mp4toannexb_ticket2991 : This is a clean example of the
problem that this patch solves: without it, a buffering period SEI
comes before the SPS/PPS. Inspecting a diff of the NAL units [2], the
only change is in the reordering of the NALs such that the SPS/PPS
comes before the buffering period SEI, rather than after.

If all that seems OK, then I can send another patch to update the fate
references to match the new values.

[0] https://patchwork.ffmpeg.org/check/104951/

[1] The first segment has extradata, but it is still in the wrong
order without the patch.

[2] https://gist.github.com/j0sh/c912056138822c4d8c9564f4062e1e7b

Josh