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

[Hirokazu Honda]

Hi Ronald,

I am sorry for the late reply.
The bool decoder for the vp8 headr context can have already reached
the end of buffer when it is no longer used.
We should not check it here.
Adding the check for coeff_c there sounds good to me because coeff_c
must not have reached the end of the buffer in calling
decode_mb_coeffs().
And thanks for merging!

Thanks,
-Hiro
On Sun, Nov 20, 2022 at 12:55 AM Ronald S. Bultje <rsbultje at gmail.com> wrote:
>
> Hi,
>
> On Fri, Nov 18, 2022 at 8:37 AM Ronald S. Bultje <rsbultje at gmail.com> wrote:
>>
>> From: Hirokazu Honda <hiroh at chromium.org>
>>
>> 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
>>
>> Co-authored-by: Ronald S. Bultje <rsbultje at gmail.com>
>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>> Signed-off-by: Ronald S. Bultje <rsbultje at gmail.com>
>> ---
>>  libavcodec/vp8.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
>> index 67f36d8933..db2419deaf 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) {
>> @@ -2470,8 +2471,11 @@ 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);
>> +        if (!mb->skip) {
>> +            if (vpx_rac_is_end(coeff_c))
>> +                return AVERROR_INVALIDDATA;
>> +            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
>
>
> Pushed.
>
> Ronald