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

[James Almer]

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?

> 
>> +        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;
>> +}
>> +
>> +int ff_cbs_write_signed(CodedBitstreamContext *ctx, PutBitContext *pbc,
>> +                        int width, const char *name,
>> +                        const int *subscripts, int32_t value,
>> +                        int32_t range_min, int32_t range_max)
>> +{
>> +    av_assert0(width > 0 && width <= 32);
>> +
>> +    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;
>> +    }
>> +
>> +    if (put_bits_left(pbc) < width)
>> +        return AVERROR(ENOSPC);
>> +
>> +    if (ctx->trace_enable) {
>> +        char bits[33];
>> +        int i;
>> +        for (i = 0; i < width; i++)
>> +            bits[i] = value & (1 << (width - i - 1)) ? '1' : '0';
> 
> As above.
> 
>> +        bits[i] = 0;
>> +
>> +        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
>> +                                    name, subscripts, bits, value);
>> +    }
>> +
>> +    if (width < 32)
>> +        put_sbits(pbc, width, value);
>> +    else
>> +        put_bits32(pbc, value);
>> +
>> +    return 0;
>> +}
>> +
>>  
>>  int ff_cbs_alloc_unit_content(CodedBitstreamContext *ctx,
>>                                CodedBitstreamUnit *unit,
>> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
>> index 53f2e5d187..6ab85679dd 100644
>> --- a/libavcodec/cbs_internal.h
>> +++ b/libavcodec/cbs_internal.h
>> @@ -81,10 +81,28 @@ int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>                            const int *subscripts, uint32_t value,
>>                            uint32_t range_min, uint32_t range_max);
>>  
>> -// The largest value representable in N bits, suitable for use as
>> +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);
>> +
>> +int ff_cbs_write_signed(CodedBitstreamContext *ctx, PutBitContext *pbc,
>> +                        int width, const char *name,
>> +                        const int *subscripts, int32_t value,
>> +                        int32_t range_min, int32_t range_max);
>> +
>> +// The largest unsigned value representable in N bits, suitable for use as
>>  // range_max in the above functions.
>>  #define MAX_UINT_BITS(length) ((UINT64_C(1) << (length)) - 1)
>>  
>> +// The largest signed value representable in N bits, suitable for use as
>> +// range_max in the above functions.
>> +#define MAX_INT_BITS(length) ((INT64_C(1) << (length - 1)) - 1)
> 
> Not so good for, say, MAX_INT_BITS(a ? b : c).

Changed locally to ((INT64_C(1) << ((length) - 1)) - 1). Same below.

> 
>> +
>> +// The smallest signed value representable in N bits, suitable for use as
>> +// range_min in the above functions.
>> +#define MIN_INT_BITS(length) (-(INT64_C(1) << (length - 1)))
> 
> Same here.
> 
>> +
>>  
>>  extern const CodedBitstreamType ff_cbs_type_av1;
>>  extern const CodedBitstreamType ff_cbs_type_h264;
>>
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>