Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()
On Sat, Jun 22, 2019 at 02:21:43PM +0200, Michael Niedermayer wrote: > Fixes: left shift of negative value -9 > Fixes: > 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/vc1_block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Will apply these 2 in a few days unless there are further objections, benchmarks, disassemblies showing a slowdown My tests indicate as already stated that the asm code generated is identical [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()
On Thu, Jun 27, 2019 at 12:40:58AM +0200, Michael Niedermayer wrote: > On Sat, Jun 22, 2019 at 04:55:47PM +0200, Paul B Mahol wrote: > > On 6/22/19, Michael Niedermayer wrote: > > > Fixes: left shift of negative value -9 > > > Fixes: > > > 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer > > > --- > > > libavcodec/vc1_block.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > > > index 7e41791832..6137252580 100644 > > > --- a/libavcodec/vc1_block.c > > > +++ b/libavcodec/vc1_block.c > > > @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v) > > > if (v->rangeredfrm) > > > for (k = 0; k < 6; k++) > > > for (j = 0; j < 64; j++) > > > -v->block[v->cur_blk_idx][block_map[k]][j] <<= > > > 1; > > > +v->block[v->cur_blk_idx][block_map[k]][j] *= > > > 2; > > > vc1_put_blocks_clamped(v, 1); > > > } else { > > > if (v->rangeredfrm) > > > for (k = 0; k < 6; k++) > > > for (j = 0; j < 64; j++) > > > -v->block[v->cur_blk_idx][block_map[k]][j] = > > > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1; > > > +v->block[v->cur_blk_idx][block_map[k]][j] = > > > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2; > > > vc1_put_blocks_clamped(v, 0); > > > } > > > > > > -- > > > 2.22.0 > > > > > > ___ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > To unsubscribe, visit link above, or email > > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > > > > > > This is much slower. > > please provide your testcase and benchmarks or disassmbly Noone ? people just claim "This is much slower." without having tested it ? ok heres the disassmbly of the 2 inner loops from the code with the patch applied (that is with the multiplications in the source tested with gcc (4.8.5) as you can see all multiplications are optimized to shifts or additions even the old gcc used optmizes it to a shift .p2align 4,,10 .p2align 3 .L1034: salw(%rax) addq$2, %rax cmpq%rdx, %rax jne .L1034 ... .p2align 4,,10 .p2align 3 .L1039: movswl (%rax), %edx addq$2, %rax leal-128(%rdx,%rdx), %edx movw%dx, -2(%rax) cmpq%rcx, %rax jne .L1039 before the patch it looked like this: .p2align 4,,10 .p2align 3 .L1034: salw(%rax) addq$2, %rax cmpq%rdx, %rax jne .L1034 ... .p2align 4,,10 .p2align 3 .L1039: movswl (%rax), %edx addq$2, %rax leal-128(%rdx,%rdx), %edx movw%dx, -2(%rax) cmpq%rcx, %rax jne .L1039 I used this patch for testing and finding the parts of the code: --- a/libavcodec/vc1_block.c +++ b/libavcodec/vc1_block.c @@ -2579,16 +2579,20 @@ static void vc1_decode_i_blocks(VC1Context *v) if (v->overlap && v->pq >= 9) { ff_vc1_i_overlap_filter(v); +__asm volatile ("MARKA\n\t"); if (v->rangeredfrm) for (k = 0; k < 6; k++) for (j = 0; j < 64; j++) -v->block[v->cur_blk_idx][block_map[k]][j] *= 2; +v->block[v->cur_blk_idx][block_map[k]][j] <<= 1; +__asm volatile ("MARKB\n\t"); vc1_put_blocks_clamped(v, 1); } else { +__asm volatile ("MARKC\n\t"); if (v->rangeredfrm) for (k = 0; k < 6; k++) for (j = 0; j < 64; j++) -v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2; +v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1; +__asm volatile ("MARKD\n\t"); vc1_put_blocks_clamped(v, 0); } [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()
On Sat, Jun 22, 2019 at 04:55:47PM +0200, Paul B Mahol wrote: > On 6/22/19, Michael Niedermayer wrote: > > Fixes: left shift of negative value -9 > > Fixes: > > 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer > > --- > > libavcodec/vc1_block.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > > index 7e41791832..6137252580 100644 > > --- a/libavcodec/vc1_block.c > > +++ b/libavcodec/vc1_block.c > > @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v) > > if (v->rangeredfrm) > > for (k = 0; k < 6; k++) > > for (j = 0; j < 64; j++) > > -v->block[v->cur_blk_idx][block_map[k]][j] <<= > > 1; > > +v->block[v->cur_blk_idx][block_map[k]][j] *= 2; > > vc1_put_blocks_clamped(v, 1); > > } else { > > if (v->rangeredfrm) > > for (k = 0; k < 6; k++) > > for (j = 0; j < 64; j++) > > -v->block[v->cur_blk_idx][block_map[k]][j] = > > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1; > > +v->block[v->cur_blk_idx][block_map[k]][j] = > > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2; > > vc1_put_blocks_clamped(v, 0); > > } > > > > -- > > 2.22.0 > > > > ___ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > > > This is much slower. please provide your testcase and benchmarks or disassmbly thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()
On 6/22/19, Michael Niedermayer wrote: > Fixes: left shift of negative value -9 > Fixes: > 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/vc1_block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > index 7e41791832..6137252580 100644 > --- a/libavcodec/vc1_block.c > +++ b/libavcodec/vc1_block.c > @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v) > if (v->rangeredfrm) > for (k = 0; k < 6; k++) > for (j = 0; j < 64; j++) > -v->block[v->cur_blk_idx][block_map[k]][j] <<= > 1; > +v->block[v->cur_blk_idx][block_map[k]][j] *= 2; > vc1_put_blocks_clamped(v, 1); > } else { > if (v->rangeredfrm) > for (k = 0; k < 6; k++) > for (j = 0; j < 64; j++) > -v->block[v->cur_blk_idx][block_map[k]][j] = > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1; > +v->block[v->cur_blk_idx][block_map[k]][j] = > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2; > vc1_put_blocks_clamped(v, 0); > } > > -- > 2.22.0 > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". This is much slower. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()
Fixes: left shift of negative value -9 Fixes: 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/vc1_block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c index 7e41791832..6137252580 100644 --- a/libavcodec/vc1_block.c +++ b/libavcodec/vc1_block.c @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v) if (v->rangeredfrm) for (k = 0; k < 6; k++) for (j = 0; j < 64; j++) -v->block[v->cur_blk_idx][block_map[k]][j] <<= 1; +v->block[v->cur_blk_idx][block_map[k]][j] *= 2; vc1_put_blocks_clamped(v, 1); } else { if (v->rangeredfrm) for (k = 0; k < 6; k++) for (j = 0; j < 64; j++) -v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1; +v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2; vc1_put_blocks_clamped(v, 0); } -- 2.22.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".