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

[James Almer]

On 4/16/2019 8:27 PM, Mark Thompson wrote:
> 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.

Changed and pushed then.

Thanks!