[FFmpeg-devel] [PATCH] lavc/cbs_av1: restore CodedBitstreamAV1Context when AVERROR(ENOSPC)
Xiang, Haihao
haihao.xiang at intel.com
Tue Sep 27 08:38:57 EEST 2022
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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list