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

James Almer jamrial at gmail.com
Thu Nov 15 00:39:33 EET 2018


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 at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 



More information about the ffmpeg-devel mailing list