GNOME Bugzilla – Bug 784847
omxvideodec: add dmabuf support for zynqultrascaleplus
Last modified: 2017-07-18 22:59:51 UTC
.
Created attachment 355430 [details] [review] include OMX_IndexExt and OMX_ComponentExt if present These files may be used by OMX implementation to define custom extensions. Include them if present as we are already doing with OMX_VideoExt.h
Created attachment 355431 [details] [review] omxvideodec: add dmabuf support for output The zynqultrascaleplus OMX implementation has a custom extension allowing decoders to output dmabuf and so avoid buffers copy between OMX and GStreamer. Make use of this extension when built on the zynqultrascaleplus. The buffer pool code should be re-usable for other platforms as well.
Review of attachment 355431 [details] [review]: Looks clean ! I wrote a few questions. ::: configure.ac @@ +159,1 @@ Like this instead: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/configure.ac#n837 ? ::: omx/gstomxbufferpool.c @@ +27,3 @@ #include "gstomxbufferpool.h" +#include <gst/allocators/gstdmabuf.h> Like this https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglupload.c#n36 instead ? @@ +395,3 @@ + gint fd; + + fd = GPOINTER_TO_INT (omx_buf->omx_buf->pBuffer); I see that you do not see the memory:DMABuf caps features, so does it mean that you assume this fd is always mappable ? Like if you do a gst_memory_map(mem, FLAG_READ) will it succeed ? Same question with flag READWRITE ? @@ +518,2 @@ /* If it's our own memory we have to set the sizes */ + if (!pool->other_pool && !pool->dmabuf) { Maybe adjust the comment above. Like "... set the sizes except for dmabuf because ..." ::: omx/gstomxvideodec.c @@ +199,3 @@ self->dec_out_port = gst_omx_component_add_port (self->dec, out_port_index); +#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS && defined(OMX_ALG_BUF_DMA) ? Was it defined in older zyq-uscale versions ? Just asking maybe you do not want to support older versions ? @@ +217,3 @@ + GST_ERROR_OBJECT (self, "Failed to set output buffer mode: %s (0x%08x)", + gst_omx_error_to_string (err), err); + buffer_mode.eMode = OMX_ALG_BUF_DMA; Just to make, this is something you always want ? I.e. no fallback if setparam(do_dmabuf) fails ? I was thinking to just self->dmabuf = FALSE instead of return FALSE, something like that. And GST_INFO instead of GST_ERROR. But this is your decision, depends what you want.
Comment on attachment 355430 [details] [review] include OMX_IndexExt and OMX_ComponentExt if present Looks good.
Review of attachment 355431 [details] [review]: Thanks Julien. ::: configure.ac @@ +159,1 @@ You mean making the allocators a soft dep? It's fine with me but we already have some many #ifdef in gst-omx... They are part of -base so is there any reason why they wouldn't be available? ::: omx/gstomxbufferpool.c @@ +395,3 @@ + gint fd; + + fd = GPOINTER_TO_INT (omx_buf->omx_buf->pBuffer); It is yes (read and write). @@ +518,2 @@ /* If it's our own memory we have to set the sizes */ + if (!pool->other_pool && !pool->dmabuf) { Actually I'm not sure why we have to set the size and offset there for GstOMXMemory. Can't we just do it ingst_omx_memory_allocator_alloc() ? ::: omx/gstomxvideodec.c @@ +199,3 @@ self->dec_out_port = gst_omx_component_add_port (self->dec, out_port_index); +#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS Nope, we can rely on it being there. @@ +217,3 @@ + GST_ERROR_OBJECT (self, "Failed to set output buffer mode: %s (0x%08x)", + gst_omx_error_to_string (err), err); + buffer_mode.eMode = OMX_ALG_BUF_DMA; Yeah I wasn't sure about that either. I figured I'd rather having it failing than passing and then having to wonder why performances are terrible. But I'm ready to be convinced to use the fallback option.
Review of attachment 355431 [details] [review]: ::: configure.ac @@ +159,1 @@ Yes making the gstdmabuf allocator a soft dep. (not the GstAllocator). I had the same question actually, I wonder why it is done like this in gstgl, looks like we could even include gstdmabuf.h on Windows for example without any error :) Maybe Nicolas knows. ::: omx/gstomxbufferpool.c @@ +395,3 @@ + gint fd; + + fd = GPOINTER_TO_INT (omx_buf->omx_buf->pBuffer); Oki, maybe just put a note about it, like: no need to use dmabuf caps feature because the fd is always garantie to be mappable for rw on zynq-uscale. Since it is a generic code here maybe add the following to make it clear: #if not ZYNQ if (!gst_memory_map(RW) && dmabuf_caps_feature not present in pool caps) { bail out; } unmap; #endif @@ +518,2 @@ /* If it's our own memory we have to set the sizes */ + if (!pool->other_pool && !pool->dmabuf) { I do not know either, maybe the reason why it is not in gst_omx_memory_allocator_alloc is because it needs to be done after append. If no reason then yes it should be moved. Does git blame tell more about it ? ::: omx/gstomxvideodec.c @@ +217,3 @@ + GST_ERROR_OBJECT (self, "Failed to set output buffer mode: %s (0x%08x)", + gst_omx_error_to_string (err), err); + buffer_mode.eMode = OMX_ALG_BUF_DMA; Right, I usually have the exact same problem. Depending if it is dev/test or prod I prefer or not to have the fallbacks or not, and/or errors. I wonder if we could have some sort of infos about the pool/allocator being used in the caps, (caps feature does not tell), but this is another story.
(In reply to Julien Isorce from comment #6) > ::: omx/gstomxbufferpool.c > @@ +395,3 @@ > + gint fd; > + > + fd = GPOINTER_TO_INT (omx_buf->omx_buf->pBuffer); > > Oki, maybe just put a note about it, like: no need to use dmabuf caps > feature because the fd is always garantie to be mappable for rw on > zynq-uscale. > Since it is a generic code here maybe add the following to make it clear: > > #if not ZYNQ > if (!gst_memory_map(RW) && dmabuf_caps_feature not present in pool caps) { > bail out; > } > unmap; > #endif > > @@ +518,2 @@ > /* If it's our own memory we have to set the sizes */ > + if (!pool->other_pool && !pool->dmabuf) { > > I do not know either, maybe the reason why it is not in > gst_omx_memory_allocator_alloc is because it needs to be done after append. > If no reason then yes it should be moved. Does git blame tell more about it ? I guess in theory we could imagine the OMX component outputing smaller buffers at some point, in which case it makes sense to do it later indeed. It won't happen here as decoded frames have a fixed size but best to stay generic. I'll change my patch to update the size/offset for DMA buffers as well.
Created attachment 355507 [details] [review] omxvideodec: add dmabuf support for output The zynqultrascaleplus OMX implementation has a custom extension allowing decoders to output dmabuf and so avoid buffers copy between OMX and GStreamer. Make use of this extension when built on the zynqultrascaleplus. The buffer pool code should be re-usable for other platforms as well.
I think I fixed all your comments. The two remaining open questions are: - Do we want the allocators as a soft dep or not? - Should we fallback to the classic mode if DMA failed? Let's see what's Nicolas advice on those. Btw, you marked the first patch as "committed" while it's not yet merged.
Comment on attachment 355430 [details] [review] include OMX_IndexExt and OMX_ComponentExt if present commit 4488ab97afa31600e262d199386547b8310883a6 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Fri Jun 2 12:36:30 2017 +0200 build: include OMX_IndexExt and OMX_ComponentExt if present These files may be used by OMX implementation to define custom extensions. Include them if present as we are already doing with OMX_VideoExt.h https://bugzilla.gnome.org/show_bug.cgi?id=784847
(In reply to Guillaume Desmottes from comment #9) > I think I fixed all your comments. The two remaining open questions are: > - Do we want the allocators as a soft dep or not? > - Should we fallback to the classic mode if DMA failed? > > Let's see what's Nicolas advice on those. Sounds good! > > Btw, you marked the first patch as "committed" while it's not yet merged. Oups, pushed.
(In reply to Guillaume Desmottes from comment #9) > I think I fixed all your comments. The two remaining open questions are: > - Do we want the allocators as a soft dep or not? The DMABuf allocator won't compile on Windows or any non-posix system because it uses the posix API directly (like mmap()). libgstallocators compile out that allocator on non-linux which forces pretty much all the users to out-compile it too. So far gst-omx has only bee deployed on Linux I think, but still, we could at least check the OS before doing the include. In current case it's easy, if we have Allegro's DMABuf extension, we include it, otherwise we ifdef it out. One suggestion, to make it easier for other platform that would like to adopt Allegro proposed API, would be to keep that code under a single ifdef (removing the if ZINQ...). > - Should we fallback to the classic mode if DMA failed? If we negotiate SystemMemory, we should, since classic mode is able to copy the frames to fix the alignment. Though, if we negotiate DMABuf caps feature, we can't, we'll have to error out if importation failed. Until we can figure-out a way to negotiate more stuff through DMABuf caps feature. Remains that this is embedded, were the allocation is well controlled, so I would not be worried either way. I would not even make it a blocker if we have no fallback.
(In reply to Nicolas Dufresne (stormer) from comment #12) > (In reply to Guillaume Desmottes from comment #9) > > I think I fixed all your comments. The two remaining open questions are: > > - Do we want the allocators as a soft dep or not? > > The DMABuf allocator won't compile on Windows or any non-posix system > because it uses the posix API directly (like mmap()). libgstallocators > compile out that allocator on non-linux which forces pretty much all the > users to out-compile it too. From what I see the DMA/FD allocator/memory are always built-in but the alloc and map fd memory API will just return NULL if they don't have mmap. So I think we should be good actually. > So far gst-omx has only bee deployed on Linux I think, but still, we could > at least check the OS before doing the include. In current case it's easy, > if we have Allegro's DMABuf extension, we include it, otherwise we ifdef it > out. One suggestion, to make it easier for other platform that would like to > adopt Allegro proposed API, would be to keep that code under a single ifdef > (removing the if ZINQ...). Ok I'll do that. > > - Should we fallback to the classic mode if DMA failed? > > If we negotiate SystemMemory, we should, since classic mode is able to copy > the frames to fix the alignment. Though, if we negotiate DMABuf caps > feature, we can't, we'll have to error out if importation failed. Until we > can figure-out a way to negotiate more stuff through DMABuf caps feature. > Remains that this is embedded, were the allocation is well controlled, so I > would not be worried either way. I would not even make it a blocker if we > have no fallback. Ok I'll add the fallback except if the negotiated caps as the dmabuf feature.
(In reply to Guillaume Desmottes from comment #13) > From what I see the DMA/FD allocator/memory are always built-in but the > alloc and map fd memory API will just return NULL if they don't have mmap. > So I think we should be good actually. You are right, I missed that. Maybe libgst GL does not link to the allocators if not needed hence compiling it out, but less ifdef sounds nice.
Created attachment 355616 [details] [review] omxvideodec: add dmabuf support for output The zynqultrascaleplus OMX implementation has a custom extension allowing decoders to output dmabuf and so avoid buffers copy between OMX and GStreamer. Make use of this extension when built on the zynqultrascaleplus. The buffer pool code should be re-usable for other platforms as well.
I added the fallback but didn't add extra #ifdef to keep things simpler.
Review of attachment 355616 [details] [review]: Looks good overall, but I have one point I don't like. ::: omx/gstomxbufferpool.h @@ +91,3 @@ GType gst_omx_buffer_pool_get_type (void); +GstBufferPool *gst_omx_buffer_pool_new (GstElement * element, GstOMXComponent * component, GstOMXPort * port, gboolean dmabuf); I'm not big fan of boolean in API, maybe an enum, something like BUFFER_MODE ?
Created attachment 355805 [details] [review] omxvideodec: add dmabuf support for output The zynqultrascaleplus OMX implementation has a custom extension allowing decoders to output dmabuf and so avoid buffers copy between OMX and GStreamer. Make use of this extension when built on the zynqultrascaleplus. The buffer pool code should be re-usable for other platforms as well.
Comment on attachment 355805 [details] [review] omxvideodec: add dmabuf support for output Thx for the updated patch. I will push it in a moment.
Comment on attachment 355805 [details] [review] omxvideodec: add dmabuf support for output commit 136714c6ed16276945dfce810b9cb1e9ffaaee67 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Tue Jul 4 12:16:39 2017 +0200 omxvideodec: add dmabuf support for output The zynqultrascaleplus OMX implementation has a custom extension allowing decoders to output dmabuf and so avoid buffers copy between OMX and GStreamer. Make use of this extension when built on the zynqultrascaleplus. The buffer pool code should be re-usable for other platforms as well. https://bugzilla.gnome.org/show_bug.cgi?id=784847