diff mbox series

[FFmpeg-devel] avcodec/cbs_av1: add a range check to tg_end

Message ID 20201027212506.21925-1-jamrial@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/cbs_av1: add a range check to tg_end
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Oct. 27, 2020, 9:25 p.m. UTC
Section 6.10.1 of the AV1 spec states:

It is a requirement of bitstream conformance that the value of tg_start is
equal to the value of TileNum at the point that tile_group_obu is invoked.
It is a requirement of bitstream conformance that the value of tg_end is
greater than or equal to tg_start.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Not sure if we should clear priv->tile_num in other places, too.

 libavcodec/cbs_av1.h                 | 1 +
 libavcodec/cbs_av1_syntax_template.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

James Almer Nov. 10, 2020, 1:01 p.m. UTC | #1
On 10/27/2020 6:25 PM, James Almer wrote:
> Section 6.10.1 of the AV1 spec states:
> 
> It is a requirement of bitstream conformance that the value of tg_start is
> equal to the value of TileNum at the point that tile_group_obu is invoked.
> It is a requirement of bitstream conformance that the value of tg_end is
> greater than or equal to tg_start.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Not sure if we should clear priv->tile_num in other places, too.

Ping.

> 
>   libavcodec/cbs_av1.h                 | 1 +
>   libavcodec/cbs_av1_syntax_template.c | 8 ++++++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index a2d78e736f..386774750a 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context {
>       int all_lossless;
>       int tile_cols;
>       int tile_rows;
> +    int tile_num;
>   
>       AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES];
>   } CodedBitstreamAV1Context;
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index 2df5619279..f0be7aab59 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -1719,6 +1719,8 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>   
>           CHECK(FUNC(uncompressed_header)(ctx, rw, current));
>   
> +        priv->tile_num = 0;
> +
>           if (current->show_existing_frame) {
>               priv->seen_frame_header = 0;
>           } else {
> @@ -1784,10 +1786,12 @@ static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>       } else {
>           tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) +
>                       cbs_av1_tile_log2(1, priv->tile_rows);
> -        fb(tile_bits, tg_start);
> -        fb(tile_bits, tg_end);
> +        fc(tile_bits, tg_start, priv->tile_num, priv->tile_num);
> +        fc(tile_bits, tg_end, current->tg_start, num_tiles - 1);
>       }
>   
> +    priv->tile_num += current->tg_end - current->tg_start + 1;
> +
>       CHECK(FUNC(byte_alignment)(ctx, rw));
>   
>       // Reset header for next frame.
>
Mark Thompson Nov. 10, 2020, 10:36 p.m. UTC | #2
On 27/10/2020 21:25, James Almer wrote:
> Section 6.10.1 of the AV1 spec states:
> 
> It is a requirement of bitstream conformance that the value of tg_start is
> equal to the value of TileNum at the point that tile_group_obu is invoked.
> It is a requirement of bitstream conformance that the value of tg_end is
> greater than or equal to tg_start.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Not sure if we should clear priv->tile_num in other places, too.
> 
>   libavcodec/cbs_av1.h                 | 1 +
>   libavcodec/cbs_av1_syntax_template.c | 8 ++++++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index a2d78e736f..386774750a 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context {
>       int all_lossless;
>       int tile_cols;
>       int tile_rows;
> +    int tile_num;
>   
>       AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES];
>   } CodedBitstreamAV1Context;
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index 2df5619279..f0be7aab59 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -1719,6 +1719,8 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>   
>           CHECK(FUNC(uncompressed_header)(ctx, rw, current));
>   
> +        priv->tile_num = 0;
> +
>           if (current->show_existing_frame) {
>               priv->seen_frame_header = 0;
>           } else {
> @@ -1784,10 +1786,12 @@ static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>       } else {
>           tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) +
>                       cbs_av1_tile_log2(1, priv->tile_rows);
> -        fb(tile_bits, tg_start);
> -        fb(tile_bits, tg_end);
> +        fc(tile_bits, tg_start, priv->tile_num, priv->tile_num);

This isn't true if you lose a tile group OBU making up part of a frame, which we should be able to handle as something better than just a parsing failure in error resilient mode.

I think set the lower bound only and let the upper bound be num_tiles - 1?

> +        fc(tile_bits, tg_end, current->tg_start, num_tiles - 1);
>       }
>   
> +    priv->tile_num += current->tg_end - current->tg_start + 1;

priv->tile_num = current->tg_end + 1;

> +
>       CHECK(FUNC(byte_alignment)(ctx, rw));
>   
>       // Reset header for next frame.
> 

Thanks,

- Mark
James Almer Nov. 11, 2020, 1:26 p.m. UTC | #3
On 11/10/2020 7:36 PM, Mark Thompson wrote:
> On 27/10/2020 21:25, James Almer wrote:
>> Section 6.10.1 of the AV1 spec states:
>>
>> It is a requirement of bitstream conformance that the value of 
>> tg_start is
>> equal to the value of TileNum at the point that tile_group_obu is 
>> invoked.
>> It is a requirement of bitstream conformance that the value of tg_end is
>> greater than or equal to tg_start.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Not sure if we should clear priv->tile_num in other places, too.
>>
>>   libavcodec/cbs_av1.h                 | 1 +
>>   libavcodec/cbs_av1_syntax_template.c | 8 ++++++--
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
>> index a2d78e736f..386774750a 100644
>> --- a/libavcodec/cbs_av1.h
>> +++ b/libavcodec/cbs_av1.h
>> @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context {
>>       int all_lossless;
>>       int tile_cols;
>>       int tile_rows;
>> +    int tile_num;
>>       AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES];
>>   } CodedBitstreamAV1Context;
>> diff --git a/libavcodec/cbs_av1_syntax_template.c 
>> b/libavcodec/cbs_av1_syntax_template.c
>> index 2df5619279..f0be7aab59 100644
>> --- a/libavcodec/cbs_av1_syntax_template.c
>> +++ b/libavcodec/cbs_av1_syntax_template.c
>> @@ -1719,6 +1719,8 @@ static int 
>> FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>>           CHECK(FUNC(uncompressed_header)(ctx, rw, current));
>> +        priv->tile_num = 0;
>> +
>>           if (current->show_existing_frame) {
>>               priv->seen_frame_header = 0;
>>           } else {
>> @@ -1784,10 +1786,12 @@ static int 
>> FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>>       } else {
>>           tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) +
>>                       cbs_av1_tile_log2(1, priv->tile_rows);
>> -        fb(tile_bits, tg_start);
>> -        fb(tile_bits, tg_end);
>> +        fc(tile_bits, tg_start, priv->tile_num, priv->tile_num);
> 
> This isn't true if you lose a tile group OBU making up part of a frame, 
> which we should be able to handle as something better than just a 
> parsing failure in error resilient mode.
> 
> I think set the lower bound only and let the upper bound be num_tiles - 1?

Done and pushed.

I suppose the CBS user should check the frame's integrity then?

> 
>> +        fc(tile_bits, tg_end, current->tg_start, num_tiles - 1);
>>       }
>> +    priv->tile_num += current->tg_end - current->tg_start + 1;
> 
> priv->tile_num = current->tg_end + 1;
> 
>> +
>>       CHECK(FUNC(byte_alignment)(ctx, rw));
>>       // Reset header for next frame.
>>
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
index a2d78e736f..386774750a 100644
--- a/libavcodec/cbs_av1.h
+++ b/libavcodec/cbs_av1.h
@@ -446,6 +446,7 @@  typedef struct CodedBitstreamAV1Context {
     int all_lossless;
     int tile_cols;
     int tile_rows;
+    int tile_num;
 
     AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES];
 } CodedBitstreamAV1Context;
diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
index 2df5619279..f0be7aab59 100644
--- a/libavcodec/cbs_av1_syntax_template.c
+++ b/libavcodec/cbs_av1_syntax_template.c
@@ -1719,6 +1719,8 @@  static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
 
         CHECK(FUNC(uncompressed_header)(ctx, rw, current));
 
+        priv->tile_num = 0;
+
         if (current->show_existing_frame) {
             priv->seen_frame_header = 0;
         } else {
@@ -1784,10 +1786,12 @@  static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw,
     } else {
         tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) +
                     cbs_av1_tile_log2(1, priv->tile_rows);
-        fb(tile_bits, tg_start);
-        fb(tile_bits, tg_end);
+        fc(tile_bits, tg_start, priv->tile_num, priv->tile_num);
+        fc(tile_bits, tg_end, current->tg_start, num_tiles - 1);
     }
 
+    priv->tile_num += current->tg_end - current->tg_start + 1;
+
     CHECK(FUNC(byte_alignment)(ctx, rw));
 
     // Reset header for next frame.