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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Tuesday, March 22, 2022 10:30 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] libavcodec/hevc_mp4toannexb_bsf:
> insert extradata before non-AUD unit
> 
> 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.
> 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 at 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