[FFmpeg-devel] [RFC] v4l2_m2m: Fix races around freeing data on close

[Jorge Ramirez]

On 10/19/2017 02:10 AM, Mark Thompson wrote:
> Refcount all of the context information.
> ---
> As discussed in the other thread, something like this.  We move most of the context into a refcounted buffer and AVCodecContext.priv_data is left as a stub holding a reference to it.
>
> Thoughts:
> * Change is rather ugly; some structures and function arguments could probably be rearranged to improve it.
> * Not very well tested - I'm only testing it with the decoder on s5p-mfc.  (Encoder might well be totally broken.)
> * It currently adds an extra atomic to each buffer to keep track of the context-references that isn't really wanted.  Cleaning up the per-plane references so we only go through the buffer-free sequence once would remove it.
>
> I found several more issues while looking at this (not treated here):
> * The refsync process with the semaphore is racy - it will fall over if the buffer unrefs are called on multiple threads at the same time.
> * Buffers are requeued once for every plane they have as a consequnce of the per-plane references (NV12 buffer -> enqueue twice).  The later enqueues are ignored by the driver (QBUF returns EINVAL; look at strace), but that should probably still be treated as a bug.
> * It seems to be able to leak all of the input packets (if refcounted?) - valgrind shows this, but I didn't investigate further.

about the last issue, I think this is related of how bsf is used in 3.4 
codecs and not to the v4l2 codec implementation itself
Running the v4l2 m2m decoding in 3.4 vs running 3.3 shows the following 
(I am using the flag to preserve the private_data in both cases)

ffmpeg: 3.4
============

[AVIOContext @ 0x590ee20] Statistics: 3997696 bytes read, 0 seeks
==2525==    at 0x12A00F8: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==2525==    by 0x12A0DD3: av_log_default_callback (log.c:355)
==2525==    by 0x21CD03: log_callback_report (cmdutils.c:110)
==2525==    by 0x12A0FAF: av_vlog (log.c:383)
==2525==    by 0x12A0F37: av_log (log.c:375)
==2525==    by 0x224287: term_exit (ffmpeg.c:317)
==2525==    by 0x224E0B: ffmpeg_cleanup (ffmpeg.c:618)
==2525==    by 0x21CDFB: exit_program (cmdutils.c:138)
==2525==    by 0x233F03: main (ffmpeg.c:4824)
==2525==
==2525== HEAP SUMMARY:
==2525==     in use at exit: 4,015,310 bytes in 617 blocks
==2525==   total heap usage: 16,338 allocs, 15,721 frees, 21,262,380 
bytes allocated
==2525==
==2525== 18,156 bytes in 1 blocks are possibly lost in loss record 5 of 8
==2525==    at 0x4844C38: malloc (vg_replace_malloc.c:298)
==2525==    by 0x4847197: realloc (vg_replace_malloc.c:785)
==2525==    by 0x12A4773: av_realloc (mem.c:144)
==2525==    by 0x128FABB: av_buffer_realloc (buffer.c:177)
==2525==    by 0x67F49F: packet_alloc (avpacket.c:77)
==2525==    by 0x68118F: av_packet_ref (avpacket.c:636)
==2525==    by 0x6078EB: add_to_pktbuf (utils.c:435)
==2525==    by 0x60B483: parse_packet (utils.c:1470)
==2525==    by 0x60BBE3: read_frame_internal (utils.c:1613)
==2525==    by 0x60C22B: av_read_frame (utils.c:1724)
==2525==    by 0x2317AF: get_input_packet (ffmpeg.c:4097)
==2525==    by 0x231CAB: process_input (ffmpeg.c:4217)
==2525==
==2525== 3,996,554 (4,920 direct, 3,991,634 indirect) bytes in 205 
blocks are definitely lost in loss record 8 of 8
==2525==    at 0x4847248: memalign (vg_replace_malloc.c:857)
==2525==    by 0x484735B: posix_memalign (vg_replace_malloc.c:1020)
==2525==    by 0x12A46D3: av_malloc (mem.c:87)
==2525==    by 0x12A49B3: av_mallocz (mem.c:224)
==2525==    by 0x128F7DF: av_buffer_ref (buffer.c:95)
==2525==    by 0x6811FF: av_packet_ref (avpacket.c:644)
==2525==    by 0x6CCEF3: avcodec_send_packet (decode.c:666)
==2525==    by 0x22B043: decode (ffmpeg.c:2265)
==2525==    by 0x22B7BF: decode_video (ffmpeg.c:2409)
==2525==    by 0x22C573: process_input_packet (ffmpeg.c:2650)
==2525==    by 0x232F93: process_input (ffmpeg.c:4442)
==2525==    by 0x23348F: transcode_step (ffmpeg.c:4553)
==2525==
==2525== LEAK SUMMARY:
==2525==    definitely lost: 4,920 bytes in 205 blocks
==2525==    indirectly lost: 3,991,634 bytes in 409 blocks
==2525==      possibly lost: 18,156 bytes in 1 blocks
==2525==    still reachable: 600 bytes in 2 blocks
==2525==         suppressed: 0 bytes in 0 blocks
==2525== Reachable blocks (those to which a pointer was found) are not 
shown.
==2525== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==2525==
==2525== For counts of detected and suppressed errors, rerun with: -v
==2525== Use --track-origins=yes to see where uninitialised values come 
from
==2525== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)


ffmpeg 3.3
==========

[AVIOContext @ 0x590ee20] Statistics: 2621440 bytes read, 0 seeks
==11047==    at 0x1246920: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==11047==    by 0x12475FB: av_log_default_callback (log.c:355)
==11047==    by 0x203BEB: log_callback_report (cmdutils.c:110)
==11047==    by 0x12477D7: av_vlog (log.c:383)
==11047==    by 0x124775F: av_log (log.c:375)
==11047==    by 0x21C1A7: term_exit (ffmpeg.c:317)
==11047==    by 0x21CD37: ffmpeg_cleanup (ffmpeg.c:619)
==11047==    by 0x203CE3: exit_program (cmdutils.c:138)
==11047==    by 0x22BE7B: main (ffmpeg.c:4798)
==11047==
==11047== HEAP SUMMARY:
==11047==     in use at exit: 52,768 bytes in 3 blocks
==11047==   total heap usage: 11,550 allocs, 11,547 frees, 18,048,065 
bytes allocated
==11047==
==11047== 52,168 bytes in 1 blocks are definitely lost in loss record 3 
of 3
==11047==    at 0x4847248: memalign (vg_replace_malloc.c:857)
==11047==    by 0x484735B: posix_memalign (vg_replace_malloc.c:1020)
==11047==    by 0x124AE67: av_malloc (mem.c:87)
==11047==    by 0x124B147: av_mallocz (mem.c:224)
==11047==    by 0xAED317: avcodec_open2 (utils.c:1307)
==11047==    by 0x5DAA4B: avformat_find_stream_info (utils.c:3480)
==11047==    by 0x20DFD7: open_input_file (ffmpeg_opt.c:1013)
==11047==    by 0x21720F: open_files (ffmpeg_opt.c:3203)
==11047==    by 0x21737F: ffmpeg_parse_options (ffmpeg_opt.c:3243)
==11047==    by 0x22BC27: main (ffmpeg.c:4760)
==11047==
==11047== LEAK SUMMARY:
==11047==    definitely lost: 52,168 bytes in 1 blocks
==11047==    indirectly lost: 0 bytes in 0 blocks
==11047==      possibly lost: 0 bytes in 0 blocks
==11047==    still reachable: 600 bytes in 2 blocks
==11047==         suppressed: 0 bytes in 0 blocks
==11047== Reachable blocks (those to which a pointer was found) are not 
shown.
==11047== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==11047==
==11047== For counts of detected and suppressed errors, rerun with: -v
==11047== Use --track-origins=yes to see where uninitialised values come 
from
==11047== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)