(In reply to Matt Fischer from comment #0)
Hi again. :)

First of all, could you upload the patch again with content-type as "patch"
so that we could see it directly.

> 1. I've had to force the chroma_type to GST_VAAPI_CHROMA_TYPE_YUV420, since
> the video format is of type RGBA and that is not supported by the display. 
> Curiously, however, the colors in the encoded video come out correctly.  It
> looks to me like the chroma type on the surface must be getting used, which
> makes me wonder why we need to bother setting the chroma type on the display
> at all.  I tried removing the VAConfigAttribRTFormat attribute completely in
> config_create() when making the context, and things still seemed to work
> properly.  What is the correct way to handle this case?

Supported format might be different depends on chipset. That's why we need to find out what formats are supported on the current chipset.
But in this case, I don't know how to handle this case.
 
> 2. The key mechanism here in transferring a texture into a surface is the
> ability to go Texture -> EGLImage -> DRM handle -> Surface.  All of the
> components of this path already existed in the codebase, so it was
> relatively straightforward to do, however this obviously doesn't work on
> non-EGL platforms.  I don't know enough about GLX to know whether there is
> some equivalent that could be done there.  For the moment, I've set it up so
> that if the texture cannot be converted to a surface, it fails gracefully
> and allows a software download to take place.  This works, but it would be
> better if a mechanism could be implemented to do this on GLX.  Does anybody
> know if that can be done?

I beleive it can be done, but IMHO, it's not necessary.
 
> 3. In order to create a display which is GL-capable, I added use of
> gst_gl_ensure_element_data()/gst_gl_handle_set_context(), so that the plugin
> works the same way that other GL plugins do.  However, I think this steps a
> bit on the code which already existed in
> gst_vaapi_plugin_base_decide_allocation(), which tried to pull a context out
> of the GLTextureUploadMeta.  Could this code simply be deleted now that the
> plugin does a full context query?

As I said in the mailing list, if your plugin pass gl context via context query, it should be fine. Because vaapi plugin will create proper display if it got it via gst_vaapi_plugin_base_find_gl_context.

Here is my test pipeline to test.
GST_GL_PLATFORM=egl gst-launch-1.0 gltestsrc ! vaapih264enc ! vaapih264dec ! glimagesink

I have to use glimagesink to get gl context from it and see what's working.
(There's some hack in vaapih264dec to get gl context)
I guess your own upstream plugin creates gl context, which means you can pass it.

Comment on attachment 342994 [details] [review]
Patch to enable GLMemory buffers

Fixed the mime and checked the patch option.

(In reply to Nicolas Dufresne (stormer) from comment #2)
> Comment on attachment 342994 [details] [review] [review]
> Patch to enable GLMemory buffers
> 
> Fixed the mime and checked the patch option.

Whoops, sorry about that.  Thank you, Nicolas.

(In reply to Hyunjun Ko from comment #1)
> (In reply to Matt Fischer from comment #0)
> Supported format might be different depends on chipset. That's why we need
> to find out what formats are supported on the current chipset.
> But in this case, I don't know how to handle this case.
That makes sense.  It's just interesting that even in a case where the chipset reports only handling YUV formats, I can pass in a surface that was created with RGB32, and it will be encoded properly.  Maybe this is behavior that happens to work in my driver, but wouldn't in other drivers?  I don't have any experience with this area, so I'm not sure how to proceed.

> I beleive it can be done, but IMHO, it's not necessary.
In that case, what I have should work, and leaves the door open for some other motivated developer to add the missing piece in the future. :)

> As I said in the mailing list, if your plugin pass gl context via context
> query, it should be fine. Because vaapi plugin will create proper display if
> it got it via gst_vaapi_plugin_base_find_gl_context.
Ah sorry, my confusion was that I was working off of an older version, where this gst_vaapi_plugin_base_find_gl_context() function hadn't been implemented yet.  On master it looks like it should work correctly without any changes.  I'll upload a new version of my patch which removes the changes I added there.

Created attachment 343036 [details] [review]
Patch to enable GLMemory buffers

Updated patch to remove unnecessary GL mechanics, and add GLMemory to vaapisink as well.

Review of attachment 343036 [details] [review]:

Hi,

Thanks for this contribution!

The idea looks good, but I my idea is to get rid of the custom GL API in gstreamer-vaapi and use GstGL instead. Then, many of this code will be removed and we should think in another way to keep this functionality.

If I understand correctly, only the we could import EGL textures to VASurfaces. Again, IIUC, this is only possible if the EGL textures are EGL Images and they can export its internal dmabuf descriptor. Bearing this in mind, I think that the code would be improved and simplified if the "uploader" function, just check if the incoming GstMemory is EGL and has an exportable dmabuf. If so, grab the dmabuf descriptor and "acquire" it to import it as a VASurface.

But this gets trickier, since we would only negotiate GstGLMemory if upstream provides EGL based buffers with exportable dmabufs.

Certainly the best case would be if we can export a GLMemory on all platforms, so that the functionality can always be depended on.  I imagine there is probably some way to do this with GLX, I just don't know GLX well enough to know for sure.  If somebody who understood it better could comment on whether there's a way to do it, that would be very helpful.

For the EGL case, the path that's available is to wrap the texture in an EGLImage.  That image can then be exported either to a DRM buffer using the MESA_drm_image extension, or to a dmabuf using the MESA_image_dma_buf_export extension.  Either of these can then be imported to a VASurface by making a buffer proxy out of them.  I chose to do it via the DRM buffer route because that's the method used by the gst_vaapi_surface_new_with_egl_image() function which has already been implemented.  In principle I think that could just as easily be done using dmabuf instead, but I didn't see any reason to change what had already been written since it seems to work.  How would you like to see this patch proceed?

Also, do you have any comments on the chroma_type issue I mentioned above?  That would still need to be resolved regardless of what technique is used to do the actual buffer importing.

(In reply to Matt Fischer from comment #7)
> Certainly the best case would be if we can export a GLMemory on all
> platforms, so that the functionality can always be depended on.  I imagine
> there is probably some way to do this with GLX, I just don't know GLX well
> enough to know for sure.  If somebody who understood it better could comment
> on whether there's a way to do it, that would be very helpful.

In deed. In the case of GLX, (again) IIUC, it would need to dump the texture's pixels into a pixmap and then copy it into a system's memory GstMemory. It won't be zero-copy.

Which brings me to another approach to this functionality: the element gldownload.

That element downloads a GstGLMemory into a system's memory based GstMemory. Which is normally the case without zero-copy.

We could extend this functionality to the case where the GstGLImage is EGL, and EGL_MESA_image_dma_buf_export is available, to generate a GstDmaBufMemory, which could be imported by gstreamer-vaapi as is.

What do you think Matthew??

> 
> For the EGL case, the path that's available is to wrap the texture in an
> EGLImage.  That image can then be exported either to a DRM buffer using the
> MESA_drm_image extension, or to a dmabuf using the MESA_image_dma_buf_export
> extension.  Either of these can then be imported to a VASurface by making a
> buffer proxy out of them.  I chose to do it via the DRM buffer route because
> that's the method used by the gst_vaapi_surface_new_with_egl_image()
> function which has already been implemented.  In principle I think that
> could just as easily be done using dmabuf instead, but I didn't see any
> reason to change what had already been written since it seems to work.  How
> would you like to see this patch proceed?
> 
> Also, do you have any comments on the chroma_type issue I mentioned above? 
> That would still need to be resolved regardless of what technique is used to
> do the actual buffer importing.

IIUC, with the approach above this wouldn't be necessary, since this information is provided by EGL_MESA_image_dma_buf_export.

That's an interesting idea.  So in that case, there wouldn't really need to be any patch to gstreamer-vaapi at all, it would just be a change to gldownloadelement.  I'll have a look at that and see whether it would work.

Even if that does work, though, there will still be buffers coming in with a color format of RGBA.  That's going to cause the encoder plugin to reject the caps, because the VADisplay will only report that it supports chroma_type of YUV.  How should that issue be addressed?

(In reply to Víctor Manuel Jáquez Leal from comment #8)
> In deed. In the case of GLX, (again) IIUC, it would need to dump the
> texture's pixels into a pixmap and then copy it into a system's memory
> GstMemory. It won't be zero-copy.
> 
> Which brings me to another approach to this functionality: the element
> gldownload.
> 
> That element downloads a GstGLMemory into a system's memory based GstMemory.
> Which is normally the case without zero-copy.

Which is actually just a simple wrapper for the caps negotiation to be correct currently, all the transfers happen in the memory when mapped/unmapped.

> We could extend this functionality to the case where the GstGLImage is EGL,
> and EGL_MESA_image_dma_buf_export is available, to generate a
> GstDmaBufMemory, which could be imported by gstreamer-vaapi as is.
> 
> What do you think Matthew??

That's certainly a possibility.  There's still the option of the dma-buf import failing so I think gldownload will have to push a combined memory type so that on dma-buf import failure, one can fallback to a texture download and reupload into vaapi.

Created attachment 343387 [details] [review]
Patch to add dmabuf export to gldownload

Here's a very hacked-together patch that implements the gldownload strategy.  It still needs a bunch of work before it would be ready to go in, but it seems to work correctly, at least on my system.

Questions about this patch:
  1.  The way this is set up, it will export the dmabuf every frame.  That's too much work, we should be able to just export the buffer once and then hang on to the exported dmabuf fd.  What's the best way to set that up?
  2.  Right now it performs this export whenever it can.  Would there be situations where we wouldn't want a dmabuf export?  It seems like as long as the resulting buffer is mappable, the downstream plugins can still get at it in software if they want to, so things would still work the way they expect.  But I'm not too familiar with the intricacies of dmabufs, so I don't know if there would be other reasons that we might not want to do a dmabuf export in some cases.

(In reply to Matt Fischer from comment #11)
> Created attachment 343387 [details] [review] [review]
> Patch to add dmabuf export to gldownload
> 
> Here's a very hacked-together patch that implements the gldownload strategy.
> It still needs a bunch of work before it would be ready to go in, but it
> seems to work correctly, at least on my system.
> 
> Questions about this patch:
>   1.  The way this is set up, it will export the dmabuf every frame.  That's
> too much work, we should be able to just export the buffer once and then
> hang on to the exported dmabuf fd.  What's the best way to set that up?

That would probably be best performed by a buffer pool somewhere.

>   2.  Right now it performs this export whenever it can.  Would there be
> situations where we wouldn't want a dmabuf export?  It seems like as long as
> the resulting buffer is mappable, the downstream plugins can still get at it
> in software if they want to, so things would still work the way they expect.
> But I'm not too familiar with the intricacies of dmabufs, so I don't know if
> there would be other reasons that we might not want to do a dmabuf export in
> some cases.

I'm not sure either however it is certainly possible for dmabuf's to be unmappable by software elements which is where the caps feature is useful in order to explicitly request (or not) a certain feature.

An option is to push a completely new memory type from gldownload so that symem mapping will always work (as far as is permitted by GL and dmabuf).

Another completely different option is for vaapi to propose a glbufferpool that already deals with creating the egl images attached to textures. (probably not)

The other option is for vaapi to do the transformation to eglimages itself although that's less exciting. (probably not)

(In reply to Matthew Waters (ystreet00) from comment #12)
> That would probably be best performed by a buffer pool somewhere.

Yeah, I can add a buffer pool, but we need more than just that here.  You don't just want to preserve the buffers you made, you also need to be able to look up the output buffer associated with a specific input buffer when it comes in.  That way, when buffer A comes in, you can pull out the output buffer associated with its texture, and when buffer B comes in, you can get its buffer, etc.  I don't know that buffer pools provide a way to do this, do they?

I think you'd need some kind of map where you could save them all, but then you get into tricky issues of when to clean them up and stuff like that.  Maybe it'd be enough to just drop them all when the element stops or something.

> I'm not sure either however it is certainly possible for dmabuf's to be
> unmappable by software elements which is where the caps feature is useful in
> order to explicitly request (or not) a certain feature.

My understanding is that when the dmabuf is not mappable, you are supposed to use memory:DMABuf, to tell the downstream element that it must use special API's to access the data.  But if the dmabuf can be mapped, then you aren't supposed to use any special features, since it can be trivially treated like a software buffer if the downstream element doesn't know any better.  Is that correct?

I don't see any documentation in MESA_image_dma_buf_export about whether the dmabufs it produces are mappable, though, so I'm not sure what to say for this case.

> An option is to push a completely new memory type from gldownload so that
> symem mapping will always work (as far as is permitted by GL and dmabuf).

That would probably work, although you would need to make sure that gst_is_dmabuf_memory() and gst_dmabuf_memory_get_fd() still work properly on it, so that the existing VAAPI code can still import the dmabuf directly.

> Another completely different option is for vaapi to propose a glbufferpool
> that already deals with creating the egl images attached to textures.
> (probably not)

You would probably need to invent a GstGLTextureDownloadMeta that the bufferpool could put on the buffers, so that gldownload knows how to tell it to perform the download, right?  Might be nice for symmetry with GstGLTextureUploadMeta, but sounds like a lot of work.

> The other option is for vaapi to do the transformation to eglimages itself
> although that's less exciting. (probably not)

That's the way my original patch on this bug did it.  I just added memory:GLMemory to all of the elements' input caps, and added code to detect getting a GstGLMemory and perform an EGLImage -> DRM buffer -> VASurface conversion.

Now that you can see these two methods side-by-side, do you guys have any thoughts on which one would be the better way to go about this?

(In reply to Matt Fischer from comment #13)
> (In reply to Matthew Waters (ystreet00) from comment #12)
> > That would probably be best performed by a buffer pool somewhere.
> 
> Yeah, I can add a buffer pool, but we need more than just that here.  You
> don't just want to preserve the buffers you made, you also need to be able
> to look up the output buffer associated with a specific input buffer when it
> comes in.  That way, when buffer A comes in, you can pull out the output
> buffer associated with its texture, and when buffer B comes in, you can get
> its buffer, etc.  I don't know that buffer pools provide a way to do this,
> do they?
> 
> I think you'd need some kind of map where you could save them all, but then
> you get into tricky issues of when to clean them up and stuff like that. 
> Maybe it'd be enough to just drop them all when the element stops or
> something.

Perhaps you don't need a buffer pool, but "associates" the input buffer with a created output buffer, using gst_mini_object_set_qdata(). If the input buffer has already an associated output buffer, just reuse it. In theory, it should have the same dmabuf fd if the texture hasn't changed. This is what GstGLUpload does.

> 
> > I'm not sure either however it is certainly possible for dmabuf's to be
> > unmappable by software elements which is where the caps feature is useful in
> > order to explicitly request (or not) a certain feature.
> 
> My understanding is that when the dmabuf is not mappable, you are supposed
> to use memory:DMABuf, to tell the downstream element that it must use
> special API's to access the data.  But if the dmabuf can be mapped, then you
> aren't supposed to use any special features, since it can be trivially
> treated like a software buffer if the downstream element doesn't know any
> better.  Is that correct?
> 
> I don't see any documentation in MESA_image_dma_buf_export about whether the
> dmabufs it produces are mappable, though, so I'm not sure what to say for
> this case.

An approach would be to try to map the first generated dmabuf buffer. If maps fails, memory:DMABuf capfeature is negotiated, otherwise no features.

> 
> > An option is to push a completely new memory type from gldownload so that
> > symem mapping will always work (as far as is permitted by GL and dmabuf).
> 
> That would probably work, although you would need to make sure that
> gst_is_dmabuf_memory() and gst_dmabuf_memory_get_fd() still work properly on
> it, so that the existing VAAPI code can still import the dmabuf directly.
> 
> > Another completely different option is for vaapi to propose a glbufferpool
> > that already deals with creating the egl images attached to textures.
> > (probably not)
> 
> You would probably need to invent a GstGLTextureDownloadMeta that the
> bufferpool could put on the buffers, so that gldownload knows how to tell it
> to perform the download, right?  Might be nice for symmetry with
> GstGLTextureUploadMeta, but sounds like a lot of work.

I don't see this. Though I was one who asked for the merge of GstGLTextureUploadMeta, now I think that it is not the best idea, it is very problematic (gl context sharing and so -- not only GstGL elements, but clutter and maybe others).

> > The other option is for vaapi to do the transformation to eglimages itself
> > although that's less exciting. (probably not)
> 
> That's the way my original patch on this bug did it.  I just added
> memory:GLMemory to all of the elements' input caps, and added code to detect
> getting a GstGLMemory and perform an EGLImage -> DRM buffer -> VASurface
> conversion.
> 
> Now that you can see these two methods side-by-side, do you guys have any
> thoughts on which one would be the better way to go about this?

As you may see, I'm a bit reluctant to clutter gstreamer-vaapi more as it is now.

Created attachment 343748 [details] [review]
Patch to add dmabuf export to gldownload

Using gst_mini_object_set_qdata() seems to work great.  I've uploaded a new version which uses it to save the images it exports, so now it's a bit less offensive.

I still don't really know what to say about the mappable vs. non-mappable issue.  Detecting it during playback and renegotiating caps seems kind of tricky.  I'm not an expert on dmabufs, so I really don't know how to predict whether in practice these exported images would always be mappable, or not.  So it's hard to know whether we're safe doing this all the time, or whether that will cause problems in some circumstances.

I know some other elements like v4lsrc have a property which can be set on them, which directs them to attempt a dmabuf export.  That's a little bit clunky, but it would be one option to ensure that we don't accidentally do this in settings where it wouldn't work properly.  Would that be a good option to pursue, or do you have some other thoughts on how this could best be integrated?

Review of attachment 343748 [details] [review]:

I guess you shall first check if you context is EGL and it has the required feature to export dmabuf. If does, you could instantiate the dmabuf allocator and then enable the call _try_export_dmabuf(). Otherwise keep the old code path.

Created attachment 343926 [details] [review]
Patch to add dmabuf export to gldownload

Here's an updated patch that cleans up the implementation a bit.  It checks whether the context is EGL before attempting an export.  If the context does not possess the appropriate extensions, then the function will fail gracefully.

This works well for downloading an OpenGL buffer into a dmabuf, which is then picked up by the vaapi plugins correctly.  However, on my system, the dmabuf is not mappable, so any other use of gldownload (i.e. waylandsink) doesn't work, because the buffer cannot be mapped into a regular CPU buffer.  So this approach is definitely not sufficient by itself, because we don't know when to export and when not to export.

One answer would be to negotiate the memory:DMABuf feature, and only attempt the export when that feature was negotiated.  For that to work, the VAAPI plugins would need to add memory:DMABuf as a possible input caps feature.  I don't know enough about the history of memory:DMABuf to know whether or not that will be an option.  Some of the discussion in https://bugzilla.gnome.org/show_bug.cgi?id=755072 made it seem like something that can't be used here.  Can you provide some context?

Another possible option would be to invent a new meta that we could attach to the buffer.  So the main buffer would remain as it is, but dmabuf-aware elements could grab the meta and use it to perform the export.  This would require adding new code into vaapipluginbase to make use of it, though, and would add more complication to what seems to already be a very complicated issue, so maybe it's not a good option.

Do you have any suggestions on how I might proceed with this patch?  Thanks for all of your help so far.

Ping.  Does anybody have any thoughts on these patches?  I'd really like to be able to move forward with one or the other of these strategies.

(In reply to Matt Fischer from comment #15)
> Created attachment 343748 [details] [review] [review]
> Patch to add dmabuf export to gldownload
> 
> I still don't really know what to say about the mappable vs. non-mappable
> issue.  Detecting it during playback and renegotiating caps seems kind of
> tricky.  I'm not an expert on dmabufs, so I really don't know how to predict
> whether in practice these exported images would always be mappable, or not. 
> So it's hard to know whether we're safe doing this all the time, or whether
> that will cause problems in some circumstances.

I think the general idea is that dma-buf's from the graphics driver are just not mappable (unless you're using dumb buffers, which can't be allocated from OpenGL).

(In reply to Matt Fischer from comment #17)
> Created attachment 343926 [details] [review] [review]
> Patch to add dmabuf export to gldownload
> 
> One answer would be to negotiate the memory:DMABuf feature, and only attempt
> the export when that feature was negotiated.  For that to work, the VAAPI
> plugins would need to add memory:DMABuf as a possible input caps feature.  I
> don't know enough about the history of memory:DMABuf to know whether or not
> that will be an option.  Some of the discussion in
> https://bugzilla.gnome.org/show_bug.cgi?id=755072 made it seem like
> something that can't be used here.  Can you provide some context?

Why can't memory:DMABuf be used?  IIRC, memory:DMABuf is for this exact case where the dmabuf is not mappable.

> Another possible option would be to invent a new meta that we could attach
> to the buffer.  So the main buffer would remain as it is, but dmabuf-aware
> elements could grab the meta and use it to perform the export.  This would
> require adding new code into vaapipluginbase to make use of it, though, and
> would add more complication to what seems to already be a very complicated
> issue, so maybe it's not a good option.

This is ugly :(

> Do you have any suggestions on how I might proceed with this patch?  Thanks
> for all of your help so far.

I think making the assumption (for now) that the DMABuf is not mappable and therefore requiring the memory:DMABuf caps feature for gldownload to push dmabuf's is the way forward.

I took the liberty to move this bug reporto gst-plugins-bad for gstgl. I still think that this issue should be solved by gldownload. I hope we agree on that.

Review of attachment 343926 [details] [review]:

::: ext/gl/gstgldownloadelement.c
@@ +162,3 @@
 }
 
+#if GST_GL_HAVE_PLATFORM_EGL && GST_GL_HAVE_DMABUF

We don't only need compilation-time validation of the EGL and dmabuf, we must need check it at run-time.

See, for example, this patch that I hope to merge today in gstreamer-vaapi

https://github.com/ceyusa/gstreamer-vaapi/commit/0bdb0f98ad02775bf6de8d7bd52fc0e86ed4dd62

So, if your current GL context is EGL and it provides eglExportDMABUFImageMESA, then you could enable and negotiate dmabuf to downstream.

@@ +317,3 @@
+#if GST_GL_HAVE_PLATFORM_EGL && GST_GL_HAVE_DMABUF
+  if (!features || gst_caps_features_contains (features,
+          GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY)) {

We should add  memory:DMABuf in the gldownload src template caps, but only if the GL context is EGL with dmabuf export support, otherwise that caps should be blacklisted.

Created attachment 345062 [details] [review]
Patch to add dmabuf export to gldownload

Here's a patch which adds the use of the memory:DMABuf feature.  That does seem like the right way to go, although it will require the VAAPI plugins to expose that cap feature on their sink pads before this will work correctly.  Victor, if this patch were accepted, would that be an ok thing to add to the VAAPI plugins?

Currently it exposes the feature whenever dmabuf support and EGL support is compiled in.  It does not yet attempt to query the context at runtime to determine if EGLImage exporting is possible.  I'm not exactly sure how to do that timing-wise, because we have to announce the caps during negotiation, but we may not have a context until some time later.  So I'm worried that by the time we get a context, it will be too late to disable the dmabuf feature if we discover the context doesn't support it.  Are there any examples of this kind of thing being done right now that I could go and look at?

Review of attachment 345062 [details] [review]:

::: ext/gl/gstgldownloadelement.c
@@ +58,3 @@
+    GST_STATIC_CAPS (
+#if GST_GL_HAVE_PLATFORM_EGL && GST_GL_HAVE_DMABUF
+        "video/x-raw(memory:DMABuf); "

Please use GST_CAPS_FEATURE_MEMORY_DMABUF, this way we are sure there is no typo.

@@ +311,3 @@
+  if (buffer)
+    gst_buffer_add_video_meta_full (buffer, vmeta->flags, vmeta->format,
+        vmeta->width, vmeta->height, vmeta->n_planes, offset, stride);

You need to check if downstream supports GstVideoMeta. If not, you may check (option) that the list os strides and offsets matches the default set to GstVideoInfo when doing gst_video_info_set_format(). If that does not match, dmabuf exportation is not possible, return NULL.

@@ +339,1 @@
+  if (!buffer) {

I think it's a good case were using a goto would be more readable.

::: gst-libs/gst/gl/egl/gsteglimage.c
@@ +481,3 @@
+  gst_eglExportDMABUFImageQueryMESA =
+      gst_gl_context_get_proc_address (image->context,
+      "eglExportDMABUFImageQueryMESA");

We have function tables already, so you don't endup doing get_proc_address() every image plane.

@@ +506,3 @@
+    return FALSE;
+
+  /* Don't allow multi-plane dmabufs */

Why ? What's the problem ? and why checking this so late ? We already support multiple FD when importing. I don't think this is very useful if you limit this to packed formats.

(In reply to Nicolas Dufresne (stormer) from comment #23)
> Please use GST_CAPS_FEATURE_MEMORY_DMABUF, this way we are sure there is no
> typo.
> 
Will do.

> You need to check if downstream supports GstVideoMeta. If not, you may check
> (option) that the list os strides and offsets matches the default set to
> GstVideoInfo when doing gst_video_info_set_format(). If that does not match,
> dmabuf exportation is not possible, return NULL.
> 
How does one check if downstream supports a given meta?  I was under the impression that you just attached whatever meta you wanted, and hoped that downstream paid attention to it.  Since I am already relying on there being a GstVideoMeta on the incoming buffer, I had thought that it would be safe to assume that I could just create one on the outgoing buffer as well.

> I think it's a good case were using a goto would be more readable.
> 
Will do.

> We have function tables already, so you don't endup doing get_proc_address()
> every image plane.
> 
I'm not sure I understand what you're asking me to do here.  Can you clarify?

> Why ? What's the problem ? and why checking this so late ? We already
> support multiple FD when importing. I don't think this is very useful if you
> limit this to packed formats.
>
This is just a comment about multiple planes within the dmabuf export.  A planar image will have multiple GstMemory objects, each of which will get its own EGLImage and its own dmabuf.  See the loop in _try_export_dmabuf() to see how it will loop for multiple planes.

As far as I understand, that will correctly handle the planar cases.  If we try to handle the case where each of those individual EGLImages also exports multiple dmabufs, I wasn't clear how we would handle the N * M dmabuf handles that would result.  Can you comment on what we ought to do in that case?

(In reply to Matt Fischer from comment #24)
> (In reply to Nicolas Dufresne (stormer) from comment #23)
> > You need to check if downstream supports GstVideoMeta. If not, you may check
> > (option) that the list os strides and offsets matches the default set to
> > GstVideoInfo when doing gst_video_info_set_format(). If that does not match,
> > dmabuf exportation is not possible, return NULL.
> > 
> How does one check if downstream supports a given meta?  I was under the
> impression that you just attached whatever meta you wanted, and hoped that
> downstream paid attention to it.  Since I am already relying on there being
> a GstVideoMeta on the incoming buffer, I had thought that it would be safe
> to assume that I could just create one on the outgoing buffer as well.

I miss that in the review, you indeed need to check if the vmeta pointer is null, otherwise your code may crash. To check if downstream supports GstVideoMeta, you need to look into the allocation query. See "propose_allocation" function in _UploadMethod structure. There is a "decide_query" pointer that contains this information. Then you can get the information using:

  gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)

> > We have function tables already, so you don't endup doing get_proc_address()
> > every image plane.
> > 
> I'm not sure I understand what you're asking me to do here.  Can you clarify?

You should not be calling get_proc_address() for each frame, but cache those function pointer. That being said, I just noticed Mathew started doing that, so you took that from existing code. We can fix that later, it's just slightly inefficient.

> 
> > Why ? What's the problem ? and why checking this so late ? We already
> > support multiple FD when importing. I don't think this is very useful if you
> > limit this to packed formats.
> >
> This is just a comment about multiple planes within the dmabuf export.  A
> planar image will have multiple GstMemory objects, each of which will get
> its own EGLImage and its own dmabuf.  See the loop in _try_export_dmabuf()
> to see how it will loop for multiple planes.

Ok, that looks correct, nevermind then ;-P

> 
> As far as I understand, that will correctly handle the planar cases.  If we
> try to handle the case where each of those individual EGLImages also exports
> multiple dmabufs, I wasn't clear how we would handle the N * M dmabuf
> handles that would result.  Can you comment on what we ought to do in that
> case?

Well, in theory we could support N Texture/Plane coming out as 1 DMABuf. But in practice, I doubt it's really going to happen, and it's probably complicated in the current code, so nothing you should worry about.

(In reply to Nicolas Dufresne (stormer) from comment #25)
> I miss that in the review, you indeed need to check if the vmeta pointer is
> null, otherwise your code may crash. To check if downstream supports
> GstVideoMeta, you need to look into the allocation query. See
> "propose_allocation" function in _UploadMethod structure. There is a
> "decide_query" pointer that contains this information. Then you can get the
> information using:
> 
>   gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)
> 
That makes sense, and I can add that if necessary.  However, I was wondering if we need to go to that much trouble.  Would it be sufficient simply to say if the inbuf has a VideoMeta then we're allowed to put one on the outbuf, and if it doesn't have one, then just don't bother with exporting at all?  The offset and stride have to be communicated on the outbuf through a VideoMeta, and so if we can't put one on there, then we really can't perform the export at all.  And if the input buffer had a VideoMeta, then it seems like the pipeline had already negotiated that it was ok to pass them downstream anyway.  Would that be sufficient, or is that missing some case that we need to cover?

> You should not be calling get_proc_address() for each frame, but cache those
> function pointer. That being said, I just noticed Mathew started doing that,
> so you took that from existing code. We can fix that later, it's just
> slightly inefficient.
> 
Yeah, I hadn't seen any other code doing that, so I didn't try to do it in here either.  It's worth noting that the gst_egl_image_export_dmabuf() calls only happen once when we start up, and from then on are reused, so this is not happening per frame.

(In reply to Matt Fischer from comment #26)
> (In reply to Nicolas Dufresne (stormer) from comment #25)
> > I miss that in the review, you indeed need to check if the vmeta pointer is
> > null, otherwise your code may crash. To check if downstream supports
> > GstVideoMeta, you need to look into the allocation query. See
> > "propose_allocation" function in _UploadMethod structure. There is a
> > "decide_query" pointer that contains this information. Then you can get the
> > information using:
> > 
> >   gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)
> > 
> That makes sense, and I can add that if necessary.  However, I was wondering
> if we need to go to that much trouble.  Would it be sufficient simply to say
> if the inbuf has a VideoMeta then we're allowed to put one on the outbuf,
> and if it doesn't have one, then just don't bother with exporting at all? 

No, because the reason gldownload element have GstVideoMeta attached has input is because it will advertise it in it's main "propose_allocation". It is completely unrelated with downstream support.

> The offset and stride have to be communicated on the outbuf through a
> VideoMeta, and so if we can't put one on there, then we really can't perform
> the export at all.  And if the input buffer had a VideoMeta, then it seems
> like the pipeline had already negotiated that it was ok to pass them
> downstream anyway.  Would that be sufficient, or is that missing some case
> that we need to cover?

You should always be able to copy the data to get rid of the uncommon strides and offsets (gst_video_frame_copy() will do that). Your download code should fail, so another download method can take care. Pushing buffers with GstVideoMeta when downsteam don't support it is incorrect.

(In reply to Nicolas Dufresne (stormer) from comment #27)
> > That makes sense, and I can add that if necessary.  However, I was wondering
> > if we need to go to that much trouble.  Would it be sufficient simply to say
> > if the inbuf has a VideoMeta then we're allowed to put one on the outbuf,
> > and if it doesn't have one, then just don't bother with exporting at all? 
> 
> No, because the reason gldownload element have GstVideoMeta attached has
> input is because it will advertise it in it's main "propose_allocation". It
> is completely unrelated with downstream support.
> 
Sorry for causing so much hassle here, but I have just one more question on this.  As it stands now, the gldownload element does not have either a propose_allocation or a decide_allocation method.  My understanding is that this means it will just pass along any allocation queries from downstream to upstream without modification.  So if upstream is sending us VideoMetas, doesn't that mean that the downstream element said that it supported them, and our element simply passed that information back upstream?

I can understand your statement in cases where gldownload had its own propose/decide methods, and could therefore send something upstream which is different than what is downstream.  But in cases where it's simply passing the requests through transparently, can't we just react to what the upstream element is sending, based on the fact that it negotiated that feature with the downstream element during the allocation phase?

I can certainly implement a decide_allocation method, check the query for the VideoMeta feature, and save off a bit telling the transform function that it's ok to add VideoMetas to the buffers that it creates.  I'm just having trouble understanding the case where that would result in different behavior than simply examining the input buffer, given that gldownload passes allocation requests through unmodified.

gldownload is a subclass of GstGlBaseFilter, which implements a decide_allocation method. It's not possible to just "pass-through" the allocation method unless your element is also pass-through. or in-place. As the caps feature different for your addition, this is not passthrough, and it's not in-place since you create a new buffer.

gldownload probably have some existing issues, we just need to make sure to not make it worst. While at it, are you sure you don't need to maintain the inbuf alive in your code path ?

That being said, as you require the new DMABuf caps feature, we could also document that for that caps feature video meta is mandatory (it's probably the case defacto for GLMemory caps feature). I would like a line in -base allocators doc for that though.

(well, mandatory for raw video, dmabuf is not specific to video even though it's only used there atm)

Created attachment 352050 [details] [review]
Patch to add dmabuf export to gldownload

Turns out I was able to completely avoid looking for the video meta on the input side, and get all of the parameters I needed from the video info instead.  On the output side, if the allocation supports the video meta, I add it, and if not, then I check the offsets/strides to see if they match the video info.  If they don't, then I reject the export and we fall back to the previous case.

Here's a new patch that implements that, as well as all of the previous suggestions.  Does that cover all of the things you brought up?

Created attachment 352051 [details] [review]
Patch to add dmabuf export to gldownload

Review of attachment 352051 [details] [review]:

Looks good now, I'll do a final check when I get online tomorrow. The only remaining concern is that we don't remember if dmabuf export failed, and keep retrying. I think this may make the worst case even worst. Do you think it would be possible to add something to remember if the export failed ?

Review of attachment 352051 [details] [review]:

This looks mostly fine, a couple of issues below.

::: ext/gl/gstgldownloadelement.c
@@ +113,3 @@
+  features = gst_caps_get_features (out_caps, 0);
+  if (gst_caps_features_contains (features, GST_CAPS_FEATURE_MEMORY_DMABUF)) {
+    download->dmabuf_allocator = gst_dmabuf_allocator_new ();

You don't free this anywhere.

::: gst-libs/gst/gl/egl/gsteglimage.c
@@ +518,3 @@
+        "Failed to retrieve GstGLDisplayEGL from %" GST_PTR_FORMAT,
+        image->context->display);
+    return EGL_NO_IMAGE_KHR;

return FALSE;

Created attachment 352109 [details] [review]
Patch to add dmabuf export to gldownload

Have a look at this version.  It frees the allocator when the element state goes to NULL, and also records when a dmabuf export fails, and uses it to short-circuit further exports.

Review of attachment 352109 [details] [review]:

Not quite there :)

::: ext/gl/gstgldownloadelement.c
@@ +97,3 @@
       "Downloads data from OpenGL", "Matthew Waters <matthew@centricular.com>");
+
+  element_class->state_changed = gst_gl_download_element_state_changed;

This is typically done in change_state rather than state_changed.

@@ +417,3 @@
+
+  if (gst_query_find_allocation_meta (query, GST_VIDEO_META_API_TYPE, NULL))
+    download->add_videometa = TRUE;

And what about the failure case where on a reconfiguration, video meta is not supported anymore?

@@ +432,3 @@
+    if (download->dmabuf_allocator) {
+      gst_object_unref (GST_OBJECT (download->dmabuf_allocator));
+      download->dmabuf_allocator = NULL;

This could also just as easily done in _finalize()

Created attachment 352661 [details] [review]
Patch to add dmabuf export to gldownload

Here's a new version that addresses the comments from above.  I also had to add a call to copy_metadata(), because without it the new buffer doesn't get the right timestamps put on it.  That code was yanked from GstBaseTransform's implementation of prepare_output_buffer(), which we don't get to here because we override it and don't chain up.  In a perfect world, wouldn't we be doing all of this in the transform function and not from prepare_output_buffer()?

In any case, this seems to work for me now.  Any further comments?

Created attachment 352667 [details] [review]
Patch to add dmabuf export to gldownload

(In reply to Matt Fischer from comment #39)
> Created attachment 352667 [details] [review] [review]
> Patch to add dmabuf export to gldownload

The patch won't apply on git master, would it be possible to rebase ?

Created attachment 352676 [details] [review]
Patch to add dmabuf export to gldownload

With Meson:
/home/nicolas/Sources/gstreamer-master/gst-plugins-bad/build/../ext/gl/gstgldownloadelement.c:265: undefined reference to `gst_dmabuf_allocator_alloc'
ext/gl/gstopengl@sha/gstgldownloadelement.c.o: In function `gst_gl_download_element_set_caps':
/home/nicolas/Sources/gstreamer-master/gst-plugins-bad/build/../ext/gl/gstgldownloadelement.c:125: undefined reference to `gst_dmabuf_allocator_new'

Apprently some deps don't get propagated with automake but not meson. For automake, we have ext/gl that depends on libgstgl, that depends on badallocators which finally depends on gstreamer-allocators-1.0.

badallocators is not integrated in Meson. That is only used for the PhysMem for VIV extension. I don't think we should unconditionally link to in in the automake side. And on both side we should have an explicit dependency to gstreamer-allocators-1.0.

Created attachment 352798 [details] [review]
gldownload: Add dependency to gstallocators

Something is not quite right here, this is EGL only feature, and yet:

GST_GL_PLATFORM=egl gst-launch-1.0 -v gltestsrc ! gldownload ! video/x-raw\(memory:DMABuf\) ! fakesink

Works as if it was exporting DMABuf.

Ok, I managed to validate that this is reproducable (using hacked version of waylandsink, cause weston is still broken). Though I notice that it won't negotiate DMABuf between gldownload and waylandsink, you need a caps filter. We could theoritically blame waylandsink that puts DMABuf caps feature second, but upstream should decide the optimal way. Basically we need to make a choice between sorting in gldownload, or in waylandsink here.

Some extra note, it would make sense to add DMABuf caps feature to gldownload too, so we test with GL.

Review of attachment 352676 [details] [review]:

::: gst-libs/gst/gl/egl/gsteglimage.c
@@ +509,3 @@
+
+  if (!gst_eglExportDMABUFImageMESA (egl_display, image->image, fd, stride,
+          (EGLint *) offset))

Subtle, but very bad. I was testing with glupload to import the exported buffer and it didn't work due to bad offset, and then just found this.

You are casting a gsize* to an int* here, on X86_64 that means a 64bit array to a 32bit array. Only half of the gsize will be written in, leaving the rest as random.

Review of attachment 352676 [details] [review]:

::: ext/gl/gstgldownloadelement.c
@@ +315,3 @@
+
+    if (info) {
+      offset[i] = info->offset;

There is another bug here, the offset is from the start of the DMABuf, while the info->offset refer to the offset in the GstBuffer. To create a valid GstBuffer offset, you need the previous offset + previous memory size + info->offset.

After fixing that, it seems to work. When using glupload though I see this error once:

ERROR               glmemory gstglmemoryegl.c:201:_gl_mem_copy: GstGLMemoryEGL does not support copy

Would need to be investigated. Further note, with waylandsink I notice that performance are worst then with gldownload to memory. I notice that waylandsink will cache the wl_buffer on GstBuffer object, but create new GstBuffer all the time. That introduce a lot of round-trips with the compositor. It's a bit linked, since we also don't use a texture cache in many places in libgstgl, causing new texture to arrive in gldownload, in which case our EGLImage cache isn't useful. I think it's acceptable first implementation, assuming we fix it to not accept memory:DMABuf when in GLX, but performance is just terrible.

Comment on attachment 352798 [details] [review]
gldownload: Add dependency to gstallocators

This was in fact generic, upstream meson build was broken.

Created attachment 354656 [details] [review]
Patch to add dmabuf export to gldownload

Doh, sorry about the 64-bit thing.  Here's a version that should fix it.  I made local duplicates of the fd and the stride as well just for consistency.

I've also changed the offset logic to what I think you meant in your previous comment.  Can you confirm that it's right, though?  I can't seem to find a good test case because everything is always just 0 in my tests, but let me know if I misunderstood you and I'll get it fixed.

Does this address everything?  I didn't quite follow some of your comments above, so I'm not sure if there was anything else you wanted me to correct.

Review of attachment 354656 [details] [review]:

This is not a complete review, I'll have a look more in dept later, I also need to test it again. I've added one comment that was not address, it's a regression when running with GLX platform (GST_GL_PLATFORM=GLX).

::: ext/gl/gstgldownloadelement.c
@@ +372,3 @@
     features = gst_caps_get_features (src_caps, 0);
 
+#if GST_GL_HAVE_PLATFORM_EGL && GST_GL_HAVE_DMABUF

You are missing an extra run-time check that platform is EGL. On mesa, the export does not fail on GLX, but EGLImage won't work later on causing issues. It's also otherwise pointless to try on anything else then EGL.

I have a block at the top of _try_export_dmabuf() which is supposed to do that.  Is it not working correctly in your tests?

(In reply to Matt Fischer from comment #54)
> I have a block at the top of _try_export_dmabuf() which is supposed to do
> that.  Is it not working correctly in your tests?

Hmm, indeed, I'll update and investigate further tomorrow. Sorry for the delay, I review and test on my spare time.

Running out of time, but I wanted to mention I'm not forgetting about it.

Created attachment 364304 [details] [review]
gldownload: Add dmabuf exporting

I've rebased the patch on top of master and started cleaning it up. I have added
TODO in places were I'm not done yet. So right now, whenever it fails exporting,
it does not downgrade downstream caps to sysmem when possible, or fail with
not-negotiated.

Some small interregotation I had was also about the API, shall we do
gst_egl_image_to_dmabuf() to the oppsite of existing _from_dmabuf() ?

Other then that this patchset seems to work fine now. I've changed the
prefered order to be GLMemory (or do nothing), DMABuf and finally SysMem.

For those interested, I'm currently testing on gnome-shell 3.26, on Intel driver with:

  GST_GL_PLATFORM=egl GST_DEBUG="waylandsink:5"  gst-launch-1.0 -v v4l2src ! glupload ! glcolorconvert ! gldownload ! waylandsink

Which will get dmabuf to glupload in YUY2, shader convert to BGRx, transform to DMABuf (linearization) and pass the to gnome-shell. A fun fact, this uses X11 EGL context to pass buffer to wayland ;-P

Created attachment 364830 [details] [review]
gldownload: Add dmabuf exporting

This patch adds code to gldownload to export the image as a
dmabuf if requested.  The element now exposes memory:DMABuf as
a cap feature, and if it is selected, the element exports the
texture to an EGL image and then a dmabuf. It also implements a
fallback to system memory download in case the exportation failed.

Comment on attachment 364830 [details] [review]
gldownload: Add dmabuf exporting

Thanks Matt for you work. It's upstream now and works. For those curious, this allow sharing a stream to one of more process without copies (that include pipewire process).

commit 9f65c316e4898781757bebfbed9b46fbb6a7f7bb
Author: Matt Fischer <matt.fischer@garmin.com>
Date:   Tue Jan 10 19:23:58 2017 -0600

    gldownload: Add dmabuf exporting
    
    This patch adds code to gldownload to export the image as a
    dmabuf if requested.  The element now exposes memory:DMABuf as
    a cap feature, and if it is selected, the element exports the
    texture to an EGL image and then a dmabuf. It also implements a
    fallback to system memory download in case the exportation failed.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776927