* [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
@ 2022-11-16 8:44 Hirokazu Honda
2022-11-16 13:20 ` Ronald S. Bultje
2022-11-18 13:28 ` Ronald S. Bultje
0 siblings, 2 replies; 7+ messages in thread
From: Hirokazu Honda @ 2022-11-16 8:44 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Hirokazu Honda
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-16 8:44 [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check Hirokazu Honda
@ 2022-11-16 13:20 ` Ronald S. Bultje
2022-11-16 13:22 ` Ronald S. Bultje
2022-11-18 13:28 ` Ronald S. Bultje
1 sibling, 1 reply; 7+ messages in thread
From: Ronald S. Bultje @ 2022-11-16 13:20 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Hirokazu Honda
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-16 13:20 ` Ronald S. Bultje
@ 2022-11-16 13:22 ` Ronald S. Bultje
0 siblings, 0 replies; 7+ messages in thread
From: Ronald S. Bultje @ 2022-11-16 13:22 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Hirokazu Honda
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread
* [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-16 8:44 [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check Hirokazu Honda
2022-11-16 13:20 ` Ronald S. Bultje
@ 2022-11-18 13:28 ` Ronald S. Bultje
2022-11-18 13:37 ` Ronald S. Bultje
1 sibling, 1 reply; 7+ messages in thread
From: Ronald S. Bultje @ 2022-11-18 13:28 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Ronald S . Bultje, Hirokazu Honda
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
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread* [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-18 13:28 ` Ronald S. Bultje
@ 2022-11-18 13:37 ` Ronald S. Bultje
2022-11-19 15:55 ` Ronald S. Bultje
0 siblings, 1 reply; 7+ messages in thread
From: Ronald S. Bultje @ 2022-11-18 13:37 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Ronald S . Bultje, Hirokazu Honda
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-18 13:37 ` Ronald S. Bultje
@ 2022-11-19 15:55 ` Ronald S. Bultje
2022-11-21 10:49 ` Hirokazu Honda
0 siblings, 1 reply; 7+ messages in thread
From: Ronald S. Bultje @ 2022-11-19 15:55 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Hirokazu Honda
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check
2022-11-19 15:55 ` Ronald S. Bultje
@ 2022-11-21 10:49 ` Hirokazu Honda
0 siblings, 0 replies; 7+ messages in thread
From: Hirokazu Honda @ 2022-11-21 10:49 UTC (permalink / raw)
To: Ronald S. Bultje; +Cc: ffmpeg-devel
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-21 10:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-16 8:44 [FFmpeg-devel] [PATCH] avcodec/vp8: Fix wrong vpx_rac_is_end() check Hirokazu Honda
2022-11-16 13:20 ` Ronald S. Bultje
2022-11-16 13:22 ` Ronald S. Bultje
2022-11-18 13:28 ` Ronald S. Bultje
2022-11-18 13:37 ` Ronald S. Bultje
2022-11-19 15:55 ` Ronald S. Bultje
2022-11-21 10:49 ` Hirokazu Honda
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git