(In reply to comment #0)
> == Platform Support ==
> [...]
> 
> An EGL context is a requirement for the extension, but
> EGL_EXT_image_dma_buf_import is not necessarily an OpenGL ES only feature.
> But currently the Intel i965 driver does not implement it for Desktop GL.
> http://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/drivers/dri/i965/intel_tex_image.c#n387
> 
> To achieve the dma_buf_import on ES, a GL_OES_EGL_image_external texture target
> is needed.

This can also work with targets such as TEXTURE_2D (it does on Mali for RGB formats), but this is not implemented by Intel at all. In general, Intel's support for this extension is quite immature, but it's not a huge amount of work to fix.

> == Patch 3 [details] # glimagesink: implement dmabuf display
> 
> This patch was originally authored by George Kiagiadakis.
> The glsink needs to use the different texture target for displaying the dmabuf.
> The target needs also to be specified in the shader. To recognize a dmabuf,
> gst_is_dmabuf_memory is used.

Again, this can also use texture2D() if applicable. There are multiple scenarios:
  - RGB imported buffer is accessed as a single buffer with texture2D()
  - planar YUV buffers are accessed as multiple buffers with texture2D() and colourspace conversion done with shaders (supported by Weston)
  - planar YUV buffers are accessed as single buffer with textureExternalOES() and colourspace conversion done internally for client to sample as RGB (also supported by Weston)

In general, there is no way for users to detect which method should be used. The eglQueryWaylandBufferWL() interface provides this capability for native Wayland buffers; dmabuf would require something equivalent, or trial and error.

> == Patch 4 [details] # dmabuf: add drm format support without conversion
> To avoid inclusion of this header I am currently redefining these in a switch
> to convert from GStreamer to drm format.
> The "known working formats" problem could be solved by testing all drm formats
> and checking for EGL errors.

The problem is that the EGL implementation can also reject buffers for any other constraints, such as alignment of buffer pitch, so it is very difficult to come up with a generic test to see if formats are supported. There has been a proposal to include an EGL query API to list the supported formats, but this has not yet been moved through Khronos.

> == Patch 7 [details] # WIP: glconvert: enable conversion for dmabuf
> This shows how glconvert would needed to be modified to support conversion for
> formats drm does not support.
> All the shaders would require to be modified. In multiplane cases, more than
> the sampler type would needed to be changed.
> 
> After a new texture is created during conversion, the sink needs to display a
> GL_TEXTURE_2D target.
> The question is if this should be done at all, and how this could be solved in
> a proper way.

It would be nice if this was a capability from glsink, e.g. if it could be negotiated that TEXTURE_EXTERNAL_OES was accepted instead of TEXTURE_2D to potentially avoid format-converting blits.

> == Multiplane format support ==
> 
> I was not able yet to run multiplane support, since the Intel driver does not
> support it.
> 
> Following EGL error is reported when using I420:
> libEGL debug: EGL user error 0x3004 (EGL_BAD_ATTRIBUTE) in too many plane
> attributes
> 
> I hopefully will be able to test this on the Mali driver.

Making the Intel driver also support this is relatively trivial.

Small note, DMABUF may have an offset from start (just like any memory object). To get the offset, simply call gst_memory_get_sizes(), one of the return parameter is the offset the need to be added to the memory return when doing mmap.

That's great to see some work around this.

Some remarks: (I may have missed something though :) )

I can see you have not added code here: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/egl/gsteglimagememory.c, any reason ?  (Also as a side note I can see about eglimage some duplicated code added recently in gstglupload since last time I looked)

Anyway, with omx/eglimage_with_context/gltexture, the sink can suggest a pool of eglimage_with_gl_context_and_texture that are later filled by the upstream decoder. But I do not see either a pool of eglimage in your case. It seems you create an eglimage for every buffer. I do not see the egldestroy btw.

In your scenario I do not see any pool of eglimage_no_context_and_with_dmaf_buf .
Also here I do not see any mention to "dma" here http://cgit.collabora.com/git/user/lubosz/gst-plugins-bad.git/tree/ext/gl/gstglimagesink.c?id=cc89ca45b172919ccdd9c68e237e6be3abe3ec06#n1206 .

Depending on the allocator selected by the upstream decoder, the downstream pool will be setup with or without eglimage, see http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbufferpool.c#n149  and  then http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbufferpool.c#n245 through gst_egl_image_memory_setup_buffer.

Well here to distinguish either eglimage_with_context_and_texture or eglimage_no_context_and_with_dmaf_buf  we could make it a bit generic by using the existing GstAllocationParams/GstMemoryFlags with some improvements or add gstgl API used by the decoder (see http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n41)
That would avoid to introduce memory:dmabuf and re-use memory:EGLImage

Finally about the multiple plane case, even if the driver supports it, maybe you need to create one eglimage per plane as done here: https://github.com/raspberrypi/userland/blob/master/host_applications/linux/apps/raspicam/RaspiTex.h#L162 (or here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/egl/gsteglimagememory.c#n480 but not yet tested) . In your code I can see only one call to eglcreateimage, but again I may have missed something.

Out of curiosity, could you paste cpu perf tool output ? and intel_gpu_top (http://dri.freedesktop.org/wiki/IntelPerformanceTuning/) ?

(In reply to comment #3)
> I can see you have not added code here:
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/egl/gsteglimagememory.c,
> any reason ?  (Also as a side note I can see about eglimage some duplicated
> code added recently in gstglupload since last time I looked)

I have proposed to make the uploader create EGLImage with GstEglImageMemory  with this code as an intermediate step and then use the uploader there. That would mean adding some logic so we know what texture target is supported. This would get rid of the duplicate.

> 
> Anyway, with omx/eglimage_with_context/gltexture, the sink can suggest a pool
> of eglimage_with_gl_context_and_texture that are later filled by the upstream
> decoder. But I do not see either a pool of eglimage in your case. It seems you
> create an eglimage for every buffer. I do not see the egldestroy btw.

The decoder in question here does not know about EGL. It's simply a DMABUF producer. I don't think we should add a dependency to EGL in there (also you'll get plugins-good linking to plugins-bad issue). I think the EGL pool should remain internal to the sink.

Creating new image for every buffer is unfortunate, but as attaching the DMABUF isn't separate from the creation of the EGLImage, I don't think it's possible to optimized this easily (not with an upstream pool). I'll leave the egldestroy part to lubosz for comments.

> 
> In your scenario I do not see any pool of eglimage_no_context_and_with_dmaf_buf
> .
> Also here I do not see any mention to "dma" here
> http://cgit.collabora.com/git/user/lubosz/gst-plugins-bad.git/tree/ext/gl/gstglimagesink.c?id=cc89ca45b172919ccdd9c68e237e6be3abe3ec06#n1206
> .

This is mainly my requirement. Because of the nature of DMABUF, even if you take all the possible care to negotiate thing, you still have no guaranty that you will be able to import them.

While I think it will be needed eventually to have a dmabuf caps feature, I think most our implementation should not rely on this. glimagesink has shaders doing color convertionm hence is a good candidate to not depend on that.

The general idea is that it will decide the best upload path when it receives the buffer. If dmabuf does not work, then it will fallback at uploading and color converting the usual way in a transparent manner (we can cache the results for performance reason obviously).

> 
> Depending on the allocator selected by the upstream decoder, the downstream
> pool will be setup with or without eglimage, see
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbufferpool.c#n149
>  and  then
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbufferpool.c#n245
> through gst_egl_image_memory_setup_buffer.
> 
> Well here to distinguish either eglimage_with_context_and_texture or
> eglimage_no_context_and_with_dmaf_buf  we could make it a bit generic by using
> the existing GstAllocationParams/GstMemoryFlags with some improvements or add
> gstgl API used by the decoder (see
> http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n41)
> That would avoid to introduce memory:dmabuf and re-use memory:EGLImage

I don't think that fits. In this design, upstream decoder (omx) know about EGL Images. In our use case, upstream only produce sysmem backed by an FD which is a DMABUF. DMABUF is the common denominator here.

> 
> Finally about the multiple plane case, even if the driver supports it, maybe
> you need to create one eglimage per plane as done here:
> https://github.com/raspberrypi/userland/blob/master/host_applications/linux/apps/raspicam/RaspiTex.h#L162
> (or here
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/egl/gsteglimagememory.c#n480
> but not yet tested) . In your code I can see only one call to eglcreateimage,
> but again I may have missed something.

This has been discussed as an intermediate. The design of DMABUF import is that you can have up to 3 planes attached to a single EGLImage. In this case, the texture target is TEXTURE_OES, and color conversion is taken care by the driver. Daniel explained all the cases. The intermediate (when multiple planes does not work) would be to bind each dmabuf plane to an EGLImage. The unknown so far is if the set of DRM format will be sufficient to represent 8 bits/1 component and 16bits / 2 components formats, or at least if there is a way around so the sampler can behave in a way that works with our shaders. This will need research, and is mostly an optimization for driver that only do single plane per EGLImage.

(In reply to comment #4)
> (In reply to comment #3)
> > Finally about the multiple plane case, even if the driver supports it, maybe
> > you need to create one eglimage per plane as done here:
> > https://github.com/raspberrypi/userland/blob/master/host_applications/linux/apps/raspicam/RaspiTex.h#L162
> > (or here
> > http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/egl/gsteglimagememory.c#n480
> > but not yet tested) . In your code I can see only one call to eglcreateimage,
> > but again I may have missed something.
> 
> This has been discussed as an intermediate. The design of DMABUF import is that
> you can have up to 3 planes attached to a single EGLImage. In this case, the
> texture target is TEXTURE_OES, and color conversion is taken care by the
> driver.

This particular case can be seen in the Mali driver, where importing a YUV dmabuf and binding it to a textureExternalOES() sampler results in RGB values being returned.

> Daniel explained all the cases. The intermediate (when multiple planes
> does not work) would be to bind each dmabuf plane to an EGLImage. The unknown
> so far is if the set of DRM format will be sufficient to represent 8 bits/1
> component and 16bits / 2 components formats, or at least if there is a way
> around so the sampler can behave in a way that works with our shaders. This
> will need research, and is mostly an optimization for driver that only do
> single plane per EGLImage.

DRM formats are perfectly fine for that. The format specifies all the planes (e.g. NV12 is one single-byte component in plane 0 and two one-byte components at half-width/half-height in plane 1), and each plane has its own stride and offset specifier. This bit is totally fine.

What's missing though is decent EGLImage API for the multi-plane case. Creating multiple EGLImages is effectively lying by the formats (e.g. picking R or RG), and might hit various limitations within the driver. But it's the best choice, given that if we create a composite EGLImage representing all planes, we don't have a good way to bind that to multiple samplers and do our own colour conversion. Wayland currently implements this by the EGL_WAYLAND_PLANE_WL attrib to eglCreateImageKHR, which lets you specify the full image information up-front, but also specify which plane you'd like to be sampling from when bound to a texture unit.

My thoughts on the previous comments/current code.

1. I want memory:dmabuf caps features.
2. This does not seem to utilise the already existing EGLImage support mentioned by Julien.  From what I've seen of our eglimage code and what I'm reading here, our EGLImage paths are not generic enough to support all different cases of formats available.
3. The conversion between dmabuf and EGLImage does not seem to be well defined.
4. Shaders and texture targets/templating.
5. overall architecture somewhat influenced by bug 743974.

decoder ! memory:dmabuf,format=NV12,etc ! ( glupload ! memory:EGLImage,format=NV12/RGBA ! glupload ! memory:GLMemory,format=NV12/RGBA ! glcolorconvert ! memory:GLMemory,format=RGBA ! glelement ! glcolorconvert ! gldownload [optional] ) ! ...

note: the () brackets denote a bin

The list of things that needs to be done to support dmabuf
1. add memory:dmabuf caps features
2.1 make EGLImage upload work with all the formats supported by GstGLMemory inside glupload with one plane per EGLImage/GstGLMemory
2.2 multi-plane EGLImage upload case.
3. Some kind of 'quirks' table needs to be implemented within GstGLContext and generated based on the chosen gl context/renderer/etc due to the loosely specified nature of some GL operations.  Even just a GstStructure based key, value pairs would suffice.  Particularly useful for the quirks in dmabuf.
4. instead of hardcoding the entire shader we split them up and/or attempt to template specific sections.
5. fix bug 743974.

Patch 1: This is the baseline for fully supporting texture targets in GstGLMemory.
Patch 2: Mostly looks good.
Patch 3: The texture target should be retrieved from GstGLMemory.  Look into simple splitting/templating shaders.
Patch 4: wouldn't it be a simple matter to try the representative drm format/s for the video format and cache that on success?
Patch 5: Testing patch which I don't see as necessary with properly defined caps features
Patch 6: dmabuf caps features are good!
Patch 7: Currently, glupload only outputs GstGLMemory.  It would be needed to teach glcolorconvert and the encompassing elements about EGLImages in order to pass EGLimages between glupload and glcolorconvert generically.  Depending on the GL impl it might be simpler to do EGLimage->GstGLMemory inside glupload.

What's the rational to enforce using memory:dmabuf ?

Same reasons you add any caps feature.

1. Simplicity - you know exactly what you're getting.
2. Extensibility - possiblity to add extra required fields to the caps.
3. Selection - You may want to select a specific memory type to connect to appsink for example.
4. Negotiation - element can limit pools, allocators and meta to those that support the specific caps feature.  Can also prioritise element links with caps features in decodebin/playbin.
5. Mapping - the feature might not support mapping to sysmem (doesn't seem to apply here)

(In reply to comment #8)
> Same reasons you add any caps feature.
> 
> 1. Simplicity - you know exactly what you're getting.

There is real use case where we can't guaranty that all buffers will be DMABUF. A copied buffer to system memory might sneek in (it's a known way to speed up reclaim when it's strictly required). If it's in the caps, that would be illegal. So far, my take on supporting DMABUF is that it should first be considered system memory, and the rest is optimization on top.

> 2. Extensibility - possiblity to add extra required fields to the caps.

Not sure what use with DMABUF, but fair. The only extra I'm thinking of would be if the kernel ever expose the allocated memory type. But meanwhile, we should assume not being able to import DMABUF is expected, and fallback to using DMABUF as if it was normal memory.

> 3. Selection - You may want to select a specific memory type to connect to
> appsink for example.

There is no cost of exporting DMABUF. Neither upstream (downstream pool) or dowstream (upstream pool) really need to be aware. The case where DMABUF is strictly required is extremely rare. The fact is that the main exporter for DMABUF (v4l2 drivers) will always produce DMABUF in a very near feature. Because there is not cost, and allow nice optimization in many elements (like inter process elements). They don't need to know in advenced that the buffers will have FD to share with another process. In fact, I think it's a important feature if FD and non FD base buffer can be aliased (e.g. payloading). But that falls outside of GL I can agree.

> 4. Negotiation - element can limit pools, allocators and meta to those that
> support the specific caps feature.  Can also prioritise element links with caps
> features in decodebin/playbin.

Not sure about this one.

> 5. Mapping - the feature might not support mapping to sysmem (doesn't seem to
> apply here)

Indeed.

The only case where this caps feature is needed, is when you have a disjoint set of color formats to support system memory and dmabuf. And for this is reason, I think the caps feature will be required. But I don't think it should make any difference for GL. There is no documented way (without having a DMABUF in hand) to trial and error and figure-out if a format is supported or not. Also, failing importation does not mean the format isn't supported. So in general, if the buffer is a dmabuf we should try something, hence the dynamic check.

(In reply to comment #9)
> (In reply to comment #8)
> > Same reasons you add any caps feature.
> > 
> > 1. Simplicity - you know exactly what you're getting.
> 
> There is real use case where we can't guaranty that all buffers will be DMABUF.
> A copied buffer to system memory might sneek in (it's a known way to speed up
> reclaim when it's strictly required). If it's in the caps, that would be
> illegal. So far, my take on supporting DMABUF is that it should first be
> considered system memory, and the rest is optimization on top.

This is very much like GstGLMemory.  It can also be considered system memory with optimization on top for GL specific handling.

The check for GL will also allow dmabuf/sysmem features in the dmabuf uploader.  But if you provide a non-dmabuf memory.  glupload will fallback to raw upload and stay there for subsequent buffers until new caps are set.  If you can't guarantee dmabuf for every buffer, then you should not export dmabuf caps features.

> > 3. Selection - You may want to select a specific memory type to connect to
> > appsink for example.
> 
> There is no cost of exporting DMABUF. Neither upstream (downstream pool) or
> dowstream (upstream pool) really need to be aware.

Uh, bufferpools need to allocate dmabuf which means they definitely need to be aware of it.  Also, how does one select between GLMemory/EGLImage/dmabuf/upload meta without caps features for element links that provide multiple ways of producing GL data along with multiple ways one can map the memory as sysmem (GLMemory/dmabuf)?  And what if the app wants too choose?

> The case where DMABUF is strictly required is extremely rare.

But it still exists.

> > 5. Mapping - the feature might not support mapping to sysmem (doesn't seem to
> > apply here)
> 
> Indeed.
> 
> The only case where this caps feature is needed, is when you have a disjoint
> set of color formats to support system memory and dmabuf. And for this is
> reason, I think the caps feature will be required. But I don't think it should
> make any difference for GL. There is no documented way (without having a DMABUF
> in hand) to trial and error and figure-out if a format is supported or not.
> Also, failing importation does not mean the format isn't supported. So in
> general, if the buffer is a dmabuf we should try something, hence the dynamic
> check.

For GL, if the resolved video format from the dmabuf is a different format to the guessed output caps (from the dmabuf uploader), then the glupload element will reconfigure with the new output caps and we go from there.

Also, it is possible for GL to export dma-bufs http://airlied.livejournal.com/78704.html

Re caps features, it should be perfectly fine to use sysmem caps features *if* the dmabuf memory is guaranteed to behave like sysmem. That is, it must be read-write mappable.

Requiring dmabuf in capsfeatures is only needed if a) it does not behave like sysmem, or b) an element can't work with anything else than dmabuf. Not sure why dmabuf in caps features must be a requirement here (you can just check what kind of memory you get, right?). And couldn't you reconfigure glupload based on what your current memory is, and switch between sysmem and dmabuf uploading all the time if required?

Yes, but switching all the time is relatively expensive to do as it would require renegotiating downstream.  Especially if the dmabuf->EGLImage->GstGLMemory upload converts to a different video format than the raw memory upload.  It can be done though.

I'm not saying glupload 'requires' dmabuf caps features much like we don't require the GstGLMemory caps features to use the gl memory uploader.  Just that it is useful to have for the aforementioned reasons.

Agreed, having caps features potentially allows more optimal configurations :)

A couple of things:
  - being able to mmap() dmabufs is not only not guaranteed, but also not implemented by DRM/KMS exporters
  - dma_buf_export isn't merged into Mesa, hasn't even been proposed for merging, and is unlikely to see wide adoption for a spectacular variety of issues (tl;dr: importing is easy - exporting forces you to change the entire buffer allocation process - and given we've never been able to agree on generic buffer allocation ...)

I've no particular opinion on the rest.

BTW, I had proposed an EXT_image_dma_buf_import_with_glformat extension that would allow you to map each individual plane as a separate EGLImage akin to what we do in Wayland, but requires you to write your own shader to perform YUV to RGB conversion. Is that something that interests you and that I should persue on pushing out?

Full support for GL_OES_EGL_image_external with planar YUV formats would require much more work in Mesa, especially if you additionally want to support fancy stuff like field selection, customization of the color matrix for conversion (bt.601 vs. bt.709 vs XXX), etc.

(In reply to Matthew Waters from comment #12)
> Yes, but switching all the time is relatively expensive to do as it would
> require renegotiating downstream.  Especially if the
> dmabuf->EGLImage->GstGLMemory upload converts to a different video format
> than the raw memory upload.  It can be done though.
> 
> I'm not saying glupload 'requires' dmabuf caps features much like we don't
> require the GstGLMemory caps features to use the gl memory uploader.  Just
> that it is useful to have for the aforementioned reasons.

I also value dmabuf capsfeature for the above-mentioned reasons. In particular, allow for construction of bufferpools very early.

+1 for dmabuf caps feature but I think the patches should be updated to demonstrate how this is useful between v4l2 elements and glimagesink. (as currently the patches only use it for inteldmabufupload).

Also to avoid re-creating eglimages each time, a solution is to improve GstGLBufferPool to use a custom associative array (or other data structure) instead of the default internal queue (inherited from GstBufferPool) to manage the GstBuffers that wrap EGLImages.

The key would be "gst_dmabuf_memory_get_fd" and the value a GstBuffer that wraps an EGLImage.

This way glimagesink could use its own buffer pool while insuring that the re-used buffer matches the incoming buffer.

The challenge I see is that the pool would require to know all the "fd" keys when being activated. (to avoid initializing it empty and feed it later with buffers).
So somehow the gstv4l2bufferpool will be activated first and let the GstGlBufferPool knows about the available "fd"s, so that it can pre-create the eglimages.
Maybe using dmabuf caps feature.

Uh, why?

Just have a proper EGLImage pool/memory and you get reuse for free.

Unless I am missing something this is because the v4l2 driver manages its own queue so you cannot assume that they will come in the same order as the gst_(gl)buffer_pool_acquire would give you the wraped eglimage. I also start from the fact that you cannot change fd attribute param of an eglimage after the eglimage creation.
Also for example v4l2dec manages its own "src" pool so it cannot negotiate downstream gstglbufferpool.

So make every acquire, create a new EGLImage.  As long as the bufferpool correctly handles the allocation/deallocation requirements of your memory wrt reusing buffers, you're all sweet.  Besides, EGLImage's are intended to be opaque handles to already existent memory.  They're not meant to allocate anything of significance.

The MMU manipulation can still be quite expensive. Caching them where possible really does help.

There is some caching gain of always matching the same EGLImage with the same DMABUF. Though this highly depends on the type of memory being used. There is the same gain if you can always map the same buffer to the same USERPTR, or the same DMABUF to the same v4l2 dmabuf import. Though, it's terribly hard to implement with GstBufferPool as a base class. I endup not implementing it in v4l2.

Also, replacing BufferPool internal queue creates some difficulty. We could think of adding come API to help that though in the long term. If we already pool the EGLImage, I think it's already good enough.

Huh? You specify the dmabuf FDs when you create an EGLImage, and you cannot modify that afterwards.

(In reply to Daniel Stone from comment #23)
> Huh? You specify the dmabuf FDs when you create an EGLImage, and you cannot
> modify that afterwards.

Seems exact. EGLImages are created for a set of FDs. The concern being that in current draft, each time a set of FDs arrive, we create a new EGLImage (and texture), which we dispose when we are done with it. We could potentially optimize this since FDs comes from a pool and will be recurrent. That being said, the complexity comes from the fact the pool model does not work, we need a cache where we can retrieve a specific EGLImage depending on specific set of FDs. We also need a smart way to invalidate the cache (a mini object weak pointer probably works here, if not racy).

Is Weston doing this kind of caching ? Is it worth considering this kind of optimization so early ?

Makes sense to me. Weston caches internally - if you reuse the same wl_buffer, we won't repeatedly create and destroy EGLImages. If you're creating new wl_buffers every time, then we can't make any use of caching: as the fds are passed over a UNIX socket, they will be different fds to the ones we received before, and our cache check fails.

So, in a GSt context, depends on whether or not waylandsink is caching.

I rebased patches 1, 2 and 3(+ _dma_buf_drm_fourcc_from_info from patch 4) from comment #1:
https://github.com/CapOM/gst-plugins-bad/commits/dmabuf

I think patch 1 is actually not needed anymore. Because "gst_gl_memory_wrapped_texture" now takes texture_target as parameter. (Also GstGLMemory has a field tex_target now.)

So if this is right I can apply what I suggest in the second rebased patch, the fixme around gst_gl_memory_setup_buffer. I.e. drop gst_gl_memory_setup_buffer and use gst_gl_memory_wrapped_texture.
And so drop patch 1.

Again +1 for memory:dmabuf caps feature.

Nicolas, about the cache problem, I'll probably open a bug to track it properly. It also needed for gst-omx, because right now it unpopulate the downstream pool.

I forgot to mention that I reworked initial patch 3 a bit and patch 6 is not needed anymore, _dma_buf_upload_caps + _dma_buf_upload_transform_caps are enough.

(updated the title to what should be done now with the new design)

Some notes:

About patch glupload: make texture target parameterizable, we should not extend gst_gl_memory_setup_buffer. Instead we should use existing: gst_egl_image_memory_setup_buffer and gst_egl_image_allocator_wrap. They are actually there to handle the eglimage live cycle. Because with current patches we cannot properly handle the eglimage destruction. Instead the eglimage has to be a field in the GstMemory, i.e. existing GstEGLImageMemory.

But gst_egl_image_memory_setup_buffer has to be changed to use GstGLMemory instead of the deprecated "gst_gl_generate_texture_full".

(In reply to Julien Isorce from comment #29)
> Some notes:
> 
> About patch glupload: make texture target parameterizable, we should not
> extend gst_gl_memory_setup_buffer. Instead we should use existing:
> gst_egl_image_memory_setup_buffer and gst_egl_image_allocator_wrap. They are
> actually there to handle the eglimage live cycle. Because with current
> patches we cannot properly handle the eglimage destruction. Instead the
> eglimage has to be a field in the GstMemory, i.e. existing GstEGLImageMemory.
> 
> But gst_egl_image_memory_setup_buffer has to be changed to use GstGLMemory
> instead of the deprecated "gst_gl_generate_texture_full".

Yes, and Lubosz was looking into that. The EGL stuff also need some work. We need to probe what set of extensions is available to decide the type of texture is going to be used (EXTERNAL_OES or standard texture like on RPi).

vaapidecodebin ! video/x-raw(memory:DMABuf)! glimagesink now works on desktop ( tested with nouveau (with mesa st/va) and intel driver. See #755072
While running the pipeline you can see the dma buffers in use with:
sudo cat /sys/kernel/debug/dma_buf/bufinfo

I rebased the patches to current master.

https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/log/?h=dmabuf

1. Use gst_gl_memory_wrapped_texture

The first patch is not needed anymore. I am now using gst_gl_memory_wrapped_texture that supports different texture targets, as Julien suggested.
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf&id=c21db3b334fb2df3286329a8b050118859567ab9

2. Do not _gst_mem_copy dmabuf

Also _gst_mem_copy produces GL errors with GL_TEXTURE_EXTERNAL_OES. They occur during initialization and a window resize event that causes renegotation of the pipeline. 

Bypassing _gl_mem_copy_thread for GL_TEXTURE_EXTERNAL_OES fixes this
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf&id=5d2ca69b35fdaece6acbcc709cbd68a4ad6cd42f

3. supported formats with dmabuf caps features

It is possible to negotiate a pipeline with dmabuf caps features, when using the inteldmabuf upload element. v4l2src needs a patch to expose dmabuf caps features.
This patch allows multiple color formats with dmabuf caps features:
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf&id=40db525528d9acf6ca6027041507388414ba8ff1

4. Multiplane formats and conversion

We should not expect the driver to do the color conversion, some drivers might actually do this, which also is dependent of the hardware implementation.
Mesa for example does not intent to do this. For example, in the Intel driver multi planar dmabufs result in __DRI_IMAGE_ERROR_BAD_MATCH, which is exposed as EGL error.

http://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/drivers/dri/i965/intel_screen.c#n736

So I guess for multi planar formats glcolorconvert needs to be used with an EGL texture per plane. Not sure on which GL implementations samplerExternalOES needs to be used, that would require templating of the texture sampler definition in glconvert's shaders. Currently glconvert is bypassed for dmabuf pipelines.
I have an experimental patch on an old gst-bad branch which did color conversion in glsl:
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf-colorconvert&id=70caaadb3c9893d3ab641b295ab601f289bc9dcf

Here is a recent piglit example by Intel where NV12 conversion is set up with
a texture sampler per plane in a shader:
http://cgit.freedesktop.org/piglit/tree/tests/spec/ext_image_dma_buf_import/transcode-nv12-as-r8-gr88.c

A couple of quick comments from my quick perusal:

- glupload: Implement dmabuf upload
Need to make sure that none of this is actually built when dmabuf is not available, not just on every EGL && GLES2 implementation.  Add a g_assert() asserting we don't overrun the attribs array..

- glupload : Use gst_gl_memory_wrapped_texture for dmabuf
Can be squashed into the previous patch after removing the double glBindTexture.  Also needs to keep track of and destroy the external texture with the wrapped memory and the EGLImage so probably needs some work on the lifetimes and possibly GstEGLImageMemory.

- glimagesink: Implement dmabuf display
Again, need to make sure that none of this is actually built when dmabuf is not available, not just on EGL && GLES2.  Otherwise it looks fine.

- glupload: properly error out if cannot find a method
The first hunk of this is not needed with master.  The rest is just debugging so can be committed at leisure.

- glupload: Support more color formats in dmabufs caps
Looks good.  Can be squashed as well.

- glmemory: Do not copy GL_TEXTURE_EXTERNAL_OES
Perhaps return NULL; instead of return ret; to be more explicit.  Otherwise, good.

Yes, glcolorconvert probably needs to learn how to deal with EGLImage's which effectively means it could be a second glupload for RGBA but that's ok :).  glcolorconvert can still negotiate passthrough if both sides support EGLImage and the format is the same, etc.

In addition to Matthew remarks:

"Use gst_gl_memory_wrapped_texture"

In comment #29 I maybe was not clear enough but my last thought was it should use GstEGLImageMemory instead, not gst_gl_memory_wrapped_texture. Sorry it is extra work but this is the way to go, it is designed for it to properly handle liftime of the egl image. Also it allows to move all the eglCreateImage+setup attribs to the file that owns GstEGLImageMemory. See #29.

"glupload dmabuf caps"

Take a look at first part of https://github.com/CapOM/gst-plugins-bad/commit/80e4293584a398be1a7fd92d2c3221417799b09d. And second part too. It think it is correct but would require to double check.

"glimagesink: Implement dmabuf display"

A: Feel free to squash https://github.com/CapOM/gst-plugins-bad/commit/b44122c27b7c52653fedb083888500efefa14b46 in it.
B: I think you can add target param to gst_gl_shader_compile_with_default_vf_and_check in order to use the right shader. You can reuse the one in gstglshader.c and add "%s".

"glcolorconvert"
I made it generic:
https://github.com/CapOM/gst-plugins-bad/commit/2d830a2b14d3e003457084f175a8085f2d1ba9be
Though it could be refactored a bit to make the patch less verbose.
It tested for RGBx to RGBA and it works fine for GL_TEXTURE_EXTERNAL_OES.


Good luck Lubosz, I think if you make it use GstEGLImageMemory it will be candidate for pushing it upstream.

From last discussion there is still a problem which is:

In case of memory:EGLimage,  the GstEGLImageMemory own a gl texture from which it has been created. So gst_is_egl_image_memory(mem) will return true and you can access the eglimage through gst_egl_image_memory_get_image(mem). But there is currently no way to access the owned gl texture.

In case of memory:GLMemory (created from dmabuf), the GstGLMemory owns an eglimage. (actually should own because the current solution just create and destroy it right after). So gst_is_gl_memory(mem) return true and you can access the gl texture through gst_gl_memory_get_texture_id (mem) (at least this function should exist instead of using mem->tex_id).

So as you can see there are reverse way. First eglimage own texture, second gl texture owns eglimage.

Currently GstGLMemory and GstEGLImageMemory do not have any kind of relation. Should it possible to use the GstMemory::parent for this or is it only about memory sharing ?
I was thinking in the first case, GstEGLImageMemory is parent of GstGLMemory and in the second case GstGLMemory is parent of GstEGLImageMemory.

In any case we need to find a nice way to handle the 2 cases. Any suggestion ?

Using that parent didn't work for me on the V4L2 side. Instead, I ended up sing the GstMiniObject qdata. Though, pools reset function needs to be aware (if a pool is used) to detach those when the memory is returned.

The following patchset adds support for dmabufs with various single and multi-planar formats,
creating one EGL image per plane and passing it as GstGLMemory to the sink.
It uses GL_TEXTURE_2D as target and glcolorconvert, so it can be also used with Desktop GL contexts.
It works fine with most v4l2src / vivid format combinations and
native (not emulated) formats with my webcam hardware on Mesa with Intel.

Example pipeline:
GST_GL_PLATFORM=egl gst-launch-1.0 v4l2src device=/dev/video0 io-mode=4 ! video/x-raw,format=YUY2 ! glimagesink

Created attachment 316102 [details] [review]
configure.ac: Fix warnings.

Fixes following 3 warnings:
libtoolize: Remember to add 'LT_INIT' to configure.ac.
libtoolize: Consider adding 'AC_CONFIG_MACRO_DIRS([m4])' to configure.ac,
configure.ac:71: warning: The 'AM_PROG_MKDIR_P' macro is deprecated, and its use is discouraged.

Created attachment 316103 [details] [review]
glmemory: Function to get texture type as string.

Created attachment 316104 [details] [review]
gstglcontext_egl: Expose gst_gl_context_egl_get_error_string.

Created attachment 316105 [details] [review]
glimagesink: Show error when video frame is not mapped.

Adds more meaningful error than
"Failed to convert multiview video buffer", which is always used
when prepare_next_buffer() fails in gst_glimage_sink_prepare().

Created attachment 316106 [details] [review]
build: Add dmabuf build condition.

configure.ac: Build dmabuf when EGL and drm_fourcc.h is available.
gl: Link gst-allocators.

Created attachment 316107 [details] [review]
eglimagememory: Methods to create GstGLMemory from dmabufs

Maps GstVideoFormats to suitable DRM fourccs
which work with glcolorconvert, using gst_gl_memory_alloc().
Creates a GstGLMemory from EGLImageKHR with
EGL_LINUX_DMA_BUF_EXT for every plane.
    
Compatible with Desktop GL, since GL_TEXTURE_2D is used.
    
Adds support for various single and multi planar formats:
    
'BGRA', 'BGRx', 'GRAY8', 'I420', 'NV12', 'NV21',
'RGB16', 'xRGB', 'Y42B', 'YV12', 'UYVY', 'YUY2'

Created attachment 316108 [details] [review]
glupload: Add dmabuf upload method.

Upload method for buffers with "memory:dmabuf" caps features,
when an EGL context is used.

Adds method to create a _value_list_from_strings(), in order to
accept "memory:dmabuf" caps features in _dma_buf_upload_transform_caps().

Tries to create an EGL image in accept _dma_buf_upload_accept(),
to gain the ability of falling back to a different uploader, mostly raw.

Example pipeline:

GST_GL_PLATFORM=egl \
gst-launch-1.0 v4l2src device=/dev/video1 io-mode=4 ! \
               video/x-raw,format=NV12 ! glimagesink

There are also 3 experimental / WIP patches I am currently working on:

== Buggy format combinations ==

3 additional formats supported by glcolorconvert and v4l2src do not work this way:
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf-dev&id=92c59d2d819405a2e3c0b0f65eb0c088c349aa57

ARGB can be used by a swizzle in glcolorconvert, but BGR and RGB are are not usable since there seems to be a bug in vivid / drm. 

== video_frame_map() with dmabufs ==

video_frame_map() seems to not be needed in pipelines with dmabuf:
https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf-dev&id=f7d7d2b55b84d1eadb0463981c9a815b8e2d3b05

Since I am using GL_TEXUTRE_2D in above patches, there needs to be an additional way to distinguish between dmabuf and non-dmabuf textures in the sink, besides the texture target.

_gl_mem_copy() can also be skipped for dmabuf gl buffers with GL_TEXTURE_2D, which is mandatory with GL_TEXTURE_EXTERNAL_OES, as seen in:
http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/diff/gst-libs/gst/gl/gstglmemory.c?id=e61d504556870adf2b5d58b86b09a3327816dec2


== Usage of GL_TEXTURE_EXTERNAL_OES ==

Since glmemory has been extended to support multiple targets, usage of GL_TEXTURE_EXTERNAL_OES texture tatgets
can also work with gles2 contexts. This is a WIP patch to make this work:

https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/?h=dmabuf-dev&id=758983e6fbf2e0079b5b8261c4977a02705a93fe

I was not able to negotiate the EXTERNAL_OES target with glconvert this way. Any ideas how to make this work properly?

(In reply to Lubosz Sarnecki from comment #45)
> == Usage of GL_TEXTURE_EXTERNAL_OES ==
> 
> Since glmemory has been extended to support multiple targets, usage of
> GL_TEXTURE_EXTERNAL_OES texture tatgets
> can also work with gles2 contexts. This is a WIP patch to make this work:
> 
> https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/
> ?h=dmabuf-dev&id=758983e6fbf2e0079b5b8261c4977a02705a93fe
> 
> I was not able to negotiate the EXTERNAL_OES target with glconvert this way.
> Any ideas how to make this work properly?

To be honest, I'd recommend pretending that TEXTURE_EXTERNAL never existed. The only driver I know of which implements it is Mali-6xx, but that essentially just rewrites your shaders to insert colourspace conversion, so is not really any better than doing it in shaders yourself. One less codepath to go wrong and all that ...

Review of attachment 316107 [details] [review]:

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +379,3 @@
+  attribs[atti++] = EGL_DMA_BUF_PLANE0_FD_EXT;
+  attribs[atti++] =
+      gst_dmabuf_memory_get_fd (gst_buffer_peek_memory (in_buffer, 0));

What if you have multiple FDs ?

Review of attachment 316107 [details] [review]:

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +443,3 @@
+  for (int i = 0; i < meta->n_planes; i++)
+    GST_DEBUG ("meta: offset[%d]: %ld, stride[%d]: %d",
+        meta->n_planes, meta->offset[0], meta->n_planes, meta->stride[0]);

That trace is kind of broken. meta->n_planes instead of i, meta->offset[0] rather then the i index and same for stride.

Review of attachment 316107 [details] [review]:

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +387,3 @@
+  attribs[atti] = EGL_NONE;
+
+  for (int i = 0; i < atti; i++)

That's only valid in C99, declare i outside.

Review of attachment 316103 [details] [review]:

Looks ok

::: gst-libs/gst/gl/gstglmemory.c
@@ +139,3 @@
+  switch (type) {
+    case GST_VIDEO_GL_TEXTURE_TYPE_LUMINANCE:
+      return "LUMINANCE";

I would prefer "luminance" and "luminance-alpha" (i.e. lower case for words) here to match the texture-target definition.  Acronyms can have capital letters.

Review of attachment 316104 [details] [review]:

This should pass the error value into the function when exposing the function as public and the eglGetError() should be done outside this function.

Review of attachment 316106 [details] [review]:

Looks fine.

::: configure.ac
@@ +812,3 @@
+HAVE_DRM_FOURCC_HEADER=no
+AC_CHECK_HEADER(libdrm/drm_fourcc.h,
+  HAVE_DRM_FOURCC_HEADER=yes, )

nitpick. Indentation (4 spaces)

@@ +816,3 @@
+HAVE_GST_ALLOCATORS=no
+PKG_CHECK_MODULES(GST_ALLOCATORS, gstreamer-allocators-1.0,
+  HAVE_GST_ALLOCATORS=yes, )

nitpick. Indentation. (4 spaces)

@@ +821,3 @@
+        "x$HAVE_GST_ALLOCATORS" = "xyes" -a \
+        "x$HAVE_EGL" = "xyes"; then
+          AC_DEFINE(GST_GL_HAVE_DMABUF, [1] , [DMABUF available for gl plugins])

nitpick. Indentation. (2 spaces)

(In reply to Lubosz Sarnecki from comment #45)
> == video_frame_map() with dmabufs ==
> 
> video_frame_map() seems to not be needed in pipelines with dmabuf:
> https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/commit/
> ?h=dmabuf-dev&id=f7d7d2b55b84d1eadb0463981c9a815b8e2d3b05

It's also not technically needed for regular 2D textures except for the GLMemory state machinery to know where the most recent texture data is to upload/download as necessary.  Don't add a conditional path just because "it is not needed".  Mapping when the texture data is already in the correct place is a no-op.  You may need to map(WRITE | GL) when creating the texture to ensure that the GLMemory state machinery knows where the latest data is.

> Since I am using GL_TEXUTRE_2D in above patches, there needs to be an
> additional way to distinguish between dmabuf and non-dmabuf textures in the
> sink, besides the texture target.

Why?

> _gl_mem_copy() can also be skipped for dmabuf gl buffers with GL_TEXTURE_2D,
> which is mandatory with GL_TEXTURE_EXTERNAL_OES, as seen in:
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/diff/gst-libs/gst/gl/
> gstglmemory.c?id=e61d504556870adf2b5d58b86b09a3327816dec2

What bad thing happens if dmabuf textures are copied?  If nothing, why are you proposing to skip copying dmabuf textures?

> == Usage of GL_TEXTURE_EXTERNAL_OES ==
> I was not able to negotiate the EXTERNAL_OES target with glconvert this way.
> Any ideas how to make this work properly?

Depends on exactly what doesn't work.  Caps negotiation? GL errors? etc

Review of attachment 316107 [details] [review]:

Some comments.

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +383,3 @@
+  attribs[atti++] = GST_VIDEO_INFO_PLANE_OFFSET (in_info, plane);
+  attribs[atti++] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
+  attribs[atti++] = GST_VIDEO_INFO_PLANE_STRIDE (in_info, plane);

What about multi-planar video? Wouldn't you need PLANE1/2 here instead for them?

Review of attachment 316108 [details] [review]:

Some comments

::: gst-libs/gst/gl/gstglupload.c
@@ +524,3 @@
+GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE_WITH_FEATURES
+    ("memory:" GST_ALLOCATOR_DMABUF,
+        "{ ABGR, ARGB, BGRA, BGRx, RGB16, RGBA, RGBx, xBGR, xRGB }"));

The example pipeline in the blurb won't work with this template.  There are no YUV formats here so format=NV12 is using the raw uploader which means nothing has changed.

@@ +592,3 @@
+      gst_buffer_ref (buffer);
+      gst_mini_object_set_qdata ((GstMiniObject *) image->outbuf,
+          quark, buffer, (GDestroyNotify) gst_buffer_unref);

Perhaps the GstParentBufferMeta would work better than setting some qdata.

Created attachment 316411 [details] [review]
glmemory: Function to get texture type as string.

Changed strings to lower case.

Created attachment 316412 [details] [review]
gstglcontext_egl: Expose gst_gl_context_egl_get_error_string.

Put eglGetError() outside of gst_gl_context_egl_get_error_string

Created attachment 316413 [details] [review]
build: Add dmabuf build condition.

Changed all indent to 2 spaces.

Created attachment 316414 [details] [review]
eglimagememory: Methods to create GstGLMemory from dmabufs.

Fix debug print, declare int i outside of for statement, update gst_gl_context_egl_get_error_string call.

> What if you have multiple FDs ?

I only have one FD, since I only have one GstMemory in the GstBuffer.
This would require gst_buffer_n_memory (in_buffer) to be greater than 1, which I did not encounter using vivid and v4l2src.
This code is also not reachable for gst_buffer_n_memory > 1, since I am checking for that.
Do you know any pipelines where multiple memories could be the case?

> What about multi-planar video? Wouldn't you need PLANE1/2 here instead for them?

Mesa (at least i965) does not support multiple planes here, and does not plan to. An EGL image has to be created for each plane.
See https://bugs.freedesktop.org/show_bug.cgi?id=93138.

According to Hans, there is a way to get multiple FDs from vivid, then MFC and CODA decoder will give you multiple FDs. I think we need support for that.

(In reply to Nicolas Dufresne (stormer) from comment #61)
> According to Hans, there is a way to get multiple FDs from vivid, then MFC
> and CODA decoder will give you multiple FDs. I think we need support for
> that.

I re-read the laster code, and it's fairly easy to add that later, so never mind.

(In reply to Matthew Waters from comment #55)
> Review of attachment 316108 [details] [review] [review]:
> 
> Some comments
> 
> ::: gst-libs/gst/gl/gstglupload.c
> @@ +524,3 @@
> +GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE_WITH_FEATURES
> +    ("memory:" GST_ALLOCATOR_DMABUF,
> +        "{ ABGR, ARGB, BGRA, BGRx, RGB16, RGBA, RGBx, xBGR, xRGB }"));
> 
> The example pipeline in the blurb won't work with this template.  There are
> no YUV formats here so format=NV12 is using the raw uploader which means
> nothing has changed.

In fact, it's using it, though not with memory:DMABuf caps feature. How to use that new caps feature isn't well defined, and is not essential for libgstgl (because we have fallbacks for everything). So let's remove that for now, George is also working on improving how we will use that caps feature, we can then implement seperatly and consistently.

> 
> @@ +592,3 @@
> +      gst_buffer_ref (buffer);
> +      gst_mini_object_set_qdata ((GstMiniObject *) image->outbuf,
> +          quark, buffer, (GDestroyNotify) gst_buffer_unref);
> 
> Perhaps the GstParentBufferMeta would work better than setting some qdata.

It's a good idea. Currently it make no difference because outbuf is not pooled, but if we fix this "issue", it will break.

Just a note, I got a crash here:

gl_api = GST_GL_API_OPENGL, eglCreateImage = 0x0, eglDestroyImage = 0x0

We should check the eglCreateImage and eglDestroyImage symbols are available and fail cleanly if not. This is running mesa on nouveau on Fedora 23 / Wayland. Seems like a bug though, but still, we should check maybe.

Review of attachment 316411 [details] [review]:

Looks good.

Review of attachment 316412 [details] [review]:

Looks good.

Review of attachment 316413 [details] [review]:

Looks good.

Review of attachment 316414 [details] [review]:

Some comments

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +438,3 @@
+
+  gint n_mem = gst_buffer_n_memory (in_buffer);
+  GstVideoMeta *meta = gst_buffer_get_video_meta (in_buffer);

The input buffer may not have GstVideoMeta which is crash prone.

@@ +472,3 @@
+
+        if (!plane1 || !plane2)
+          return NULL;

memory leak on failure of plane1 or plane2.

@@ +496,3 @@
+
+        if (!plane1 || !plane2 || !plane3)
+          return NULL;

memory leak on failure.

::: gst-libs/gst/gl/egl/gsteglimagememory.h
@@ +65,3 @@
     GstVideoGLTextureOrientation orientation);
 
+#if GST_GL_HAVE_DMABUF

This does not interact will with the build addition patch and other libraries attempting to use libgstgl's dmabuf.  The build patch only does a AC_DEFINE which places it in config.h to which any library using libgstgl would need to replicate.  The GST_GL_HAVE_DMABUF should go into gstglconfig.h so that libraries always get the correct API.

(In reply to Nicolas Dufresne (stormer) from comment #63)
> (In reply to Matthew Waters from comment #55)
> > Review of attachment 316108 [details] [review] [review] [review]:
> > 
> > Some comments
> > 
> > ::: gst-libs/gst/gl/gstglupload.c
> > @@ +524,3 @@
> > +GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE_WITH_FEATURES
> > +    ("memory:" GST_ALLOCATOR_DMABUF,
> > +        "{ ABGR, ARGB, BGRA, BGRx, RGB16, RGBA, RGBx, xBGR, xRGB }"));
> > 
> > The example pipeline in the blurb won't work with this template.  There are
> > no YUV formats here so format=NV12 is using the raw uploader which means
> > nothing has changed.
> 
> In fact, it's using it, though not with memory:DMABuf caps feature. How to
> use that new caps feature isn't well defined, and is not essential for
> libgstgl (because we have fallbacks for everything). So let's remove that
> for now, George is also working on improving how we will use that caps
> feature, we can then implement seperatly and consistently.
> 

These caps are not necessary when using the uploader with v4l2src, since it does not support the memory:dmabuf caps feature.
The dmabuf uploader accepts the upload when gst_is_dmabuf_memory is true.

The code is mandatory to accept dmabuf caps with gkiagia's inteldmabufupload element, and in the future with v4l2src, when it will support this caps feature in the correct io-mode. gkiagia is currently working on a drmdmabufupload element which will surely also support this caps feature.

The formats above are the ones working with inteldmabufupload, but they can in theory be expanded to the capabilities of glconvert.
I can remove the caps features part, since there is currently no upstream element supporting them.

Created attachment 316599 [details] [review]
eglimagememory: Methods to create GstGLMemory from dmabufs

* check if video meta is available
* unref partially initialized planes

Created attachment 316600 [details] [review]
configure.ac: Check for dmabuf availability.

Use gstglconfig.h

Created attachment 316601 [details] [review]
glupload: check for EGL_KHR_image_base gl feature.

(In reply to Nicolas Dufresne (stormer) from comment #64)
> Just a note, I got a crash here:
> 
> gl_api = GST_GL_API_OPENGL, eglCreateImage = 0x0, eglDestroyImage = 0x0
> 
> We should check the eglCreateImage and eglDestroyImage symbols are available
> and fail cleanly if not. This is running mesa on nouveau on Fedora 23 /
> Wayland. Seems like a bug though, but still, we should check maybe.

Does this help?

Review of attachment 316601 [details] [review]:

::: gst-libs/gst/gl/gstglupload.c
@@ +561,3 @@
+    ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_GL_MEMORY);
+  } else {
+    ret = _set_caps_features (caps, "memory:" GST_ALLOCATOR_DMABUF);

This is still wrong. We need a proper define and documentation for that caps feature before introducing it. In V4L2src, you will never require that caps feature, we will produce dmabuf regardless in the near future.

Created attachment 316605 [details] [review]
glupload: Add dmabuf upload method.

* check for EGL_KHR_image_base
* do not use memory:dmabuf caps

(In reply to Nicolas Dufresne (stormer) from comment #73)
> We need a proper define and documentation for that caps feature before introducing it.

Should it be defined in gst-plugins-base/gst-libs/gst/allocators/gstdmabuf.h :

#define GST_ALLOCATOR_DMABUF "dmabuf"
+ #define GST_CAPS_FEATURE_MEMORY_DMABUF "memory:DMABuf"

?  (or in gst core next to "memory:SystemMemory", I can see also GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY with an "S". Worth to mention GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY occurrence in gst-inspect.c)

(In reply to Julien Isorce from comment #75)
> (In reply to Nicolas Dufresne (stormer) from comment #73)
> > We need a proper define and documentation for that caps feature before introducing it.
> 
> Should it be defined in gst-plugins-base/gst-libs/gst/allocators/gstdmabuf.h
> :
> 
> #define GST_ALLOCATOR_DMABUF "dmabuf"
> + #define GST_CAPS_FEATURE_MEMORY_DMABUF "memory:DMABuf"
> 
> ?  (or in gst core next to "memory:SystemMemory", I can see also
> GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY with an "S". Worth to mention
> GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY occurrence in gst-inspect.c)

I think I prefer that in the allocator. And yes, it's CAPS_FEATURES with an S.

(In reply to Nicolas Dufresne (stormer) from comment #76)
> (In reply to Julien Isorce from comment #75)
> > (In reply to Nicolas Dufresne (stormer) from comment #73)
> > > We need a proper define and documentation for that caps feature before introducing it.
> > 
> > Should it be defined in gst-plugins-base/gst-libs/gst/allocators/gstdmabuf.h
> > :
> > 
> > #define GST_ALLOCATOR_DMABUF "dmabuf"
> > + #define GST_CAPS_FEATURE_MEMORY_DMABUF "memory:DMABuf"
> > 
> > ?  (or in gst core next to "memory:SystemMemory", I can see also
> > GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY with an "S". Worth to mention
> > GST_CAPS_FEATURES_MEMORY_SYSTEM_MEMORY occurrence in gst-inspect.c)
> 
> I think I prefer that in the allocator. And yes, it's CAPS_FEATURES with an
> S.

I created https://bugzilla.gnome.org/show_bug.cgi?id=759358 , I'll try to make a patch.

Review of attachment 316599 [details] [review]:

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +327,3 @@
+ */
+static int
+_drm_fourcc_from_info (GstVideoInfo * info)

The mapping are really weird and does not work for Big Endian. I believe we could make it better by assuming a direct mapping for 32bits RGB formats. This way the converter will passthrough for those formats.

Another way, similar to what you did, would have a fallback to importing everything as RGBA/RGBx (DRM_FORMAT_ABGR8888/XBGR8888 in little endian). The last would at least ensure that our shaders matches. This mostly seems like what you have done, except some weirdness described lower.

@@ +337,3 @@
+      return DRM_FORMAT_RGB565;
+    case GST_VIDEO_FORMAT_RGBA:
+      return DRM_FORMAT_ARGB8888;

That's odd, RGBA should be 1-1 match with BGRA. I believe this should cause the red and blue to be swapped.

@@ +343,3 @@
+      return DRM_FORMAT_XBGR8888;
+    case GST_VIDEO_FORMAT_xRGB:
+      return DRM_FORMAT_ABGR8888;

This is really weird, why use an alpha channel enabled format for the opaque format, and use opaque for for BGRA ? We never know if the sampler will optimize out the alpha on certain target. I think the best it so stick with ABGR for everything, our shaders do set the alpha to opaque already.

@@ +345,3 @@
+      return DRM_FORMAT_ABGR8888;
+    case GST_VIDEO_FORMAT_GRAY8:
+      return fourcc_code ('R', '8', ' ', ' ');

Isn't there any defined for those ?

@@ +348,3 @@
+    case GST_VIDEO_FORMAT_YUY2:
+    case GST_VIDEO_FORMAT_UYVY:
+      return fourcc_code ('G', 'R', '8', '8');

Same, note, you have can use at the start a define like:

#ifndef DRM_FORMAT_GR88
#define ...
#endif

To avoid having to ifdef the code.

@@ +448,3 @@
+
+  for (int i = 0; i < meta->n_planes; i++)
+    GST_DEBUG ("meta: offset[%d]: %ld, stride[%d]: %d",

I had a warning on 32bit platform with that %ld. I believe this is a gsize, for which you should use %" G_GSIZE_FORMAT "...

@@ +462,3 @@
+    out_buffer = gst_buffer_new ();
+    gst_buffer_insert_memory (out_buffer, 0, (GstMemory *) gl_mem);
+  } else if (n_mem == 1 && meta->n_planes > 1) {

Instead of forcing having 1 n_mem, you could use the same technique as in map_default found in GstVideoMeta. This way you can support both single and multiple memory cases without having two code path.

::: gst-libs/gst/gl/egl/gsteglimagememory.h
@@ +66,3 @@
 
+#if GST_GL_HAVE_DMABUF
+GstBuffer *

Remove newline here.

Review of attachment 316605 [details] [review]:

::: gst-libs/gst/gl/gstglupload.c
@@ +545,3 @@
+    for (i = 0; i < n; i++) {
+      GstStructure *s = gst_caps_get_structure (ret, i);
+      gst_structure_remove_fields (s, "texture-target", NULL);

Why is this the only place we need to do that ?

@@ +588,3 @@
+
+  if (gst_gl_context_check_feature (dmabuf->upload->context,
+          "EXT_image_dma_buf_import")) {

Why do you check this ?

(In reply to Nicolas Dufresne (stormer) from comment #73)
> Review of attachment 316601 [details] [review] [review]:
> 
> ::: gst-libs/gst/gl/gstglupload.c
> @@ +561,3 @@
> +    ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_GL_MEMORY);
> +  } else {
> +    ret = _set_caps_features (caps, "memory:" GST_ALLOCATOR_DMABUF);
> 
> This is still wrong. We need a proper define and documentation for that caps
> feature before introducing it. In V4L2src, you will never require that caps
> feature, we will produce dmabuf regardless in the near future.

Lubosz, I know Nicolas said the above comment but then you replaced it by "ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY);"

I think you should set both. Upstream elements should still be aware of GST_CAPS_FEATURE_MEMORY_DMABUF but it should not be a requirement.
(also see EGLIMAGE caps feature: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglupload.c#n362)

Thx Lubosz for all that great work, I hope we can get all of this merge soon enough.

I said in a separate patch and that it can happen later.

Ok, I've rebased on git master today, to find out it didn't work. Looks like mesa debug (or some of our doing?) have a nice little fence against GL operation run on the wrong thread. While fixing that, I realize that we could do a little better in term of code organization. So here's what I propose:

In _accept():
1) Only wrap the dmabufs into EGLImage

Right now we also create and bind a texture. This seems like something to be done in the perform() step. Also this is what was run on the wrong thread, while creating EGLImage can be done from any thread.

2) Use the GstEGLImageMemory wrapper

By wrapping the EGLImage from dmabuf, we will not be able to cache those. As my experiment on MALI showed, there is good load of kernel and GL stack CPU that can be saved by associating the EGLImages with the incoming DMABufMemory. In a first experiment, I'll simply attach a quark to each DMABufMemory. Seems fair method considering the one-to-one relation with the DMABuf. Also, as EGLImage are not associate with a context, they can be used from multiple sink (even though this will not happen, as tee will like prevent that).

3) Replace the quark with parent_buffer_meta

As I get to know this new meta, it is indeed an excellent fit for out needs here.

All this will make that uploader much more similar to other uploaders. And will be able to benifit from pools when that is implemented for _raw and _gl_upload_meta.

Sounds like a good plan to me.

Ok, the rework is done now. I think I can wave the formats limitation we had and expose everything we usually support now. I'll be doing more testing today and tomorrow before merging. If anyone have time testing here's my branch:

https://git.collabora.com/cgit/user/nicolas/gst-plugins-bad.git/log/?h=dmabuf

Nicolas, I tried your dmabuf branch with ChromiumGStreamerBackend and it worked like a charm :)

Though I had to do the 2 last commits here: https://github.com/CapOM/gst-plugins-bad/commits/dmabuf_18dec
First one is because I still have not updated my gstreamer-vaapi branch so that's another problem.
For the revert it is because I got:

autopoint: *** The AM_GNU_GETTEXT_VERSION declaration in your configure.ac
               file requires the infrastructure from gettext-0.19 but this version
               is older. Please upgrade to gettext-0.19 or newer.
autopoint: *** Stop.

Aside note I noticed that in gstreamer repo:
gstreamer/tests/check/gst/gstcaps.c
GST_START_TEST (test_features)
"memory:EGLImage"
"memory:VASurface"
"memory:dmabuf"

So please push :) Thx!

I've been iterating over all formats (using vivid) and found small issue, but seems to hold on for me to. I think I'll drop the gettext version bump for now. While I'd love to see warnings going away, I'd prefer not messing up everybody's builds just now.

Attachment 316105 [details] pushed as ebeae8b - glimagesink: Show error when video frame is not mapped.
Attachment 316412 [details] pushed as eb73cf3 - gstglcontext_egl: Expose gst_gl_context_egl_get_error_string.
Attachment 316599 [details] pushed as 1cd9786 - eglimagememory: Methods to create GstGLMemory from dmabufs
Attachment 316605 [details] pushed as 420a175 - glupload: Add dmabuf upload method.

Comment on attachment 316102 [details] [review]
configure.ac: Fix warnings.

Marking as rejected, but please, open a new ticket to see if there is a way forward fixing those warnings.

Some more patches:

commit 7b335c4dd5dc14901ace884988e206a5ebcb08ba
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Dec 18 15:52:46 2015 -0500

    eglimagememory: Also import BGR16, ABGR, xBGR, AYUV, GRAY16_LE/BE and Y444
    
    https://bugzilla.gnome.org/show_bug.cgi?id=743345

commit b17a732d5f02ef4e46641b0c62109fbe9874af36
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Dec 18 11:08:29 2015 -0500

    eglimagememory: Add RGB/BGR DMABuf importation support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=743345

commit 937e249b4453bae1da7f2a2f84c9db579fedd002
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Dec 18 15:36:40 2015 -0500

    glconvert: Fix compilation of GRAY16_LE/BE shader

And in gst-plugins-good/v4l2:

commit 2538fee2fd8fdb74b05f0a511281bc4707e7cc44
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Dec 18 15:49:43 2015 -0500

    v4l2object: Update formats table
    
    This change add all the new RGB based format. Those format removes the
    ambiguity with the ALPHA channel. Some other missing multiplanar format
    has been added with some additional cleanup.

And a special thanks to Lubosz, who worked so hard to figure-out how to make this happen ! Have good holidays all !