Bug 111097 – [Chromium] Use Texture Mailboxes to Transport WebGL Buffers

• Component: WebGL
• Status: RESOLVED WONTFIX

We need to send WebGL buffers to the Chromium compositor via mailboxes to properly track their ownership and ensure proper synchronization.

Created attachment 190774 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

This is a very early CL and still has a number of known issues and unresolved corner cases.

Uploading this now to get some early high level feedback.

[WebKit Review Bot]

Comment on attachment 190774 [details]
Patch

Attachment 190774 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16849117

[WebKit Review Bot]

Comment on attachment 190774 [details]
Patch

Attachment 190774 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16848172

[John Bauman]

Comment on attachment 190774 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:314
> +    // FIXME: this is not working yet, need a good test case.

Try printing https://www.khronos.org/registry/webgl/sdk/demos/google/shiny-teapot/index.html and see if the teapot shows up.

[Kenneth Russell]

Comment on attachment 190774 [details]
Patch

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

> Source/Platform/chromium/public/WebCompositorSupport.h:84
> +    virtual WebExternalTextureLayer* createExternalTextureLayer(WebExternalTextureLayerClient* = 0, bool mailbox = false) { return 0; }

Minor comment: adding bool arguments to methods makes call sites difficult to read. Either add a descriptive enum, or add another virtual function. I would suggest the latter since it will probably make landing the patch easier.

[Kenneth Russell]

Comment on attachment 190774 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:191
> +        if (m_drawingBuffer->graphicsContext3D()->getExtensions()->getGraphicsResetStatusARB() != GraphicsContext3D::NO_ERROR)

Is it strictly necessary to check this here and short-circuit things?

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:234
> +        context()->flush(); // FIXME: we shouldn't need this and use a sync point instead, but mac is unhappy.

What exactly happens on Mac OS?

[Antoine Labour]

Comment on attachment 190774 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:231
> +        context()->genMailboxCHROMIUM(mailbox.name);

So, there's a restriction in mailboxes that a given texture gets attached to a given mailbox name the first time you consume or produce. If m_pendingFrontBuffer is a texture that was returned via consumeMailbox, you'll trigger an assert here.
More generally, creating a mailbox name isn't free (although there's pooling, it does sync calls to the GPU process), so we should reuse the mailbox name that came back.

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:233
> +        context()->deleteTexture(m_pendingFrontBuffer);

Also, it would be even better to keep the texture id for when the mailbox gets returned. both deleteTexture and createTexture cause a shallow flush.

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:234
>> +        context()->flush(); // FIXME: we shouldn't need this and use a sync point instead, but mac is unhappy.
> 
> What exactly happens on Mac OS?

The flush is needed either way, to ensure visibility of the change of this context onto the compositor's.
You shouldn't need a sync point in this case, because WebGL and the compositor are on the same channel.

[Peter Beverloo (cr-android ews)]

Comment on attachment 190774 [details]
Patch

Attachment 190774 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16876016

Created attachment 193157 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193157 [details]
Patch

Attachment 193157 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17024989

[WebKit Review Bot]

Comment on attachment 193157 [details]
Patch

Attachment 193157 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17194222

[Peter Beverloo (cr-android ews)]

Comment on attachment 193157 [details]
Patch

Attachment 193157 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17183301

[James Robinson]

Comment on attachment 193157 [details]
Patch

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

I don't think it's a great idea to delete the texture based path in the same patch as you add the mailbox based path.  There's no way to stage this across repos and it makes the patch a bit harder to follow.  Also if there end up being bugs in either the WebKit or the chromium side we'll just have to revert the whole thing.  Instead, can you add the new path while leaving the old one intact until we can switch over?

> Source/Platform/chromium/public/WebCompositorSupport.h:80
> +    virtual WebExternalTextureLayer* createExternalTextureLayer(WebExternalTextureLayerClient* = 0, bool mailbox = false) { return 0; }

bool parameters in multi-parameter APIs like this are generally a bad idea since it's really hard to tell at callsites what the value means.  For this one, it's pretty much impossible to guess what "createExternalTextureLayer(..., true)" means at the callsite without opening this header up too.  Please use a two-state enum if you want this to be conditional.

> Source/Platform/chromium/public/WebExternalTextureLayerClient.h:44
> +class Mailbox {

The WebKit API follows a one-class-per-header rule (which WebTextureUpdater is actually breaking above).  It's also pretty weird to have implementation logic in a public header.  Also, WebKit::Mailbox is a very generic namespace to be grabbing in a public header.  If you need this logic to be part of the public WebKit API, please put this in its own header and figure out where the implementation should go.  In general the WebKit API for something like this should just be a transport mechanism so I'm not sure why we need logic here.

> Source/Platform/chromium/public/WebExternalTextureLayerClient.h:52
> +        for (int i = 0; i < (int)arraysize(name); ++i) {

we don't use c-style casts in WebKit anywhere. Why not just make the loop variable a size_t (or whatever arraysize() wants)

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:220
> +

no blank line here.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:65
> +    virtual bool prepareMailbox(WebKit::Mailbox*) OVERRIDE;
> +    virtual void mailboxReleased(const WebKit::Mailbox&) OVERRIDE;

Why not just inline these definitions?  They're trivial

[Antoine Labour]

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

At a high level, functionally it looks reasonable. Some WebGL expert should certainly look at this in detail, in particular all the possible combinations between rAF or not, multisample or not, discard or not.

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:127
> +        unsigned serviceId;

nit: serviceId is misleading. The service IDs are the "real" GL texture names, that only exist in the GPU process and that the client never sees. How about "textureId" ?

> I don't think it's a great idea to delete the texture based path in the same patch as you add the mailbox based path.  There's no way to stage this across repos and it makes the patch a bit harder to follow.  Also if there end up being bugs in either the WebKit or the chromium side we'll just have to revert the whole thing.

Originally I was thinking of uploading this to show the full picture and then breaking it up to smaller pieces to deal with deps. But I think you are right about keeping the texture path for now. I'll keep both behind a switch.

> bool parameters in multi-parameter APIs like this are generally a bad idea since it's really hard to tell at callsites what the value means.  For this one, it's pretty much impossible to guess what 

Will do.

> > Source/Platform/chromium/public/WebExternalTextureLayerClient.h:44
> > +class Mailbox {
> 
> The WebKit API follows a one-class-per-header rule (which WebTextureUpdater is actually breaking above).  It's also pretty weird to have implementation logic in a public header. 

How about something like this instead?

struct WebExternalTextureMailbox {
    int8 name[64];
    unsigned syncPoint;
};

(In reply to comment #16)
> View in context: https://bugs.webkit.org/attachment.cgi?id=193157&action=review
> 
> At a high level, functionally it looks reasonable. Some WebGL expert should certainly look at this in detail, in particular all the possible combinations between rAF or not, multisample or not, discard or not.

Yeah, that would be helpful. I did test resize, mutisample or not, discard or not. What is rAF, btw?

[Dana Jansens]

(In reply to comment #18)
> (In reply to comment #16)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193157&action=review
> > 
> > At a high level, functionally it looks reasonable. Some WebGL expert should certainly look at this in detail, in particular all the possible combinations between rAF or not, multisample or not, discard or not.
> 
> Yeah, that would be helpful. I did test resize, mutisample or not, discard or not. What is rAF, btw?

requestAnimationFrame. A way to request a js callback for the next frame.

> requestAnimationFrame. A way to request a js callback for the next frame.

Right, I tested that too.

[Antoine Labour]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=193157&action=review
> > > 
> > > At a high level, functionally it looks reasonable. Some WebGL expert should certainly look at this in detail, in particular all the possible combinations between rAF or not, multisample or not, discard or not.
> > 
> > Yeah, that would be helpful. I did test resize, mutisample or not, discard or not. What is rAF, btw?
> 
> requestAnimationFrame. A way to request a js callback for the next frame.

Specifically, it may impact this in that with rAF, the page will draw exactly once right before we grab the frame, whereas without, it can draw any arbitrary number of times, including 0, between each frame that we grab.

Created attachment 193358 [details]
Patch

Created attachment 193362 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193362 [details]
Patch

Attachment 193362 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17187295

[WebKit Review Bot]

Comment on attachment 193362 [details]
Patch

Attachment 193362 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17129276

[Peter Beverloo (cr-android ews)]

Comment on attachment 193362 [details]
Patch

Attachment 193362 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17145296

Created attachment 193901 [details]
Patch

[James Robinson]

Comment on attachment 193901 [details]
Patch

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

> Source/Platform/chromium/public/WebExternalTextureLayer.h:50
> +    virtual void clearTexture() { };

no ;

> Source/Platform/chromium/public/WebExternalTextureLayerClient.h:51
> +    virtual bool prepareMailbox(WebExternalTextureMailbox*) = 0;
> +    virtual void mailboxReleased(const WebExternalTextureMailbox&) = 0;

The interaction of this with the top 2 could use a bit of documentation

> Source/Platform/chromium/public/WebExternalTextureMailbox.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

2013

> Source/Platform/chromium/public/WebExternalTextureMailbox.h:32
> +    int8 name[64];

where is the type 'int8' supposed to come from in this header?

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:332
> +    // We must restore the texture binding since creating new textures,
> +    // consuming and producing mailboxes changes it.
> +    ScopedTextureBindingRestorer restorer(m_context.get());

This looks too expensive to do on every frame.  This runs on the WebGL context, right?  The WebGL context shadows state fully, we should use its state and not attempt to query it off the context

Created attachment 194285 [details]
Patch

> where is the type 'int8' supposed to come from in this header?

Copy paste from content, where it was defined in base. Fixed.

> > +    ScopedTextureBindingRestorer restorer(m_context.get());
> 
> This looks too expensive to do on every frame.  This runs on the WebGL context, right?  The WebGL context shadows state fully, we should use its state and not attempt to query it off the context

Turns out I overlooked that there is enough information cached by WebGL context on the DrawingBuffer already to do this without the query.

[Kenneth Russell]

Yes, this patch will be handled elsewhere. Closing.