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

[Xiang, Haihao]

On Tue, 2022-03-22 at 10:29 +0100, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xiang at 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 at 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