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

[Jorge Ramirez]

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!