Bug 196966

> 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.

Created attachment 367520 [details] Patch

So, is this patch needed?

(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.

(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)

(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

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.

(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!

(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 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.

At least initializing the GstGL wrapped context (without swapping contexts at frame texture drawing) the bluish color always appear.

(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?

(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.

BTW, after calling webkitContext->makeContextCurrent() it shouldn't be necessary to call gst_gl_context_activate() as the context should be current already.

(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.

(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

(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.

(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.

Created attachment 373009 [details] Patch

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 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

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

If I understand correctly, mac-debug failures are unrelated with this patch.

Created attachment 373028 [details] Patch

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 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 :)

Created attachment 373228 [details] Patch

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

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 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 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?

(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 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.

Created attachment 375229 [details] Patch

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.

Created attachment 375617 [details] Patch

Comment on attachment 375617 [details] Patch The GStreamer part lgtm. Zan, would you do the honors for the graphics?

Comment on attachment 375617 [details] Patch Clearing flags on attachment: 375617 Committed r248464: <https://trac.webkit.org/changeset/248464>

All reviewed patches have been landed. Closing bug.