[FFmpeg-devel] avcodec/frame_thread_encoder: Fix AV_OPT_TYPE_STRING handling in avctx

Submitted by Reimar Döffinger on Sept. 13, 2017, 5:12 p.m.

Details

Message ID 20170913171248.5028-1-Reimar.Doeffinger@gmx.de
State Accepted
Commit a149fa97d9501d3a1749232cc60b6f122d9d2de8
Headers show

Commit Message

Reimar Döffinger Sept. 13, 2017, 5:12 p.m.
This is the equivalent to what 7d317d4706b49d572a1eb5269438753be18362c7
did for the codec-specific options.
av_opt_copy has specific handling so it's fine that we already copied
the whole context before.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 libavcodec/frame_thread_encoder.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Reimar Döffinger Sept. 13, 2017, 6:11 p.m.
On Wed, Sep 13, 2017 at 07:12:48PM +0200, Reimar Döffinger wrote:
> This is the equivalent to what 7d317d4706b49d572a1eb5269438753be18362c7
> did for the codec-specific options.
> av_opt_copy has specific handling so it's fine that we already copied
> the whole context before.

Btw, if someone can make time for reviewing it that would likely
be time well spent.
For example it seems the code also leaks the memory for the AVPacket
it mallocs sometimes.
Michael Niedermayer Sept. 15, 2017, 12:16 a.m.
On Wed, Sep 13, 2017 at 08:11:38PM +0200, Reimar Döffinger wrote:
> On Wed, Sep 13, 2017 at 07:12:48PM +0200, Reimar Döffinger wrote:
> > This is the equivalent to what 7d317d4706b49d572a1eb5269438753be18362c7
> > did for the codec-specific options.
> > av_opt_copy has specific handling so it's fine that we already copied
> > the whole context before.

the change looks reasonable


> 
> Btw, if someone can make time for reviewing it that would likely
> be time well spent.

you mean reviewing the whole frame_thread_encoder code searching
for issues unrelated to tickets or patches ?
ATM i think i have too much i should have done "yesterday" to add that


> For example it seems the code also leaks the memory for the AVPacket
> it mallocs sometimes.

is there some way by which this can be reproduced or is it completely
random ?

[...]
Reimar Döffinger Sept. 16, 2017, 2:41 p.m.
On Fri, Sep 15, 2017 at 02:16:58AM +0200, Michael Niedermayer wrote:
> On Wed, Sep 13, 2017 at 08:11:38PM +0200, Reimar Döffinger wrote:
> > On Wed, Sep 13, 2017 at 07:12:48PM +0200, Reimar Döffinger wrote:
> > > This is the equivalent to what 7d317d4706b49d572a1eb5269438753be18362c7
> > > did for the codec-specific options.
> > > av_opt_copy has specific handling so it's fine that we already copied
> > > the whole context before.
> 
> the change looks reasonable

Thanks!

> > Btw, if someone can make time for reviewing it that would likely
> > be time well spent.
> 
> you mean reviewing the whole frame_thread_encoder code searching
> for issues unrelated to tickets or patches ?
> ATM i think i have too much i should have done "yesterday" to add that

Sure, I understand. Though just some comments around the thinking or
such (e.g. if it's intentional or just outdated API that it is not using
av_packet_alloc, or what exactly is the av_dup_packet there for).

> > For example it seems the code also leaks the memory for the AVPacket
> > it mallocs sometimes.
> 
> is there some way by which this can be reproduced or is it completely
> random ?

It's not random, but so far the test-case is only mencoder, so
I can't guarantee the issue is in that code. The only reason
I believe that is because I only see a AVPacket leak when threads
are enabled, but there are other leaks that need fixing in mencoder...
But otherwise, just
valgrind --leak-check=full --show-leak-kinds=all
./mencoder ~/Downloads/small_test2.dv -o test.avi -ovc lavc -nosound
-lavcopts vcodec=dvvideo:threads=10 -frames 2
works, giving:
==3992== 144,032 bytes in 1 blocks are indirectly lost in loss record 58 of 59
==3992==    at 0x4C2DA5F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3992==    by 0x4C2FDDF: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3992==    by 0xB57636: av_buffer_realloc (buffer.c:177)
==3992==    by 0x477DFE: copy_packet_data (avpacket.c:210)
==3992==    by 0x477F3D: av_dup_packet (avpacket.c:259)
==3992==    by 0x4D1822: avcodec_encode_video2 (encode.c:322)
==3992==    by 0x4FDA17: worker (frame_thread_encoder.c:89)
==3992==    by 0x50496D9: start_thread (pthread_create.c:456)
==3992== 
==3992== 144,184 (88 direct, 144,096 indirect) bytes in 1 blocks are definitely lost in loss record 59 of 59
==3992==    at 0x4C2FE96: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3992==    by 0x4C2FFA1: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3992==    by 0xB64243: av_malloc (mem.c:87)
==3992==    by 0xB6444D: av_mallocz (mem.c:224)
==3992==    by 0x4FD93A: worker (frame_thread_encoder.c:73)
==3992==    by 0x50496D9: start_thread (pthread_create.c:456)
Michael Niedermayer Sept. 16, 2017, 3:27 p.m.
On Sat, Sep 16, 2017 at 04:41:00PM +0200, Reimar Döffinger wrote:
> On Fri, Sep 15, 2017 at 02:16:58AM +0200, Michael Niedermayer wrote:
> > On Wed, Sep 13, 2017 at 08:11:38PM +0200, Reimar Döffinger wrote:
> > > On Wed, Sep 13, 2017 at 07:12:48PM +0200, Reimar Döffinger wrote:
> > > > This is the equivalent to what 7d317d4706b49d572a1eb5269438753be18362c7
> > > > did for the codec-specific options.
> > > > av_opt_copy has specific handling so it's fine that we already copied
> > > > the whole context before.
> > 
> > the change looks reasonable
> 
> Thanks!
> 
> > > Btw, if someone can make time for reviewing it that would likely
> > > be time well spent.
> > 
> > you mean reviewing the whole frame_thread_encoder code searching
> > for issues unrelated to tickets or patches ?
> > ATM i think i have too much i should have done "yesterday" to add that
> 
> Sure, I understand. Though just some comments around the thinking or
> such (e.g. if it's intentional or just outdated API that it is not using
> av_packet_alloc, or what exactly is the av_dup_packet there for).

IIRC av_dup_packet() is there so cases of packets that have their
memory reused in the next call work.
av_dup_packet() properly allocates a packet or if its already properly
allocated it does nothing

its possibly av_dup_packet() is not needed anymore

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
index 35a37c4..31a9fe9 100644
--- a/libavcodec/frame_thread_encoder.c
+++ b/libavcodec/frame_thread_encoder.c
@@ -199,6 +199,9 @@  int ff_frame_thread_encoder_init(AVCodecContext *avctx, AVDictionary *options){
             goto fail;
         tmpv = thread_avctx->priv_data;
         *thread_avctx = *avctx;
+        int ret = av_opt_copy(thread_avctx, avctx);
+        if (ret < 0)
+            goto fail;
         thread_avctx->priv_data = tmpv;
         thread_avctx->internal = NULL;
         if (avctx->codec->priv_class) {