Created attachment 222797 [details] [review]
call stop on the buffer pool to trigger a v4l2_munmap before the new v4l2_mmap

this is rfc .. calling stop fix the device or resource busy ... 
but raised another issue (the stop calls gst_v4l2_buffer_pool_free_buffer on buffers[i] but buffers[i] can have been made NULL in gst_v4l2_buffer_pool_dqbuf as to "mark the buffer outstanding" 
thus I made a quick hack ... ie I used a new buffers_hack[] which is never marked as outstanding which hold the same GstV4l2Meta(s) as buffers[]

v4l2src should wait until all the buffers returned to the pool and then renegotiate. You can't really unmap the buffer when it is outstanding and still in use.

There is no guarantee that the buffers will return, an encoder might just hold on to a buffer until it gets a new buffer. We maybe need a new event to reclaim the outstanding buffers.

(In reply to comment #2)
> There is no guarantee that the buffers will return, an encoder might just hold
> on to a buffer until it gets a new buffer. We maybe need a new event to reclaim
> the outstanding buffers.

I've had a project where i've hit exactly this issue, inventing an event for that to get the encoder to drop its cached buffer worked out there (the encoder simply used a keyframe for the next frame).

Wouldn't EOS->FLUSH_START->FLUSH_STOP do it ?

in fact, the DRAIN query could do exactly that, after it returns, no more buffers should be in the pipeline and renegotiation can happen.

It could, but you don't necessarily need to DRAIN the full pipeline, only the buffers from one specific bufferpool. I don't think you want to necessarily wait until the pipeline is drained before starting to capture new buffers again, especially when doing live encoding where there is some buffering in the local pipeline.

(In reply to comment #6)
> It could, but you don't necessarily need to DRAIN the full pipeline, only the
> buffers from one specific bufferpool. I don't think you want to necessarily
> wait until the pipeline is drained before starting to capture new buffers
> again, especially when doing live encoding where there is some buffering in the
> local pipeline.

Some options:

1) add option to the DRAIN query to only drain the buffers from the pool. Like a shallow DRAIN or something.

2) add new serialized query (DRAIN_POOL maybe) instead of adding this to the DRAIN query.

3) add new event (RELEASE_BUFFERS) to release all bufferpool buffers. This would also need a new callback or a _buffer_pool_wait() function to wait until the buffers are returned to the pool.

About 1): overloading events with too many functions doesn't seem like a good idea. 2) would be better and then you can also add the bufferpool to drain in the query.

I think 3) is the best option.

Another thing is that the encoder (or queue) could make a copy of the bufferpool buffers and release the original ones back into the pool to release the buffers quicker.

I'm a bit concerned on how having another similar-but-slightly different event to flush the pipeline is going to be even more confusing for element authors (we have flush, eos & drain already).

Tested with v4l2src ! fakesink and sending reconfigure events to the source every second.

commit d1b26e1d594ab2b63324e43a36330475e98cdf18
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Tue Sep 11 15:38:23 2012 +0200

    v4l2: disable renegotiation
    
    We can't yet wait for the bufferpool to DRAIN before starting renegotiation so
    disable it for now.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=682770

changing bug to reflect current status.

This does not block GNOME 3.6 any longer.

While taking a picture/recording a video with camerabin/v4l2src, gst_base_src_loop fails in pad_push with GST_FLOW_NOT_NEGOTIATED, and right after it tries to check the pad flag (gst_pad_needs_reconfigure) and with the patch from comment #9 this flag is never set, causing an "Internal data flow error".

This happens cause v4l2src has to renegotiate its caps to the picture/video format, but renegotiation is disabled so the pipeline fails.

I guess the other way to fix this it to have v4l2src always memcpy the buffers instead of using a bufferpool before we fix the drain.

First attempts here.

http://cgit.freedesktop.org/~wtay/gstreamer/log/?h=release-pool

It seems to work rather well but it requires all elements that hold onto buffers to release them again. The interesting bit however is that copies can be made, like how the queue handles the event.

This looks nice, almost suspiciously simple :)

Something like this (requesting elements to release the buffer pool and buffers) would also be needed for providing buffer pools in gst-omx. The situation is very similar to V4L2 there.

Shouldn't basesink also release pre-rolled buffers ? Also you probably need to implement something like that in GstAdapter and all of the other base classes ?

The other question is if we also need something like a gst_buffer_deep_copy() to force a copy of all the underlying GstMemory chunks.

(In reply to comment #17)
> Shouldn't basesink also release pre-rolled buffers ? Also you probably need to
> implement something like that in GstAdapter and all of the other base classes ?

The event is serialized, the event has to wait until the preroll finishes. Same as when Basesink is waiting for the clock. Should it be a non-serialized event, you think?

Yes, all elements that keep on to buffers need to release. If those buffers end up in things like adapter, they have to be able to use a method to release them from adapter etc. Like how a method was needed in arrayqueue to copy the buffers.

Would it make sense to use this new functionality to also wait for a pool to have 0 outstanding buffers before letting a source like v4l2src (or shmsrc) go into NULL?

slomo: what's missing to get the release-pool stuff in master btw?
 wtay: it needs to be a oob event
slomo: nothing else?
 wtay: then probably something to handle returning memory instead of only pool buffers
 wtay: maybe rename to reclaim_memory or so..

Created attachment 241773 [details]
test to show how current situation is broken

Actually, the current situation is even more broken than I expected. You basically can't stop v4l2src without stopping the entire pipeline. I suggest that for the 1.0.x branch (and 1.1.x unless Wim finishes his branch), we revert to the 0.10 situation of always making a memcpy.

@wim: Doesn't your branch here assume that the GstMemorys in the GstBuffers don't change? Since bufferpool creates buffers with a refcount of 1, downstream can modify them. So on release they could contain any memory it wants. Shouldn't we somehow tracking the GstMemorys instead of GstBuffers? It might make sense to have gst_buffer_pool_release() just do gst_buffer_remove_all_memory.

Yes, the current branch is not optimal yet. What would be needed is something that works on GstMemory (and e.g. copies all inside a buffer to system memory when releasing). Wim had some ideas how to implement that but it was all a bit annoying and not optimal yet.

I was reviewing camerabin in 1.x and it won't work correctly until we fix this issue. Also, it seems there is no way to let subclasses tell basesrc that they can't reconfigure. So what happens is that the reconfigure event returns true but no new caps is configured. This leads to some assertions in camerabin.

1) should we add an optional function to basesrc like 'can_renegotiate'?

2) Any news on the memory releasing event?

Sorry, still need to remove a duplicate account.

I'll try and implement this for 1.4. There is 2 scenarios right now:

a) We are copying to downstream (or generic pool)
b) We are pushing buffer from our pool

Note that a is not currently present, though it's a bug, as if downstream don't support strides, and our driver has a stride that don't match the default stride provided by VideoInfo, we should be copying.

For case a), there is nothing to do, we can just de-activate, reconfigure, re-activate our pool

For case b), in the context of 1.4, we will operate a drain query. It is not ideal, it will introduce larger gap then needed, but it should work. The drain query purpose will be to reclaim our buffers. Obviously, if the result of a drain query didn't manage to reclaim our buffers, we won't be renegotiating. Camerabin should get ready for that imho, and should not assume the renegotiation applied immediately. A mix or non-fixed caps and videoscaler could allow immediate result I think, regardless of how many buffer the sources may be emitting, before actually changing resolution (if it ever change).

As a first step to this, I need to implement an allocator, in order to actually track the memory.

Review of attachment 222797 [details] [review]:

As it's documented as a hack ...

Comment on attachment 241773 [details]
test to show how current situation is broken

This is an interesting case. Here you simply simulate the case where you replace the current source by itself. Interestingly that works already if you use a pipeline lile:

  v4l2src name=src ! xvimagesink enable-last-sample=0

Without a way to reclaim buffers, this case cannot really work. I'll need to think about this, but I think GstBaseSink will need to cooperate. Note that a pipeline like:
  
  v4l2src name=src io-mode=userptr ! xvimagesink

Should have worked, though it seems I forgot to deactivate the downstream pool (right now we only unref it). I will fix that.

Created attachment 298324 [details] [review]
examples: v4l2: add example for v4l2src renegotiation

Created attachment 298325 [details] [review]
v4l2object: add gst_v4l2_object_try_format

Similar to set_format but it uses TRY_FMT instead of S_FMT

Created attachment 298327 [details] [review]
v4l2src: delay renegotiation until it is likely buffers were reclaimed

Allow renegotiation to happen when buffers have returned after an allocation
query. As the allocation query is serialized, all buffers from the pool
should have returned and we can stop it to create a new one for the
new format

Created attachment 298328 [details] [review]
basesink: drain on allocation query

This patch depends on the fix for https://bugzilla.gnome.org/show_bug.cgi?id=745287,
it might also make sense to release the last buffer on the caps event. We might need
to discuss this part a bit

I think you forgot the patch for gst_buffer_copy_deep().

I did a quick test here, I confirm it works. I see one corrupted frame with C920 when going from higher to lower resolution in raw (not visible on the other cam). With JPEGs it's all clean. I'll probably investigate a bit, but I think it's the cam. I wonder if we didn't miss a error flag or something.

commit 5e15d4aa603305d71cad5dd8b76fa9f5b25f4779
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Mar 13 18:53:11 2015 +0000

    basesink: drain on allocation query
    
    Allows buffers to be reclaimed when caps is to be renegotiated so
    that bufferpools can be stopped. As the allocation query is
    serialized all buffers have been already drained from the pipeline,
    except this last_sample one.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=682770

commit 0a945e7099159950f5ff14b13f7545dc53d2a57c
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Mar 13 18:48:03 2015 +0000

    v4l2src: delay renegotiation until it is likely buffers were reclaimed
    
    Allow renegotiation to happen when buffers have returned after an allocation
    query. As the allocation query is serialized, all buffers from the pool
    should have returned and we can stop it to create a new one for the
    new format
    
    https://bugzilla.gnome.org/show_bug.cgi?id=682770

commit 6cfa6c0da84f98a2efd5349340abe8afb2ae85fc
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Mar 13 18:47:55 2015 +0000

    v4l2object: add gst_v4l2_object_try_format
    
    Similar to set_format but it uses TRY_FMT instead of S_FMT
    
    https://bugzilla.gnome.org/show_bug.cgi?id=682770

Comment on attachment 298324 [details] [review]
examples: v4l2: add example for v4l2src renegotiation

This was just an example to reproduce the issue

Created attachment 303433 [details] [review]
a fix for exynos driver

I find the try fmt can't work the exynos driver in kernel 3.16.
This patch will only solve the problem in CAPTURE side.
The sizeimage is not correct, the driver just request it is not zero.
The OUTPUT side need num_planes to be set correctly, but I have on idea to get the correct number in that function.

Review of attachment 303433 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +2376,2 @@
+  if (is_mplane) {
+    fmt.fmt.pix_mp.plane_fmt[0].sizeimage = (*width) * (*height);

This seems like random value to me.

Created attachment 355126 [details] [review]
examples: v4l2: add example for v4l2src renegotiation

Based on work from Thiago Santos <thiagoss@osg.samsung.com>

Comment on attachment 355126 [details] [review]
examples: v4l2: add example for v4l2src renegotiation

Attachment 355126 [details] pushed as 4e8ad58 - examples: v4l2: add example for v4l2src renegotiation

From 3cfec119b1771da6cb5eda476bf0217f5253a27a Mon Sep 17 00:00:00 2001
From: "Reynaldo H. Verdejo Pinochet" <reynaldo@osg.samsung.com>
Date: Fri, 7 Jul 2017 23:49:44 -0700
Subject: [PATCH] examples: v4l2: fix wrong initializations brought by
 4e8ad583022671c5

https://bugzilla.gnome.org/show_bug.cgi?id=682770
---
 tests/examples/v4l2/v4l2src-renegotiate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/examples/v4l2/v4l2src-renegotiate.c b/tests/examples/v4l2/v4l2src-renegotiate.c
index c56dde8cb..1bf8891c7 100644
--- a/tests/examples/v4l2/v4l2src-renegotiate.c
+++ b/tests/examples/v4l2/v4l2src-renegotiate.c
@@ -27,17 +27,17 @@
 #include <gst/gst.h>
 
 /* Options */
-static gchar *device = "/dev/video0";
-static gchar *videosink = "autovideosink";
+static const gchar *device = "/dev/video0";
+static const gchar *videosink = "autovideosink";
 static gboolean enable_dmabuf = FALSE;
-static gchar *def_resolutions[] = {
+static const gchar *def_resolutions[] = {
   "320x240",
   "1280x720",
   "640x480",
   NULL
 };
 
-static gchar **resolutions = def_resolutions;
+static const gchar **resolutions = def_resolutions;
 
 static GOptionEntry entries[] = {
   {"device", 'd', 0, G_OPTION_ARG_STRING, &device, "V4L2 Camera Device",
-- 
2.11.0