[FFmpeg-devel] libavcodec/hevc_mp4toannexb_bsf: update the extradata in codec par if change detected

From: Linjie Fu <linjie.justin.fu@gmail.com>

Container may support multiple sample descriptions in a single
bitstream, like multiple stsd in mov, which introduces different
sequence header(e.g.profile/bit_depth) in the middle of the bitstream.

Update the extradata field in context parameter once packet with
different extradata is got.

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
 libavcodec/hevc_mp4toannexb_bsf.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Linjie Fu:
> From: Linjie Fu <linjie.justin.fu@gmail.com>
> 
> Container may support multiple sample descriptions in a single
> bitstream, like multiple stsd in mov, which introduces different
> sequence header(e.g.profile/bit_depth) in the middle of the bitstream.
> 
> Update the extradata field in context parameter once packet with
> different extradata is got.
> 
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> index 790dfb0394..36a83d0c95 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -125,6 +125,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>      int got_irap = 0;
>      int i, ret = 0;
>  
> +    uint8_t *extradata;
> +    size_t extradata_size = 0;
> +
>      ret = ff_bsf_get_packet(ctx, &in);
>      if (ret < 0)
>          return ret;
> @@ -135,6 +138,26 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>          return 0;
>      }
>  
> +    extradata = av_packet_get_side_data(in, AV_PKT_DATA_NEW_EXTRADATA, &extradata_size);
> +    if (extradata && extradata_size &&
> +        (ctx->par_in->extradata_size != extradata_size ||
> +         memcmp(ctx->par_in->extradata, extradata, extradata_size))) {
> +        av_log(ctx, AV_LOG_VERBOSE, "Update extradata, size = %zu\n", extradata_size);

We use SIZE_SPECIFIER instead of zu due to MSVC compatibility. I don't
know if we actually support MSVC versions that old any more, but it is
still used.

> +        /* Update extradata */
> +        av_freep(&ctx->par_in->extradata);
> +        ctx->par_in->extradata_size = extradata_size;
> +        ctx->par_in->extradata      = av_mallocz(extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!ctx->par_in->extradata)
> +            return AVERROR(ENOMEM);
> +        memcpy(ctx->par_in->extradata, extradata, extradata_size);
> +        /* Reinit */
> +        ret = hevc_extradata_to_annexb(ctx);
> +        if (ret < 0)
> +            return ret;
> +        s->length_size      = ret;
> +        s->extradata_parsed = 1;
> +    }
> +
>      bytestream2_init(&gb, in->data, in->size);
>  
>      while (bytestream2_get_bytes_left(&gb)) {
> 

1. This is an API violation: par_in is set by the user before
initializing the BSF, par_out is set by the BSF in init. After init,
both are immutable.
2. Instead you should make hevc_extradata_to_annexb() accept a buffer
with length field (or maybe even factor everything that
hevc_mp4toannexb_init() does out of it and make that function accept a
buffer and a length field; and another field to return the reformatted
extradata and its size).
3. Notice that you need to actually modify the packet's
AV_PKT_DATA_NEW_EXTRADATA side-data to make it Annex B. Otherwise the
output will be incorrect.
4. And in case this BSF actually inserts the extradata in-band, it would
need to retain an internal copy of this new extradata.
(Andriy Gelman once sent a good patch that never got applied that added
the correct extradata in-band even when there are in-band updates to the
extradata. I suggest you look at this patch if you want to tackle this
problem.)

- Andreas

Andreas:
On Sun, Dec 5, 2021 at 7:34 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Linjie Fu:
> > From: Linjie Fu <linjie.justin.fu@gmail.com>
> >
> > Container may support multiple sample descriptions in a single
> > bitstream, like multiple stsd in mov, which introduces different
> > sequence header(e.g.profile/bit_depth) in the middle of the bitstream.
> >
> > Update the extradata field in context parameter once packet with
> > different extradata is got.
> >
> > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> b/libavcodec/hevc_mp4toannexb_bsf.c
> > index 790dfb0394..36a83d0c95 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -125,6 +125,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext
> *ctx, AVPacket *out)
> >      int got_irap = 0;
> >      int i, ret = 0;
> >
> > +    uint8_t *extradata;
> > +    size_t extradata_size = 0;
> > +
> >      ret = ff_bsf_get_packet(ctx, &in);
> >      if (ret < 0)
> >          return ret;
> > @@ -135,6 +138,26 @@ static int hevc_mp4toannexb_filter(AVBSFContext
> *ctx, AVPacket *out)
> >          return 0;
> >      }
> >
> > +    extradata = av_packet_get_side_data(in, AV_PKT_DATA_NEW_EXTRADATA,
> &extradata_size);
> > +    if (extradata && extradata_size &&
> > +        (ctx->par_in->extradata_size != extradata_size ||
> > +         memcmp(ctx->par_in->extradata, extradata, extradata_size))) {
> > +        av_log(ctx, AV_LOG_VERBOSE, "Update extradata, size = %zu\n",
> extradata_size);
>
> We use SIZE_SPECIFIER instead of zu due to MSVC compatibility. I don't
> know if we actually support MSVC versions that old any more, but it is
> still used.
>
> +        /* Update extradata */
> > +        av_freep(&ctx->par_in->extradata);
> > +        ctx->par_in->extradata_size = extradata_size;
> > +        ctx->par_in->extradata      = av_mallocz(extradata_size +
> AV_INPUT_BUFFER_PADDING_SIZE);
> > +        if (!ctx->par_in->extradata)
> > +            return AVERROR(ENOMEM);
> > +        memcpy(ctx->par_in->extradata, extradata, extradata_size);
> > +        /* Reinit */
> > +        ret = hevc_extradata_to_annexb(ctx);
> > +        if (ret < 0)
> > +            return ret;
> > +        s->length_size      = ret;
> > +        s->extradata_parsed = 1;
> > +    }
> > +
> >      bytestream2_init(&gb, in->data, in->size);
> >
> >      while (bytestream2_get_bytes_left(&gb)) {
> >
>
> 1. This is an API violation: par_in is set by the user before
> initializing the BSF, par_out is set by the BSF in init. After init,
> both are immutable.
> 2. Instead you should make hevc_extradata_to_annexb() accept a buffer
> with length field (or maybe even factor everything that
> hevc_mp4toannexb_init() does out of it and make that function accept a
> buffer and a length field; and another field to return the reformatted
> extradata and its size).
> 3. Notice that you need to actually modify the packet's
> AV_PKT_DATA_NEW_EXTRADATA side-data to make it Annex B. Otherwise the
> output will be incorrect.
> 4. And in case this BSF actually inserts the extradata in-band, it would
> need to retain an internal copy of this new extradata.
> (Andriy Gelman once sent a good patch that never got applied that added
> the correct extradata in-band even when there are in-band updates to the
> extradata. I suggest you look at this patch if you want to tackle this
> problem.)
>

Will check Andriy‘s patch first, seems that it may handle this case
properly, thanks for the review comments and pointing this out.

 - Linjie