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

[Josh Allmann]

On Thu, 1 Aug 2024 at 00:58, Anton Khirnov <anton at khirnov.net> wrote:
>

Thanks for the review.

> Quoting Josh Allmann (2024-07-09 21:05:47)
> > 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.
> > ---
> >
> > Notes:
> >     v2: Updated FATE test refs
> >
> >  libavcodec/bsf/h264_mp4toannexb.c          | 13 +++++++++++++
> >  tests/ref/fate/h264-bsf-mp4toannexb        |  2 +-
> >  tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +++++++++---------
> >  tests/ref/fate/segment-mp4-to-ts           | 12 ++++++------
> >  4 files changed, 29 insertions(+), 16 deletions(-)
> >
> > diff --git a/libavcodec/bsf/h264_mp4toannexb.c b/libavcodec/bsf/h264_mp4toannexb.c
> > index 92af6a6881..6607f1e91a 100644
> > --- a/libavcodec/bsf/h264_mp4toannexb.c
> > +++ b/libavcodec/bsf/h264_mp4toannexb.c
> > @@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
> >              if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] & 0x80))
> >                  new_idr = 1;
> >
> > +            /* If this is a buffering period SEI without a corresponding sps/pps
> > +             * then prepend any existing sps/pps before the SEI */
> > +            if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen && !pps_seen) {
>
> That 0 should be SEI_TYPE_BUFFERING_PERIOD, right?
>

Yes - fixed

> > +                if (s->sps_size) {
> > +                    count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
> > +                    sps_seen = 1;
> > +                }
> > +                if (s->pps_size) {
> > +                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
> > +                    pps_seen = 1;
> > +                }
>
> Is there a reason to insert the PPS? IIUC only the SPS is needed.
>

I believe it would be needed if using this bsf with the segment muxer,
and segmentation happens on a recovery point (with a buffering
period), eg in the test fate-segment-mp4-to-ts . Granted it is kind of
incidental that this patch actually fixes that specific case. I have
never seen a SPS without a PPS though.

Josh