Bug 87725 – GraphicsSurface: allow importing and exporting of textures directly on GPU side.

• Component: WebKit2
• Status: RESOLVED FIXED

[Zeno Albisser]

Add API to GraphicsSurface to allow reading in a texture directly from an FBO.
Further we should have a possibility to export a texture ID, so that the texture can directly be rebound in the GPU.
These changes should make the transferring of the texture from the UIProcess to the WebProcess much faster.
This is in particular necessary for WebGL.

[Zeno Albisser]

Created attachment 144508 [details]
patch for review.

[Kenneth Rohde Christiansen]

Up for review?

[Zeno Albisser]

(In reply to comment #2)
> Up for review?

absolutely... if you have time. :)

[Kenneth Rohde Christiansen]

Comment on attachment 144508 [details]
patch for review.

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

> Source/WebCore/ChangeLog:9
> +        This allows binding/blitting the texture directly in the GPU.

on* ?

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:67
> +    void copyFromFramebuffer(uint32_t fbo, const IntRect& sourceRect);

FrameBuffer ? what is used elsewhere?

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:69
>      uint32_t exportToken();
> +    uint32_t exportTexture();

What does the exportToken do? Isn't it also like a texture id ?

> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:94
> +void GraphicsSurface::platformCopyFromFramebuffer(uint32_t originFbo, const IntRect& sourceRect)

Maybe FrameBufferObject? fbo?

> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:101
> +    glPushAttrib(GL_ALL_ATTRIB_BITS);
> +    if (!m_texture)
> +        m_texture = createTexture(m_platformSurface);
> +    if (!m_fbo)
> +        glGenFramebuffers(1, &m_fbo);
> +    GLint oldFBO;

I like newlines before and after if clauses

[Noam Rosenthal]

Comment on attachment 144508 [details]
patch for review.

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

Good direction, let's fix the nitpicks :)

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:49
> +uint32_t GraphicsSurface::exportTexture()

Let's call it getTextureID(). export is for cross-process sharing.

>> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:69
>> +    uint32_t exportTexture();
> 
> What does the exportToken do? Isn't it also like a texture id ?

exportToken is a cross-process number that can mean anything.

[Zeno Albisser]

Comment on attachment 144508 [details]
patch for review.

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

>> Source/WebCore/ChangeLog:9
>> +        This allows binding/blitting the texture directly in the GPU.
> 
> on* ?

fixed.

>> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:49
>> +uint32_t GraphicsSurface::exportTexture()
> 
> Let's call it getTextureID(). export is for cross-process sharing.

fixed.

>> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:67
>> +    void copyFromFramebuffer(uint32_t fbo, const IntRect& sourceRect);
> 
> FrameBuffer ? what is used elsewhere?

The abbreviation fbo is used quite frequently in GraphicsContext3DQt/GraphicsContext3DOpenGLCommon. I think FBO is quite well known in this "context".

>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:69
>>> +    uint32_t exportTexture();
>> 
>> What does the exportToken do? Isn't it also like a texture id ?
> 
> exportToken is a cross-process number that can mean anything.

In this specific case, the token identifies a GraphicsSurface, where the texture is a texture id that can be used directly with GL on the GPU.
I renamed "exportTexture" to "getTextureID" as proposed by Noam.

>> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:94
>> +void GraphicsSurface::platformCopyFromFramebuffer(uint32_t originFbo, const IntRect& sourceRect)
> 
> Maybe FrameBufferObject? fbo?

dito.

>> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:101
>> +    GLint oldFBO;
> 
> I like newlines before and after if clauses

I don't, for single lines in a small function. :-$

[Zeno Albisser]

Created attachment 144552 [details]
patch for review. - fixed issues as commented before.

[Simon Fraser (smfr)]

This patch is changing GraphicsSurfaceMac.cpp, but how is it used? I don't think Mac even uses GraphicsSurface* anywhere.

[Zeno Albisser]

(In reply to comment #8)
> This patch is changing GraphicsSurfaceMac.cpp, but how is it used? I don't think Mac even uses GraphicsSurface* anywhere.
As far as i know GraphicsSurfaceMac is currently only used by the Qt port. (when compiling Qt/WebKit for Mac)

[Noam Rosenthal]

(In reply to comment #9)
> (In reply to comment #8)
> > This patch is changing GraphicsSurfaceMac.cpp, but how is it used? I don't think Mac even uses GraphicsSurface* anywhere.
> As far as i know GraphicsSurfaceMac is currently only used by the Qt port. (when compiling Qt/WebKit for Mac)

It's a question I have.
GraphicsSurfaceMac is Mac-specific code, but we only use it in Qt.
There's nothing in the code that's Qt specific, but it's not used by "the mac port".
What would be the way to go about files like that? I want to do the right thing but I don't have a strong opinion other way.

[Noam Rosenthal]

Comment on attachment 144552 [details]
patch for review. - fixed issues as commented before.

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

> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:22
> +#include "GraphicsContext3D.h"

What is this for?

[Zeno Albisser]

Comment on attachment 144552 [details]
patch for review. - fixed issues as commented before.

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

>> Source/WebCore/platform/graphics/surfaces/mac/GraphicsSurfaceMac.cpp:22
>> +#include "GraphicsContext3D.h"
> 
> What is this for?

I'll remove it.
Thanks for reviewing. :-)

[Zeno Albisser]

Committed r118987: <http://trac.webkit.org/changeset/118987>