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

[Mark Thompson]

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.

> +        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).

> +
> +// 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