| Message ID | 20221116084412.2537585-1-hiroh@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vp8: Fix wrong vpx_rac_is_end() check | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi, On Wed, Nov 16, 2022 at 3:44 AM Hirokazu Honda <hiroh@chromium.org> wrote: > The check of vpx_rac_is_end check(s) are added originally from > 1afd246960202917e244c844c534e9c1e3c323f5. It causes a regression > of some vp8 stream. b6b9ac5698c8f911841b469af77199153278c55c fixes > the regression by a sort of band-aid way. This fixes the wrongness > of the original commit. vpx_rac_is_end() should be called against > the bool decoder for the vp8 headr context, not one for each > coefficient. Reference is vp8_dixie_tokens_process_row() in token.c > in spec 20.16. > > Fixes: Ticket 8069 > Fixes: regression of 1afd246960202917e244c844c534e9c1e3c323f5. > Fixes: b6b9ac5698c8f911841b469af77199153278c55c > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > libavcodec/vp8.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c > index 67f36d8933..77f5a6b657 100644 > --- a/libavcodec/vp8.c > +++ b/libavcodec/vp8.c > @@ -2404,7 +2404,8 @@ static av_always_inline int > decode_mb_row_no_filter(AVCodecContext *avctx, void > int num_jobs = s->num_jobs; > const VP8Frame *prev_frame = s->prev_frame; > VP8Frame *curframe = s->curframe; > - VPXRangeCoder *c = &s->coeff_partition[mb_y & > (s->num_coeff_partitions - 1)]; > + VPXRangeCoder *coeff_c = &s->coeff_partition[mb_y & > (s->num_coeff_partitions - 1)]; > + > VP8Macroblock *mb; > uint8_t *dst[3] = { > curframe->tf.f->data[0] + 16 * mb_y * s->linesize, > @@ -2412,7 +2413,7 @@ static av_always_inline int > decode_mb_row_no_filter(AVCodecContext *avctx, void > curframe->tf.f->data[2] + 8 * mb_y * s->uvlinesize > }; > > - if (vpx_rac_is_end(c)) > + if (vpx_rac_is_end(&s->c)) > return AVERROR_INVALIDDATA; > > if (mb_y == 0) > @@ -2443,7 +2444,7 @@ static av_always_inline int > decode_mb_row_no_filter(AVCodecContext *avctx, void > td->mv_bounds.mv_max.x = ((s->mb_width - 1) << 6) + MARGIN; > > for (mb_x = 0; mb_x < s->mb_width; mb_x++, mb_xy++, mb++) { > - if (vpx_rac_is_end(c)) > + if (vpx_rac_is_end(&s->c)) > return AVERROR_INVALIDDATA; > // Wait for previous thread to read mb_x+2, and reach mb_y-1. > if (prev_td != td) { > @@ -2471,7 +2472,7 @@ static av_always_inline int > decode_mb_row_no_filter(AVCodecContext *avctx, void > prefetch_motion(s, mb, mb_x, mb_y, mb_xy, VP8_FRAME_PREVIOUS); > > if (!mb->skip) > - decode_mb_coeffs(s, td, c, mb, s->top_nnz[mb_x], > td->left_nnz, is_vp7); > + decode_mb_coeffs(s, td, coeff_c, mb, s->top_nnz[mb_x], > td->left_nnz, is_vp7); > > if (mb->mode <= MODE_I4x4) > intra_predict(s, td, dst, mb, mb_x, mb_y, is_vp7); > -- > 2.38.1.431.g37b22c650d-goog > Thanks for the fix. So, you're transferring the truncation check from the per-slice coef buffer to the "main" mode/mv buffer. Shouldn't we check both? Or is one of them allowed to be truncated? (That seems unlikely, but that appears to be what you're suggesting here, otherwise there wouldn't be any artifacts.) If that *is* the case, this needs a comment somewhere, otherwise we'll forget this over time. Ronald
Hi, On Wed, Nov 16, 2022 at 8:20 AM Ronald S. Bultje <rsbultje@gmail.com> wrote: > On Wed, Nov 16, 2022 at 3:44 AM Hirokazu Honda <hiroh@chromium.org> wrote: > >> @@ -2471,7 +2472,7 @@ static av_always_inline int >> decode_mb_row_no_filter(AVCodecContext *avctx, void >> prefetch_motion(s, mb, mb_x, mb_y, mb_xy, VP8_FRAME_PREVIOUS); >> >> if (!mb->skip) >> - decode_mb_coeffs(s, td, c, mb, s->top_nnz[mb_x], >> td->left_nnz, is_vp7); >> + decode_mb_coeffs(s, td, coeff_c, mb, s->top_nnz[mb_x], >> td->left_nnz, is_vp7); >> >> if (mb->mode <= MODE_I4x4) >> intra_predict(s, td, dst, mb, mb_x, mb_y, is_vp7); >> -- >> 2.38.1.431.g37b22c650d-goog >> > > Thanks for the fix. So, you're transferring the truncation check from the > per-slice coef buffer to the "main" mode/mv buffer. Shouldn't we check > both? Or is one of them allowed to be truncated? (That seems unlikely, but > that appears to be what you're suggesting here, otherwise there wouldn't be > any artifacts.) > To clarify: I think a vpx_rac_is_end(coeff_c) in the "if (!mb->skip)" branch here would be helpful, I think. Outside, we don't use coeff_c, so no truncation check is needed. Assuming that still fixes the issue, then. Ronald
diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c index 67f36d8933..77f5a6b657 100644 --- a/libavcodec/vp8.c +++ b/libavcodec/vp8.c @@ -2404,7 +2404,8 @@ static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void int num_jobs = s->num_jobs; const VP8Frame *prev_frame = s->prev_frame; VP8Frame *curframe = s->curframe; - VPXRangeCoder *c = &s->coeff_partition[mb_y & (s->num_coeff_partitions - 1)]; + VPXRangeCoder *coeff_c = &s->coeff_partition[mb_y & (s->num_coeff_partitions - 1)]; + VP8Macroblock *mb; uint8_t *dst[3] = { curframe->tf.f->data[0] + 16 * mb_y * s->linesize, @@ -2412,7 +2413,7 @@ static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void curframe->tf.f->data[2] + 8 * mb_y * s->uvlinesize }; - if (vpx_rac_is_end(c)) + if (vpx_rac_is_end(&s->c)) return AVERROR_INVALIDDATA; if (mb_y == 0) @@ -2443,7 +2444,7 @@ static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void td->mv_bounds.mv_max.x = ((s->mb_width - 1) << 6) + MARGIN; for (mb_x = 0; mb_x < s->mb_width; mb_x++, mb_xy++, mb++) { - if (vpx_rac_is_end(c)) + if (vpx_rac_is_end(&s->c)) return AVERROR_INVALIDDATA; // Wait for previous thread to read mb_x+2, and reach mb_y-1. if (prev_td != td) { @@ -2471,7 +2472,7 @@ static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void prefetch_motion(s, mb, mb_x, mb_y, mb_xy, VP8_FRAME_PREVIOUS); if (!mb->skip) - decode_mb_coeffs(s, td, c, mb, s->top_nnz[mb_x], td->left_nnz, is_vp7); + decode_mb_coeffs(s, td, coeff_c, mb, s->top_nnz[mb_x], td->left_nnz, is_vp7); if (mb->mode <= MODE_I4x4) intra_predict(s, td, dst, mb, mb_x, mb_y, is_vp7);
The check of vpx_rac_is_end check(s) are added originally from 1afd246960202917e244c844c534e9c1e3c323f5. It causes a regression of some vp8 stream. b6b9ac5698c8f911841b469af77199153278c55c fixes the regression by a sort of band-aid way. This fixes the wrongness of the original commit. vpx_rac_is_end() should be called against the bool decoder for the vp8 headr context, not one for each coefficient. Reference is vp8_dixie_tokens_process_row() in token.c in spec 20.16. Fixes: Ticket 8069 Fixes: regression of 1afd246960202917e244c844c534e9c1e3c323f5. Fixes: b6b9ac5698c8f911841b469af77199153278c55c Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- libavcodec/vp8.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)