[FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check

[Ronald S. Bultje]

Hi,

On Wed, Nov 16, 2022 at 3:44 AM Hirokazu Honda <hiroh at 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 at 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