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

From: Hirokazu Honda <hiroh@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@gmail.com>
Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
---
 libavcodec/vp8.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Hi,

On Fri, Nov 18, 2022 at 8:37 AM Ronald S. Bultje <rsbultje@gmail.com> wrote:

> From: Hirokazu Honda <hiroh@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@gmail.com>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Ronald S. Bultje <rsbultje@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

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@gmail.com> wrote:
>
> Hi,
>
> On Fri, Nov 18, 2022 at 8:37 AM Ronald S. Bultje <rsbultje@gmail.com> wrote:
>>
>> From: Hirokazu Honda <hiroh@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@gmail.com>
>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>> Signed-off-by: Ronald S. Bultje <rsbultje@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