v4l2src: added dmabuf support #2
Conversation
There was a problem hiding this comment.
I see some work and cleanups are needed. The commits and message and the tile is miss-leading. The dmabuf import is already supported (and gained some fixes in 1.22 upstream). What you are actually trying to achieve is to maintain buffer index / dmabuf association, which has been missing since the start. Your commit message title and long description should detail that, and detail your solution. Without this, updating GStreamer later becomes really hard. You should also give it a try and submit this upstream, so we can discuss further.
The main questionable change is that before, we'd push the "other_pool" original buffer. This was done like this as some downstream element may check if the buffer is from their own pool implementation to actually do zero-copy. Its no longer the case in kmssink, but other code path may still have this shortcut.
| @@ -27,7 +27,7 @@ | |||
|
|
|||
| #include <gst/video/video.h> | |||
| #include <gst/gl/gl.h> | |||
| #include <gst/gl/gstglfuncs.h> | |||
| #include <ext/qt/gstqtgl.h> | |||
There was a problem hiding this comment.
This change is unrelated to the PR topic. Please remove and submit separately.
There was a problem hiding this comment.
Modified as per the suggestion
| @@ -25,7 +25,7 @@ | |||
| #include <stdio.h> | |||
|
|
|||
| #include <gst/video/video.h> | |||
| #include <gst/gl/gstglfuncs.h> | |||
| #include <ext/qt/gstqtgl.h> | |||
There was a problem hiding this comment.
Same as previous change.
There was a problem hiding this comment.
Modified as per the suggestion
| @@ -357,7 +357,11 @@ gst_v4l2_allocator_release (GstV4l2Allocator * allocator, GstV4l2Memory * mem) | |||
|
|
|||
| switch (allocator->memory) { | |||
| case V4L2_MEMORY_DMABUF: | |||
| if (V4L2_TYPE_IS_CAPTURE(allocator->obj->type) && (allocator->obj->mode == GST_V4L2_IO_DMABUF_IMPORT)) | |||
| break; | |||
There was a problem hiding this comment.
Please, consider backporting the upstream patch fixing this instead of deviating. This type of change would make future GStreamer upgrade difficult.
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5101
There was a problem hiding this comment.
Modified as per the suggestion
There was a problem hiding this comment.
By backporting, I meant to port the patch "v4l2: allocator: Don't close foreign dmabuf", and apply it as its own patch. This way we know where the change is from as it has cross-reference and original authorship.
There was a problem hiding this comment.
Modified as per your suggestion. Please review and comment.
| close (mem->dmafd); | ||
| GST_LOG_OBJECT (allocator, "close fd: %d", mem->dmafd); |
There was a problem hiding this comment.
Don't add debugging in your fork, it will simply make GStreamer update difficult later. This code is changed with https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5101, it is now limited to V4L2_MEMORY_MMAP (when the driver is exporter).
There was a problem hiding this comment.
Have added this change, But the above merge is done on 1.22 while we are using the 1.20 repo of gstreamer.
Will there be any issue in it?
| gst_v4l2_allocator_alloc_dmabufin_capture (GstV4l2Allocator * allocator, | ||
| GstAllocator * dmabuf_allocator, GstBuffer* downstream_buffer) | ||
| { | ||
| #if 1 |
There was a problem hiding this comment.
Don't leave personal debugging code please.
| group = gst_v4l2_allocator_alloc_dmabufin_capture (pool->vallocator, pool->allocator, downstream_buffer); | ||
| if (group == NULL) { | ||
| gst_buffer_unref (downstream_buffer); | ||
| GST_DEBUG ("fail to create buf"); |
There was a problem hiding this comment.
Ust GST_DEBUG_OBJECT(), so we see which pool is being traced. Improve your trace as its missing the dmabuf import for capture device context.
| if (group->mem[i] == NULL) { | ||
| dma_mem = gst_buffer_peek_memory (downstream_buffer, i); | ||
| int downstream_fd = gst_dmabuf_memory_get_fd (dma_mem); | ||
| int dup_fd = dup (downstream_fd); |
There was a problem hiding this comment.
This will be leaked as we no longer close dmabuf FD in dmabuf import mode. You also ref the foreign memory and store in as a quark, so you really don't have to dup the FD, you can borrow it instead.
| @@ -1268,6 +1365,7 @@ gst_v4l2_allocator_qbuf (GstV4l2Allocator * allocator, | |||
| for (i = 0; i < group->n_mem; i++) | |||
| gst_memory_ref (group->mem[i]); | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Spurious, please remove.
| if (V4L2_TYPE_IS_OUTPUT(obj->type)) | ||
| group = gst_v4l2_allocator_alloc_dmabufin (pool->vallocator); | ||
| else { | ||
| gst_buffer_pool_acquire_buffer (pool->other_pool, &downstream_buffer, NULL); |
There was a problem hiding this comment.
This can fail (notably during tear down and flush operations. Please check the return value and handle it correctly.
|
|
||
| } else if (V4L2_TYPE_IS_CAPTURE(obj->type) && (obj->mode == GST_V4L2_IO_DMABUF_IMPORT)) { | ||
| // since we reuse right index, so let's keep buffer number of downstream pool same with v4l2buffer pool | ||
| GST_DEBUG ("got min: %d own_min: %d max: %d min_buffers:%d", min, own_min, max, obj->min_buffers); |
There was a problem hiding this comment.
Again use GST_DEBUG_OBJECT(), use C99 comments /* */ to match this file style.
karanamd123
force-pushed
the
v4l2src_add_dmabuf_offset
branch
from
bf48303 to
7362fdd
Compare
Imported dmabuf are not being duped, so they should never be closed. Instead, we ensure their live time by having strong reference on their original buffer. This should fix potential flickering due to dmabuf being closed too early. Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5101>
karanamd123
force-pushed
the
v4l2src_add_dmabuf_offset
branch
from
7362fdd to
54a5b52
Compare
Added a dma fd offset support at gstreamer v4l2src. The patch is inspired from https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4550.