[FFmpeg-devel,2/2] avcodec/cbs_av1: fix parsing signed integer values

Submitted by James Almer on Nov. 11, 2018, 2:24 a.m.

Details

Message ID 20181111022414.10964-2-jamrial@gmail.com
State Accepted
Headers show

Commit Message

James Almer Nov. 11, 2018, 2:24 a.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
See https://0x0.st/sljR.webm

 libavcodec/cbs_av1.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

Comments

James Almer Nov. 14, 2018, 1:47 a.m.
On 11/10/2018 11:24 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> See https://0x0.st/sljR.webm
> 
>  libavcodec/cbs_av1.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index ff32a6fca5..215f9384e8 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
>                             int width, const char *name,
>                             const int *subscripts, int32_t *write_to)
>  {
> -    uint32_t magnitude;
> -    int position, sign;
> +    int position;
>      int32_t value;
>  
>      if (ctx->trace_enable)
>          position = get_bits_count(gbc);
>  
> -    if (get_bits_left(gbc) < width + 1) {
> +    if (get_bits_left(gbc) < width) {
>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
>                 "%s: bitstream ended.\n", name);
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    magnitude = get_bits(gbc, width);
> -    sign      = get_bits1(gbc);
> -    value     = sign ? -(int32_t)magnitude : magnitude;
> +    value = get_sbits(gbc, width);
>  
>      if (ctx->trace_enable) {
>          char bits[33];
>          int i;
>          for (i = 0; i < width; i++)
> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
> -        bits[i] = sign ? '1' : '0';
> -        bits[i + 1] = 0;
> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
> +        bits[i] = 0;
>  
>          ff_cbs_trace_syntax_element(ctx, position,
>                                      name, subscripts, bits, value);
> @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc,
>                              int width, const char *name,
>                              const int *subscripts, int32_t value)
>  {
> -    uint32_t magnitude;
> -    int sign;
> -
> -    if (put_bits_left(pbc) < width + 1)
> +    if (put_bits_left(pbc) < width)
>          return AVERROR(ENOSPC);
>  
> -    sign      = value < 0;
> -    magnitude = sign ? -value : value;
> -
>      if (ctx->trace_enable) {
>          char bits[33];
>          int i;
>          for (i = 0; i < width; i++)
> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
> -        bits[i] = sign ? '1' : '0';
> -        bits[i + 1] = 0;
> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
> +        bits[i] = 0;
>  
>          ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
>                                      name, subscripts, bits, value);
>      }
>  
> -    put_bits(pbc, width, magnitude);
> -    put_bits(pbc, 1, sign);
> +    put_sbits(pbc, width, value);
>  
>      return 0;
>  }

Ping. Some real world samples depend on this.
Mark Thompson Nov. 14, 2018, 10:24 p.m.
On 11/11/18 02:24, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> See https://0x0.st/sljR.webm
> 
>  libavcodec/cbs_av1.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index ff32a6fca5..215f9384e8 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
>                             int width, const char *name,
>                             const int *subscripts, int32_t *write_to)
>  {
> -    uint32_t magnitude;
> -    int position, sign;
> +    int position;
>      int32_t value;
>  
>      if (ctx->trace_enable)
>          position = get_bits_count(gbc);
>  
> -    if (get_bits_left(gbc) < width + 1) {
> +    if (get_bits_left(gbc) < width) {
>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
>                 "%s: bitstream ended.\n", name);
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    magnitude = get_bits(gbc, width);
> -    sign      = get_bits1(gbc);
> -    value     = sign ? -(int32_t)magnitude : magnitude;
> +    value = get_sbits(gbc, width);
>  
>      if (ctx->trace_enable) {
>          char bits[33];
>          int i;
>          for (i = 0; i < width; i++)
> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
> -        bits[i] = sign ? '1' : '0';
> -        bits[i + 1] = 0;
> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';

Not sure if we care, but right-shifting a negative value is implementation-defined.

> +        bits[i] = 0;
>  
>          ff_cbs_trace_syntax_element(ctx, position,
>                                      name, subscripts, bits, value);
> @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc,
>                              int width, const char *name,
>                              const int *subscripts, int32_t value)
>  {
> -    uint32_t magnitude;
> -    int sign;
> -
> -    if (put_bits_left(pbc) < width + 1)
> +    if (put_bits_left(pbc) < width)
>          return AVERROR(ENOSPC);
>  
> -    sign      = value < 0;
> -    magnitude = sign ? -value : value;
> -
>      if (ctx->trace_enable) {
>          char bits[33];
>          int i;
>          for (i = 0; i < width; i++)
> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
> -        bits[i] = sign ? '1' : '0';
> -        bits[i + 1] = 0;
> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';

And here.

> +        bits[i] = 0;
>  
>          ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
>                                      name, subscripts, bits, value);
>      }
>  
> -    put_bits(pbc, width, magnitude);
> -    put_bits(pbc, 1, sign);
> +    put_sbits(pbc, width, value);
>  
>      return 0;
>  }
> 

Otherwise fine.  No idea where the version with the sign bit at the other end came from; oh well.

Thanks,

- Mark
James Almer Nov. 14, 2018, 10:39 p.m.
On 11/14/2018 7:24 PM, Mark Thompson wrote:
> On 11/11/18 02:24, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> See https://0x0.st/sljR.webm
>>
>>  libavcodec/cbs_av1.c | 30 +++++++++---------------------
>>  1 file changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>> index ff32a6fca5..215f9384e8 100644
>> --- a/libavcodec/cbs_av1.c
>> +++ b/libavcodec/cbs_av1.c
>> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
>>                             int width, const char *name,
>>                             const int *subscripts, int32_t *write_to)
>>  {
>> -    uint32_t magnitude;
>> -    int position, sign;
>> +    int position;
>>      int32_t value;
>>  
>>      if (ctx->trace_enable)
>>          position = get_bits_count(gbc);
>>  
>> -    if (get_bits_left(gbc) < width + 1) {
>> +    if (get_bits_left(gbc) < width) {
>>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
>>                 "%s: bitstream ended.\n", name);
>>          return AVERROR_INVALIDDATA;
>>      }
>>  
>> -    magnitude = get_bits(gbc, width);
>> -    sign      = get_bits1(gbc);
>> -    value     = sign ? -(int32_t)magnitude : magnitude;
>> +    value = get_sbits(gbc, width);
>>  
>>      if (ctx->trace_enable) {
>>          char bits[33];
>>          int i;
>>          for (i = 0; i < width; i++)
>> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
>> -        bits[i] = sign ? '1' : '0';
>> -        bits[i + 1] = 0;
>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
> 
> Not sure if we care, but right-shifting a negative value is implementation-defined.

Is casting value to unsigned enough, or should i use something like
zero_extend()?

> 
>> +        bits[i] = 0;
>>  
>>          ff_cbs_trace_syntax_element(ctx, position,
>>                                      name, subscripts, bits, value);
>> @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>                              int width, const char *name,
>>                              const int *subscripts, int32_t value)
>>  {
>> -    uint32_t magnitude;
>> -    int sign;
>> -
>> -    if (put_bits_left(pbc) < width + 1)
>> +    if (put_bits_left(pbc) < width)
>>          return AVERROR(ENOSPC);
>>  
>> -    sign      = value < 0;
>> -    magnitude = sign ? -value : value;
>> -
>>      if (ctx->trace_enable) {
>>          char bits[33];
>>          int i;
>>          for (i = 0; i < width; i++)
>> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
>> -        bits[i] = sign ? '1' : '0';
>> -        bits[i + 1] = 0;
>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
> 
> And here.
> 
>> +        bits[i] = 0;
>>  
>>          ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
>>                                      name, subscripts, bits, value);
>>      }
>>  
>> -    put_bits(pbc, width, magnitude);
>> -    put_bits(pbc, 1, sign);
>> +    put_sbits(pbc, width, value);
>>  
>>      return 0;
>>  }
>>
> 
> Otherwise fine.  No idea where the version with the sign bit at the other end came from; oh well.

From a lack of samples to test and confirm that the code works :p

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mark Thompson Nov. 14, 2018, 11:04 p.m.
On 14/11/18 22:39, James Almer wrote:
> On 11/14/2018 7:24 PM, Mark Thompson wrote:
>> On 11/11/18 02:24, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> See https://0x0.st/sljR.webm
>>>
>>>  libavcodec/cbs_av1.c | 30 +++++++++---------------------
>>>  1 file changed, 9 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>>> index ff32a6fca5..215f9384e8 100644
>>> --- a/libavcodec/cbs_av1.c
>>> +++ b/libavcodec/cbs_av1.c
>>> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
>>>                             int width, const char *name,
>>>                             const int *subscripts, int32_t *write_to)
>>>  {
>>> -    uint32_t magnitude;
>>> -    int position, sign;
>>> +    int position;
>>>      int32_t value;
>>>  
>>>      if (ctx->trace_enable)
>>>          position = get_bits_count(gbc);
>>>  
>>> -    if (get_bits_left(gbc) < width + 1) {
>>> +    if (get_bits_left(gbc) < width) {
>>>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
>>>                 "%s: bitstream ended.\n", name);
>>>          return AVERROR_INVALIDDATA;
>>>      }
>>>  
>>> -    magnitude = get_bits(gbc, width);
>>> -    sign      = get_bits1(gbc);
>>> -    value     = sign ? -(int32_t)magnitude : magnitude;
>>> +    value = get_sbits(gbc, width);
>>>  
>>>      if (ctx->trace_enable) {
>>>          char bits[33];
>>>          int i;
>>>          for (i = 0; i < width; i++)
>>> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
>>> -        bits[i] = sign ? '1' : '0';
>>> -        bits[i + 1] = 0;
>>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
>>
>> Not sure if we care, but right-shifting a negative value is implementation-defined.
> 
> Is casting value to unsigned enough, or should i use something like
> zero_extend()?

I think using left-shift instead is safe:

value & 1 << (...) ? '1' : '0'
James Almer Nov. 14, 2018, 11:55 p.m.
On 11/14/2018 8:04 PM, Mark Thompson wrote:
> On 14/11/18 22:39, James Almer wrote:
>> On 11/14/2018 7:24 PM, Mark Thompson wrote:
>>> On 11/11/18 02:24, James Almer wrote:
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> See https://0x0.st/sljR.webm
>>>>
>>>>  libavcodec/cbs_av1.c | 30 +++++++++---------------------
>>>>  1 file changed, 9 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>>>> index ff32a6fca5..215f9384e8 100644
>>>> --- a/libavcodec/cbs_av1.c
>>>> +++ b/libavcodec/cbs_av1.c
>>>> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
>>>>                             int width, const char *name,
>>>>                             const int *subscripts, int32_t *write_to)
>>>>  {
>>>> -    uint32_t magnitude;
>>>> -    int position, sign;
>>>> +    int position;
>>>>      int32_t value;
>>>>  
>>>>      if (ctx->trace_enable)
>>>>          position = get_bits_count(gbc);
>>>>  
>>>> -    if (get_bits_left(gbc) < width + 1) {
>>>> +    if (get_bits_left(gbc) < width) {
>>>>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
>>>>                 "%s: bitstream ended.\n", name);
>>>>          return AVERROR_INVALIDDATA;
>>>>      }
>>>>  
>>>> -    magnitude = get_bits(gbc, width);
>>>> -    sign      = get_bits1(gbc);
>>>> -    value     = sign ? -(int32_t)magnitude : magnitude;
>>>> +    value = get_sbits(gbc, width);
>>>>  
>>>>      if (ctx->trace_enable) {
>>>>          char bits[33];
>>>>          int i;
>>>>          for (i = 0; i < width; i++)
>>>> -            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
>>>> -        bits[i] = sign ? '1' : '0';
>>>> -        bits[i + 1] = 0;
>>>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
>>>
>>> Not sure if we care, but right-shifting a negative value is implementation-defined.
>>
>> Is casting value to unsigned enough, or should i use something like
>> zero_extend()?
> 
> I think using left-shift instead is safe:
> 
> value & 1 << (...) ? '1' : '0'

Changed and both patches applied and backported. Thanks!

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index ff32a6fca5..215f9384e8 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -189,30 +189,26 @@  static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc,
                            int width, const char *name,
                            const int *subscripts, int32_t *write_to)
 {
-    uint32_t magnitude;
-    int position, sign;
+    int position;
     int32_t value;
 
     if (ctx->trace_enable)
         position = get_bits_count(gbc);
 
-    if (get_bits_left(gbc) < width + 1) {
+    if (get_bits_left(gbc) < width) {
         av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
                "%s: bitstream ended.\n", name);
         return AVERROR_INVALIDDATA;
     }
 
-    magnitude = get_bits(gbc, width);
-    sign      = get_bits1(gbc);
-    value     = sign ? -(int32_t)magnitude : magnitude;
+    value = get_sbits(gbc, width);
 
     if (ctx->trace_enable) {
         char bits[33];
         int i;
         for (i = 0; i < width; i++)
-            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = sign ? '1' : '0';
-        bits[i + 1] = 0;
+            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
+        bits[i] = 0;
 
         ff_cbs_trace_syntax_element(ctx, position,
                                     name, subscripts, bits, value);
@@ -226,29 +222,21 @@  static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc,
                             int width, const char *name,
                             const int *subscripts, int32_t value)
 {
-    uint32_t magnitude;
-    int sign;
-
-    if (put_bits_left(pbc) < width + 1)
+    if (put_bits_left(pbc) < width)
         return AVERROR(ENOSPC);
 
-    sign      = value < 0;
-    magnitude = sign ? -value : value;
-
     if (ctx->trace_enable) {
         char bits[33];
         int i;
         for (i = 0; i < width; i++)
-            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = sign ? '1' : '0';
-        bits[i + 1] = 0;
+            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
+        bits[i] = 0;
 
         ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
                                     name, subscripts, bits, value);
     }
 
-    put_bits(pbc, width, magnitude);
-    put_bits(pbc, 1, sign);
+    put_sbits(pbc, width, value);
 
     return 0;
 }