Created attachment 340190 [details] [review]
glupload: add GST_CAPS_FEATURE_MEMORY_DMABUF

Why are the dmabuf features set after the sysmem ones?

When the dmabuf is mappable the user does not want the gst elements to work in pass-through mode (and these elements do not want to have conscience this is dmabuf) in between the export element (upstream elem) and the importer element (downstream elem).

Having it after systemMem tells the exporter to follow the rule above. If we put it after then it will not let the exporter the chance to push the dma buffers without the caps feature.

For clarity, dmabuf passed as SystemMemory can fallback if the import fails, while dmabuf with the CapsFeature set can't. You cannot trust the result of mapping the dmabuf when the caps feature is set. It may fail, or it may work and give you something black (e.g. ION secured surfaces)

(In reply to Julien Isorce from comment #3)
> When the dmabuf is mappable the user does not want the gst elements to work
> in pass-through mode (and these elements do not want to have conscience this
> is dmabuf) in between the export element (upstream elem) and the importer
> element (downstream elem).

Huh?

"the gst elements to work in pass-through mode"? What are we talking about here?  Are we talking about other non-dmabuf aware elements like videoconvert/scale/etc that will passthrough caps-features?  Then I bring up the relevance to dmabuf.  Unless you still want them to work as usual?

"and these elements do not want to have conscience this is dmabuf"

"conscience this is"?  Maybe you mean "knowledge of"?, "code for"

Maybe an example would help here?

> Having it after systemMem tells the exporter to follow the rule above. If we
> put it after then it will not let the exporter the chance to push the dma
> buffers without the caps feature.

Assuming "it" = sysmem caps feature.

So, you want to add a caps feature but not use the caps feature?  That sounds backwards.  Why can't the source choose based on the available features?

My main problem with this argument is that you're moving who chooses the caps feature into an element that doesn't actually know very much about the properties of the dmabuf, sysmem-mappable or not.  The source has a much better idea on what is possible with the dma-buf they are pushing and has the ultimate say on what they produce.

(In reply to Nicolas Dufresne (stormer) from comment #4)
> For clarity, dmabuf passed as SystemMemory can fallback if the import fails,
> while dmabuf with the CapsFeature set can't. You cannot trust the result of
> mapping the dmabuf when the caps feature is set. It may fail, or it may work
> and give you something black (e.g. ION secured surfaces)

Sure.  That doesn't really explain why the dmabuf capsfeature is after the sysmem one though.

As mentioned here https://bugzilla.gnome.org/show_bug.cgi?id=755072#c87 things have changed so I will modify the patch to put the dmabuf feature before systemmem.

Created attachment 354577 [details] [review]
glupload: add GST_CAPS_FEATURE_MEMORY_DMABUF

Review of attachment 354577 [details] [review]:

Looks good.

::: gst-libs/gst/gl/gstglupload.c
@@ +530,3 @@
+    ret = gst_caps_merge (ret,
+        _set_caps_features_with_passthrough (caps,
+            GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, passthrough));

The raw memory uploader will do this exact caps operation so adding these caps are not really necessary and they will probably be folded in together so the end result is extra work for no gain :).  I'm fine with either keeping or removing it.

(In reply to Matthew Waters (ystreet00) from comment #8)
> Review of attachment 354577 [details] [review] [review]:
> 
> Looks good.
> 
> ::: gst-libs/gst/gl/gstglupload.c
> @@ +530,3 @@
> +    ret = gst_caps_merge (ret,
> +        _set_caps_features_with_passthrough (caps,
> +            GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, passthrough));
> 
> The raw memory uploader will do this exact caps operation so adding these
> caps are not really necessary and they will probably be folded in together
> so the end result is extra work for no gain :).  I'm fine with either
> keeping or removing it.

Indeed I verified and it works like you said. So I applied the change. Thx!

Comment on attachment 354577 [details] [review]
glupload: add GST_CAPS_FEATURE_MEMORY_DMABUF

commit 86ae9777addf7141e196e836e1ab69caf3f3d916
Author: Julien Isorce <j.isorce@samsung.com>
Date:   Wed Mar 9 22:01:12 2016 +0000

    glupload: add GST_CAPS_FEATURE_MEMORY_DMABUF
    
    Insert before SystemMemory to advice upstream elements that it is
    preferable for them to push dmabuf with the caps feature.
    
    Examples:
    
    /* Discard memory:DMABuf caps feature */
    GST_GL_PLATFORM=egl GST_GL_API=gles2 GST_GL_WINDOW=x11 gst-launch-1.0 \
        filesrc location=test.mp4 ! qtdemux ! h264parse ! vaapih264dec ! \
        capsfilter caps="video/x-raw(memory:SystemMemory)" ! glimagesink
    
    /* Force memory:DMABuf caps feature. */
    GST_GL_PLATFORM=egl GST_GL_API=gles2 GST_GL_WINDOW=x11 gst-launch-1.0 \
        filesrc location=test.mp4 ! qtdemux ! h264parse ! vaapih264dec ! \
        capsfilter caps="video/x-raw(memory:DMABuf)" ! glimagesink
    
    /* Auto select memory:DMABuf caps feature.  */
    GST_GL_PLATFORM=egl GST_GL_API=gles2 GST_GL_WINDOW=x11 gst-launch-1.0 \
        filesrc location=test.mp4 ! qtdemux ! h264parse ! vaapih264dec ! \
        glimagesink
    
    https://bugzilla.gnome.org/show_bug.cgi?id=774649