[FFmpeg-devel] [PATCH 2/2] avcodec/cbs_av1: also copy the last frame header's decomposed content when parsing redundant frame headers

[Mark Thompson]

On 28/03/2022 02:08, James Almer wrote:
> This prevents CBS from propagating zeroed AV1RawFrameHeader units in reading
> scenarios.
> Writing scenarios remain unaffected as the content of these units is not used
> to assemble the bitstream.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>   libavcodec/cbs_av1.c                 | 11 +++++++++++
>   libavcodec/cbs_av1.h                 |  2 ++
>   libavcodec/cbs_av1_syntax_template.c |  6 ++++++
>   3 files changed, 19 insertions(+)

I took me a while to work out what the issue was here, maybe make it a bit clearer that the problematic thing is that decomposed unit content for the redundant frame header OBU isn't filled if there was a frame header (in which case that content is never used, but someone could technically look at it if they wanted and observe that it is wrong).

> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index ecd775ea2a..6cb832210c 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -969,6 +969,15 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
>                                                   unit->data_ref);
>               if (err < 0)
>                   return err;
> +
> +            if (priv->frame_header)
> +                break;
> +
> +            av_buffer_unref(&priv->frame_header_ref);
> +            priv->frame_header_ref = av_buffer_ref(unit->content_ref);
> +            if (!priv->frame_header_ref)
> +                return AVERROR(ENOMEM);
> +            priv->frame_header = &obu->obu.frame_header;
>           }
>           break;
>       case AV1_OBU_TILE_GROUP:
> @@ -1251,6 +1260,7 @@ static void cbs_av1_flush(CodedBitstreamContext *ctx)
>   
>       av_buffer_unref(&priv->frame_header_data_ref);
>       priv->sequence_header = NULL;
> +    priv->frame_header = NULL;
>       priv->frame_header_data = NULL;
>   
>       memset(priv->ref, 0, sizeof(priv->ref));
> @@ -1264,6 +1274,7 @@ static void cbs_av1_close(CodedBitstreamContext *ctx)
>       CodedBitstreamAV1Context *priv = ctx->priv_data;
>   
>       av_buffer_unref(&priv->sequence_header_ref);
> +    av_buffer_unref(&priv->frame_header_ref);
>       av_buffer_unref(&priv->frame_header_data_ref);
>   }
>   
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index d4776b7a30..138b273470 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -431,6 +431,8 @@ typedef struct CodedBitstreamAV1Context {
>       AVBufferRef          *sequence_header_ref;
>   
>       int     seen_frame_header;
> +    AVBufferRef *frame_header_ref;
> +    AV1RawFrameHeader *frame_header;
>       AVBufferRef *frame_header_data_ref;
>       uint8_t     *frame_header_data;
>       size_t       frame_header_data_size;
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index bd50cfbe38..aadfa34b3c 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -1708,6 +1708,11 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>                   xf(b, frame_header_copy[i],
>                      val, val, val, 1, i / 8);
>               }
> +
> +#ifdef READ
> +            av_assert0(priv->frame_header_ref && priv->frame_header);
> +            memcpy(current, priv->frame_header, sizeof(*current));
> +#endif
>           }
>       } else {
>           if (redundant)
> @@ -1730,6 +1735,7 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>           } else {
>               priv->seen_frame_header = 1;
>   
> +            priv->frame_header = NULL;
>               av_buffer_unref(&priv->frame_header_data_ref);
>   
>   #ifdef READ

LGTM in any case.  (Previous patch fine too given this one.)

- Mark