Bug 87738

Summary: Make TextureMapper work with GraphicsSurface.
Product: WebKit Reporter: Zeno Albisser <zeno>
Component: WebKit2Assignee: Zeno Albisser <zeno>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, noam, ossy, sam, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87725    
Bug Blocks: 86214    
Attachments:
Description Flags
patch for review.
noam: review-, webkit-ews: commit-queue-
patch for review. - fixed issues as commented before.
zeno: review-, zeno: commit-queue-
patch for review. - adding a second GraphicSurface as a buffer.
noam: review-
patch for review. - fixed issues as commented before. noam: review+

Description Zeno Albisser 2012-05-29 06:24:35 PDT
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.
Comment 1 Noam Rosenthal 2012-05-29 06:55:35 PDT
wouldn't this mean we move from triple-buffering to double-buffering, which might cause sync issues?
Comment 2 Zeno Albisser 2012-05-29 08:03:49 PDT
(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?
Comment 3 Zeno Albisser 2012-05-29 08:09:50 PDT
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.
Comment 4 Early Warning System Bot 2012-05-29 09:33:19 PDT
Comment on attachment 144563 [details]
patch for review.

Attachment 144563 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12845566
Comment 5 Noam Rosenthal 2012-05-29 10:09:41 PDT
(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...
Comment 6 Early Warning System Bot 2012-05-29 11:19:41 PDT
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
Comment 7 Zeno Albisser 2012-05-30 02:04:15 PDT
> 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! :-)
Comment 8 Noam Rosenthal 2012-05-30 07:09:07 PDT
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.
Comment 9 Zeno Albisser 2012-05-30 10:58:08 PDT
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.
Comment 10 Zeno Albisser 2012-05-30 10:59:17 PDT
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.
Comment 11 Noam Rosenthal 2012-05-30 11:38:43 PDT
Can you point out the exact point where the contents from the graphics surface are copied to the backing store?
Comment 12 Zeno Albisser 2012-05-30 13:29:33 PDT
(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. :-)
Comment 13 Noam Rosenthal 2012-05-30 15:00:36 PDT
> 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.
Comment 14 Zeno Albisser 2012-05-30 16:29:43 PDT
(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.
Comment 15 Noam Rosenthal 2012-05-30 16:35:29 PDT
> 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.
Comment 16 Zeno Albisser 2012-05-31 02:30:37 PDT
> 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.
Comment 17 Zeno Albisser 2012-06-01 05:47:20 PDT
Created attachment 145279 [details]
patch for review. - adding a second GraphicSurface as a buffer.
Comment 18 Noam Rosenthal 2012-06-01 06:32:08 PDT
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
Comment 19 Zeno Albisser 2012-06-01 07:43:39 PDT
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.
Comment 20 Zeno Albisser 2012-06-01 07:44:15 PDT
Created attachment 145306 [details]
patch for review. - fixed issues as commented before.
Comment 21 Noam Rosenthal 2012-06-01 09:04:32 PDT
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
Comment 22 Zeno Albisser 2012-06-01 10:02:24 PDT
Committed r119247: <http://trac.webkit.org/changeset/119247>
Comment 23 Csaba Osztrogon√°c 2012-06-01 10:22:00 PDT
(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?
Comment 24 Zeno Albisser 2012-06-01 10:29:34 PDT
(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. :)