GNOME Bugzilla – Bug 742074
v4l2videodec: leaks filedescriptors
Last modified: 2018-08-02 11:06:11 UTC
Created attachment 293414 [details] [review] Strawmen patch After playing back a video file on exynos 2 file descriptors stay open referring to the video device. Tracing those down points to the BufferPool and Allocate attached to the src pad (output object). What happens when the decoder is torn down is that the pool is de-activate and dropped (from gst_v4l2_object_stop): gst_buffer_pool_set_active (v4l2object->pool, FALSE); gst_object_unref (v4l2object->pool); v4l2object->pool = NULL; The idea being that the BufferPool will get disposed once all buffers have been returned to it. However for _OUTPUT the buffers enqueued in the video device are also acquired out of the pool (as opposed to _CAPTURE buffers, which are seenas "in" the pool while enqueued). Unfortunately, this means that if there are any buffers still enqueued in the _OUTPUT queue when gst_v4l2_object_stop is called, then the BufferPool will never get all its buffers back, thus never gets cleaned up. This is "solved" in the attached patch by calling stream off and dropping all enqueued buffers once the pool is set to flushing. However I'm not entirely happy with that solution, it might be nicer & more in line with the handling of_CAPTURE buffers to regard enqueued _OUTPUT buffers as part of the pool rather then buffers taken out of the pool, which prevents this issue and might make the handling more aligned.
After some research, what I like of releasing the output buffers from flush_start() is that it means it works well when the pool is used from the outside of the element. This might be particularly handy for being able to implement re-negotiation. Now, the problem is that doing so means we play on buffers array while some qbuf/dqbuf might be being called. Acquire/Release was originally lockless by design, hence would only change the array on flush_stop(), which happens before threads start doing parrallel access. I'm saying was, since we introduced a lock to workaround a bug in videobuf2 when the queue is empty. We also do an iotcl, which could possibly be a pre-emption point. So all this being considered, we should maybe simply buffer array with a lock. It should be light locking so we can still run acquire/release in parallel and minimise the risk of deadlock.
Comment on attachment 293414 [details] [review] Strawmen patch Conceptually correct, and the most flexible approach, though not yet thread safe.
Just went with a completly different approach, it concist of reverting back to old behaviour of not keeping a ref on queued buffer. Instead, we just don't hand it over to the free list until dequeued. commit e47a5708f007753a0ef46757b6b547dfb1f6f174 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Wed Jan 7 17:55:14 2015 -0500 v4l2bufferpool: Don't clean buffer array in dispose This should already have been done, plus this code is incorrect and may lead to crash. https://bugzilla.gnome.org/show_bug.cgi?id=742074 commit bbcfd594e487cf2036644741600f0963fa4e8228 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Wed Jan 7 17:48:31 2015 -0500 v4l2bufferpool: Don't ref queued output buffer This partly revert to the old 1.2 behavior. Instead of keeping a reference to the output buffer queued, we simply release them but don't forward it to GstBufferPool. This way, the buffer pool don't need to be flushed to be stopped. https://bugzilla.gnome.org/show_bug.cgi?id=742074
Also in 1.4 ab60576 v4l2bufferpool: Don't clean buffer array in dispose bca79b4 v4l2bufferpool: Don't ref queued output buffer
Below code cause race condition when gst-launch-1.0 videotestsrc ! v4l2videoenc ! fakesink out->pool == NULL will race with _gst_video_codec_frame_free() in gst_video_encoder_finish_frame(). Can I only revert below code without other code change? Is there any potential issue? @@ -1781,15 +1781,19 @@ gst_v4l2_buffer_pool_process (GstV4l2BufferPool * pool, GstBuffer ** buf) goto start_failed; } + /* Remove our ref, we will still hold this buffer in acquire as needed, + * otherwise the pool will think it is outstanding and will refuse to stop. */ + gst_buffer_unref (to_queue); + if (g_atomic_int_get (&pool->num_queued) >= pool->min_latency) { GstBuffer *out; /* all buffers are queued, try to dequeue one and release it back * into the pool so that _acquire can get to it again. */ ret = gst_v4l2_buffer_pool_dqbuf (pool, &out); - if (ret == GST_FLOW_OK) + if (ret == GST_FLOW_OK && out->pool == NULL) /* release the rendered buffer back into the pool. This wakes up any * thread waiting for a buffer in _acquire(). */ - gst_buffer_unref (out); + gst_v4l2_buffer_pool_release_buffer (bpool, out); } break; }
This is a very ancient change, if you think you found a bug, best is to file a new bug, but most importantly, provide a way to reproduce.