[FFmpeg-devel] libavcodec/hevc_mp4toannexb_bsf: insert extradata before non-AUD unit

From: Haihao Xiang <haihao.xiang@intel.com>

It is possible that an IRAP frame in input AVPacket contains VPS, SPS
and PPS, and these headers should take effect. However the prepended
extradata might override these headers. This patch inserts extradata
before non-AUD unit, hence VPS, SPS and PPS from the input AVPacket will
take effect if they are present.

This should fix #7799

Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
---
 libavcodec/hevc_mp4toannexb_bsf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

On Thu, 2022-03-17 at 14:35 +0800, Xiang, Haihao wrote:
> From: Haihao Xiang <haihao.xiang@intel.com>
> 
> It is possible that an IRAP frame in input AVPacket contains VPS, SPS
> and PPS, and these headers should take effect. However the prepended
> extradata might override these headers. This patch inserts extradata
> before non-AUD unit, hence VPS, SPS and PPS from the input AVPacket will
> take effect if they are present.
> 
> This should fix #7799
> 
> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> b/libavcodec/hevc_mp4toannexb_bsf.c
> index 790dfb0394..77551ba221 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -124,6 +124,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *out)
>  
>      int got_irap = 0;
>      int i, ret = 0;
> +    int prev_nalu_is_aud = 0, extradata_offset = 0;
>  
>      ret = ff_bsf_get_packet(ctx, &in);
>      if (ret < 0)
> @@ -169,14 +170,21 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *out)
>  
>          prev_size = out->size;
>  
> +        if (prev_nalu_is_aud)
> +            extradata_offset = prev_size;
> +
>          ret = av_grow_packet(out, 4 + nalu_size + extra_size);
>          if (ret < 0)
>              goto fail;
>  
> -        if (extra_size)
> -            memcpy(out->data + prev_size, ctx->par_out->extradata,
> extra_size);
> +        if (extra_size) {
> +            memmove(out->data + extradata_offset + extra_size, out->data +
> extradata_offset, prev_size - extradata_offset);
> +            memcpy(out->data + extradata_offset, ctx->par_out->extradata,
> extra_size);
> +        }
> +
>          AV_WB32(out->data + prev_size + extra_size, 1);
>          bytestream2_get_buffer(&gb, out->data + prev_size + 4 + extra_size,
> nalu_size);
> +        prev_nalu_is_aud = nalu_type == HEVC_NAL_AUD;
>      }
>  
>      ret = av_packet_copy_props(out, in);

User verified this patch works (https://trac.ffmpeg.org/ticket/7799#comment:19)
and is asking to merge the patch (See 
https://trac.ffmpeg.org/ticket/7799#comment:21). Is there any comment ?

Thanks
Haihao

Xiang, Haihao:
> From: Haihao Xiang <haihao.xiang@intel.com>
> 
> It is possible that an IRAP frame in input AVPacket contains VPS, SPS
> and PPS, and these headers should take effect. However the prepended
> extradata might override these headers. This patch inserts extradata
> before non-AUD unit, hence VPS, SPS and PPS from the input AVPacket will
> take effect if they are present.
> 
> This should fix #7799
> 
> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> index 790dfb0394..77551ba221 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -124,6 +124,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>  
>      int got_irap = 0;
>      int i, ret = 0;
> +    int prev_nalu_is_aud = 0, extradata_offset = 0;
>  
>      ret = ff_bsf_get_packet(ctx, &in);
>      if (ret < 0)
> @@ -169,14 +170,21 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>  
>          prev_size = out->size;
>  
> +        if (prev_nalu_is_aud)
> +            extradata_offset = prev_size;
> +
>          ret = av_grow_packet(out, 4 + nalu_size + extra_size);
>          if (ret < 0)
>              goto fail;
>  
> -        if (extra_size)
> -            memcpy(out->data + prev_size, ctx->par_out->extradata, extra_size);
> +        if (extra_size) {
> +            memmove(out->data + extradata_offset + extra_size, out->data + extradata_offset, prev_size - extradata_offset);
> +            memcpy(out->data + extradata_offset, ctx->par_out->extradata, extra_size);
> +        }
> +
>          AV_WB32(out->data + prev_size + extra_size, 1);
>          bytestream2_get_buffer(&gb, out->data + prev_size + 4 + extra_size, nalu_size);
> +        prev_nalu_is_aud = nalu_type == HEVC_NAL_AUD;
>      }
>  
>      ret = av_packet_copy_props(out, in);

1. prev_nalu_is_aud is unnecessary: You can just use "if (nalu_type ==
HEVC_NAL_AUD) extradata_offset = out->size;" at the end of the loop.
2. This only mitigates a certain case of wrongly inserted extradata; it
does not fix the underlying issue which is that this BSF does not track
the current state of extradata and therefore inserts parameter sets that
have already been superseded by new in-band extradata. Your patch
ensures that the extradata parameter sets will be prepended to the
in-band extradata. Yet the already deactivated parameter sets will still
be inserted. The output can still be invalid, because 7.4.2.4.2 of the
HEVC spec requires the following: "Any SPS NAL unit with nuh_layer_id
equal to 0 containing the value of sps_seq_parameter_set_id for the
active SPS
RBSP for the base layer for a CVS shall have the same content as that of
the active SPS RBSP for the base layer for the CVS, unless it follows
the last access unit of the CVS and precedes the first VCL NAL unit and
the first SEI NAL unit containing an active parameter sets SEI message
(when present) of another CVS." Furthermore in case a preceding packet
contained updated parameter sets that (perhaps partially) overwrite
parameter sets from extradata and the current packet does not
contain/repeat these parameter sets, then the above code will still
insert the outdated and incorrect parameter set and these parameter sets
will not be overwritten before being used.
3. Andriy Gelman once proposed a patchset that tracked the parameter
sets and inserted only the needed ones. See
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191016025040.31273-2-andriy.gelman@gmail.com/
The problem with this patchset was the complexity emanating from HEVC's
layers.
4. Lacking proper tracking of parameter sets we should probably err on
the side of caution and stop inserting parameter sets if the input
contained in-band parameter sets (similar to h264_mp4toannexb). I can
write a patch for this.

- Andreas

On Tue, 2022-03-22 at 10:29 +0100, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xiang@intel.com>
> > 
> > It is possible that an IRAP frame in input AVPacket contains VPS, SPS
> > and PPS, and these headers should take effect. However the prepended
> > extradata might override these headers. This patch inserts extradata
> > before non-AUD unit, hence VPS, SPS and PPS from the input AVPacket will
> > take effect if they are present.
> > 
> > This should fix #7799
> > 
> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> > b/libavcodec/hevc_mp4toannexb_bsf.c
> > index 790dfb0394..77551ba221 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -124,6 +124,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> > AVPacket *out)
> >  
> >      int got_irap = 0;
> >      int i, ret = 0;
> > +    int prev_nalu_is_aud = 0, extradata_offset = 0;
> >  
> >      ret = ff_bsf_get_packet(ctx, &in);
> >      if (ret < 0)
> > @@ -169,14 +170,21 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> > AVPacket *out)
> >  
> >          prev_size = out->size;
> >  
> > +        if (prev_nalu_is_aud)
> > +            extradata_offset = prev_size;
> > +
> >          ret = av_grow_packet(out, 4 + nalu_size + extra_size);
> >          if (ret < 0)
> >              goto fail;
> >  
> > -        if (extra_size)
> > -            memcpy(out->data + prev_size, ctx->par_out->extradata,
> > extra_size);
> > +        if (extra_size) {
> > +            memmove(out->data + extradata_offset + extra_size, out->data +
> > extradata_offset, prev_size - extradata_offset);
> > +            memcpy(out->data + extradata_offset, ctx->par_out->extradata,
> > extra_size);
> > +        }
> > +
> >          AV_WB32(out->data + prev_size + extra_size, 1);
> >          bytestream2_get_buffer(&gb, out->data + prev_size + 4 + extra_size,
> > nalu_size);
> > +        prev_nalu_is_aud = nalu_type == HEVC_NAL_AUD;
> >      }
> >  
> >      ret = av_packet_copy_props(out, in);
> 
> 1. prev_nalu_is_aud is unnecessary: You can just use "if (nalu_type ==
> HEVC_NAL_AUD) extradata_offset = out->size;" at the end of the loop.

Thanks for reviewing the patch.

> 2. This only mitigates a certain case of wrongly inserted extradata; it
> does not fix the underlying issue which is that this BSF does not track
> the current state of extradata and therefore inserts parameter sets that
> have already been superseded by new in-band extradata. Your patch
> ensures that the extradata parameter sets will be prepended to the
> in-band extradata. Yet the already deactivated parameter sets will still
> be inserted. The output can still be invalid, because 7.4.2.4.2 of the
> HEVC spec requires the following: "Any SPS NAL unit with nuh_layer_id
> equal to 0 containing the value of sps_seq_parameter_set_id for the
> active SPS
> RBSP for the base layer for a CVS shall have the same content as that of
> the active SPS RBSP for the base layer for the CVS, unless it follows
> the last access unit of the CVS and precedes the first VCL NAL unit and
> the first SEI NAL unit containing an active parameter sets SEI message
> (when present) of another CVS." Furthermore in case a preceding packet
> contained updated parameter sets that (perhaps partially) overwrite
> parameter sets from extradata and the current packet does not
> contain/repeat these parameter sets, then the above code will still
> insert the outdated and incorrect parameter set and these parameter sets
> will not be overwritten before being used.

I agree this patch doesn't resolve the underlying issue and user might still
experience issues in some cases, but in practice lots of issues disappear after
applying this patch, including the issues in #7799 and some issues I experienced
when using hevc_qsv encoder. May we merge such patch for these issues ? If yes,
I'll update the patch. 

> 3. Andriy Gelman once proposed a patchset that tracked the parameter
> sets and inserted only the needed ones. See
> 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191016025040.31273-2-andriy.gelman@gmail.com/
> The problem with this patchset was the complexity emanating from HEVC's
> layers.

So this patch won't be merged, right ?

> 4. Lacking proper tracking of parameter sets we should probably err on
> the side of caution and stop inserting parameter sets if the input
> contained in-band parameter sets (similar to h264_mp4toannexb). I can
> write a patch for this.

I'm looking forward to test your patch. 

Thanks
Haihao

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Tuesday, March 22, 2022 10:30 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] libavcodec/hevc_mp4toannexb_bsf:
> insert extradata before non-AUD unit
> 
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xiang@intel.com>
> >
> > It is possible that an IRAP frame in input AVPacket contains VPS,
> SPS
> > and PPS, and these headers should take effect. However the
> prepended
> > extradata might override these headers. This patch inserts
> extradata
> > before non-AUD unit, hence VPS, SPS and PPS from the input AVPacket
> will
> > take effect if they are present.
> >
> > This should fix #7799
> >
> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> b/libavcodec/hevc_mp4toannexb_bsf.c
> > index 790dfb0394..77551ba221 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -124,6 +124,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext
> *ctx, AVPacket *out)
> >
> >      int got_irap = 0;
> >      int i, ret = 0;
> > +    int prev_nalu_is_aud = 0, extradata_offset = 0;
> >
> >      ret = ff_bsf_get_packet(ctx, &in);
> >      if (ret < 0)
> > @@ -169,14 +170,21 @@ static int
> hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
> >
> >          prev_size = out->size;
> >
> > +        if (prev_nalu_is_aud)
> > +            extradata_offset = prev_size;
> > +
> >          ret = av_grow_packet(out, 4 + nalu_size + extra_size);
> >          if (ret < 0)
> >              goto fail;
> >
> > -        if (extra_size)
> > -            memcpy(out->data + prev_size, ctx->par_out->extradata,
> extra_size);
> > +        if (extra_size) {
> > +            memmove(out->data + extradata_offset + extra_size,
> out->data + extradata_offset, prev_size - extradata_offset);
> > +            memcpy(out->data + extradata_offset, ctx->par_out-
> >extradata, extra_size);
> > +        }
> > +
> >          AV_WB32(out->data + prev_size + extra_size, 1);
> >          bytestream2_get_buffer(&gb, out->data + prev_size + 4 +
> extra_size, nalu_size);
> > +        prev_nalu_is_aud = nalu_type == HEVC_NAL_AUD;
> >      }
> >
> >      ret = av_packet_copy_props(out, in);
> 
> 1. prev_nalu_is_aud is unnecessary: You can just use "if (nalu_type
> ==
> HEVC_NAL_AUD) extradata_offset = out->size;" at the end of the loop.
> 2. This only mitigates a certain case of wrongly inserted extradata;
> it
> does not fix the underlying issue which is that this BSF does not
> track
> the current state of extradata and therefore inserts parameter sets
> that
> have already been superseded by new in-band extradata. Your patch
> ensures that the extradata parameter sets will be prepended to the
> in-band extradata. Yet the already deactivated parameter sets will
> still
> be inserted. The output can still be invalid, because 7.4.2.4.2 of
> the
> HEVC spec requires the following: "Any SPS NAL unit with nuh_layer_id
> equal to 0 containing the value of sps_seq_parameter_set_id for the
> active SPS
> RBSP for the base layer for a CVS shall have the same content as that
> of
> the active SPS RBSP for the base layer for the CVS, unless it follows
> the last access unit of the CVS and precedes the first VCL NAL unit
> and
> the first SEI NAL unit containing an active parameter sets SEI
> message
> (when present) of another CVS." Furthermore in case a preceding
> packet
> contained updated parameter sets that (perhaps partially) overwrite
> parameter sets from extradata and the current packet does not
> contain/repeat these parameter sets, then the above code will still
> insert the outdated and incorrect parameter set and these parameter
> sets
> will not be overwritten before being used.
> 3. Andriy Gelman once proposed a patchset that tracked the parameter
> sets and inserted only the needed ones. See
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191016025040.3127
> 3-2-andriy.gelman@gmail.com/
> The problem with this patchset was the complexity emanating from
> HEVC's
> layers.
> 4. Lacking proper tracking of parameter sets we should probably err
> on
> the side of caution and stop inserting parameter sets if the input
> contained in-band parameter sets (similar to h264_mp4toannexb). I can
> write a patch for this.

Hi Andreas,

I had missed this conversation and just recently noticed that Haihao's
Patch fixes HEVC decoding issues with certain files that I'm seeing 
for a long time already. The cases I know are with the hevc_qsv, 
hevc_cuvid and mediacodec hevc decoders, but obviously it's about 
all usages of the bitstream filter.

You mentioned you might want to write a patch. Would you still be able
to do so? Would this be a complete fix for the problem or just about
a part of it?

Or would this patch be acceptable as a temporary workaround?

Thanks,
softworkz