[mythtv-commits] Ticket #11329: [PATCH] 16 bytes leaked per frame

[MythTV]

#11329: [PATCH] 16 bytes leaked per frame
-------------------------------------+-------------------------------------
 Reporter:  Bradley Baetz            |           Type:  Patch - Bug Fix
  <bbaetz@…>                         |       Priority:  minor
   Status:  new                      |      Component:  MythTV - Video
Milestone:  unknown                  |  Decoding
  Version:  0.26-fixes               |       Severity:  medium
 Keywords:  patch                    |  Ticket locked:  0
-------------------------------------+-------------------------------------
 The fix for #11029 [92016b94983a4b8a6cb75dbc48893f5c05ce4620] applied a
 mythtv-specific patch to libavformat, chaning a call from
 av_init_packet(pkt) to av_new_packet(pkt, 0), in order to properly
 initialise some fields.

 However, av_new_packet ends up mallocing (size+
 FF_INPUT_BUFFER_PADDING_SIZE) (FF_INPUT_BUFFER_PADDING_SIZE is #defined as
 16). This data is never freed, at least according to valgrind.

 After watching a 2 hour show (recorded as an MPEG2 TS stream from an
 hdhomerun) 4MB of data was leaked:

 {{{

 ==5837== 4,279,136 bytes in 267,446 blocks are definitely lost in loss
 record 2,969 of 2,971
 ==5837==    at 0x4C29BE8: memalign (in /usr/lib/valgrind
 /vgpreload_memcheck-amd64-linux.so)
 ==5837==    by 0x4C29C97: posix_memalign (in /usr/lib/valgrind
 /vgpreload_memcheck-amd64-linux.so)
 ==5837==    by 0xAE9DAC1: av_malloc (mem.c:95)
 ==5837==    by 0xA0A15C7: av_new_packet (avpacket.c:71)
 ==5837==    by 0x9D88A8A: ff_read_packet (utils.c:712)
 ==5837==    by 0x9D89C46: read_frame_internal (utils.c:1310)
 ==5837==    by 0x9D8A6F2: av_read_frame (utils.c:1413)
 ==5837==    by 0x5301CF5: AvFormatDecoder::GetFrame(DecodeTypes)
 (avformatdecoder.cpp:4509)
 ==5837==    by 0x52930E3: MythPlayer::DecoderGetFrame(DecodeTypes, bool)
 (mythplayer.cpp:3221)
 ==5837==    by 0x5293381: MythPlayer::DecoderLoop(bool)
 (mythplayer.cpp:3145)
 ==5837==    by 0x5284EE5: DecoderThread::run() (mythplayer.cpp:108)
 ==5837==    by 0x8938FCA: ??? (in /usr/lib/x86_64-linux-
 gnu/libQtCore.so.4.8.1)
 ==5837==    by 0x720BE99: start_thread (pthread_create.c:308)
 ==5837==    by 0x9398CBC: clone (clone.S:112)

 }}}

 Upstream appears to have fixed the same bug with
 http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=1cc569dddadfedabe970ce7408dba7ddf381e98b
 which just initialises those fields. I suspect that that's the better fix
 (I could only find the git commit and no mailing list conversation; not
 sure if someone else reported the same issue or if that fix was prompted
 from a report by one of the mythtv developers)

 Reverting the mythtv patch and applying the upstream one fixes the leak
 for me, and from code inspection this does the same as av_new_packet
 (without the malloc). (I can't easily check for the bug that that ticket
 has; it was apparently hard to reproduce, plus mythtv crashes very often
 for me under valgrind anyway for some reason)

 Note that this data should be freed by av_free_packet, which implies that
 its not being called in this case. Not sure why not, but it may be
 internal to libavformat and related to the mythtv-local patch (the library
 isn't expecting allocated data at that point?) Or it may be another bug -
 AvFormatDecoder::GetFrame's use of av_dup_packet looks a bit odd (dupes
 the data and then just drops the original? Not sure though - documentation
 for av_dup_packet is a bit missing)

 Full valgrind log and patch attached. (This was noticed while I'm trying
 to reproduce a much larger frontend leak)

-- 
Ticket URL: <http://code.mythtv.org/trac/ticket/11329>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center