| Message ID | 259cfc93-3e34-4081-8640-82890edbf76a@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] cbs_av1: Reject thirty-two zero bits in uvlc code | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/configure_x86 | warning | Failed to apply patch |
On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote: > The spec allows at least thirty-two zero bits followed by a one to mean > 2^32-1, with no constraint on the number of zeroes. The libaom > reference decoder does not match this, instead reading thirty-two zeroes > but not the following one to mean 2^32-1. These two interpretations are > incompatible and other implementations may follow one or the other. > Therefore reject thirty-two zeroes because the intended behaviour is not > clear. > --- > libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping at thirty-two zeroes and not reading the one. gav1 just rejects thirty-two zeroes. I would suggest to contact the authors of the spec to bring this discrepancy to their attention, unless this is already known and unless this sequence is declared invalid by some other part of the spec (which would make the discrepancy only occur in invalid sequences) thx [...]
On 25/10/2023 21:55, Michael Niedermayer wrote: > On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote: >> The spec allows at least thirty-two zero bits followed by a one to mean >> 2^32-1, with no constraint on the number of zeroes. The libaom >> reference decoder does not match this, instead reading thirty-two zeroes >> but not the following one to mean 2^32-1. These two interpretations are >> incompatible and other implementations may follow one or the other. >> Therefore reject thirty-two zeroes because the intended behaviour is not >> clear. >> --- >> libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping at thirty-two zeroes and not reading the one. gav1 just rejects thirty-two zeroes. > > I would suggest to contact the authors of the spec to bring this discrepancy > to their attention, unless this is already known and > unless this sequence is declared invalid by some other part of the spec > (which would make the discrepancy only occur in invalid sequences) <https://github.com/AOMediaCodec/av1-spec/pull/343> Since this only occurs in nonconforming streams, fixing the spec seems like the better option. Thanks, - Mark
Hi On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote: > The spec allows at least thirty-two zero bits followed by a one to mean > 2^32-1, with no constraint on the number of zeroes. The libaom > reference decoder does not match this, instead reading thirty-two zeroes > but not the following one to mean 2^32-1. These two interpretations are > incompatible and other implementations may follow one or the other. > Therefore reject thirty-two zeroes because the intended behaviour is not > clear. > --- > libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping at thirty-two zeroes and not reading the one. gav1 just rejects thirty-two zeroes. > > This is also a source of arbitrarily large single syntax elements to hit <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-October/315973.html>. > > libavcodec/cbs_av1.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index 1d9ac5ab44..13c749a25b 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc, > CBS_TRACE_READ_START(); > > zeroes = 0; > - while (1) { > + while (zeroes < 32) { what happened with this patch ? the git master code still aborts thx [...]
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index 1d9ac5ab44..13c749a25b 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc, CBS_TRACE_READ_START(); zeroes = 0; - while (1) { + while (zeroes < 32) { if (get_bits_left(gbc) < 1) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at " "%s: bitstream ended.\n", name); @@ -49,10 +49,18 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc, } if (zeroes >= 32) { - // Note that the spec allows an arbitrarily large number of - // zero bits followed by a one bit in this case, but the - // libaom implementation does not support it. - value = MAX_UINT_BITS(32); + // The spec allows at least thirty-two zero bits followed by a + // one to mean 2^32-1, with no constraint on the number of + // zeroes. The libaom reference decoder does not match this, + // instead reading thirty-two zeroes but not the following one + // to mean 2^32-1. These two interpretations are incompatible + // and other implementations may follow one or the other. + // Therefore we reject thirty-two zeroes because the intended + // behaviour is not clear. + av_log(ctx->log_ctx, AV_LOG_ERROR, "Thirty-two zero bits in " + "%s uvlc code: considered invalid due to conflicting " + "standard and reference decoder behaviour.\n", name); + return AVERROR_INVALIDDATA; } else { if (get_bits_left(gbc) < zeroes) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at "