[FFmpeg-devel] avcodec/cbs_av1: add missing value range constrains to timecode Metadata OBU

Submitted by James Almer on April 17, 2019, 3:48 p.m.

Details

Message ID 20190417154824.5516-1-jamrial@gmail.com
State Accepted
Commit 67f9d3f461f65395f39d3e01801ce8b572d0aa12
Headers show

Commit Message

James Almer April 17, 2019, 3:48 p.m.
Also infer the value time_offset_length as 0 when it's not present.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Fun thing, this metadata OBU is clearly based on the H264/5 timecode SEI, yet
time_offset_length is unsigned here :p

 libavcodec/cbs_av1_syntax_template.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Mark Thompson April 28, 2019, 10:28 p.m.
On 17/04/2019 16:48, James Almer wrote:
> Also infer the value time_offset_length as 0 when it's not present.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Fun thing, this metadata OBU is clearly based on the H264/5 timecode SEI, yet
> time_offset_length is unsigned here :p
> 
>  libavcodec/cbs_av1_syntax_template.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index 59a98b18c9..b04cd51d55 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -1753,19 +1753,19 @@ static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
>      fb(9, n_frames);
>  
>      if (current->full_timestamp_flag) {
> -        fb(6, seconds_value);
> -        fb(6, minutes_value);
> -        fb(5, hours_value);
> +        fc(6, seconds_value, 0, 59);

What, no leap seconds?  :P

> +        fc(6, minutes_value, 0, 59);
> +        fc(5, hours_value,   0, 23);
>      } else {
>          flag(seconds_flag);
>          if (current->seconds_flag) {
> -            fb(6, seconds_value);
> +            fc(6, seconds_value, 0, 59);
>              flag(minutes_flag);
>              if (current->minutes_flag) {
> -                fb(6, minutes_value);
> +                fc(6, minutes_value, 0, 59);
>                  flag(hours_flag);
>                  if (current->hours_flag)
> -                    fb(5, hours_value);
> +                    fc(5, hours_value, 0, 23);
>              }
>          }
>      }
> @@ -1773,6 +1773,8 @@ static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
>      fb(5, time_offset_length);
>      if (current->time_offset_length > 0)
>          fb(current->time_offset_length, time_offset_value);
> +    else
> +        infer(time_offset_length, 0);
>  
>      return 0;
>  }
> 

LGTM.

Thanks,

- Mark
James Almer April 28, 2019, 10:49 p.m.
On 4/28/2019 7:28 PM, Mark Thompson wrote:
> On 17/04/2019 16:48, James Almer wrote:
>> Also infer the value time_offset_length as 0 when it's not present.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Fun thing, this metadata OBU is clearly based on the H264/5 timecode SEI, yet
>> time_offset_length is unsigned here :p
>>
>>  libavcodec/cbs_av1_syntax_template.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
>> index 59a98b18c9..b04cd51d55 100644
>> --- a/libavcodec/cbs_av1_syntax_template.c
>> +++ b/libavcodec/cbs_av1_syntax_template.c
>> @@ -1753,19 +1753,19 @@ static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
>>      fb(9, n_frames);
>>  
>>      if (current->full_timestamp_flag) {
>> -        fb(6, seconds_value);
>> -        fb(6, minutes_value);
>> -        fb(5, hours_value);
>> +        fc(6, seconds_value, 0, 59);
> 
> What, no leap seconds?  :P

Not even the Linux Kernel could handle those properly at some point :p

> 
>> +        fc(6, minutes_value, 0, 59);
>> +        fc(5, hours_value,   0, 23);
>>      } else {
>>          flag(seconds_flag);
>>          if (current->seconds_flag) {
>> -            fb(6, seconds_value);
>> +            fc(6, seconds_value, 0, 59);
>>              flag(minutes_flag);
>>              if (current->minutes_flag) {
>> -                fb(6, minutes_value);
>> +                fc(6, minutes_value, 0, 59);
>>                  flag(hours_flag);
>>                  if (current->hours_flag)
>> -                    fb(5, hours_value);
>> +                    fc(5, hours_value, 0, 23);
>>              }
>>          }
>>      }
>> @@ -1773,6 +1773,8 @@ static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
>>      fb(5, time_offset_length);
>>      if (current->time_offset_length > 0)
>>          fb(current->time_offset_length, time_offset_value);
>> +    else
>> +        infer(time_offset_length, 0);
>>  
>>      return 0;
>>  }
>>
> 
> LGTM.

Pushed. Look at the h2645 patchset set i sent before this patch as well
whenever you can.

Thanks!

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
index 59a98b18c9..b04cd51d55 100644
--- a/libavcodec/cbs_av1_syntax_template.c
+++ b/libavcodec/cbs_av1_syntax_template.c
@@ -1753,19 +1753,19 @@  static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
     fb(9, n_frames);
 
     if (current->full_timestamp_flag) {
-        fb(6, seconds_value);
-        fb(6, minutes_value);
-        fb(5, hours_value);
+        fc(6, seconds_value, 0, 59);
+        fc(6, minutes_value, 0, 59);
+        fc(5, hours_value,   0, 23);
     } else {
         flag(seconds_flag);
         if (current->seconds_flag) {
-            fb(6, seconds_value);
+            fc(6, seconds_value, 0, 59);
             flag(minutes_flag);
             if (current->minutes_flag) {
-                fb(6, minutes_value);
+                fc(6, minutes_value, 0, 59);
                 flag(hours_flag);
                 if (current->hours_flag)
-                    fb(5, hours_value);
+                    fc(5, hours_value, 0, 23);
             }
         }
     }
@@ -1773,6 +1773,8 @@  static int FUNC(metadata_timecode)(CodedBitstreamContext *ctx, RWContext *rw,
     fb(5, time_offset_length);
     if (current->time_offset_length > 0)
         fb(current->time_offset_length, time_offset_value);
+    else
+        infer(time_offset_length, 0);
 
     return 0;
 }