After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759860 - glupload: Attaches composition overlay when not supported
glupload: Attaches composition overlay when not supported
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-12-25 12:05 UTC by Florent Thiéry
Modified: 2016-01-05 02:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glupload: passthrough composition caps features (6.74 KB, patch)
2015-12-29 10:50 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
glupload: passthrough composition caps features (7.13 KB, patch)
2015-12-29 14:09 UTC, Matthew Waters (ystreet00)
committed Details | Review
glupload: always add texture-target field to GL caps (10.92 KB, patch)
2016-01-04 09:48 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Florent Thiéry 2015-12-25 12:05:33 UTC
The expected result is to see the timeoverlay on every cube face instead of the top left sink corner:

gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glcolorconvert ! glfiltercube ! glimagesink

The expected result can be obained by moving timeoverly before glupload (but in that case, 
gst-launch-1.0 videotestsrc ! timeoverlay ! glupload ! glcolorconvert ! glfiltercube ! glimagesink

Also, since actual timeoverlay rendering is left to the sink, why is the following failing to link (resulting in requiring a glcolorconvert element) ?
gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glfiltercube ! glimagesink
WARNING: erroneous pipeline: could not link timeoverlay0 to glfiltercube0

These issues are shared by textoverlay and timeoverlay.
Comment 1 Florent Thiéry 2015-12-25 12:06:45 UTC
Sorry, hit submit too soon; i meant that by moving timeoverlay before glupload, the text rendering is done in software.
Comment 2 Nicolas Dufresne (ndufresne) 2015-12-25 15:55:09 UTC
The state of GL plugins seems a little shaky on git master, so it's a git hard for me to test (maybe it's GL/Nouveau). As timeoverlay does not support meory:GLMemory, you'll need downstream pipeline to accept meta:GstVideoCompositionOverlay or to do the download/upload dance again. As glfilters don't support the compositon overlay, you'd need this silly pipeline (basically it's better to place timeoverlay before the upload, unless you can teach glfiltercube to handle it).

gst-launch-1.0 videotestsrc ! glupload ! gldownload ! timeoverlay ! glupload ! glfiltercube ! glimagesink

Now that pipeline can reproduce the issue, which is that the composition get attached instead of software composed. This is a bug in the negotiation it seems. I'll keep this bug for fixing that, if you are interested in getting this accelerated though, filter another bug, this is much more work.
Comment 3 Florent Thiéry 2015-12-25 19:41:09 UTC
Thanks; is this bug somehow related to why the following pipeline fails to link ?
gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glfiltercube ! glimagesink

I created bug #759867
Comment 4 Matthew Waters (ystreet00) 2015-12-28 02:15:01 UTC
(In reply to Florent Thiery from comment #0)
> The expected result is to see the timeoverlay on every cube face instead of
> the top left sink corner:
> 
> gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glcolorconvert !
> glfiltercube ! glimagesink

This won't link at all...

> The expected result can be obained by moving timeoverly before glupload (but
> in that case, 
> gst-launch-1.0 videotestsrc ! timeoverlay ! glupload ! glcolorconvert !
> glfiltercube ! glimagesink

Correct

> Also, since actual timeoverlay rendering is left to the sink, why is the
> following failing to link (resulting in requiring a glcolorconvert element) ?
> gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glfiltercube !
> glimagesink
> WARNING: erroneous pipeline: could not link timeoverlay0 to glfiltercube0
> 
> These issues are shared by textoverlay and timeoverlay.

The overlay rendering is not always left to the sink though.

> I created bug #759867

And what's the difference with this bug?
Comment 5 Nicolas Dufresne (ndufresne) 2015-12-28 02:45:18 UTC
(In reply to Matthew Waters (ystreet00) from comment #4)
> And what's the difference with this bug?

One is a feature request, which is to add ability to compose in GL the overlay at any point in the pipeline. I believe the right way is our first idea, which is to create an element that only apply the composition and output a new texture. I believe this element does not have to be a new one, glvideomixer seems to be a good fit (it's at least very similar task).

But for this bug, I'd simply fix it, so something like:

  upload ! .. ! download ! overlay ! glimagesink

Should render the text in software and not attach wrongly the composition likes it does now. I do remember having tested this, so this is likely a regression, but I would not bet on that.
Comment 6 Matthew Waters (ystreet00) 2015-12-28 03:02:38 UTC
(In reply to Nicolas Dufresne (stormer) from comment #5)
>   upload ! .. ! download ! overlay ! glimagesink

And why wouldn't that cause overlaying in the sink (like it does now)?

Essentially, I cannot reproduce this with any of the pipelines above, or any other pipelines I've tried.  The overlay is always in the correct place for the pipeline.
Comment 7 Nicolas Dufresne (ndufresne) 2015-12-28 04:13:30 UTC
Because the visual result is totally incorrect. Here's two pipeline:

1. overlay ! effect ! sink
2. effect ! overlay ! sink

If you attach the composition in 1. you'll get the result of pipeline 2 (overlay applied after the effect). So when an effect that cannot transform the meta properly is applied, the overlay should compose right away (in this case in software).
Comment 8 Matthew Waters (ystreet00) 2015-12-28 04:43:07 UTC
Any specific pipelines that exhibit this issue (as I cannot reproduce from the descriptions...)

e.g. for 1)
videotestsrc ! timeoverlay ! glupload ! glfiltercube ! glimagesink
overlays over each cube face
and for 2)
videotestsrc ! glupload ! glfiltercube ! timeoverlay ! glimagesink
overlays the time over the entire frame

Which is the expected behaviour.
Comment 9 Florent Thiéry 2015-12-28 12:07:16 UTC
(In reply to Matthew Waters (ystreet00) from comment #8)
> Any specific pipelines that exhibit this issue (as I cannot reproduce from
> the descriptions...)

Actually, the bug can be reproduced in stable (1.6):

gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glcolorconvert ! glfiltercube ! glimagesink

Indeed, in master it fails to link, whatever i try to add before/after
gst-launch-1.0 videotestsrc ! glupload ! timeoverlay ! glcolorconvert ! glfiltercube ! glimagesink

Does this mean that the bug is closed (since linking is now impossible) ?
Comment 10 Nicolas Dufresne (ndufresne) 2015-12-28 20:18:47 UTC
In both 1.6 and master, the error can be reproduced using:

GST_XINITTHREADS=1 gst-launch-1.0 videotestsrc ! glupload ! gldownload ! clockoverlay ! glupload ! glcolorconvert ! glfiltercube ! glimagesinkelement

The time should be rendered in software, but is instead attached (hence displayed top left).
Comment 11 Matthew Waters (ystreet00) 2015-12-29 10:50:45 UTC
Created attachment 318001 [details] [review]
glupload: passthrough composition caps features

This only passes through the composition features if they already exist in the transformed caps.

However, this causes accept-caps in glimagesink to fail for some reason performing a subset check with a caps that doesn't have texture-target (that the template does) which is a separate bug.
Comment 12 Nicolas Dufresne (ndufresne) 2015-12-29 13:26:31 UTC
Review of attachment 318001 [details] [review]:

I wanted to test this, and notice it breaks the build:

  CC       libgstgl_1.0_la-gstglupload.lo
gstglupload.c: In function '_dma_buf_upload_transform_caps':
gstglupload.c:622:11: error: implicit declaration of function '_set_caps_features' [-Werror=implicit-function-declaration]
     ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_GL_MEMORY);
           ^
gstglupload.c:622:5: error: nested extern declaration of '_set_caps_features' [-Werror=nested-externs]
     ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_GL_MEMORY);
     ^
gstglupload.c:622:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
     ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_GL_MEMORY);
         ^
gstglupload.c:626:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
     ret = _set_caps_features (caps, GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY);
         ^
gstglupload.c:636:27: error: 'passthrough' undeclared (first use in this function)
   gst_caps_features_free (passthrough);
                           ^
gstglupload.c:636:27: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
Comment 13 Matthew Waters (ystreet00) 2015-12-29 14:09:09 UTC
Created attachment 318006 [details] [review]
glupload: passthrough composition caps features

Ah, I didn't have the dmabuf changes in my tree yet :)

Note that this doesn't quite work everywhere as-is due to the issue mentioned above (is_subset() failing in various elements because of texture-target in template caps).
Comment 14 Matthew Waters (ystreet00) 2016-01-04 09:48:56 UTC
Created attachment 318204 [details] [review]
glupload: always add texture-target field to GL caps
Comment 15 Matthew Waters (ystreet00) 2016-01-04 09:49:24 UTC
Now, the subset checks don't fail anymore :)
Comment 16 Nicolas Dufresne (ndufresne) 2016-01-04 15:25:53 UTC
Review of attachment 318204 [details] [review]:

Looks good at first sight.
Comment 17 Nicolas Dufresne (ndufresne) 2016-01-04 17:05:21 UTC
Review of attachment 318006 [details] [review]:

Looks good, a little redundant, but works ;-P
Comment 18 Nicolas Dufresne (ndufresne) 2016-01-04 17:05:41 UTC
Review of attachment 318204 [details] [review]:

Just tested, and works great.
Comment 19 Matthew Waters (ystreet00) 2016-01-05 02:51:15 UTC
commit 479dcdc3b4393a4583d4e6d352f7bd934647a247
Author: Matthew Waters <matthew@centricular.com>
Date:   Mon Jan 4 20:26:09 2016 +1100

    glupload: always add texture-target field to GL caps
    
    1. Various elements/base classes only perform a subset check on accept-caps
    2. Some GL elements have texture-target in their pad template
    3. When checking subsets, only the caps to check are allowed to contain extra
       fields.  If the 'template' caps have extra fields, the subset fails.
    Thus without texture-target on the caps, various accept-caps implementations
    were failing.
    
    Also, add some convenience functions for setting and retrieving
    texture targets to/from GValue.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=759860

commit 65f03061ad40173db8ccf9862eb0ca5cb5d794bb
Author: Matthew Waters <matthew@centricular.com>
Date:   Tue Dec 29 18:16:04 2015 +1100

    glupload: passthrough composition caps features
    
    Don't unconditionally add it to any and all caps transformations.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=759860