After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 583890 - v4l2: Implement V4L2_MEMORY_DMABUF/USERPTR support
v4l2: Implement V4L2_MEMORY_DMABUF/USERPTR support
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 796986
 
 
Reported: 2009-05-26 13:35 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2018-11-03 14:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
implement pad_alloc_buffer usage (19.05 KB, patch)
2009-05-26 13:41 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
implement pad_alloc_buffer usage (22.59 KB, patch)
2009-05-27 13:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
implement pad_alloc_buffer usage (22.38 KB, patch)
2009-05-27 15:41 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
needs-work Details | Review
implement pad_alloc_buffer usage (26.89 KB, patch)
2010-03-03 11:50 UTC, Arnout Vandecappelle
none Details | Review
implement pad_alloc_buffer usage (26.75 KB, patch)
2010-03-04 17:01 UTC, Arnout Vandecappelle
none Details | Review
implement pad_alloc_buffer usage (26.78 KB, patch)
2010-03-04 17:57 UTC, Arnout Vandecappelle
needs-work Details | Review
v4l2: Add support for USERPTR mode in VIDEO_OUTPUT. (17.26 KB, patch)
2013-12-31 12:02 UTC, Aurélien Zanelli
none Details | Review
USERPTR for OUTPUT (28.01 KB, patch)
2014-01-15 22:29 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
USERPTR for CAPTURE (16.16 KB, patch)
2014-01-15 22:29 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
Dynamically enable USERPTR (11.44 KB, patch)
2014-01-15 22:29 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
Warn if libv4l2 is detected along with USERPTR (1.09 KB, patch)
2014-01-15 22:30 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
v4l2bufferpool: Validate that capture buffers were queued (1.58 KB, patch)
2018-07-14 03:13 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2src: Simplify format handling (2.85 KB, patch)
2018-08-17 13:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2bufferpool: Activate the other pool first (1.45 KB, patch)
2018-08-17 13:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2object: Only allow DMABuf export for STREAMING device (1017 bytes, patch)
2018-08-17 13:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2bufferpool: Only queue buffer if preparation worked (1.63 KB, patch)
2018-08-17 13:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2bufferpool: Fix typo in error message (815 bytes, patch)
2018-08-17 13:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2allocator: Trace the buffer index we import to (2.28 KB, patch)
2018-08-17 13:03 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2object: Add a method to try and import buffers (6.65 KB, patch)
2018-08-17 13:03 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2bufferpool: Validate stride/offset when importing (1.71 KB, patch)
2018-08-17 13:03 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-26 13:35:44 UTC
v4l2src uses own mmapped buffer pool, but should ideall request buffers from xvideo for zerocopy. initial patch follows.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-26 13:41:46 UTC
Created attachment 135383 [details] [review]
implement pad_alloc_buffer usage

For testing, please run:
GST_DEBUG="v4l2*:3" gst-launch-0.10 2>&1 v4l2src always-copy=false ! "video/x-raw-yuv,width=640,height=480" ! xvimagesink

if you get this, then the driver does not support userspace pointers when streaming :/
v4l2src_calls.c:1437:gst_v4l2src_capture_init:<v4l2src0> allocate buffers via mmap
v4l2src_calls.c:1459:gst_v4l2src_capture_init:<v4l2src0> capturing buffers via streaming

If you see this, it works
v4l2src_calls.c:1450:gst_v4l2src_capture_init:<v4l2src0> allocate buffers via pad_alloc
v4l2src_calls.c:1460:gst_v4l2src_capture_init:<v4l2src0> capturing buffers via streaming

Another test is to use GST_DEBUG="xv*:5" and grep for "slow". It should not appear anymore when userspace pointer are in use.
Comment 2 Jan Schmidt 2009-05-26 14:48:21 UTC
With this patch, I'm still getting mmap and "slow bufferpool" copies:

0:00:00.456976926 11980  0x8650078 INFO               v4l2src v4l2src_calls.c:1437:gst_v4l2src_capture_init:<v4l2src0> allocate buffers via mmap
0:00:00.457176186 11980  0x8650078 INFO               v4l2src v4l2src_calls.c:1459:gst_v4l2src_capture_init:<v4l2src0> capturing buffers via streaming

I'm testing with a USB connected built-in iSight.
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-26 15:25:39 UTC
Forgot to tell about one detail - in order to use pad_alloc the patch probes if the diver supports userspace buffers before it uses mmaped ones. Not all cameras support user-space buffers. So my question above was more about who has a camera that supports it.

Jan, thanks for confirming that my patch did not broke usual operation.

One detail that needs thought is, that if userspace pointers work, one still needs to set always-copy=false. I wonder how many of todays drivers need that flag to be true by default.
Comment 4 Olivier Crête 2009-05-26 15:38:37 UTC
Even if the buffer is not used, the source should still probably do the pad_alloc if we want upstream renegotiation to work...
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-27 13:47:31 UTC
Created attachment 135437 [details] [review]
implement pad_alloc_buffer usage

This patch
* calls REQBUFS once more - there are drivers that want a num-of-buffers instead of 0 (according to the specs)
* v4l2 associates buffer addresses with buffer index when a buffer is used first. thus we can't requeue different buffers later on.
Comment 6 Jan Schmidt 2009-05-27 15:00:36 UTC
Go and run 'git submodule update' right now :) That'll get rid of 'common' from your diff, and remove any chance that you'll accidentally make a commit that reverts common to an old version.

This patch works OK for me - no regressions afaics. I still don't get downstream buffer allocations on this iSight though.
Comment 7 Jan Schmidt 2009-05-27 15:10:56 UTC
I get this from the driver:

0:00:00.393741915 24116  0x8e92078 DEBUG              v4l2src v4l2src_calls.c:1473:gst_v4l2src_capture_init:<v4l2src0> VIDIOC_REQBUFS(V4L2_MEMORY_USERPTR) failed 22:Invalid argument
0:00:00.393757280 24116  0x8e92078 DEBUG              v4l2src v4l2src_calls.c:1478:gst_v4l2src_capture_init:<v4l2src0> VIDIOC_REQBUFS(V4L2_MEMORY_USERPTR) for 3 buffers failed 22:Invalid argument

It doesn't seem to like any queue-size I feed it, 2, 3, 4, 8, so I guess the driver just doesn't support V4L2_MEMORY_USERPTR. It's uvcvideo, which is possibly the most common web camera driver already, or soon will be.
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-27 15:41:53 UTC
Created attachment 135446 [details] [review]
implement pad_alloc_buffer usage

This time without common changes. Need to remmeber that git diff ignores the directory I am in :/
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-27 15:44:29 UTC
Olivier, you mean if should check the CAPS from pad_alloc'ed and switch to closest video resolution or new fps?
imho, v4l2 is not good at switching modes on the fly.

Jan, I will see if I can reach some v4l2 devs and check about which drivers actualy implement this feature (except the nokia ones :).

Comment 10 Olivier Crête 2009-05-27 15:54:53 UTC
Stefan, yes thats exactly what I'm thinking of.. Since I'm thinking of implementing switching the resolution/framerate farsight sends on the fly depending on network conditions. So pushing that change as far up the pipeline as possible seems the most sensible thing to me.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-27 18:59:31 UTC
urgs, zc0301 (Logitech, Inc. QuickCam for Notebooks) does not support userio as well :/. (it works fine with always-copy=false).

Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2009-05-28 09:57:42 UTC
also uvcvideo (Logitech, Inc. QuickCam Communicate Deluxe) does not support it. I grepped the kernel sources under
http://bugzilla.gnome.org/show_bug.cgi?id=583890
and gspc driver seems to support it.
Comment 13 Filippo Argiolas 2009-05-28 12:57:13 UTC
Stefan, I tried both with 0c45:62c0 Microdia Sonix USB 2.0 Camera (uvcvideo) and 046d:092f Logitech, Inc. QuickCam Express Plus (gspca, in tree driver, aka gspca v2).
Unfortunately it seems that neither of them supports userspace buffers, I always get: "allocate buffers via mmap".
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2009-06-01 07:47:46 UTC
Started a thread on linux-media kernel mlist:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/6209
Comment 15 Sebastian Dröge (slomo) 2009-08-14 09:31:14 UTC
What's the status of this bug? Can the patch be committed?
Comment 16 Stefan Sauer (gstreamer, gtkdoc dev) 2009-08-17 20:45:21 UTC
It currently only brings benefits for the OMAP3 v4l2 driver which is open source but not yet part of the official linux kernel. I know that there is some work beeing done to add support for the in the UVC driver. If this gets into the linux kernel tree, its easier to verify. Maybe we should wait for that. Most probably I need to update the patch now to work with the recent v4l2sink changes too.
Comment 17 Arnout Vandecappelle 2010-03-02 16:13:15 UTC
I'm trying to port this feature to 0.10.16.  The patch fails since commit f19cfbda introduced the bufferpool in a separate file.  However, while fixing it, I noticed the following.

In the mmap way, v4l2_buffers are requeued in the gst_v4l2_buffer_finalize function.  However, in the pad_alloc way, the buffers are not GstV4l2Buffers, so the finalize function isn't called.  It seems to me that the buffers are never requeued.  Am I right?
Comment 18 Arnout Vandecappelle 2010-03-02 16:15:36 UTC
Argh, I moved too soon: didn't notice the gst_v4l2_buffer_pool_update function because it wasn't merged in yet...
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-03 11:25:58 UTC
I also tried to port my patch forward but it failed. I think some small parts could be split out and pushed easily. I think my mail problem was the buffer recovery and also make it work so that it does not affect v4l2sink.
Comment 20 Arnout Vandecappelle 2010-03-03 11:47:52 UTC
I have something that works now, except that it doesn't :-P

I'm using it on an OMAP3.  However, the omap v4l2 driver requires video buffers to be 32-byte aligned.  AFAIK there is no way to specify alignment constraints on a pad_alloc, and anyway there is no way to know that the alignment constraints are :-).  Suggestions on how to solve it are welcome, but for me it doesn't matter because the downstream element will be hardware accelerated element using DMAI buffers.

For the v4l2sink, I think it won't be affected by my patch but I can't test it (for lack of a v4l2sink).
Comment 21 Arnout Vandecappelle 2010-03-03 11:50:18 UTC
Created attachment 155125 [details] [review]
implement pad_alloc_buffer usage

Port of the patch to 0.10.16.  Should also apply relatively cleanly to HEAD.
Comment 22 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-03 13:44:15 UTC
Arnout, regarding the alignment, see bug #596832. We use this in maemo.
Comment 23 Arnout Vandecappelle 2010-03-03 19:01:04 UTC
I suspect that this patch will not when it would actually be useful (i.e. for a hardware buffer), because the kernel videobuf implementation doesn't support VM_IO memory.  See http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/16831

Regarding the alignment: I'll forget about that then.
Comment 24 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-03 21:13:11 UTC
(In reply to comment #23)
> I suspect that this patch will not when it would actually be useful (i.e. for a
> hardware buffer), because the kernel videobuf implementation doesn't support
> VM_IO memory.  See
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/16831
> 
> Regarding the alignment: I'll forget about that then.

We're discussing that in Maemo also. We are passing framebuffer segments and those should not need VM_IO. The memory is real memory and no maped register sets.
Comment 25 Arnout Vandecappelle 2010-03-04 17:01:10 UTC
Created attachment 155243 [details] [review]
implement pad_alloc_buffer usage

The downstream-allocated buffers were loosing their ref.  Indeed, BaseSrc's call of create() owns the ref of the returned buffer.  In the old implementation, the buffers were recovered in the finalize functions, but for downstream-allocated buffers we have no control over the finalize.  Now they are recovered in bufferpool_update(), but by that time it's already too late (buffer has been killed).

So, what I do is I take a ref when a buffer is dequeued.  This way, all the buffers in the pool always have at least one ref.

Stefan, didn't you have this problem too in your original implementation?
Comment 26 Arnout Vandecappelle 2010-03-04 17:57:40 UTC
Created attachment 155248 [details] [review]
implement pad_alloc_buffer usage

Buffer should not be requeued if either V4L2_BUF_FLAG_QUEUED or V4L2_BUF_FLAG_DONE is set (was only checking for queued).
Comment 27 Sebastian Dröge (slomo) 2011-05-20 06:32:29 UTC
What's missing here now to get these patch committed?
Comment 28 Olivier Crête 2011-05-20 14:45:42 UTC
From a quick look, the patch does not verify that the returned buffer from pad alloc has the requested caps/size. It should discard it if it doesn't and try to renegotiate I guess?
Comment 29 Sebastian Dröge (slomo) 2011-05-23 08:27:05 UTC
(In reply to comment #28)
> From a quick look, the patch does not verify that the returned buffer from pad
> alloc has the requested caps/size. It should discard it if it doesn't and try
> to renegotiate I guess?

Yes
Comment 30 Tim-Philipp Müller 2011-05-23 08:43:09 UTC
Updating bug summary a little, since there are two aspects here: (a) use gst_pad_alloc_buffer() to support upstream re-negotiation. This could be done independently of USERPTR support, at least for the default always-copy=true mode. (b) support capturing into downstream-allocated buffers, zero-copy / USERPTR, etc.
Comment 31 Wim Taymans 2013-04-25 07:20:23 UTC
USERPTR is not yet supported in 1.0. is this still relevant?
Comment 32 Arnout Vandecappelle 2013-04-26 06:48:57 UTC
I needed it three years ago to be able to make a zerocopy camera-to-hardware-encoder pipeline on a DaVinci. But since the company using that has gone bankrupt, it's not really relevant to me anymore, no.
Comment 33 Tim-Philipp Müller 2013-04-26 11:45:25 UTC
> USERPTR is not yet supported in 1.0. is this still relevant?

Is there any other way to capture into down-stream allocated buffers? If no, then it's still interesting IMHO.
Comment 34 Wim Taymans 2013-04-26 12:14:07 UTC
(In reply to comment #33)
> > USERPTR is not yet supported in 1.0. is this still relevant?
> 
> Is there any other way to capture into down-stream allocated buffers? If no,
> then it's still interesting IMHO.

Well, we use write for that currently but I guess USERPTR could be more efficient. I don't have the hardware to test this, though.
Comment 35 Nicolas Dufresne (ndufresne) 2013-11-20 20:38:00 UTC
Right now I'm into experimenting USERPTR support for output device. The patter would be:

decoder -> v4l2sink

The decoder would produce mmap buffer and v4l2sink will consume them using USERPTR. The other way around would have been nice, but is not support on the HW I'm working on (Exynos 4). None of the patches here are useful for 1.0.
Comment 36 Sebastian Dröge (slomo) 2013-11-25 12:57:40 UTC
The problem with USERPTR is that it's completely unknown what the requirements for the memory are. Is it enough to be some memory? Does it need to be physically contiguous? Does it need to be something else?

This information can't be retrieved via V4L2 and is completely dependant on the hardware and driver AFAIK. Due to this I don't think it makes sense to generically implement USERPTR support as it will never work reliably in all situations.
Comment 37 Nicolas Dufresne (ndufresne) 2013-11-25 17:17:00 UTC
(In reply to comment #36)
> This information can't be retrieved via V4L2 and is completely dependant on the
> hardware and driver AFAIK. Due to this I don't think it makes sense to
> generically implement USERPTR support as it will never work reliably in all
> situations.

It's true that V4L2 lacks some way to probe this. A driver that supports USERPTR in some ways, will succeed REQBUF, that we can probe easily. We can also try and queue malloc() buffer, the problem is that we cannot guaranty our probing is not accidentally physically contiguous.

On the V4L2 side, there is currently 3 type of allocators being used.

The vmalloc allocator (used notably by the UVC driver and most devices using serial line) is the most flexible for userspace, and will do userptr from malloc().

The original dma allocator, this is used by the majority of the drivers since it's the easiest fit for any uses cases. This allocated produce and consume physically contiguous memory. Drivers that do support USERPTR and use that allocator will only work if the passed buffer are contiguous (regardless of the source, it will check each pages and make sure one is next to the other).

Finally, there is the rest, which are custom allocators, can be based on framebuffer. It usually check the location of the blocks, contiguous or not, in memory.

In the long term, the IOMMUs should remove this limitation, but meanwhile there is no way to be 100% sure through probing what is being used. The solution we'd like to bring here, is to be able to force this mode and let the user make that decision (io-mode=userptr), this is already in the API. For the automatic mode, we'd like to use white list for activating it.

For the vmalloc case, it quite simple, and it should benefit one of the most used driver, which is UVC (if the is a format match, ovbiously). e.g.

gst-launch-1.0 v4l2src io-mode=userptr ! xvimagesink

It's useful now, but I'm aware that if DMABUF make it to XV, it would become obsolete. On Wayland, dmabuf shall be used. Note that the code for dmabuf handling will be very similar to USERPTR in the end.

For the dma case, we'd have another white list, and base on that we would set the unused GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS when producing mmap buffers. It's a lot more work to handle this case, we will probably have a fallback to dma (or r/w) in the likely case incoming buffer do not have this flags (I'm not sure this flag can be negotiated really). We can try and avoid this fallback by retaining two buffers in our userptr pool, though this assumed two first buffer are right. the important part I think is that we don't fail because of us attempting to optimize things.
Comment 38 Sebastian Dröge (slomo) 2013-11-26 12:38:46 UTC
Alright, I'm fine with a) having a way to force userptr behaviour and b) having white-lists for automatically using that if nothing better can be used (e.g. dmabuf) :) Just don't enable it by default for all devices, and also don't do any unreliable probing of the capabilities.
Comment 39 Aurélien Zanelli 2013-12-31 12:02:09 UTC
Hi,

I've also works on user pointer support for v4l2/v4l2sink.
My first goal was to enable a zero copy path between gst-omx and v4l2sink. In this case, buffers are allocated by the video decoder (hardware allocated contiguous memory) and provided to v4l2sink.

I wrote a hack based on gst-plugins-good 1.2 branch (rebased from 1.2.2 release) for its purpose.
It enables USERTPR mode for output video only. It tracks incoming buffer across qbuf/dqbuf mechanisms by using V4l2Meta structure.
Also in case the upstream element uses v4l2bufferpool, buffers are allocated in userspace, but some devices requires memory alignment and/or contiguous memory as said in previous post, so it could fail.

Also i could test other patch about this topic. Feel free to ask questions.

Regards,
Comment 40 Aurélien Zanelli 2013-12-31 12:02:19 UTC
Created attachment 265065 [details] [review]
v4l2: Add support for USERPTR mode in VIDEO_OUTPUT.
Comment 41 Julien Isorce 2013-12-31 14:45:00 UTC
(In reply to comment #39)
(In reply to comment #40)
> 
> 
> Also i could test other patch about this topic. Feel free to ask questions.
> 
> Regards,

Hi,

Nice!, I have also something here for both 1.2.2 and master:

http://cgit.collabora.com/git/user/julien/gst-plugins-good.git/log/?h=v4l2videodec-all

I added support for USERPTR mode in both VIDEO_OUTPUT and VIDEO_CAPTURE.
Would be good to compare with your VIDEO_OUTPUT solution.
Have you tried the VIDEO_CAPTURE case ?

Cheers,
Julien
Comment 42 Aurélien Zanelli 2013-12-31 15:07:51 UTC
(In reply to comment #41) 
> http://cgit.collabora.com/git/user/julien/gst-plugins-good.git/log/?h=v4l2videodec-all
I have a quick look at it and it seems your solution is more elegant and better.
I will try to test it as soon as possible.

> Have you tried the VIDEO_CAPTURE case ?
I didn't try to implement the VIDEO_CAPTURE case since i don't have a capture device.
Comment 43 Sebastian Dröge (slomo) 2014-01-03 09:26:50 UTC
Should we mark this as duplicate of the other bug then and close it? Julien?
Comment 44 Julien Isorce 2014-01-03 09:57:21 UTC
(In reply to comment #43)
> Should we mark this as duplicate of the other bug then and close it? Julien?
I think it's ok to keep it. Once the v4l2 decoders upstream I'll attach USERPTR patchs here. (it allows zero-copy but it's not a requirement to enable decoders)
Comment 45 Aurélien Zanelli 2014-01-06 09:03:13 UTC
Julien, I made a quick test of the '1.2-cc-delivery' branch.

I try the following pipeline:
filesrc ! demux ! parser ! omxdecoder ! v4l2sink io-mode=3

The behavior seems good, omxdecoder provides its bufferpool of allocated "hardware" memory to v4l2sink. In v4l2bufferpool, it uses the right code path: (/* transfer userptr on the fly */) but it fails to add metadata to buffer since it is not writable. It leads to a SIGSEGV due to following memset in gst_v4l2_buffer_pool_add_meta_helper().
Comment 46 Julien Isorce 2014-01-06 11:03:44 UTC
(In reply to comment #45)
> but it fails to add metadata to buffer since it is not writable.

I have not tested this case (here the incoming buffer comes from v4l2videodecoder so it already has a meta) because I have not the material to do that. Indeed on the cc device, I have not the support for vmalloc for userptr (maybe due to kernel 3.4 or s5p-mmixer driver), I mean I cannot do videotestsrc ! v4l2sink userptr.
Whereas on my desktop I can do v4l2src userptr ! videoconvert ! ximagesink  (kerlnel 3.11, uvcvideo, remove libv4l). I have v4l2loopback for the sink but it does not support userptr.


Anyway :) Try the 3 last commits:

http://cgit.collabora.com/git/user/julien/gst-plugins-good.git/log/?h=1.2-omxdec_v4l2sink-USERPTR

When entering "gst_v4l2_buffer_pool_process" the incoming buffer must have a reference counter equal to 1 to be writable. (Calling gst_buffer_make_writable is not useful here as it would make a copy)

For some reasons here the v4l2videodecoder always gives me a buffer with refcount equal to 2 (but I already have the meta so no pb) but maybe with omxdecoder it will be 1. (the first of the 3 commits fixes the fact that I was adding the meta after increasing the refcount) (another problem will be with the preroll frame which has one more ref count so I inactivate it in the third commit for now)
Comment 47 Aurélien Zanelli 2014-01-07 09:02:48 UTC
(In reply to comment #46)
> When entering "gst_v4l2_buffer_pool_process" the incoming buffer must have a
> reference counter equal to 1 to be writable. (Calling gst_buffer_make_writable
> is not useful here as it would make a copy)
I agree.

> For some reasons here the v4l2videodecoder always gives me a buffer with
> refcount equal to 2 (but I already have the meta so no pb) but maybe with
> omxdecoder it will be 1.
The incoming buffer has also a refcount of 2. I will try to understand why.
Comment 48 Aurélien Zanelli 2014-01-07 13:56:01 UTC
I finally success to play a video using your branch with v4l2sink using gst-omx buffers.
It's gst_base_sink_set_last_buffer_unlocked() which increase the refcount of the incoming buffer. So i disable the last-sample feature of the base sink (with enable-last-sample property).
By doing so the incoming buffer has a refcount of one.
Also i found a little mistake in your code after "/* transfer userptr on the fly */" ( v4l2bufferpool.c:gst_buffer_pool_process() ):
Whan the buffer doesn't contain meta. you call "gst_v4l2_buffer_pool_add_meta_helper (pool, to_queue);" but you don'"t assign the return value to meta. ie : it should be:

> if (!meta)
>   meta = gst_v4l2_buffer_pool_add_meta_helper (pool, to_queue);
Comment 49 Julien Isorce 2014-01-07 19:05:51 UTC
(In reply to comment #48)
> I finally success to play a video using your branch with v4l2sink using gst-omx
> buffers.

Cool!

> It's gst_base_sink_set_last_buffer_unlocked() which increase the refcount of
> the incoming buffer. So i disable the last-sample feature of the base sink
> (with enable-last-sample property).
> By doing so the incoming buffer has a refcount of one.

Ah nice catch.

> Also i found a little mistake in your code after "/* transfer userptr on the
> fly */" ( v4l2bufferpool.c:gst_buffer_pool_process() ):
> Whan the buffer doesn't contain meta. you call
> "gst_v4l2_buffer_pool_add_meta_helper (pool, to_queue);" but you don'"t assign
> the return value to meta. ie : it should be:
> 
> > if (!meta)
> >   meta = gst_v4l2_buffer_pool_add_meta_helper (pool, to_queue);

Thx. I reported it in one of my branch: http://cgit.collabora.com/git/user/julien/gst-plugins-good.git/commit/?h=1.2-cc-delivery&id=bec4169411007bf3c745fb84168b9894c43044e2

I found a way to test this case, just doing videotestsrc ! v4l2sink userptr
It fallbacks to mmap but at least it reaches the code you mentioned.
Comment 50 Nicolas Dufresne (ndufresne) 2014-01-15 22:29:04 UTC
Created attachment 266405 [details] [review]
USERPTR for OUTPUT
Comment 51 Nicolas Dufresne (ndufresne) 2014-01-15 22:29:27 UTC
Created attachment 266406 [details] [review]
USERPTR for CAPTURE
Comment 52 Nicolas Dufresne (ndufresne) 2014-01-15 22:29:45 UTC
Created attachment 266407 [details] [review]
Dynamically enable USERPTR
Comment 53 Nicolas Dufresne (ndufresne) 2014-01-15 22:30:21 UTC
Created attachment 266408 [details] [review]
Warn if libv4l2 is detected along with USERPTR

Libv4l does not support USERPTR :-(
Comment 54 Julien Isorce 2014-01-21 08:51:09 UTC
(In reply to comment #48)
> I finally success to play a video using your branch with v4l2sink using gst-omx
> buffers.

Hi Aurélien, on which platform/board have you run this ? :)
Comment 55 Aurélien Zanelli 2014-01-21 15:56:15 UTC
(In reply to comment #54)
> (In reply to comment #48)
> > I finally success to play a video using your branch with v4l2sink using gst-omx
> > buffers.
> 
> Hi Aurélien, on which platform/board have you run this ? :)

Hi Julien, i ran this on my company hardware. It have an OMX decoder component and a v4l2 compliant device.
Unfortunately, i don't know 'public' hardware on which i could use the same pipeline. If i'm right, Raspberry Pi render frame in an EGL context and have no v4l2 output device. and Pandaboard use TI Ducati API to decode video.

However, if i'm wrong about these two boards or if you know boards in which i could make the same test, i can try to get it and run  gst-omx --> v4l2sink in userptr mode.
Comment 56 Nicolas Dufresne (ndufresne) 2014-01-21 21:36:20 UTC
I think with your report, and our testing, we have enough data to know that it works. There exist some issues for cases where one need a large queue, but it would be a problem even in mmap. I tempted to merge this, unless there is objections of course.
Comment 57 Aurélien Zanelli 2014-04-03 09:12:50 UTC
What is the status of userptr integration into GStreamer ?
Comment 58 Nicolas Dufresne (ndufresne) 2014-04-03 15:46:44 UTC
I'm currently working on implementing a GstV4l2Allocator, as right now in master we randomly loose buffers (they don't come back into our pool) for various reason. This is a bit incompatible with this branch, USERPTR support will need a new design. What I have in mind:

What's new:
GstV4l2Allocator
  GstV4l2MemoryGroup
  GstV4l2Memory

the method we care for userptr (just drafting here):
GstV4l2MemoryGroup *gst_v4l2_allocator_alloc_userptr (allocator, mem[], stride[], offset[], user_data);

For capture:
Pool would fill the allocator from downstream pool using that method. Mem being extracted and reffed from the buffer. The pool would create an internal buffer with that, rather then pooling the downstream buffer. When dqbuf, the pool will be able to get back the original buffer using the user_data; The pool will be responsible for correct recounting of that buffer.

For output:
Pool would try and fill the internal pool the same way, with buffers as they come. And will ensure to keep at least two (of min buffers for output). Unlike previous attempt, a pool need to always be offered on propose_allocation, and pool that has been started/stop should not affect the decision. This remains risky mode, as we need to trust upstream for respecting the requirement we have exposed in propose_allocation. Also, it's not possible to fallback after first buffer has been enqueued, hence we'll fail completly if second buffer is incompatible (will nearly never happen), and reused what we have queued afterward.
Comment 59 Nicolas Dufresne (ndufresne) 2014-05-08 20:18:08 UTC
This is supported (if manually set) now.
Comment 60 Nicolas Dufresne (ndufresne) 2014-05-08 20:25:20 UTC
Sorry, I did not realized this was also about dynamic use of it. This is to be ported to the new code.
Comment 61 Tim-Philipp Müller 2016-01-19 17:27:14 UTC
What's up with this?
Comment 62 Nicolas Dufresne (ndufresne) 2016-01-19 20:39:30 UTC
(In reply to Tim-Philipp Müller from comment #61)
> What's up with this?

It's still not dynamic. It's a valid feature request, not a priority though.
Comment 63 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-20 08:13:58 UTC
(In reply to Nicolas Dufresne (stormer) from comment #62)
> (In reply to Tim-Philipp Müller from comment #61)
> > What's up with this?
> 
> It's still not dynamic. It's a valid feature request, not a priority though.

By dynamic, you mean changing "io-mode" while the pipeline runs? I would not mind tracking this as a new request and I agree on the prio.
Comment 64 Nicolas Dufresne (ndufresne) 2016-01-20 22:03:35 UTC
Sure, I have no problem with that. Now, it's not "changing" the user requested io-mode. That would not be correct. The default io-mode is called "auto". Right now it's auto in the sense that it will choose between legacy read/write and mmap. What Julien's patch was doing, is to add on top to that the ability to automatically import external memory into that driver using USERPTR. Due to alignment, type of memory and other restrictions, one cannot assume that will always work, so this comes with a fallback to mmap. It's similar for DMABuf importation.
Comment 65 Nicolas Dufresne (ndufresne) 2018-07-14 03:13:47 UTC
Created attachment 373056 [details] [review]
v4l2bufferpool: Validate that capture buffers were queued

When the pool is started, we allocate and release buffer, expecting
the pool release-buffer handler to queue them. Though, as we rely
on release function, there is no direct way to detect that this
process didn't work.

To check this, validate that the number of queued buffer is the same
as the number of allocated buffers. This allow returning an error
when buffer importation was refused by the driver.
Comment 66 Nicolas Dufresne (ndufresne) 2018-07-14 03:15:43 UTC
Working on this now, this patch is really the very first step. I'll merge immediately as without that the pipeline just stall if you force io-mode=userptr and the downstream buffer size didn't match what the driver expected. Of course, there is plenty of ways the size can match but yet, the buffer isn't right, so more validation will come.
Comment 67 Nicolas Dufresne (ndufresne) 2018-07-14 03:16:04 UTC
Comment on attachment 373056 [details] [review]
v4l2bufferpool: Validate that capture buffers were queued

Attachment 373056 [details] pushed as 9e79821 - v4l2bufferpool: Validate that capture buffers were queued
Comment 68 Nicolas Dufresne (ndufresne) 2018-08-17 13:02:30 UTC
Created attachment 373380 [details] [review]
v4l2src: Simplify format handling

Always initially use try_format(), delaying set_format() to when the
allocation is being negotiated. This avoid having two code paths, and
will be help adding support for properly importing buffers of specific
strides and offsets.
Comment 69 Nicolas Dufresne (ndufresne) 2018-08-17 13:02:40 UTC
Created attachment 373381 [details] [review]
v4l2bufferpool: Activate the other pool first

This change has no effect. We will need to acquire a buffer from the
pool later in order to validate / adapt with the video alignment for
the downstream buffers.
Comment 70 Nicolas Dufresne (ndufresne) 2018-08-17 13:02:47 UTC
Created attachment 373382 [details] [review]
v4l2object: Only allow DMABuf export for STREAMING device

DMABuf exportation requires mmap, which requires STREAMING
capabilities.
Comment 71 Nicolas Dufresne (ndufresne) 2018-08-17 13:02:53 UTC
Created attachment 373383 [details] [review]
v4l2bufferpool: Only queue buffer if preparation worked

The preparation code imports the buffer, doing bunch of
validation. Only queue the buffer in the driver if the
importation worked. This way we don't rely on the driver
to validate.
Comment 72 Nicolas Dufresne (ndufresne) 2018-08-17 13:02:58 UTC
Created attachment 373384 [details] [review]
v4l2bufferpool: Fix typo in error message
Comment 73 Nicolas Dufresne (ndufresne) 2018-08-17 13:03:05 UTC
Created attachment 373385 [details] [review]
v4l2allocator: Trace the buffer index we import to
Comment 74 Nicolas Dufresne (ndufresne) 2018-08-17 13:03:10 UTC
Created attachment 373386 [details] [review]
v4l2object: Add a method to try and import buffers

This method will check if a buffer, base on it's video meta,
can be imported. It will also try and adapt the request stride
in case this is the only that miss-match.
Comment 75 Nicolas Dufresne (ndufresne) 2018-08-17 13:03:16 UTC
Created attachment 373387 [details] [review]
v4l2bufferpool: Validate stride/offset when importing

This will prevent situation where buffer size allow importing but rendering
goes wrong due to a miss-match in expected stride and offset.
Comment 76 Nicolas Dufresne (ndufresne) 2018-08-27 17:42:06 UTC
Attachment 373380 [details] pushed as fe3a70f - v4l2src: Simplify format handling
Attachment 373381 [details] pushed as 90a70e0 - v4l2bufferpool: Activate the other pool first
Attachment 373382 [details] pushed as 24368c1 - v4l2object: Only allow DMABuf export for STREAMING device
Attachment 373383 [details] pushed as 77c052f - v4l2bufferpool: Only queue buffer if preparation worked
Attachment 373384 [details] pushed as 73555b5 - v4l2bufferpool: Fix typo in error message
Attachment 373385 [details] pushed as d6d8187 - v4l2allocator: Trace the buffer index we import to
Attachment 373386 [details] pushed as 1c729be - v4l2object: Add a method to try and import buffers
Attachment 373387 [details] pushed as 480a7bc - v4l2bufferpool: Validate stride/offset when importing
Comment 77 Nicolas Dufresne (ndufresne) 2018-08-27 17:45:12 UTC
Wanted to keep this one open, so next step would be to implement the same for sink pads. After that, we should be able to enable this automatically. In parallel, there is some work to add height padding support to GstV4L2Transform, https://bugzilla.gnome.org/show_bug.cgi?id=796986
Comment 78 GStreamer system administrator 2018-11-03 14:39:49 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/14.