[FFmpeg-devel] [PATCH 1/5] avcodec/cbs: add helper functions and macros to read and write signed values

[Mark Thompson]

On 16/04/2019 23:54, James Almer wrote:
> On 4/16/2019 7:45 PM, Mark Thompson wrote:
>> On 15/04/2019 22:17, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavcodec/cbs.c          | 79 +++++++++++++++++++++++++++++++++++++++
>>>  libavcodec/cbs_internal.h | 20 +++++++++-
>>>  2 files changed, 98 insertions(+), 1 deletion(-)
>>
>> Looks like a sensible addition, some comments below.
>>
>>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>>> index c388be896b..726bd582f5 100644
>>> --- a/libavcodec/cbs.c
>>> +++ b/libavcodec/cbs.c
>>> @@ -504,6 +504,85 @@ int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>      return 0;
>>>  }
>>>  
>>> +int ff_cbs_read_signed(CodedBitstreamContext *ctx, GetBitContext *gbc,
>>> +                       int width, const char *name,
>>> +                       const int *subscripts, int32_t *write_to,
>>> +                       int32_t range_min, int32_t range_max)
>>> +{
>>> +    int32_t value;
>>> +    int position;
>>> +
>>> +    av_assert0(width > 0 && width <= 32);
>>> +
>>> +    if (get_bits_left(gbc) < width) {
>>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid value at "
>>> +               "%s: bitstream ended.\n", name);
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>> +    if (ctx->trace_enable)
>>> +        position = get_bits_count(gbc);
>>> +
>>> +    value = get_sbits_long(gbc, width);
>>> +
>>> +    if (ctx->trace_enable) {
>>> +        char bits[33];
>>> +        int i;
>>> +        for (i = 0; i < width; i++)
>>> +            bits[i] = value & (1 << (width - i - 1)) ? '1' : '0';
>>
>> 1 << 31 is undefined behaviour for 32-bit int.
>>
>> The unsigned versions are careful to right-shift unsigned values to avoid overflow problems; a possible fix might be to strip the sign bit and then do the same thing.
> 
> Would "1U << (width - i - 1)" be enough?

Probably?  A negative value would be promoted to unsigned as positive 1111xxxx, which I think does the right thing.

>>> +        bits[i] = 0;
>>> +
>>> +        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
>>> +                                    bits, value);
>>> +    }
>>> +
>>> +    if (value < range_min || value > range_max) {
>>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
>>> +               "%"PRId32", but must be in [%"PRId32",%"PRId32"].\n",
>>> +               name, value, range_min, range_max);
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>> +    *write_to = value;
>>> +    return 0;
>>> +}

- Mark