| Message ID | 20170913171248.5028-1-Reimar.Doeffinger@gmx.de |
|---|---|
| State | Accepted |
| Commit | a149fa97d9501d3a1749232cc60b6f122d9d2de8 |
| Headers | show |
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.
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 ? [...]
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)
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 [...]
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) {
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(+)