[FFmpeg-devel,3/4,v2] avcodec/vc1: rewrite vc1_decode_i_blocks to align with VC-1 spec

Submitted by Jerome Borsboom on June 8, 2018, 9:01 a.m.

Details

Message ID b721e88b-c384-e74b-715f-49d748948c7a@carpalis.nl
State New
Headers show

Commit Message

Jerome Borsboom June 8, 2018, 9:01 a.m.
Change vc1_decode_i_blocks to use vc1_put_blocks_clamped and
ff_vc1_i_loop_filter.

Signed-off-by: Jerome Borsboom <jerome.borsboom@carpalis.nl>
---
 libavcodec/vc1_block.c | 77 +++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

Comments

Michael Niedermayer June 9, 2018, 5:26 p.m.
On Fri, Jun 08, 2018 at 11:01:30AM +0200, Jerome Borsboom wrote:
> Change vc1_decode_i_blocks to use vc1_put_blocks_clamped and
> ff_vc1_i_loop_filter.
> 
> Signed-off-by: Jerome Borsboom <jerome.borsboom@carpalis.nl>
> ---
>  libavcodec/vc1_block.c | 77 +++++++++++++++++++-------------------------------
>  1 file changed, 29 insertions(+), 48 deletions(-)

crashes:

[vc1 @ 0x11b77200] Slice header damaged
==2065==    at 0x120D69C: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4550)
==2065==    by 0x120E11C: av_log_default_callback (log.c:351)
==2065==    by 0x120E2BB: av_vlog (log.c:377)
==2065==    by 0x120E27B: av_log (log.c:369)
==2065==    by 0xC3B5B5: vc1_decode_frame (vc1dec.c:1014)
==2065==    by 0x8A6BAB: decode_simple_internal (decode.c:398)
==2065==    by 0x8A7832: decode_simple_receive_frame (decode.c:594)
==2065==    by 0x8A78FD: decode_receive_frame_internal (decode.c:612)
==2065==    by 0x8A7B75: avcodec_send_packet (decode.c:674)
==2065==    by 0x43373E: decode (ffmpeg.c:2238)
==2065==    by 0x433F98: decode_video (ffmpeg.c:2382)
==2065==    by 0x434FE0: process_input_packet (ffmpeg.c:2623)
==2065==    by 0x43C207: process_input (ffmpeg.c:4461)
==2065==    by 0x43C7B0: transcode_step (ffmpeg.c:4581)
==2065==    by 0x43C92C: transcode (ffmpeg.c:4635)
==2065==    by 0x43D199: main (ffmpeg.c:4842)
==2065== Invalid read of size 8
==2065==    at 0xD790BD: ??? (libavcodec/x86/vc1dsp_loopfilter.asm:302)
==2065==    by 0xC26AA6: vc1_i_v_loop_filter (vc1_loopfilter.c:239)
==2065==    by 0xC26BFB: ff_vc1_i_loop_filter (vc1_loopfilter.c:266)
==2065==    by 0xC23C72: vc1_decode_i_blocks (vc1_block.c:2594)
==2065==    by 0xC259D4: ff_vc1_decode_blocks (vc1_block.c:2969)
==2065==    by 0xC3B7D6: vc1_decode_frame (vc1dec.c:1042)
==2065==    by 0x8A6BAB: decode_simple_internal (decode.c:398)
==2065==    by 0x8A7832: decode_simple_receive_frame (decode.c:594)
==2065==    by 0x8A78FD: decode_receive_frame_internal (decode.c:612)
==2065==    by 0x8A7B75: avcodec_send_packet (decode.c:674)
==2065==    by 0x43373E: decode (ffmpeg.c:2238)
==2065==    by 0x433F98: decode_video (ffmpeg.c:2382)
==2065==    by 0x434FE0: process_input_packet (ffmpeg.c:2623)
==2065==    by 0x43C207: process_input (ffmpeg.c:4461)
==2065==    by 0x43C7B0: transcode_step (ffmpeg.c:4581)
==2065==    by 0x43C92C: transcode (ffmpeg.c:4635)
==2065==    by 0x43D199: main (ffmpeg.c:4842)
==2065==  Address 0x11f25100 is not stack'd, malloc'd or (recently) free'd

....

==2065== Process terminating with default action of signal 11 (SIGSEGV)
==2065==  General Protection Fault
==2065==    at 0x13786A6: ??? (in ffmpeg/ffmpeg_g)
==2065==    by 0x11F8D78: av_buffer_pool_uninit (buffer.c:285)
==2065==    by 0xBF5F7B: avcodec_close (utils.c:1089)
==2065==    by 0x43CCB5: transcode (ffmpeg.c:4697)
==2065==    by 0x43D199: main (ffmpeg.c:4842)

[...]
Jerome Borsboom June 10, 2018, 8:18 a.m.
Thank you for the rigorous testing of my patches. I try to be careful
when changing things, but every now and then I unwittingly may break
things that do not surface in setup.

There is something strange with the backtraces. The 'Slice header
damaged' error indicates that the source file is coded with slices.
Slices, however, only occur in VC-1 advanced profile. The backtrace with
the invalid read contains vc1_decode_i_blocks which is only used in
Simple and Main profile. I currently do not see how this adds up.

Is this crash related to a specific source file? If so, could you share it?


Regards,
Jerome

> 
> crashes:
> 
> [vc1 @ 0x11b77200] Slice header damaged
> ==2065==    at 0x120D69C: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4550)
> ==2065==    by 0x120E11C: av_log_default_callback (log.c:351)
> ==2065==    by 0x120E2BB: av_vlog (log.c:377)
> ==2065==    by 0x120E27B: av_log (log.c:369)
> ==2065==    by 0xC3B5B5: vc1_decode_frame (vc1dec.c:1014)
> ==2065==    by 0x8A6BAB: decode_simple_internal (decode.c:398)
> ==2065==    by 0x8A7832: decode_simple_receive_frame (decode.c:594)
> ==2065==    by 0x8A78FD: decode_receive_frame_internal (decode.c:612)
> ==2065==    by 0x8A7B75: avcodec_send_packet (decode.c:674)
> ==2065==    by 0x43373E: decode (ffmpeg.c:2238)
> ==2065==    by 0x433F98: decode_video (ffmpeg.c:2382)
> ==2065==    by 0x434FE0: process_input_packet (ffmpeg.c:2623)
> ==2065==    by 0x43C207: process_input (ffmpeg.c:4461)
> ==2065==    by 0x43C7B0: transcode_step (ffmpeg.c:4581)
> ==2065==    by 0x43C92C: transcode (ffmpeg.c:4635)
> ==2065==    by 0x43D199: main (ffmpeg.c:4842)
> ==2065== Invalid read of size 8
> ==2065==    at 0xD790BD: ??? (libavcodec/x86/vc1dsp_loopfilter.asm:302)
> ==2065==    by 0xC26AA6: vc1_i_v_loop_filter (vc1_loopfilter.c:239)
> ==2065==    by 0xC26BFB: ff_vc1_i_loop_filter (vc1_loopfilter.c:266)
> ==2065==    by 0xC23C72: vc1_decode_i_blocks (vc1_block.c:2594)
> ==2065==    by 0xC259D4: ff_vc1_decode_blocks (vc1_block.c:2969)
> ==2065==    by 0xC3B7D6: vc1_decode_frame (vc1dec.c:1042)
> ==2065==    by 0x8A6BAB: decode_simple_internal (decode.c:398)
> ==2065==    by 0x8A7832: decode_simple_receive_frame (decode.c:594)
> ==2065==    by 0x8A78FD: decode_receive_frame_internal (decode.c:612)
> ==2065==    by 0x8A7B75: avcodec_send_packet (decode.c:674)
> ==2065==    by 0x43373E: decode (ffmpeg.c:2238)
> ==2065==    by 0x433F98: decode_video (ffmpeg.c:2382)
> ==2065==    by 0x434FE0: process_input_packet (ffmpeg.c:2623)
> ==2065==    by 0x43C207: process_input (ffmpeg.c:4461)
> ==2065==    by 0x43C7B0: transcode_step (ffmpeg.c:4581)
> ==2065==    by 0x43C92C: transcode (ffmpeg.c:4635)
> ==2065==    by 0x43D199: main (ffmpeg.c:4842)
> ==2065==  Address 0x11f25100 is not stack'd, malloc'd or (recently) free'd
> 
> ....
> 
> ==2065== Process terminating with default action of signal 11 (SIGSEGV)
> ==2065==  General Protection Fault
> ==2065==    at 0x13786A6: ??? (in ffmpeg/ffmpeg_g)
> ==2065==    by 0x11F8D78: av_buffer_pool_uninit (buffer.c:285)
> ==2065==    by 0xBF5F7B: avcodec_close (utils.c:1089)
> ==2065==    by 0x43CCB5: transcode (ffmpeg.c:4697)
> ==2065==    by 0x43D199: main (ffmpeg.c:4842)
> 
> [...]
Michael Niedermayer June 11, 2018, 12:03 a.m.
On Sun, Jun 10, 2018 at 10:18:45AM +0200, Jerome Borsboom wrote:
> Thank you for the rigorous testing of my patches. I try to be careful
> when changing things, but every now and then I unwittingly may break
> things that do not surface in setup.
> 
> There is something strange with the backtraces. The 'Slice header
> damaged' error indicates that the source file is coded with slices.
> Slices, however, only occur in VC-1 advanced profile. The backtrace with
> the invalid read contains vc1_decode_i_blocks which is only used in
> Simple and Main profile. I currently do not see how this adds up.
> 
> Is this crash related to a specific source file? If so, could you share it?

will mail it to you privatly

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c
index 1dc8c6422d..a7ba261ccb 100644
--- a/libavcodec/vc1_block.c
+++ b/libavcodec/vc1_block.c
@@ -2541,26 +2541,24 @@  static void vc1_decode_i_blocks(VC1Context *v)
         s->mb_x = 0;
         init_block_index(v);
         for (; s->mb_x < v->end_mb_x; s->mb_x++) {
-            uint8_t *dst[6];
+            int16_t (*block)[64] = v->block[v->cur_blk_idx];
             ff_update_block_index(s);
-            dst[0] = s->dest[0];
-            dst[1] = dst[0] + 8;
-            dst[2] = s->dest[0] + s->linesize * 8;
-            dst[3] = dst[2] + 8;
-            dst[4] = s->dest[1];
-            dst[5] = s->dest[2];
-            s->bdsp.clear_blocks(s->block[0]);
+            s->bdsp.clear_blocks(block[0]);
             mb_pos = s->mb_x + s->mb_y * s->mb_width;
             s->current_picture.mb_type[mb_pos]                     = MB_TYPE_INTRA;
             s->current_picture.qscale_table[mb_pos]                = v->pq;
-            s->current_picture.motion_val[1][s->block_index[0]][0] = 0;
-            s->current_picture.motion_val[1][s->block_index[0]][1] = 0;
+            for (int i = 0; i < 4; i++) {
+                s->current_picture.motion_val[1][s->block_index[i]][0] = 0;
+                s->current_picture.motion_val[1][s->block_index[i]][1] = 0;
+            }
 
             // do actual MB decoding and displaying
             cbp = get_vlc2(&v->s.gb, ff_msmp4_mb_i_vlc.table, MB_INTRA_VLC_BITS, 2);
             v->s.ac_pred = get_bits1(&v->s.gb);
 
             for (k = 0; k < 6; k++) {
+                v->mb_type[0][s->block_index[k]] = 1;
+
                 val = ((cbp >> (5 - k)) & 1);
 
                 if (k < 4) {
@@ -2570,52 +2568,30 @@  static void vc1_decode_i_blocks(VC1Context *v)
                 }
                 cbp |= val << (5 - k);
 
-                vc1_decode_i_block(v, s->block[k], k, val, (k < 4) ? v->codingset : v->codingset2);
+                vc1_decode_i_block(v, block[k], k, val, (k < 4) ? v->codingset : v->codingset2);
 
                 if (CONFIG_GRAY && k > 3 && (s->avctx->flags & AV_CODEC_FLAG_GRAY))
                     continue;
-                v->vc1dsp.vc1_inv_trans_8x8(s->block[k]);
-                if (v->pq >= 9 && v->overlap) {
-                    if (v->rangeredfrm)
+                v->vc1dsp.vc1_inv_trans_8x8(block[k]);
+            }
+
+            if (v->overlap && v->pq >= 9) {
+                ff_vc1_i_overlap_filter(v);
+                if (v->rangeredfrm)
+                    for (k = 0; k < 6; k++)
                         for (j = 0; j < 64; j++)
-                            s->block[k][j] <<= 1;
-                    s->idsp.put_signed_pixels_clamped(s->block[k], dst[k],
-                                                      k & 4 ? s->uvlinesize
-                                                            : s->linesize);
-                } else {
-                    if (v->rangeredfrm)
+                            block[k][j] <<= 1;
+                vc1_put_blocks_clamped(v, 1);
+            } else {
+                if (v->rangeredfrm)
+                    for (k = 0; k < 6; k++)
                         for (j = 0; j < 64; j++)
-                            s->block[k][j] = (s->block[k][j] - 64) << 1;
-                    s->idsp.put_pixels_clamped(s->block[k], dst[k],
-                                               k & 4 ? s->uvlinesize
-                                                     : s->linesize);
-                }
+                            block[k][j] = (block[k][j] - 64) << 1;
+                vc1_put_blocks_clamped(v, 0);
             }
 
-            if (v->pq >= 9 && v->overlap) {
-                if (s->mb_x) {
-                    v->vc1dsp.vc1_h_overlap(s->dest[0], s->linesize);
-                    v->vc1dsp.vc1_h_overlap(s->dest[0] + 8 * s->linesize, s->linesize);
-                    if (!CONFIG_GRAY || !(s->avctx->flags & AV_CODEC_FLAG_GRAY)) {
-                        v->vc1dsp.vc1_h_overlap(s->dest[1], s->uvlinesize);
-                        v->vc1dsp.vc1_h_overlap(s->dest[2], s->uvlinesize);
-                    }
-                }
-                v->vc1dsp.vc1_h_overlap(s->dest[0] + 8, s->linesize);
-                v->vc1dsp.vc1_h_overlap(s->dest[0] + 8 * s->linesize + 8, s->linesize);
-                if (!s->first_slice_line) {
-                    v->vc1dsp.vc1_v_overlap(s->dest[0], s->linesize);
-                    v->vc1dsp.vc1_v_overlap(s->dest[0] + 8, s->linesize);
-                    if (!CONFIG_GRAY || !(s->avctx->flags & AV_CODEC_FLAG_GRAY)) {
-                        v->vc1dsp.vc1_v_overlap(s->dest[1], s->uvlinesize);
-                        v->vc1dsp.vc1_v_overlap(s->dest[2], s->uvlinesize);
-                    }
-                }
-                v->vc1dsp.vc1_v_overlap(s->dest[0] + 8 * s->linesize, s->linesize);
-                v->vc1dsp.vc1_v_overlap(s->dest[0] + 8 * s->linesize + 8, s->linesize);
-            }
             if (v->s.loop_filter)
-                ff_vc1_loop_filter_iblk(v, v->pq);
+                ff_vc1_i_loop_filter(v);
 
             if (get_bits_count(&s->gb) > v->bits) {
                 ff_er_add_slice(&s->er, 0, 0, s->mb_x, s->mb_y, ER_MB_ERROR);
@@ -2623,6 +2599,11 @@  static void vc1_decode_i_blocks(VC1Context *v)
                        get_bits_count(&s->gb), v->bits);
                 return;
             }
+
+            v->topleft_blk_idx = (v->topleft_blk_idx + 1) % (v->end_mb_x + 2);
+            v->top_blk_idx = (v->top_blk_idx + 1) % (v->end_mb_x + 2);
+            v->left_blk_idx = (v->left_blk_idx + 1) % (v->end_mb_x + 2);
+            v->cur_blk_idx = (v->cur_blk_idx + 1) % (v->end_mb_x + 2);
         }
         if (!v->s.loop_filter)
             ff_mpeg_draw_horiz_band(s, s->mb_y * 16, 16);