GNOME Bugzilla – Bug 707534
v4l2: GstBuffers should not need to be marked no-share, need to track GstMemories separately from pool
Last modified: 2014-06-22 12:36:34 UTC
In 1.x, the GstBufferPool only keep track of the GstBuffer structures, not of the GstMemory, so they don't keep track of the underlying memory, it also means that the buffers that are released back into the buffer pools may contain completely different memory from what they originally had. This causes numerous bugs (#699517, #706083, #699382, #705678, #682770, etc). This also forces v4l2src to set all the memory as NO_SHARE, forcing useless copies, etc. I propose to make GstBufferPools track GstMemory independently from the Buffers. My approach is multi-step: 1. We need GstMemory to have a dispose method like GstBuffer. The problem is that to make it useful, we need to attach some userdata to the GstMemory, sadly, the GstMemory structure is not extensible. So I propose a nasty hack where the subclasses of GstMemory provide the storage and register an offset into the GstAllocator. 2. I made the GstBufferPool keep track of the GstMemory and set itself has the dispose handler, when a buffer is released, it will now do gst_buffer_remove_all_memory() to allow the memory to return to the pool. 3. I added the possibility for GstAllocators to keep track of all memory they have allocated and a method on the allocator to wait until all that memory has been released. 4. I added a method to GstBufferPool to say if they require their memory back before being de-activated and I modified GstBaseSrc (as an example) to do a drain query before deactivating the pool if required. I also made dmabuf not a singleton (API change, this bit should probably go in before 1.2 is released), this way, it can keep track of the allocated memory. I haven't tested dmabuf as it means testing on a I also implemented all of this in v4l2src as an example: 1. Added a Allocator for V4l2 in MMAP mode (not DMABUF). 2. Removed the GstV4l2Meta, instead attaching this info (struct v4l2_buffer) to the GstMemory using qdata. I can't put it into the GstV4l2MmapMemory because one could also be using the GstDmaBufAllocator. This means I significantly refactored v4l2bufferpool to track GstMemories as well as GstBuffers.. The v4l 3. Added code to wait for all buffers before trying to do REQBUFS(0) 4. Implemented calling the drain query if required. Branches: http://cgit.collabora.com/git/user/tester/gstreamer.git/log/?h=memory-pool http://cgit.collabora.com/git/user/tester/gst-plugins-base.git/log/?h=memory-pool http://cgit.collabora.com/git/user/tester/gst-plugins-good.git/log/?h=v4l2-alloc This is not merge-ready, it needs more testing and is not fully documented, but I'd appreciate everyone's opinion on the approach. Wim's release-pool style query (from bug #682770 comment #14) where draining the pipeline is not require woudl be better, but drain works as a first step, making a better query can be done later without breaking API.
Can you create a blocker bug for the dmabuf allocator API changes?
@slomo: The dmabuf API changes only make sense if people agree that the general approach of having the allocator track the buffers is correct.
The code to hook in returning memory to the pool is rather ugly. Can't we create a GstPoolMemory class that handles that and then use that class in all memory implementations? As far as I can tell the GstMemory subclasses are all private API so that should be possible. The stuff with draining is orthogonal to the memory change, right? This makes sence with buffers as well, right? Abuout the v4l2 stuff: Can you please split the first patch? It does a lot of things at once. That makes it hard to review. Why is gst_allocator_wait_for_empty() needed here? gst_v4l2_buffer_pool_free_buffers() is olny called after all buffers are back in the pool. The same should be true for memorys.
Had a long IRC discussion with Wim about this. He doesn't think we need to have a memory pool at all, he thinks the GstAllocator subclass should do that on their own, I'll re-write some of my branch to match that. I'll try to split up the patches at the same time. The draining stuff is indeed orthogonal. The reason for gst_allocator_wait_for_empty() is that once we remove the NO_SHARE flag on on the buffers (which is evil), then the GstMemory could get attached to buffers created by someone else (for example, if an element sub-buffers), so the GstBuffers may be back in the pool, but it doesn't mean all of the memory has been freed.
Handling more stuff in the allocator is a good Idea, I think. Though I'm not sure how we can handle this in a clean way in the v4l2 plugin. The GstV4l2BufferPool currently mixes buffer pool, memory allocation and v4l2 queue handling in on class. Separating the allocation parts might be tricky. Also, We'll probably have 2 possible allocators: dmabuf and mmap. Relying on the user to pick one isn't nice, but we cannot know if dmabuf is supported until we try to allocate a buffer. So the buffer pool must be able to switch allocators while allocating the first buffer... About the gst_allocator_wait_for_empty(): Right now gst_v4l2_buffer_pool_stop() and therefore gst_v4l2_buffer_pool_free_buffers() isn't called until all buffers are back in the pool. It's actually called when the last buffer returns if necessary. I think we should handle memories the same way. I think there may be dead locks with a blocking API.
My current idea is to nix the dmabuf allocator and instead make the V4l2 allocator produce dmabuf GstMemory blocks and have this all transparent for the user. We have to block stopping the v4l2 device until all v4l2_buffers are returned, otherwise we can't re-start it with different parameters.
(In reply to comment #6) > My current idea is to nix the dmabuf allocator and instead make the V4l2 > allocator produce dmabuf GstMemory blocks and have this all transparent for the > user. Good idea! > > We have to block stopping the v4l2 device until all v4l2_buffers are returned, > otherwise we can't re-start it with different parameters.
(In reply to comment #6) > My current idea is to nix the dmabuf allocator and instead make the V4l2 > allocator produce dmabuf GstMemory blocks and have this all transparent for the > user. > > We have to block stopping the v4l2 device until all v4l2_buffers are returned, > otherwise we can't re-start it with different parameters. Out of curiosity, after this change, who will the sink element detect that buffers are dmabuf ? (specially in the multi-plane use case)
Because the type will still be dmabuf, GstMemory types are just a string.
What's the plan here? Nicolas talked about things like this on IRC yesterday
Sorry, I should have updated this bug. Basically, I don't think that has to be generic, since not all buffer pool need to track memories. Having to track memory seems so far specific to OMX and V4L2, while, as an example, with VDPAU it's not needed. So I'm not sure if I could just rename this bug into a v4l2 specific one. The design I'm implementing, which I've discuss with Wim already is the following: In v4l2 I introduce GstV4l2Allocator GstV4l2MemoryGroup GstV4l2Memory The MemoryGroup ended up replacing the meta. It stores the v4l2_buffer and v4l2_plane array. A group is the only thing you can actually be allocated from the allocator (I set the CUSTOM_ALLOC flag). The Memory object, keeps a pointer to the MemoryGroup, in a way that one can get the group from the first memory and validate all the other memory (if there is more then one) using that. The Allocator become responsible for REQBUF/CREATE_BUFS, QBUFS/DQBUFS and with mmap/unmmap() or EXPBUF. It also tracks each memory, and put the group back on the free_queue when all references has been dropped. So far I'm doing it lockless (with help from atomic_queue) and didn't find any problem yet. I have a mmap only WIP, my intention is to port the buffer pool to that today and start testing. One can give a looks at the WIP at (note, I already have more changes today): http://cgit.collabora.com/git/user/nicolas/gst-plugins-good.git/log/?h=v4l2-allocator It's currently on top of another of my branches, though I'm working on splitting this up. The bottom part of that branch being general bugfix around the newly introduces decoder. There is a lot of work in correctly handling the error cases. The rest (except the allocator) is the addition of a transform element currently doing color transform.
I've pushed an allocator for v4l2 that handles that. It's a big set of patch, but for the reference: commit 2b0ac06ade6d1efec65b4e83552e33e6bb6f2795 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Apr 4 22:46:40 2014 -0400 v4l2bufferpool: Port to use GstV4l2Allocator commit fd13e9e96de440a0ee8d2881ad6d6c77d135427e Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Apr 4 22:35:48 2014 -0400 Implement V4l2 Allocator This goal of this allocator is mainly to allow tracking the memory. Currently, when a buffer memory has been modified, the buffer and it's memory is disposed and lost until the stream is restarted.