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

[James Almer]

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 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()?
> 
> I think using left-shift instead is safe:
> 
> value & 1 << (...) ? '1' : '0'

Changed and both patches applied and backported. Thanks!