diff mbox series

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

Message ID 20220328010851.1193-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cbs_av1: rename the private frame_header fields to frame_header_data | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

James Almer March 28, 2022, 1:08 a.m. UTC 
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@gmail.com>
---
 libavcodec/cbs_av1.c                 | 11 +++++++++++
 libavcodec/cbs_av1.h                 |  2 ++
 libavcodec/cbs_av1_syntax_template.c |  6 ++++++
 3 files changed, 19 insertions(+)

Comments

Mark Thompson April 30, 2022, 6:50 p.m. UTC | #1 
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@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
diff mbox series

Patch

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