GNOME Bugzilla – Bug 707793
dmabuf allocator shouldn't a singleton
Last modified: 2013-09-18 19:15:48 UTC
Created attachment 254509 [details] [review] dmabuf: Make it not a singleton dmabuf memory is most often associated with a device. For example, in v4l2, the format of the device can not be re-negotiated (one can't call VIDIOC_S_FMT) until all buffers have been freed. So we need to have a way to track when all buffers coming out of that device has been freed. This can't be tracked at the GstBuffer level unless we set the NO_SHARE flag and lose zero-copy functionality. My solution is to have the allocator do that, and that means that you need one allocator object per device. See bug #707534 for the complete solution. This is about the API change, so it needs to be reviewed/decided before 1.2 is out.
Note that zerocopy is possible with NO_SHARE flag too.
Well, you can't subbuffer, or create a buffer containing the no_share-marked memory as one component, can you ?
Yes, you can't :)
Comment on attachment 254509 [details] [review] dmabuf: Make it not a singleton I think this change makes sense independent of the other changes already.
Created attachment 254727 [details] [review] v4l2bufferpool: dmabuf is not a singleton anymore
Benjamin? Michael? You seem to be the other other patch writers for dmabuf, what do you think ?
Review of attachment 254509 [details] [review]: ::: gst-libs/gst/allocators/gstdmabuf.c @@ +311,2 @@ #else /* !HAVE_MMAP */ what happens if HAVE_MMAP isn't defined ? Maybe we need a gst_dmabuf_allocator_new() returning NULL here.
That would break binding assumptions
I agree that making it not a singleton is a step in the right direction. About the mmap problem: I don't know what you mean about the binding assumptions, but how about this: Only do the ifdef in gst_dmabuf_mem_{map,unmap}(). In theory a dmabuf memory could be useful without mapping it. Both producer and consumer could work only with the fd. Such a scenario is probably rather unlikly but this would also minimize the ifdef.
What about something a bit more radical, just not offering that API at all on Win32 (any other platforms don't have mmap?). I'm not sure if there is any other good reason to offer APIs that will never work.
It is a little to bit radical for me. What are GStreamer guide lines about this ?
So far we always provided the same API on all platforms in all configurations. I'd prefer to keep it that way. I like Michael's suggestion in comment 11.
Pushed both commit d6187c00a680cb992e028a781d1a6599d133cf6f Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Sep 4 20:21:54 2013 -0400 dmabuf: Make it not a singleton Makes it easier to track how many users there are Also make it possible to create a dmabuf struct on systems without mmap, it just won't be possible to map it. https://bugzilla.gnome.org/show_bug.cgi?id=707793 commit d1e16f7b0c425502d3aecdb50aa2b659f25b2e89 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Sep 11 14:27:02 2013 -0400 v4l2bufferpool: dmabuf is not a singleton anymore https://bugzilla.gnome.org/show_bug.cgi?id=707793
Comment on attachment 254509 [details] [review] dmabuf: Make it not a singleton Pushed a slightly different patch with only map/unmap having ifdefs