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

Submitted by James Almer on April 15, 2019, 9:17 p.m.

Details

Message ID 20190415211734.3968-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 15, 2019, 9:17 p.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs.c          | 79 +++++++++++++++++++++++++++++++++++++++
 libavcodec/cbs_internal.h | 20 +++++++++-
 2 files changed, 98 insertions(+), 1 deletion(-)

Comments

Mark Thompson April 16, 2019, 10:45 p.m.
On 15/04/2019 22:17, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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
James Almer April 16, 2019, 10:54 p.m.
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@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-request@ffmpeg.org with subject "unsubscribe".
>
Mark Thompson April 16, 2019, 11:27 p.m.
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@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
James Almer April 16, 2019, 11:47 p.m.
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@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!

Patch hide | download patch | download mbox

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';
+        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';
+        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)
+
+// 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)))
+
 
 extern const CodedBitstreamType ff_cbs_type_av1;
 extern const CodedBitstreamType ff_cbs_type_h264;