diff mbox series

[FFmpeg-devel,07/34] avcodec/dvenc: Avoid copying packet data

Message ID HE1PR0301MB21544C6C914BF0159917C91D8F439@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Superseded
Headers show
Series [FFmpeg-devel,01/34] avcodec/adpcmenc: Avoid copying packet data
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Andreas Rheinhardt April 25, 2021, 10:34 p.m. UTC
When the packet size is known in advance like here, one can avoid
an intermediate buffer for the packet data.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/dvenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer April 26, 2021, 1:04 p.m. UTC | #1
On Mon, Apr 26, 2021 at 12:34:21AM +0200, Andreas Rheinhardt wrote:
> When the packet size is known in advance like here, one can avoid
> an intermediate buffer for the packet data.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/dvenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

this seems to break fate here:
not sure why or if i did something wrong

TEST    vsynth1-dv-hd
--- ./tests/ref/vsynth/vsynth1-dv-hd	2021-04-25 11:26:39.215771845 +0200
+++ tests/data/fate/vsynth1-dv-hd	2021-04-26 15:02:16.645954326 +0200
@@ -1,4 +1,4 @@
-b2bcafc74dec5f9ca516cb25dd07459b *tests/data/fate/vsynth1-dv-hd.dv
+382fbccaf3b3071895b30d9360f96c11 *tests/data/fate/vsynth1-dv-hd.dv
 14400000 tests/data/fate/vsynth1-dv-hd.dv
 34b78cf725346c7f819c9d6209b8299a *tests/data/fate/vsynth1-dv-hd.out.rawvideo
 stddev:    4.30 PSNR: 35.45 MAXDIFF:   74 bytes:  7603200/  7603200
Test vsynth1-dv-hd failed. Look at tests/data/fate/vsynth1-dv-hd.err for details.
tests/Makefile:255: recipe for target 'fate-vsynth1-dv-hd' failed
make: *** [fate-vsynth1-dv-hd] Error 1

[...]
Andreas Rheinhardt April 26, 2021, 1:11 p.m. UTC | #2
Michael Niedermayer:
> On Mon, Apr 26, 2021 at 12:34:21AM +0200, Andreas Rheinhardt wrote:
>> When the packet size is known in advance like here, one can avoid
>> an intermediate buffer for the packet data.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/dvenc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> this seems to break fate here:
> not sure why or if i did something wrong
> 
> TEST    vsynth1-dv-hd
> --- ./tests/ref/vsynth/vsynth1-dv-hd	2021-04-25 11:26:39.215771845 +0200
> +++ tests/data/fate/vsynth1-dv-hd	2021-04-26 15:02:16.645954326 +0200
> @@ -1,4 +1,4 @@
> -b2bcafc74dec5f9ca516cb25dd07459b *tests/data/fate/vsynth1-dv-hd.dv
> +382fbccaf3b3071895b30d9360f96c11 *tests/data/fate/vsynth1-dv-hd.dv
>  14400000 tests/data/fate/vsynth1-dv-hd.dv
>  34b78cf725346c7f819c9d6209b8299a *tests/data/fate/vsynth1-dv-hd.out.rawvideo
>  stddev:    4.30 PSNR: 35.45 MAXDIFF:   74 bytes:  7603200/  7603200
> Test vsynth1-dv-hd failed. Look at tests/data/fate/vsynth1-dv-hd.err for details.
> tests/Makefile:255: recipe for target 'fate-vsynth1-dv-hd' failed
> make: *** [fate-vsynth1-dv-hd] Error 1
> 
> [...]
> 
I tested this whole patchset with ASAN and somehow all the memory
ff_alloc_packet2() returned was initially zeroed, as is the case with
av_fast_padded_malloc() (which is currently used here). This encoder
seems to rely on this. I have not done an in-depth investigation, but
the diff has lots of 80-byte blocks of which only the first three bytes
are actually written:

< 00046460: 96bf 8500 0000 0000 0000 0000 0000 0000  ................
< 00046470: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 00046480: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 00046490: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 000464a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 000464b0: 96bf 8600 0000 0000 0000 0000 0000 0000  ................
< 000464c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 000464d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 000464e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
< 000464f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................

> 00046460: 96bf 852a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ...*************
> 00046470: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 00046480: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 00046490: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 000464a0: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 000464b0: 96bf 862a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ...*************
> 000464c0: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 000464d0: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 000464e0: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************
> 000464f0: 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a 2a2a  ****************

(This is the output after I made av_realloc poison the returned memory
in case the original pointer was NULL, i.e. if it is a new allocation.
Maybe this should be added to CONFIG_MEMORY_POISON? I would have found
this issue directly if it were so.)

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c
index e0679c538c..337bc326ef 100644
--- a/libavcodec/dvenc.c
+++ b/libavcodec/dvenc.c
@@ -1164,7 +1164,8 @@  static int dvvideo_encode_frame(AVCodecContext *c, AVPacket *pkt,
     DVVideoContext *s = c->priv_data;
     int ret;
 
-    if ((ret = ff_alloc_packet2(c, pkt, s->sys->frame_size, 0)) < 0)
+    if ((ret = ff_alloc_packet2(c, pkt, s->sys->frame_size,
+                                        s->sys->frame_size)) < 0)
         return ret;
 
     c->pix_fmt                = s->sys->pix_fmt;