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

[Mark Thompson]

On 19/10/17 18:55, 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)

Where does that version segfault?  (It doesn't for me.)

> 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;

Sure, that coded_width/height information is meaningless anyway.

> 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 don't think moving it inside the only-run-once-for-each-buffer check like this works, because the refcount here is incremented once-per-plane rather than once-per-buffer.

The context_refcount check there is really just a hack around the once-per-plane behaviour - I think it would probably be better to fix that first (including the redundant submission), because then this would be easier to reason about.

> 
> 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!

Feel free to take over the patch.  (I believe you have a much better test setup than I do, and that's probably the most important thing here given the subtle differences between implementations.)

Thanks,

- Mark