| Message ID | 20181111022414.10964-2-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 11/10/2018 11:24 PM, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > See https://0x0.st/sljR.webm > > libavcodec/cbs_av1.c | 30 +++++++++--------------------- > 1 file changed, 9 insertions(+), 21 deletions(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index ff32a6fca5..215f9384e8 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, > int width, const char *name, > const int *subscripts, int32_t *write_to) > { > - uint32_t magnitude; > - int position, sign; > + int position; > int32_t value; > > if (ctx->trace_enable) > position = get_bits_count(gbc); > > - if (get_bits_left(gbc) < width + 1) { > + if (get_bits_left(gbc) < width) { > av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " > "%s: bitstream ended.\n", name); > return AVERROR_INVALIDDATA; > } > > - magnitude = get_bits(gbc, width); > - sign = get_bits1(gbc); > - value = sign ? -(int32_t)magnitude : magnitude; > + value = get_sbits(gbc, width); > > if (ctx->trace_enable) { > char bits[33]; > int i; > for (i = 0; i < width; i++) > - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; > - bits[i] = sign ? '1' : '0'; > - bits[i + 1] = 0; > + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; > + bits[i] = 0; > > ff_cbs_trace_syntax_element(ctx, position, > name, subscripts, bits, value); > @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc, > int width, const char *name, > const int *subscripts, int32_t value) > { > - uint32_t magnitude; > - int sign; > - > - if (put_bits_left(pbc) < width + 1) > + if (put_bits_left(pbc) < width) > return AVERROR(ENOSPC); > > - sign = value < 0; > - magnitude = sign ? -value : value; > - > if (ctx->trace_enable) { > char bits[33]; > int i; > for (i = 0; i < width; i++) > - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; > - bits[i] = sign ? '1' : '0'; > - bits[i + 1] = 0; > + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; > + bits[i] = 0; > > ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc), > name, subscripts, bits, value); > } > > - put_bits(pbc, width, magnitude); > - put_bits(pbc, 1, sign); > + put_sbits(pbc, width, value); > > return 0; > } Ping. Some real world samples depend on this.
On 11/11/18 02:24, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > See https://0x0.st/sljR.webm > > libavcodec/cbs_av1.c | 30 +++++++++--------------------- > 1 file changed, 9 insertions(+), 21 deletions(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index ff32a6fca5..215f9384e8 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, > int width, const char *name, > const int *subscripts, int32_t *write_to) > { > - uint32_t magnitude; > - int position, sign; > + int position; > int32_t value; > > if (ctx->trace_enable) > position = get_bits_count(gbc); > > - if (get_bits_left(gbc) < width + 1) { > + if (get_bits_left(gbc) < width) { > av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " > "%s: bitstream ended.\n", name); > return AVERROR_INVALIDDATA; > } > > - magnitude = get_bits(gbc, width); > - sign = get_bits1(gbc); > - value = sign ? -(int32_t)magnitude : magnitude; > + value = get_sbits(gbc, width); > > if (ctx->trace_enable) { > char bits[33]; > int i; > for (i = 0; i < width; i++) > - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; > - bits[i] = sign ? '1' : '0'; > - bits[i + 1] = 0; > + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; Not sure if we care, but right-shifting a negative value is implementation-defined. > + bits[i] = 0; > > ff_cbs_trace_syntax_element(ctx, position, > name, subscripts, bits, value); > @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc, > int width, const char *name, > const int *subscripts, int32_t value) > { > - uint32_t magnitude; > - int sign; > - > - if (put_bits_left(pbc) < width + 1) > + if (put_bits_left(pbc) < width) > return AVERROR(ENOSPC); > > - sign = value < 0; > - magnitude = sign ? -value : value; > - > if (ctx->trace_enable) { > char bits[33]; > int i; > for (i = 0; i < width; i++) > - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; > - bits[i] = sign ? '1' : '0'; > - bits[i + 1] = 0; > + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; And here. > + bits[i] = 0; > > ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc), > name, subscripts, bits, value); > } > > - put_bits(pbc, width, magnitude); > - put_bits(pbc, 1, sign); > + put_sbits(pbc, width, value); > > return 0; > } > Otherwise fine. No idea where the version with the sign bit at the other end came from; oh well. Thanks, - Mark
On 11/14/2018 7:24 PM, Mark Thompson wrote: > On 11/11/18 02:24, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> See https://0x0.st/sljR.webm >> >> libavcodec/cbs_av1.c | 30 +++++++++--------------------- >> 1 file changed, 9 insertions(+), 21 deletions(-) >> >> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c >> index ff32a6fca5..215f9384e8 100644 >> --- a/libavcodec/cbs_av1.c >> +++ b/libavcodec/cbs_av1.c >> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, >> int width, const char *name, >> const int *subscripts, int32_t *write_to) >> { >> - uint32_t magnitude; >> - int position, sign; >> + int position; >> int32_t value; >> >> if (ctx->trace_enable) >> position = get_bits_count(gbc); >> >> - if (get_bits_left(gbc) < width + 1) { >> + if (get_bits_left(gbc) < width) { >> av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " >> "%s: bitstream ended.\n", name); >> return AVERROR_INVALIDDATA; >> } >> >> - magnitude = get_bits(gbc, width); >> - sign = get_bits1(gbc); >> - value = sign ? -(int32_t)magnitude : magnitude; >> + value = get_sbits(gbc, width); >> >> if (ctx->trace_enable) { >> char bits[33]; >> int i; >> for (i = 0; i < width; i++) >> - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; >> - bits[i] = sign ? '1' : '0'; >> - bits[i + 1] = 0; >> + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; > > Not sure if we care, but right-shifting a negative value is implementation-defined. Is casting value to unsigned enough, or should i use something like zero_extend()? > >> + bits[i] = 0; >> >> ff_cbs_trace_syntax_element(ctx, position, >> name, subscripts, bits, value); >> @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc, >> int width, const char *name, >> const int *subscripts, int32_t value) >> { >> - uint32_t magnitude; >> - int sign; >> - >> - if (put_bits_left(pbc) < width + 1) >> + if (put_bits_left(pbc) < width) >> return AVERROR(ENOSPC); >> >> - sign = value < 0; >> - magnitude = sign ? -value : value; >> - >> if (ctx->trace_enable) { >> char bits[33]; >> int i; >> for (i = 0; i < width; i++) >> - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; >> - bits[i] = sign ? '1' : '0'; >> - bits[i + 1] = 0; >> + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; > > And here. > >> + bits[i] = 0; >> >> ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc), >> name, subscripts, bits, value); >> } >> >> - put_bits(pbc, width, magnitude); >> - put_bits(pbc, 1, sign); >> + put_sbits(pbc, width, value); >> >> return 0; >> } >> > > Otherwise fine. No idea where the version with the sign bit at the other end came from; oh well. From a lack of samples to test and confirm that the code works :p > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 14/11/18 22:39, James Almer wrote: > On 11/14/2018 7:24 PM, Mark Thompson wrote: >> On 11/11/18 02:24, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> See https://0x0.st/sljR.webm >>> >>> libavcodec/cbs_av1.c | 30 +++++++++--------------------- >>> 1 file changed, 9 insertions(+), 21 deletions(-) >>> >>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c >>> index ff32a6fca5..215f9384e8 100644 >>> --- a/libavcodec/cbs_av1.c >>> +++ b/libavcodec/cbs_av1.c >>> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, >>> int width, const char *name, >>> const int *subscripts, int32_t *write_to) >>> { >>> - uint32_t magnitude; >>> - int position, sign; >>> + int position; >>> int32_t value; >>> >>> if (ctx->trace_enable) >>> position = get_bits_count(gbc); >>> >>> - if (get_bits_left(gbc) < width + 1) { >>> + if (get_bits_left(gbc) < width) { >>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " >>> "%s: bitstream ended.\n", name); >>> return AVERROR_INVALIDDATA; >>> } >>> >>> - magnitude = get_bits(gbc, width); >>> - sign = get_bits1(gbc); >>> - value = sign ? -(int32_t)magnitude : magnitude; >>> + value = get_sbits(gbc, width); >>> >>> if (ctx->trace_enable) { >>> char bits[33]; >>> int i; >>> for (i = 0; i < width; i++) >>> - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; >>> - bits[i] = sign ? '1' : '0'; >>> - bits[i + 1] = 0; >>> + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; >> >> Not sure if we care, but right-shifting a negative value is implementation-defined. > > Is casting value to unsigned enough, or should i use something like > zero_extend()? I think using left-shift instead is safe: value & 1 << (...) ? '1' : '0'
On 11/14/2018 8:04 PM, Mark Thompson wrote: > On 14/11/18 22:39, James Almer wrote: >> On 11/14/2018 7:24 PM, Mark Thompson wrote: >>> On 11/11/18 02:24, James Almer wrote: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> See https://0x0.st/sljR.webm >>>> >>>> libavcodec/cbs_av1.c | 30 +++++++++--------------------- >>>> 1 file changed, 9 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c >>>> index ff32a6fca5..215f9384e8 100644 >>>> --- a/libavcodec/cbs_av1.c >>>> +++ b/libavcodec/cbs_av1.c >>>> @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, >>>> int width, const char *name, >>>> const int *subscripts, int32_t *write_to) >>>> { >>>> - uint32_t magnitude; >>>> - int position, sign; >>>> + int position; >>>> int32_t value; >>>> >>>> if (ctx->trace_enable) >>>> position = get_bits_count(gbc); >>>> >>>> - if (get_bits_left(gbc) < width + 1) { >>>> + if (get_bits_left(gbc) < width) { >>>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " >>>> "%s: bitstream ended.\n", name); >>>> return AVERROR_INVALIDDATA; >>>> } >>>> >>>> - magnitude = get_bits(gbc, width); >>>> - sign = get_bits1(gbc); >>>> - value = sign ? -(int32_t)magnitude : magnitude; >>>> + value = get_sbits(gbc, width); >>>> >>>> if (ctx->trace_enable) { >>>> char bits[33]; >>>> int i; >>>> for (i = 0; i < width; i++) >>>> - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; >>>> - bits[i] = sign ? '1' : '0'; >>>> - bits[i + 1] = 0; >>>> + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; >>> >>> Not sure if we care, but right-shifting a negative value is implementation-defined. >> >> Is casting value to unsigned enough, or should i use something like >> zero_extend()? > > I think using left-shift instead is safe: > > value & 1 << (...) ? '1' : '0' Changed and both patches applied and backported. Thanks!
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index ff32a6fca5..215f9384e8 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -189,30 +189,26 @@ static int cbs_av1_read_su(CodedBitstreamContext *ctx, GetBitContext *gbc, int width, const char *name, const int *subscripts, int32_t *write_to) { - uint32_t magnitude; - int position, sign; + int position; int32_t value; if (ctx->trace_enable) position = get_bits_count(gbc); - if (get_bits_left(gbc) < width + 1) { + if (get_bits_left(gbc) < width) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at " "%s: bitstream ended.\n", name); return AVERROR_INVALIDDATA; } - magnitude = get_bits(gbc, width); - sign = get_bits1(gbc); - value = sign ? -(int32_t)magnitude : magnitude; + value = get_sbits(gbc, width); if (ctx->trace_enable) { char bits[33]; int i; for (i = 0; i < width; i++) - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; - bits[i] = sign ? '1' : '0'; - bits[i + 1] = 0; + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; + bits[i] = 0; ff_cbs_trace_syntax_element(ctx, position, name, subscripts, bits, value); @@ -226,29 +222,21 @@ static int cbs_av1_write_su(CodedBitstreamContext *ctx, PutBitContext *pbc, int width, const char *name, const int *subscripts, int32_t value) { - uint32_t magnitude; - int sign; - - if (put_bits_left(pbc) < width + 1) + if (put_bits_left(pbc) < width) return AVERROR(ENOSPC); - sign = value < 0; - magnitude = sign ? -value : value; - if (ctx->trace_enable) { char bits[33]; int i; for (i = 0; i < width; i++) - bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0'; - bits[i] = sign ? '1' : '0'; - bits[i + 1] = 0; + bits[i] = value >> (width - i - 1) & 1 ? '1' : '0'; + bits[i] = 0; ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc), name, subscripts, bits, value); } - put_bits(pbc, width, magnitude); - put_bits(pbc, 1, sign); + put_sbits(pbc, width, value); return 0; }
Signed-off-by: James Almer <jamrial@gmail.com> --- See https://0x0.st/sljR.webm libavcodec/cbs_av1.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)