Bug 162904 – [GTK] Copying video textures to webgl should not depend on cairo-gl

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

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.

[Miguel Gomez]

Created attachment 290600 [details]
Patch

[Miguel Gomez]

Created attachment 290601 [details]
Patch

[Zan Dobersek]

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.

[Miguel Gomez]

Created attachment 290700 [details]
Patch

[Miguel Gomez]

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

[Zan Dobersek]

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

[Zan Dobersek]

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.

[Miguel Gomez]

Created attachment 290792 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290792 [details]
Patch

Clearing flags on attachment: 290792

Committed r206862: <http://trac.webkit.org/changeset/206862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.