When using GSTREAMER_GL, the copy of the video frames (textures) to webgl is done using cairo-gl, which adds a dependency that is not really necessary. The copy should be done without using cairo-gl.
Created attachment 290600 [details] Patch
Created attachment 290601 [details] Patch
Comment on attachment 290601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290601&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:803 > + IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo)); You can just use direct initialization here, like `IntSize size(width, height)`. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:804 > + IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size; Reuse the size object and reassign it to the transposedSize() value if necessary. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:28 > +#include "texmap/TextureMapperShaderProgram.h" You shouldn't need to list the texmap/ directory here. Hitting a compilation error without it? > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:32 > +static const GLfloat vertices[] = { 0, 0, 1, 0, 1, 1, 0, 1 }; This static can be local to the VideoTextureCopierGStreamer constructor. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:40 > + : m_framebuffer(0) > + , m_flipY(false) > + , m_orientation(DefaultImageOrientation) > + , m_width(0) > + , m_height(0) > + , m_vbo(0) These member variables should be all initialized in the class declaration since all the data is const. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:42 > + GLContext* previousContext = GLContext::current(); I suppose it's unlikely for previousContext to be null at this point, but an ASSERT() would still be useful here. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:47 > + TextureMapperShaderProgram::Options options = TextureMapperShaderProgram::Texture; You can inline this variable into the function call. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:63 > + GLContext* previousContext = GLContext::current(); Again, unlikely this is null, but ASSERT would be useful. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:64 > + PlatformDisplay::sharedDisplayForCompositing().sharingGLContext()->makeContextCurrent(); m_context3D->makeContextCurrent() should do the same. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:135 > + GLContext* previousContext = GLContext::current(); Again, unlikely this is null, but ASSERT would be useful. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:136 > + PlatformDisplay::sharedDisplayForCompositing().sharingGLContext()->makeContextCurrent(); You should be able to call m_context3D->makeContextCurrent() for the same effect. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:38 > + bool copyVideoTextureToPlatformTexture(Platform3DObject inputTexture, size_t width, size_t height, Platform3DObject outputTexture, GC3Denum outputTarget, GC3Dint level, GC3Denum internalFormat, GC3Denum format, GC3Denum type, bool flipY, ImageOrientation& sourceOrientation); width and height parameters should match the caller and be combined in an IntSize object. Same goes for the m_width and m_height member variables. > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:53 > + Platform3DObject m_vbo; m_vbo should follow m_framebuffer directly -- the GPU resources should be grouped together.
Created attachment 290700 [details] Patch
> > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:64 > > + PlatformDisplay::sharedDisplayForCompositing().sharingGLContext()->makeContextCurrent(); > > m_context3D->makeContextCurrent() should do the same. > This is not changed in the new version of the patch, cause graphic contexts created with GraphicsContext3D::createForCurrentGLContext() don't save a reference to the current gl context, so makeContextCurrent() doesn't really do anything.
(In reply to comment #5) > > > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:64 > > > + PlatformDisplay::sharedDisplayForCompositing().sharingGLContext()->makeContextCurrent(); > > > > m_context3D->makeContextCurrent() should do the same. > > > > This is not changed in the new version of the patch, cause graphic contexts > created with GraphicsContext3D::createForCurrentGLContext() don't save a > reference to the current gl context, so makeContextCurrent() doesn't really > do anything. Right, I see it now -- this behavior makes sense. What I feel might be a problem at some point is the different things we're using the sharing context for.
Comment on attachment 290700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290700&action=review > Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.h:47 > + Platform3DObject m_framebuffer = 0; > + Platform3DObject m_vbo = 0; > + bool m_flipY = false; For these we use braces: `bool m_flipY { false };' etc.
Created attachment 290792 [details] Patch
Comment on attachment 290792 [details] Patch Clearing flags on attachment: 290792 Committed r206862: <http://trac.webkit.org/changeset/206862>
All reviewed patches have been landed. Closing bug.