Bug 196966 - [GL][GStreamer] activate wrapped shared context
Summary: [GL][GStreamer] activate wrapped shared context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Víctor M. Jáquez L.
URL:
Keywords:
Depends on:
Blocks: 196200
  Show dependency treegraph
 
Reported: 2019-04-16 02:34 PDT by Víctor M. Jáquez L.
Modified: 2019-08-09 02:36 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.74 KB, patch)
2019-04-16 02:49 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2019-06-27 01:00 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (3.17 MB, application/zip)
2019-06-27 02:28 PDT, EWS Watchlist
no flags Details
Patch (7.83 KB, patch)
2019-06-27 06:42 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (10.89 KB, patch)
2019-07-01 06:25 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.50 MB, application/zip)
2019-07-01 10:19 PDT, EWS Watchlist
no flags Details
Patch (13.24 KB, patch)
2019-07-31 10:04 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (13.13 KB, patch)
2019-08-06 03:46 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Víctor M. Jáquez L. 2019-04-16 02:34:44 PDT
The shared GL context with GStreamer pipeline, and wrapped in a GstGLContext GObject, needs to be activated and configured before pass it to the message bus. Though it is not a fatal error, it seems, according to the logs, will save a couple iterations.

Also, as far as I understand, before rendering the frame's texture the shared context should be made current.
Comment 1 Miguel Gomez 2019-04-16 02:46:05 PDT
 
> Also, as far as I understand, before rendering the frame's texture the
> shared context should be made current.

Nope, this is not necessary. The frame texture is accessed from a different context but it's sharing with the GStreamer one, so it's fine.
Comment 2 Víctor M. Jáquez L. 2019-04-16 02:49:35 PDT
Created attachment 367520 [details]
Patch
Comment 3 Philippe Normand 2019-04-16 02:53:14 PDT
So, is this patch needed?
Comment 4 Miguel Gomez 2019-04-16 02:57:13 PDT
(In reply to Philippe Normand from comment #3)
> So, is this patch needed?

I'm not sure why it's required that the context is current when creating the GstGLContext, but definitely we don't want to make it current when painting a frame. Besides not being necessary to access to the frame texture, we are not guaranteed that we are not making that context current in two threads at the same time.
Comment 5 Víctor M. Jáquez L. 2019-04-16 03:02:53 PDT
(In reply to Philippe Normand from comment #3)
> So, is this patch needed?

I came to its need while working on bug 196200, until I realized that the currently used OpenGL version in webkit doesn't cover Fence and Sync.

Nonetheless, only for correctness at least,the initialization of the wrapped GstGL context is needed. While the change context at drawing the texture seems not (thought it was needed when using the sync meta for bug 196200)
Comment 6 Víctor M. Jáquez L. 2019-04-16 03:06:37 PDT
(In reply to Miguel Gomez from comment #4)
> (In reply to Philippe Normand from comment #3)
> > So, is this patch needed?
> 
> I'm not sure why it's required that the context is current when creating the
> GstGLContext

Because if it is not current, it cannot be activated and its internal info cannot be filled [1]

https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/GstGLContext.html#gst-gl-context-fill-info
Comment 7 Víctor M. Jáquez L. 2019-04-16 03:28:17 PDT
wow... I just tested the patch without making current the shared before painting, just initializing the wrapped context, and the showed color format is not correct, like if RGBA would be expected.
Comment 8 Philippe Normand 2019-04-16 03:41:48 PDT
(In reply to Víctor M. Jáquez L. from comment #7)
> wow... I just tested the patch without making current the shared before
> painting, just initializing the wrapped context, and the showed color format
> is not correct, like if RGBA would be expected.

Like, blueish? I've seen that here too sometimes!
Comment 9 Víctor M. Jáquez L. 2019-04-16 03:42:53 PDT
(In reply to Philippe Normand from comment #8)
> (In reply to Víctor M. Jáquez L. from comment #7)
> > wow... I just tested the patch without making current the shared before
> > painting, just initializing the wrapped context, and the showed color format
> > is not correct, like if RGBA would be expected.
> 
> Like, blueish? I've seen that here too sometimes!

Indeed!
Comment 10 Philippe Normand 2019-04-16 03:54:51 PDT
Comment on attachment 367520 [details]
Patch

Removing cq for now. Maybe Miguel can help sched some (non-blue) light here? Seems like the context needs to be current before drawing to avoid a colorspace issue.
Comment 11 Víctor M. Jáquez L. 2019-04-16 03:56:21 PDT
At least initializing the GstGL wrapped context (without swapping contexts at frame texture drawing) the bluish color always appear.
Comment 12 Miguel Gomez 2019-04-17 00:47:11 PDT
(In reply to Víctor M. Jáquez L. from comment #7)
> wow... I just tested the patch without making current the shared before
> painting, just initializing the wrapped context, and the showed color format
> is not correct, like if RGBA would be expected.

That's weird. Where are you seeing this? Desktop + wayland?
The current context shouldn't matter, as the colorspace of the texture should be the same always. We expect the frame texture to be BGRA, and when compositing we change it to RGBA.

Can you check that the video sink caps are ok, and that GStreamer is really giving us BGRA?
Comment 13 Miguel Gomez 2019-04-17 01:50:56 PDT
(In reply to Miguel Gomez from comment #12)
> (In reply to Víctor M. Jáquez L. from comment #7)
> > wow... I just tested the patch without making current the shared before
> > painting, just initializing the wrapped context, and the showed color format
> > is not correct, like if RGBA would be expected.
> 
> That's weird. Where are you seeing this? Desktop + wayland?
> The current context shouldn't matter, as the colorspace of the texture
> should be the same always. We expect the frame texture to be BGRA, and when
> compositing we change it to RGBA.
 
I can't reproduce any color format error. I 'm trying ToT with gstreamer 1.14.4 (the one in jhbuild), and it works fine initializing the wrapped context, both on X11+glx and wayland+egl.
Comment 14 Miguel Gomez 2019-04-17 01:55:11 PDT
BTW, after calling webkitContext->makeContextCurrent() it shouldn't be necessary to call gst_gl_context_activate() as the context should be current already.
Comment 15 Philippe Normand 2019-04-17 01:57:57 PDT
(In reply to Miguel Gomez from comment #13)
> (In reply to Miguel Gomez from comment #12)
> > (In reply to Víctor M. Jáquez L. from comment #7)
> > > wow... I just tested the patch without making current the shared before
> > > painting, just initializing the wrapped context, and the showed color format
> > > is not correct, like if RGBA would be expected.
> > 
> > That's weird. Where are you seeing this? Desktop + wayland?
> > The current context shouldn't matter, as the colorspace of the texture
> > should be the same always. We expect the frame texture to be BGRA, and when
> > compositing we change it to RGBA.
>  
> I can't reproduce any color format error. I 'm trying ToT with gstreamer
> 1.14.4 (the one in jhbuild), and it works fine initializing the wrapped
> context, both on X11+glx and wayland+egl.

It's a racy behavior, unfortunately. Doesn't always happen here.
Comment 16 Víctor M. Jáquez L. 2019-04-17 05:01:56 PDT
(In reply to Miguel Gomez from comment #13)
>  
> I can't reproduce any color format error. I 'm trying ToT with gstreamer
> 1.14.4 (the one in jhbuild), and it works fine initializing the wrapped
> context, both on X11+glx and wayland+egl.

I'm using Wayland+EGL and gst master. 

Did you tried with the first part of the patch, in MediaPlayerPrivateGStreamerBase::ensureGstGLContext() ??

(In reply to Miguel Gomez from comment #12)
> Can you check that the video sink caps are ok, and that GStreamer is really
> giving us BGRA?

Oddly, BGRx is selected by the appsink, which is the first option in the caps.
I'm going to force BGRA.

(In reply to Miguel Gomez from comment #14)
> BTW, after calling webkitContext->makeContextCurrent() it shouldn't be
> necessary to call gst_gl_context_activate() as the context should be current
> already.

I have to make the shared context current, otherwise gst_gl_context_activated()  and/or gst_gl_context_fill_info() fail
Comment 17 Miguel Gomez 2019-04-17 06:27:55 PDT
(In reply to Víctor M. Jáquez L. from comment #16)
> (In reply to Miguel Gomez from comment #13)
> >  
> > I can't reproduce any color format error. I 'm trying ToT with gstreamer
> > 1.14.4 (the one in jhbuild), and it works fine initializing the wrapped
> > context, both on X11+glx and wayland+egl.
> 
> I'm using Wayland+EGL and gst master. 
> 
> Did you tried with the first part of the patch, in
> MediaPlayerPrivateGStreamerBase::ensureGstGLContext() ??

Yep.

> (In reply to Miguel Gomez from comment #12)
> > Can you check that the video sink caps are ok, and that GStreamer is really
> > giving us BGRA?
>
> Oddly, BGRx is selected by the appsink, which is the first option in the
> caps.
> I'm going to force BGRA.

Ah, we request { BGRx, BGRA }, so BGRx is ok as well (as long as each pixel uses 4 bytes).

> (In reply to Miguel Gomez from comment #14)
> > BTW, after calling webkitContext->makeContextCurrent() it shouldn't be
> > necessary to call gst_gl_context_activate() as the context should be current
> > already.
> 
> I have to make the shared context current, otherwise
> gst_gl_context_activated()  and/or gst_gl_context_fill_info() fail

Ah, so GStreamer does something else than just calling eglMakeCurrent. I guess it keeps a flag about whether the context has been activated, so gst_gl_context_activate() is required, ok.

I could understand that maybe there was an error with the color conversion that is revealed by your patch... but I don't get how that could happen just sometimes, that's weird. You can check how we create a cairo surface from the video sample in ImageGStreamerCairo.cpp and then use cairo_surface_write_to_png to dump the contents into a file and check whether the color conversion issue is already on the video frame or it happens when we paint it.
Comment 18 Víctor M. Jáquez L. 2019-04-17 06:58:32 PDT
(In reply to Miguel Gomez from comment #17)
> (In reply to Víctor M. Jáquez L. from comment #16)
> > Oddly, BGRx is selected by the appsink, which is the first option in the
> > caps.
> > I'm going to force BGRA.
> 
> Ah, we request { BGRx, BGRA }, so BGRx is ok as well (as long as each pixel
> uses 4 bytes).

Forcing BGRA the color space looks normal.
Comment 19 Víctor M. Jáquez L. 2019-06-27 01:00:24 PDT
Created attachment 373009 [details]
Patch
Comment 20 Philippe Normand 2019-06-27 01:44:22 PDT
Comment on attachment 373009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373009&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-992
> -    if (m_videoDecoderPlatform != WebKitGstVideoDecoderPlatform::ImxVPU)

If this patch is going to be approved it looks like you can remove the ImxVPU enum all together from the code-base.
Comment 21 EWS Watchlist 2019-06-27 02:28:31 PDT
Comment on attachment 373009 [details]
Patch

Attachment 373009 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12589765

New failing tests:
webgl/1.0.2/conformance/context/context-release-upon-reload.html
Comment 22 EWS Watchlist 2019-06-27 02:28:34 PDT
Created attachment 373013 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 23 Víctor M. Jáquez L. 2019-06-27 03:37:26 PDT
If I understand correctly, mac-debug failures are unrelated with this patch.
Comment 24 Víctor M. Jáquez L. 2019-06-27 06:42:18 PDT
Created attachment 373028 [details]
Patch
Comment 25 Xabier Rodríguez Calvar 2019-06-27 23:03:47 PDT
Comment on attachment 373028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373028&action=review

Patch looks good to me though I'd appreciate if somebody like Miguel could have a look at it

> Source/WebCore/ChangeLog:10
> +        1\ When the media player is instantiaded, and it is intended to

instantiated
Comment 26 Philippe Normand 2019-06-28 01:29:59 PDT
Comment on attachment 373028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373028&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-992
> -    if (m_videoDecoderPlatform != WebKitGstVideoDecoderPlatform::ImxVPU)

https://bugs.webkit.org/show_bug.cgi?id=196966#c20 :)
Comment 27 Víctor M. Jáquez L. 2019-07-01 06:25:35 PDT
Created attachment 373228 [details]
Patch
Comment 28 EWS Watchlist 2019-07-01 10:19:37 PDT
Comment on attachment 373228 [details]
Patch

Attachment 373228 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12628394

New failing tests:
storage/indexeddb/modern/get-keyrange.html
Comment 29 EWS Watchlist 2019-07-01 10:19:41 PDT
Created attachment 373245 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 30 Miguel Gomez 2019-07-02 01:01:07 PDT
Comment on attachment 373228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373228&action=review

Comments aside, the GL part looks good. But there's a color conversion missing for the cases where the video frames are gotten using GStreamer-gl but rendered using cairo: ImageGStreamer currently expects the video frames in BGRA or ARGB formats. With this change it's going to be RGBA always, and in order to paint then with cairo the R and B components must be swapped when creating the cairo surface.

> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:200
> +        m_shaderProgram->setMatrix(m_shaderProgram->textureColorSpaceMatrixLocation(), m_colorConversionMatrix);

The shaderProgram expects the colorConversionMatrix to be always set, but you're not setting it for this case, so the result might be undefined. Just do m_colorConversionMatrix.makeIdentity() for the NoConvert case and always set the matrix here.

> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:40
> +        NoConvert,

That comma

> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:67
> +    bool m_colorConversion { true };

Don't use this, use an identity matrix as I commented before.
Comment 31 Víctor M. Jáquez L. 2019-07-02 01:52:44 PDT
Comment on attachment 373228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373228&action=review

>> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:67
>> +    bool m_colorConversion { true };
> 
> Don't use this, use an identity matrix as I commented before.

I wanted to ask you about this, precisely. My first approach was to using the identity matrix to avoid this flag. But I felt, naively perhaps, that matrix operation can be skipped altogether. I'm wrong then, right?
Comment 32 Miguel Gomez 2019-07-02 03:12:28 PDT
(In reply to Víctor M. Jáquez L. from comment #31)
> Comment on attachment 373228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373228&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:67
> >> +    bool m_colorConversion { true };
> > 
> > Don't use this, use an identity matrix as I commented before.
> 
> I wanted to ask you about this, precisely. My first approach was to using
> the identity matrix to avoid this flag. But I felt, naively perhaps, that
> matrix operation can be skipped altogether. I'm wrong then, right?

The operation is going to be performed anyway, whether you pass a matrix or not.
Comment 33 Zan Dobersek 2019-07-29 01:29:11 PDT
Comment on attachment 373228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373228&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:466
> +    if (gst_gl_context_activate(m_glContext.get(), TRUE)) {
> +        GUniqueOutPtr<GError> error;
> +        if (!gst_gl_context_fill_info(m_glContext.get(), &error.outPtr()))
> +            GST_WARNING("Failed to fill in GStreamer context: %s", error->message);

Should we not deactivate the GstGLContext after filling it? Or does that break color conversion?

AFAIU we shouldn't leave it activated since that binds the current thread inside the GstGLContext and the GstGLContext still expects the underlying GL context to remain current on that thread. But that GL context can still be stolen by some other thread, so we can hinder the wrapping GstGLContext that way.
Comment 34 Víctor M. Jáquez L. 2019-07-31 10:04:16 PDT
Created attachment 375229 [details]
Patch
Comment 35 Zan Dobersek 2019-08-06 00:37:24 PDT
Comment on attachment 375229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375229&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:477
> +        if (previousActiveContext)
> +            gst_gl_context_activate(m_glContext.get(), FALSE);

The GstGLContext has to be deactivated in any case, because the underlying shared context can be reactivated outside of the GstGLContext's control. So the previousActiveContext condition should be removed here.
Comment 36 Víctor M. Jáquez L. 2019-08-06 03:46:44 PDT
Created attachment 375617 [details]
Patch
Comment 37 Xabier Rodríguez Calvar 2019-08-06 06:17:34 PDT
Comment on attachment 375617 [details]
Patch

The GStreamer part lgtm. Zan, would you do the honors for the graphics?
Comment 38 WebKit Commit Bot 2019-08-09 02:36:51 PDT
Comment on attachment 375617 [details]
Patch

Clearing flags on attachment: 375617

Committed r248464: <https://trac.webkit.org/changeset/248464>
Comment 39 WebKit Commit Bot 2019-08-09 02:36:53 PDT
All reviewed patches have been landed.  Closing bug.