| Message ID | 20190415211734.3968-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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". >
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
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!
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;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs.c | 79 +++++++++++++++++++++++++++++++++++++++ libavcodec/cbs_internal.h | 20 +++++++++- 2 files changed, 98 insertions(+), 1 deletion(-)