Bug 87738 – Make TextureMapper work with GraphicsSurface.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Zeno Albisser]

To be able to directly reuse textures from a GraphicsSurface we need TextureMapper to work with gl texture ids.
On mac the GraphicsSurface is backed by an IOSurface which requires the texture to be a GL_TEXTURE_RECTANGLE_ARB.
Due to this we need new shader programs to allow painting these textures directly.

[Noam Rosenthal]

wouldn't this mean we move from triple-buffering to double-buffering, which might cause sync issues?

[Zeno Albisser]

(In reply to comment #1)
> wouldn't this mean we move from triple-buffering to double-buffering, which might cause sync issues?

I'm not sure i follow. How could this cause sync issues?
We are currently rendering into an FBO and then blit the RenderBuffer onto the GraphicsSurface. So I do not see, why we should need an extra blit/copy on the UIProcess side again. Am I missing something?

[Zeno Albisser]

Created attachment 144563 [details]
patch for review.

EWS will most likely fail (if executed), because this patch depends on Bug87725, and the according patch has not been landed yet.

[Early Warning System Bot]

Comment on attachment 144563 [details]
patch for review.

Attachment 144563 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12845566

[Noam Rosenthal]

(In reply to comment #2)
> (In reply to comment #1)
> > wouldn't this mean we move from triple-buffering to double-buffering, which might cause sync issues?
> 
> I'm not sure i follow. How could this cause sync issues?
> We are currently rendering into an FBO and then blit the RenderBuffer onto the GraphicsSurface. So I do not see, why we should need an extra blit/copy on the UIProcess side again. Am I missing something?

Probably :)
There are three rythms:
1. The "web" rythim - drawing with JS into the WebGL FBO (this can happen at any time)
2. The "ui" rythm - TextureMapper drawing to the screen (for example during scroll).
3. The "sync" rythm - every frame in the web process has to be synced to the ui process at the same time. This is why we need a scratch buffer, otherwise we'd be able to update the tiles directly.

In your patch you skip the sync rythm, and essentially update the ui-facing texture directly. that means that the canvas texture that's drawn to the screen is slightly newer than the other textures and geometry parameters that are currently known to TextureMapper. This might cause visible sync issues :)
/me hopes this is not more confusing...

[Early Warning System Bot]

Comment on attachment 144563 [details]
patch for review.

Attachment 144563 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12848572

[Zeno Albisser]

> There are three rythms:
> 1. The "web" rythim - drawing with JS into the WebGL FBO (this can happen at any time)
So after this step we have the "image" in the RenderBuffer.
> 2. The "ui" rythm - TextureMapper drawing to the screen (for example during scroll).
This is where we load the image from the IOSurface/GraphicsSurface.
We always need to rebind the texture in this case. And that is where the image is actually "read" from the IOSurface. I don't see why we would need another buffer in between.
> 3. The "sync" rythm - every frame in the web process has to be synced to the ui process at the same time. This is why we need a scratch buffer, otherwise we'd be able to update the tiles directly.
This is where we pass the GraphicsSurface token to the UIProcess.
> 
> In your patch you skip the sync rythm, and essentially update the ui-facing texture directly. that means that the canvas texture that's drawn to the screen is slightly newer than the other textures and geometry parameters that are currently known to TextureMapper. This might cause visible sync issues :)
I don't think this is the case. I am saving the GraphicsSurface token in the TextureMapperDirectBackingStore, which then later on is used to paint on screen at the same time as all the other content.
But I don't think this patch is conclusive about that mechanism. Some of the logic is contained in the patch for Bug86214. May be you can take a look at this one, and see if what i am saying makes sense?
Thank you very much! :-)

[Noam Rosenthal]

Comment on attachment 144563 [details]
patch for review.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:31
> +void TextureMapperDirectBackingStore::setGraphicsSurface(uint32_t graphicsSurfaceToken, const IntSize& surfaceSize)

This should be inside #if USE(GRAPHICS_SURFACE)

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:33
> +    if (graphicsSurfaceToken != m_graphicsSurfaceToken) {

Early return

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
> +                        GraphicsSurface::SupportsSoftwareRead

Wy do you need SoftwareRead?

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:57
> +    static_cast<TextureMapperGL*>(textureMapper)->drawTextureRectangleARB(m_textureId, 0, m_graphicsSurfaceSize, targetRect, adjustedTransform, opacity, mask);

This is very Mac specific. Maybe it should be inside OS(DARWIN)?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:370
> +void TextureMapperGL::drawTextureRectangleARB(uint32_t texture, Flags flags, const IntSize& textureSize, const FloatRect& targetRect, const TransformationMatrix& modelViewMatrix, float opacity, const BitmapTexture* maskTexture)

Mac specific, I think.

[Zeno Albisser]

Comment on attachment 144563 [details]
patch for review.

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:31
>> +void TextureMapperDirectBackingStore::setGraphicsSurface(uint32_t graphicsSurfaceToken, const IntSize& surfaceSize)
> 
> This should be inside #if USE(GRAPHICS_SURFACE)

fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:33
>> +    if (graphicsSurfaceToken != m_graphicsSurfaceToken) {
> 
> Early return

fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
>> +                        GraphicsSurface::SupportsSoftwareRead
> 
> Wy do you need SoftwareRead?

In fact it seems to me "SupportsSharing" is the only that is currently really needed anyway.
I'll delete "SupportsSoftwareRead".

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:57
>> +    static_cast<TextureMapperGL*>(textureMapper)->drawTextureRectangleARB(m_textureId, 0, m_graphicsSurfaceSize, targetRect, adjustedTransform, opacity, mask);
> 
> This is very Mac specific. Maybe it should be inside OS(DARWIN)?

fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:370
>> +void TextureMapperGL::drawTextureRectangleARB(uint32_t texture, Flags flags, const IntSize& textureSize, const FloatRect& targetRect, const TransformationMatrix& modelViewMatrix, float opacity, const BitmapTexture* maskTexture)
> 
> Mac specific, I think.

Currently it is only used for Mac. But i don't see a reason why this code path should remain Mac only. So I would prefer not to ifdef it.
You also suggested to switch TextureMapper for Qt/Mac completely to ARB textures to avoid having duplicate shader programs etc.
It does not seem to be totally straight forward to me, so I would prefer to research that in a separate patch.

[Zeno Albisser]

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

EWS will not pass, because the blocking bugs need to be landed first.

[Noam Rosenthal]

Can you point out the exact point where the contents from the graphics surface are copied to the backing store?

[Zeno Albisser]

(In reply to comment #11)
> Can you point out the exact point where the contents from the graphics surface are copied to the backing store?

Let me try to to illustrate that in the following call chain:

WebGraphicsLayer::syncCompositingStateForThisLayerOnly()
 -> WebGraphicsLayer::syncCanvas()
  -> LayerTreeHostQt::syncCanvas()
   -> ... IPC ...
    -> LayerTreeHostProxy::syncCanvas()
     -> WebLayerTreeRenderer::syncCanvas()
For this code path up to here, please take a look at Bug87738.

WebLayerTreeRenderer::syncCanvas then creates a TextureMapperDirectBackingStore if necessary, and calls TextureMapperDirectBackingStore::setGraphicsSurface().
This function then creates a GraphicsSurface from the token that was passed as a parameter and stores the texture id in a member variable.

The next time, the function TextureMapperDirectBackingStore::paintToTextureMapper() is being called the texture is then being painted on screen directly using drawTextureRectangleARB() with the previously stored texture id as a parameter.

I hope that answers your question. :-)

[Noam Rosenthal]

> WebLayerTreeRenderer::syncCanvas then creates a TextureMapperDirectBackingStore if necessary, and calls TextureMapperDirectBackingStore::setGraphicsSurface().
> This function then creates a GraphicsSurface from the token that was passed as a parameter and stores the texture id in a member variable.

But we only do that once, and at the next sync we already have a GraphicsSurface. How do you make it sync the next time there's an update?
Seems to me like that texture remains bound to the surface, in that case we're double-buffered instead of triple-buffered, which would create a sync issue.

[Zeno Albisser]

(In reply to comment #13)
> > WebLayerTreeRenderer::syncCanvas then creates a TextureMapperDirectBackingStore if necessary, and calls TextureMapperDirectBackingStore::setGraphicsSurface().
> > This function then creates a GraphicsSurface from the token that was passed as a parameter and stores the texture id in a member variable.
> 
> But we only do that once, and at the next sync we already have a GraphicsSurface. How do you make it sync the next time there's an update?
> Seems to me like that texture remains bound to the surface, in that case we're double-buffered instead of triple-buffered, which would create a sync issue.

This depends on the update.
There are two cases:
1. Regular painting update (a WebGL call is executed). In this case WebGLRenderingContext::markContextChanged() is executed and this marks the according layer as dirty. With the next repaint the TextureMapperDirectBackingStore::paintToTextureMapper() function will be executed which then uses drawTextureRectangleARB() to bind the texture again and paint it on screen. There is no need to recreate the GraphicsSurface or recreate the texture from the GraphicsSurface.
The CGLIOSurface.h header states:
"Put simply, in order for changes done to an IOSurface on context A to become visible to context B, you must flush context A's command stream (via an explicit call to glFlush, glFlushRenderAPPLE, etc.), and then perform a 'bind' (in this case, glBindTexture()) on context B."

2. Resizing the canvas. In this case creating a new GraphicsSurface is actually necessary. So we create a new one and replace the old one. The token will be different in this case and when TextureMapperDirectBackingStore::setGraphicsSurface() is executed, it will actually create a new GraphicsSurface in the UIProcess as well and the texture id will be updated.

[Noam Rosenthal]

> 1. Regular painting update (a WebGL call is executed). In this case WebGLRenderingContext::markContextChanged() is executed and this marks the according layer as dirty. With the next repaint the TextureMapperDirectBackingStore::paintToTextureMapper() function will be executed which then uses drawTextureRectangleARB() to bind the texture again and paint it on screen. There is no need to recreate the GraphicsSurface or recreate the texture from the GraphicsSurface.
> The CGLIOSurface.h header states:
> "Put simply, in order for changes done to an IOSurface on context A to become visible to context B, you must flush context A's command stream (via an explicit call to glFlush, glFlushRenderAPPLE, etc.), and then perform a 'bind' (in this case, glBindTexture()) on context B."

OK, this is a sync issue. It would mean that if the following sequence occurs:
1. we flush the WebGL context in the web process
2. we scroll in the UI process
3. we sync the layers in the web process
4. we sync the layer changes in the UI process

The correct place for the changes from (1) to take affect are after (4). But with your patch they happend at (2). This would create a sync issue if other things in the page update as well.

[Zeno Albisser]

> OK, this is a sync issue. It would mean that if the following sequence occurs:
> 1. we flush the WebGL context in the web process
> 2. we scroll in the UI process
> 3. we sync the layers in the web process
> 4. we sync the layer changes in the UI process
> 
> The correct place for the changes from (1) to take affect are after (4). But with your patch they happend at (2). This would create a sync issue if other things in the page update as well.

Okay, I see what you mean. Sorry, it took me a while to understand.
I was just talking to Simon about it, and he proposed to use two GraphicsSurfaces that are then being used alternated. - I'll give it a try.

[Zeno Albisser]

Created attachment 145279 [details]
patch for review. - adding a second GraphicSurface as a buffer.

[Noam Rosenthal]

Comment on attachment 145279 [details]
patch for review. - adding a second GraphicSurface as a buffer.

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

Good! Just a few nitpicks.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:34
> +    if (graphicsSurfaceToken != m_backbufferGraphicsSurfaceToken) {

backbuffer -> backBuffer

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
> +                                            | GraphicsSurface::SupportsSharing;

It actually needs TextureTarget and not CopyFromTexture... but I guess we can deal with that when we deal with hardware that makes that distinction.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:52
> +    // Swap front and backbuffer GraphicsSurface.
> +    RefPtr<GraphicsSurface> temp =  m_currentGraphicsSurface;
> +    m_currentGraphicsSurface = m_backbufferGraphicsSurface;
> +    m_backbufferGraphicsSurface = temp;
> +    uint32_t tempToken = m_currentGraphicsSurfaceToken;
> +    m_currentGraphicsSurfaceToken = m_backbufferGraphicsSurfaceToken;
> +    m_backbufferGraphicsSurfaceToken = tempToken;
> +    uint32_t tempTextureId = m_currentTextureId;
> +    m_currentTextureId = m_backbufferTextureId;
> +    m_backbufferTextureId = tempTextureId;

Would be better to have these 3 members in a struct and call std::swap

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:55
> +PassRefPtr<BitmapTexture> TextureMapperDirectBackingStore::texture() const

I think this is not pure virtual, you should be able to simply not override it.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:43
> +class TextureMapperDirectBackingStore : public TextureMapperBackingStore {

How about TextureMapperSurfaceBackingStore? It's a bit more of a direct name :)

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:253
> +    program->initialize(); // Avoid calling virtual functions in constructor.

Put comment in previous line

[Zeno Albisser]

Comment on attachment 145279 [details]
patch for review. - adding a second GraphicSurface as a buffer.

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:34
>> +    if (graphicsSurfaceToken != m_backbufferGraphicsSurfaceToken) {
> 
> backbuffer -> backBuffer

fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
>> +                                            | GraphicsSurface::SupportsSharing;
> 
> It actually needs TextureTarget and not CopyFromTexture... but I guess we can deal with that when we deal with hardware that makes that distinction.

fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:52
>> +    m_backbufferTextureId = tempTextureId;
> 
> Would be better to have these 3 members in a struct and call std::swap

added GraphicsSurfaceData structure.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:55
>> +PassRefPtr<BitmapTexture> TextureMapperDirectBackingStore::texture() const
> 
> I think this is not pure virtual, you should be able to simply not override it.

Yes it is. Maybe we could change that instead?
For the moment i just left it the way it is.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:43
>> +class TextureMapperDirectBackingStore : public TextureMapperBackingStore {
> 
> How about TextureMapperSurfaceBackingStore? It's a bit more of a direct name :)

Agree - fixed.

>> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:253
>> +    program->initialize(); // Avoid calling virtual functions in constructor.
> 
> Put comment in previous line

fixed.

[Zeno Albisser]

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

[Noam Rosenthal]

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

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

LGTM with minor nitpicks

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:41
> +    std::swap(m_backBufferGraphicsSurfaceData, m_currentBufferGraphicsSurfaceData);

current -> front

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:56
> +    uint32_t m_textureId;

Id -> ID

[Zeno Albisser]

Committed r119247: <http://trac.webkit.org/changeset/119247>

[Csaba Osztrogonác]

(In reply to comment #22)
> Committed r119247: <http://trac.webkit.org/changeset/119247>

It broke the build on Qt Windows, ARM, MIPS, SH4.
Could you check it, please?

[Zeno Albisser]

(In reply to comment #23)
> It broke the build on Qt Windows, ARM, MIPS, SH4.
> Could you check it, please?

I just landed a bugfix in r119252. - I Hope it will make the bots green again. :)