On 4/16/2019 7:45 PM, Mark Thompson wrote:
> On 15/04/2019 22:17, James Almer wrote:
>> Signed-off-by: James Almer <jamr...@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
