[Bug 735660] v4l2: fix new v4l2 code not working with certain devices (regression)

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=735660
  GStreamer | gst-plugins-good | 1.4.1

--- Comment #22 from Hans de Goede <jwrdegoede at fedoraproject.org> 2014-08-30 13:25:49 UTC ---
(In reply to comment #21)
> Review of attachment 284882 [details]:
> 
> ::: sys/v4l2/gstv4l2allocator.c
> @@ +651,1 @@
> +  GST_OBJECT_FLAG_SET (allocator, flags);
> 
> Thanks good catch
> 
> @@ +783,3 @@
>          "error releasing buffers buffers: %s", g_strerror (errno));
> +    /* Not all drivers support freeing buffers by requesting 0 buffers, so
> +       we don't set ret to an error here. */
> 
> This has the side effect, as we won't be able to restart the allocator.

Why not? Calling reqbufs multiple times, even with different buffer counts on
old videobuf drivers works fine, as long as the previous buffers are not busy
at that time (streaming needs to be off, and they need to be unmapped, but that
is the case after a gst_v4l2_allocator_stop even if the reqbufs(0) fails.

> Maybe we should empty the atomic queue and deactivate the allocator even if freeing
> buffers didn't work ?

Right, that is what my patch does by not setting ret to an error (AFAIK).

> Note, for all this to be useful, we'd need S_FMT to succeed even if there is
> buffers allocated. Hans V. seemed to think this won't work. He said said the
> only way we could change the format after having allocated buffers would be to
> close and re-open. Would it be possible to double check that ?

I've just checked the bttv driver, and s_fmt will work fine there with mapped
buffers, as long as no streaming is in progress. If the size of the needed
buffers gets bigger after a s_fmt, and you don't re-request the buffers, then
streamon will fail with -EINVAL. But since the gst code will re-request the
buffers all should be fine, which matches my testing where I can change
resolution in cheese while streaming.

> If this is the case, we'd keep the code has is, I would simply like to add
> something in the warning, so developers will know that re-negotiation isn't
> possible for this device.

May not be possible, as said for bttv it works, I've not checked any others.

> They would then have the choice between trying to port to VB2, or implement a close/re-open work
> around (both are fair amount of work).

Or fixing the old videobuf code to support reqbufs(0), now that I've taken a
closer look to answer your questions I think that may actually be pretty
doable. I'll discuss this with Hans V. still we need some kind of workaround to
get things to work with current kernels, as any fix won't be suitable for Cc:
stable, that much is certain.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.