Re: [FFmpeg-devel] [PATCH] lavc/cbs_av1: restore CodedBitstreamAV1Context when AVERROR(ENOSPC)

["Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org>]

On Tue, 2022-09-27 at 00:31 -0300, James Almer wrote:
> On 9/26/2022 4:23 AM, Xiang, Haihao wrote:
> > From: Haihao Xiang <haihao.xiang@intel.com>
> > 
> > The current pbc might be small for an obu frame, so a new pbc is
> > required then parse this obu frame again. Because
> > CodedBitstreamAV1Context has already been updated for this obu frame, we
> > need to restore CodedBitstreamAV1Context, otherwise
> > CodedBitstreamAV1Context doesn't match this obu frame when parsing obu
> > frame again, e.g. CodedBitstreamAV1Context.order_hint.
> > 
> > $ ffmpeg -i input.ivf -c:v copy -f null -
> > [...]
> > [av1_frame_merge @ 0x558bc3d6f880] ref_order_hint[i] does not match
> > inferred value: 20, but should be 22.
> > [av1_frame_merge @ 0x558bc3d6f880] Failed to write unit 1 (type 6).
> > [av1_frame_merge @ 0x558bc3d6f880] Failed to write packet.
> > [obu @ 0x558bc3d6e040] av1_frame_merge filter failed to send output
> > packet
> > ---
> >   libavcodec/cbs_av1.c | 85 ++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 67 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> > index 154d9156cf..585bc72cdb 100644
> > --- a/libavcodec/cbs_av1.c
> > +++ b/libavcodec/cbs_av1.c
> > @@ -1058,15 +1058,33 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >       AV1RawTileData *td;
> >       size_t header_size;
> >       int err, start_pos, end_pos, data_pos;
> > +    CodedBitstreamAV1Context av1ctx;
> >   
> >       // OBUs in the normal bitstream format must contain a size field
> >       // in every OBU (in annex B it is optional, but we don't support
> >       // writing that).
> >       obu->header.obu_has_size_field = 1;
> > +    av1ctx = *priv;
> > +
> > +    if (priv->sequence_header_ref) {
> > +        av1ctx.sequence_header_ref = av_buffer_ref(priv-
> > >sequence_header_ref);
> > +        if (!av1ctx.sequence_header_ref) {
> > +            err = AVERROR(ENOMEM);
> > +            goto error;
> 
> If this is called, av1ctx.frame_header_ref will be equal to 
> priv->frame_header_ref and as such unreferenced twice if not NULL. So 
> just return AVERROR(ENOMEM) here.

Thanks for pointing it out, I'll update it in v2.

> 
> > +        }
> > +    }
> > +
> > +    if (priv->frame_header_ref) {
> > +        av1ctx.frame_header_ref = av_buffer_ref(priv->frame_header_ref);
> > +        if (!av1ctx.frame_header_ref) {
> > +            err = AVERROR(ENOMEM);
> > +            goto error;
> > +        }
> > +    }
> >   
> >       err = cbs_av1_write_obu_header(ctx, pbc, &obu->header);
> >       if (err < 0)
> > -        return err;
> > +        goto error;
> >   
> >       if (obu->header.obu_has_size_field) {
> >           pbc_tmp = *pbc;
> > @@ -1084,18 +1102,21 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >               err = cbs_av1_write_sequence_header_obu(ctx, pbc,
> >                                                       &obu-
> > >obu.sequence_header);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >   
> >               av_buffer_unref(&priv->sequence_header_ref);
> >               priv->sequence_header = NULL;
> >   
> >               err = ff_cbs_make_unit_refcounted(ctx, unit);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >   
> >               priv->sequence_header_ref = av_buffer_ref(unit->content_ref);
> > -            if (!priv->sequence_header_ref)
> > -                return AVERROR(ENOMEM);
> > +            if (!priv->sequence_header_ref) {
> > +                err = AVERROR(ENOMEM);
> > +                goto error;
> > +            }
> > +
> >               priv->sequence_header = &obu->obu.sequence_header;
> >           }
> >           break;
> > @@ -1103,7 +1124,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >           {
> >               err = cbs_av1_write_temporal_delimiter_obu(ctx, pbc);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >           }
> >           break;
> >       case AV1_OBU_FRAME_HEADER:
> > @@ -1115,7 +1136,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >                                                    AV1_OBU_REDUNDANT_FRAME_H
> > EADER,
> >                                                    NULL);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >           }
> >           break;
> >       case AV1_OBU_TILE_GROUP:
> > @@ -1123,7 +1144,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >               err = cbs_av1_write_tile_group_obu(ctx, pbc,
> >                                                  &obu->obu.tile_group);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >   
> >               td = &obu->obu.tile_group.tile_data;
> >           }
> > @@ -1132,7 +1153,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >           {
> >               err = cbs_av1_write_frame_obu(ctx, pbc, &obu->obu.frame,
> > NULL);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >   
> >               td = &obu->obu.frame.tile_group.tile_data;
> >           }
> > @@ -1141,7 +1162,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >           {
> >               err = cbs_av1_write_tile_list_obu(ctx, pbc, &obu-
> > >obu.tile_list);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >   
> >               td = &obu->obu.tile_list.tile_data;
> >           }
> > @@ -1150,18 +1171,19 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >           {
> >               err = cbs_av1_write_metadata_obu(ctx, pbc, &obu-
> > >obu.metadata);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >           }
> >           break;
> >       case AV1_OBU_PADDING:
> >           {
> >               err = cbs_av1_write_padding_obu(ctx, pbc, &obu->obu.padding);
> >               if (err < 0)
> > -                return err;
> > +                goto error;
> >           }
> >           break;
> >       default:
> > -        return AVERROR(ENOSYS);
> > +        err = AVERROR(ENOSYS);
> > +        goto error;
> >       }
> >   
> >       end_pos = put_bits_count(pbc);
> > @@ -1172,7 +1194,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >           // Add trailing bits and recalculate.
> >           err = cbs_av1_write_trailing_bits(ctx, pbc, 8 - end_pos % 8);
> >           if (err < 0)
> > -            return err;
> > +            goto error;
> >           end_pos = put_bits_count(pbc);
> >           obu->obu_size = header_size = (end_pos - start_pos + 7) / 8;
> >       } else {
> > @@ -1190,14 +1212,36 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >       *pbc = pbc_tmp;
> >       err = cbs_av1_write_leb128(ctx, pbc, "obu_size", obu->obu_size);
> >       if (err < 0)
> > -        return err;
> > +        goto error;
> >   
> >       data_pos = put_bits_count(pbc) / 8;
> >       flush_put_bits(pbc);
> >       av_assert0(data_pos <= start_pos);
> >   
> > -    if (8 * obu->obu_size > put_bits_left(pbc))
> > -        return AVERROR(ENOSPC);
> > +    if (8 * obu->obu_size > put_bits_left(pbc)) {
> > +        av_buffer_unref(&priv->sequence_header_ref);
> > +        av_buffer_unref(&priv->frame_header_ref);
> > +        *priv = av1ctx;
> > +
> > +        if (av1ctx.sequence_header_ref) {
> > +            priv->sequence_header_ref =
> > av_buffer_ref(av1ctx.sequence_header_ref);
> > +            if (!priv->sequence_header_ref) {
> > +                err = AVERROR(ENOMEM);
> > +                goto error;
> > +            }
> > +        }
> > +
> > +        if (av1ctx.frame_header_ref) {
> > +            priv->frame_header_ref =
> > av_buffer_ref(av1ctx.frame_header_ref);
> > +            if (!priv->frame_header_ref) {
> > +                err = AVERROR(ENOMEM);
> > +                goto error;
> > +            }
> > +        }
> 
> Why are you creating new references? They were already copied to priv 
> and av1ctx is in the stack, so just return AVERROR(ENOSPC) directly.

I wanted to return the value in the same place, but you are right, here it is
better to return AVERROR(ENOSPC) directly. 

Thanks
Haihao

> 
> > +
> > +        err = AVERROR(ENOSPC);
> > +        goto error;
> > +    }
> >   
> >       if (obu->obu_size > 0) {
> >           memmove(pbc->buf + data_pos,
> > @@ -1213,8 +1257,13 @@ static int cbs_av1_write_obu(CodedBitstreamContext
> > *ctx,
> >   
> >       // OBU data must be byte-aligned.
> >       av_assert0(put_bits_count(pbc) % 8 == 0);
> > +    err = 0;
> >   
> > -    return 0;
> > +error:
> > +    av_buffer_unref(&av1ctx.sequence_header_ref);
> > +    av_buffer_unref(&av1ctx.frame_header_ref);
> > +
> > +    return err;
> >   }
> >   
> >   static int cbs_av1_assemble_fragment(CodedBitstreamContext *ctx,
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".