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

[Jorge Ramirez]

On 10/19/2017 07:55 PM, Jorge Ramirez wrote:
> On 10/19/2017 11:49 AM, Mark Thompson wrote:
>> On 19/10/17 08:28, Jorge Ramirez wrote:
>>> 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.
>>> um, ok ... please could you send a patch so I can apply it? I see 
>>> several problems in v4l2_free_buffer.
>> What goes wrong?  It applies fine for me on current head 
>> (f4090940bd3024e69d236257d327f11d1e496229).
>
> yes my bad.
>
>>
>>> To sum up the RFC, instead of using private_data to place the 
>>> codec's context, it uses private data to place a _pointer_ to an 
>>> allocated codec context "just" so it wont be deallocated after the 
>>> codec is closed (codec close frees the private_data)
>>>
>>> Personally I think adding AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE 
>>> and use private_data to hold the codec context is a cleaner/simpler 
>>> approach.
>>>
>>> - the codec requests private_data with a size (sizeof(type))
>>> - the code explicitly informs the API whether all memory will be 
>>> released on close or it will preserve it.
>> - All APIs in ffmpeg with this sort of private data field use them in 
>> the same way - they are allocated at create/alloc time (with the 
>> given size, for AVOptions), and freed at close/destroy time.
>> - Using the well-tested reference-counted buffer implementation is 
>> IMO strongly preferable to making ad-hoc synchronisation with atomics 
>> and semaphores.
>> - All other decoders use the reference-counted buffer approach (e.g. 
>> look at rkmpp for a direct implementation, the hwaccels all do it via 
>> hwcontext).
>
> ok so I guess there is no point to discuss further what I tried to put 
> forward (I could provide my implementation to compare against this RFC 
> - it is just a handful of lines of code - but I guess there is no 
> point given that everyone is comfortable with the current way of doing 
> things.).
>
> so let's make this work then and fix the SIGSEGV present in master 
> asap (btw this RFC also seg-fault when the above is applied)
>
> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
> index 831fd81..1dd8cf0 100644
> --- a/libavcodec/v4l2_m2m_dec.c
> +++ b/libavcodec/v4l2_m2m_dec.c
> @@ -176,8 +176,8 @@ static av_cold int v4l2_decode_init(AVCodecContext 
> *avctx)
>       * by the v4l2 driver; this event will trigger a full pipeline 
> reconfig and
>       * the proper values will be retrieved from the kernel driver.
>       */
> -    output->height = capture->height = avctx->coded_height;
> -    output->width = capture->width = avctx->coded_width;
> +    output->height = capture->height = 0; //avctx->coded_height;
> +    output->width = capture->width = 0; //avctx->coded_width;
>
>      output->av_codec_id = avctx->codec_id;
>      output->av_pix_fmt  = AV_PIX_FMT_NONE;
>
>
> also looking at your code I think we also need:
>
> [jramirez at igloo libavcodec (patches/m6 *$)]$ git diff v4l2_buffers.c
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index 9831bdb..8dec56d 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -207,15 +207,19 @@ static void v4l2_free_buffer(void *opaque, 
> uint8_t *unused)
>      V4L2Buffer* avbuf = opaque;
>      V4L2m2mContext *s = buf_to_m2mctx(avbuf);
>
> -    atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel);
> -    if (s->reinit) {
> -        if (!atomic_load(&s->refcount))
> -            sem_post(&s->refsync);
> -    } else if (avbuf->context->streamon) {
> -        ff_v4l2_buffer_enqueue(avbuf);
> -    }
> +    av_log(logger(avbuf), AV_LOG_DEBUG, "free capture %d\n", 
> avbuf->buf.index);
>
>      if (atomic_fetch_sub(&avbuf->context_refcount, 1) == 1) {
> +        atomic_fetch_sub_explicit(&s->refcount, 1, 
> memory_order_acq_rel);
> +
> +        if (s->reinit) {
> +            if (!atomic_load(&s->refcount))
> +                sem_post(&s->refsync);
> +        } else if (avbuf->context->streamon) {
> +            av_log(logger(avbuf), AV_LOG_DEBUG, "queue capture %d\n", 
> avbuf->buf.index);
> +            ff_v4l2_buffer_enqueue(avbuf);
> +        }
> +
>          av_buffer_unref(&avbuf->context_ref);
>      }
>  }
>
> I tested the encoder and it seems to work properly (havent checked 
> with valgrind but all frames are properly encoded)
>
> how do you want to proceed?
> will you create a branch somewhere and we work on this or should I 
> take it and evolve it or will you do it all? thanks!
>
>
>
fyi

testing the h264 m2m encoder

==10241== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==10241== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==10241== Command: /home/linaro/Src/git.zoltan.ffmpeg/ffmpeg_g -report 
-threads 1 -v 55 -f rawvideo -pix_fmt nv12 -s:v 1280:720 -r 25 -i 
/home/linaro/Videos/raw/freeway.yuv -c:v h264_v4l2m2m out/out.h264.mp4
==10241==

...
...

Input file #0 (/home/linaro/Videos/raw/freeway.yuv):
   Input stream #0:0 (video): 232 packets read (320716800 bytes); 232 
frames decoded;
   Total: 232 packets (320716800 bytes) demuxed
Output file #0 (out/out.h264.mp4):
   Output stream #0:0 (video): 232 frames encoded; 232 packets muxed 
(563494 bytes);
   Total: 232 packets (563494 bytes) muxed
232 frames successfully decoded, 0 decoding errors
[AVIOContext @ 0x5aa9270] Statistics: 2 seeks, 6 writeouts
[AVIOContext @ 0x59103b0] Statistics: 320716800 bytes read, 0 seeks
==10241==    at 0x129F8B8: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==10241==    by 0x12A0593: av_log_default_callback (log.c:355)
==10241==    by 0x21CEC3: log_callback_report (cmdutils.c:110)
==10241==    by 0x12A076F: av_vlog (log.c:383)
==10241==    by 0x12A06F7: av_log (log.c:375)
==10241==    by 0x224447: term_exit (ffmpeg.c:317)
==10241==    by 0x224FCB: ffmpeg_cleanup (ffmpeg.c:618)
==10241==    by 0x21CFBB: exit_program (cmdutils.c:138)
==10241==    by 0x23412B: main (ffmpeg.c:4832)
==10241==
==10241== HEAP SUMMARY:
==10241==     in use at exit: 600 bytes in 2 blocks
==10241==   total heap usage: 4,323 allocs, 4,321 frees, 325,278,370 
bytes allocated
==10241==
==10241== LEAK SUMMARY:
==10241==    definitely lost: 0 bytes in 0 blocks
==10241==    indirectly lost: 0 bytes in 0 blocks
==10241==      possibly lost: 0 bytes in 0 blocks
==10241==    still reachable: 600 bytes in 2 blocks
==10241==         suppressed: 0 bytes in 0 blocks
==10241== Reachable blocks (those to which a pointer was found) are not 
shown.
==10241== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==10241==
==10241== For counts of detected and suppressed errors, rerun with: -v
==10241== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)