Implement WEBGL_shared_resources

Implement WEBGL_shared_resources

BUG=235718

[do-not-use, 2013-06-18 08:08:00]

https://codereview.chromium.org/17230006/diff/2001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.idl (right):

https://codereview.chromium.org/17230006/diff/2001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.idl:26: interface WebGLSharedObject {
Isn't it supposed to inherit WebGLObject?
http://www.khronos.org/webgl/wiki/SharedResouces#Suggested_IDL:

[greggman, 2013-06-19 00:03:20]

Hey Ken, PTAL

Sorry it's so big. Some parts are smaller than they look (a check added to
various functions)

The other big thing that changed is I had to update how textures are checked at
rendering time because I needed to know which textures to check. The old code
was checking all textures, not just used textures.

The test is here
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl...

and passes with experimental/draft webgl extensions enabled.

https://codereview.chromium.org/17230006/diff/2001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.idl (right):

https://codereview.chromium.org/17230006/diff/2001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.idl:26: interface WebGLSharedObject {
On 2013/06/18 08:08:00, Christophe Dumez wrote:
> Isn't it supposed to inherit WebGLObject?
> http://www.khronos.org/webgl/wiki/SharedResouces#Suggested_IDL:

It could but that's not the current spec
http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/

[Ken Russell (switch to Gerrit), 2013-06-19 03:51:24]

This is brilliant work.

It's clear this was carefully written and tested; despite the fact that the
patch is large, the code reads very cleanly. It would be possible to spend days
reviewing it but the patch LGTM after spending a few hours going through it.

One question is whether you want to land this now, or wait until next Monday,
after Chrome's M29 branch point. A change as large as this has the potential to
destabilize core functionality. I trust your judgment and leave that decision to
you.

I suggest the following testing:

- WebGL 1.0.1 conformance suite on Linux (on a workstation known to pass the
tests cleanly), with Release ASAN builds, before and after this patch. There are
a lot of new objects pointing to each other and this will give good confidence
that memory errors aren't introduced.

- All functionality in the new Google Maps (basic map, Earth mode, Street View,
Photo Tours).

- Top of tree conformance suite before and after this patch to ensure no new
regressions.

Mainly a few questions; the patch is excellently written.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLAcquireSharedResourceCallback.idl (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLAcquireSharedResourceCallback.idl:27: boolean
handleEvent(WebGLSharedObject object);
Is the extension spec correct here? It looks like the object is passed as
argument to the callback (which seems good), but the extension spec says the
callback takes no arguments.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLContextGroup.h:48: // Adds the context to this
group and returns a context index of the context.
Is the context index used anywhere? It looks like it's ignored at the call site.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLDebugShaders.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLDebugShaders.cpp:1: /*
Accidental whitespace change.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.cpp:189: for (int j = 0; location >= 0 && j
< info.size; ++j) {
This inner loop is tricky to understand. Can you add a comment saying that the
point of the loop is to chain together the locations of uniform sampler arrays?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.cpp:234: location = it->value.nextLocation;
This chaining together of the locations of uniform sampler arrays is extremely
clever. I wonder whether anyone is going to understand it in the future...

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLProgram.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.h:84: void
setSampler(WebGLRenderingContext*, GC3Dint, GC3Dsizei, const GC3Dint*);
Would a more descriptive name like "conditionallySetSamplersToTextureUnits" be
useful here?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContext.cpp:1673: if
(!object->validate(functionName, this, WebGLSharedObject::Exclusive, true, 0)) {
What happens if the implementation tries to force context loss (because of page
navigation, etc.) but the user has messed up the acquire state of the object? It
looks like validation will fail and the object won't actually be deleted. Is a
"force" flag needed?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContext.cpp:4515: return false;
The code associated with resetActiveUnit isn't run in this case; I assume that's
OK because the next call with prepareToDraw=false will take care of it?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLShareGroup.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLShareGroup.h:14: * THIS SOFTWARE IS PROVIDED BY
APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
This file and some of the others should use a newer copyright header.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:45: case 4:
Should these ints come from WebGLSharedResources.h instead?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:58:
acquire(WebGLSharedObject::Exclusive, context);
Should there be an ASSERT on the result of this call?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:171: // Checks the object belonngs
to this group.
Typo: belonngs

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:234: }
Are there any race conditions with these acquire callbacks and page navigation /
termination? What about forced lost contexts or object deletion? Some of these
questions are duplicated in WebGLRenderingContext.cpp. It looks like deletion
will fail if the resource isn't acquired, so maybe that's the answer to that
question.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.h:55: static bool
IntToAcquireMode(unsigned, AcquireMode*);
nit: I think Blink naming convention is intToAcquireMode.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.h:143: WebGLRenderingContext*
m_context;
How about a comment: // Weak, but context is kept alive strongly via
m_sharedObject
(if that's correct)?

[do-not-use, 2013-06-19 05:57:38]

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.idl (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.idl:26: interface WebGLSharedObject {
from your link
(http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/):
interface WebGLSharedObject : WebGLObject {
}

so I still don't understand why this doesn't inherit WebGLObject.

[Ken Russell (switch to Gerrit), 2013-06-19 20:29:59]

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.idl (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.idl:26: interface WebGLSharedObject {
On 2013/06/19 05:57:38, Christophe Dumez wrote:
> from your link
> (http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/%29:
> interface WebGLSharedObject : WebGLObject {
> }
> 
> so I still don't understand why this doesn't inherit WebGLObject.

Thanks for pointing this out; I missed it in my review. It does look like a
mistake.

[greggman, 2013-06-21 19:54:21]

I think I covered all your comments but it looks like I'm not going to be able
to commit this


1) I think there's a debate to be had on if this is the righ direction

https://www.khronos.org/webgl/public-mailing-list/archives/1306/msg00046.html



2) When I run the conformance tests in debug on OSX the GPU process crashes
randomly while creating an offscreen view.

I can't see how this code causes that. It's certainly possible it does but the
mailbox code causes a similar issue with the texture-size.html 

https://code.google.com/p/chromium/issues/detail?id=251628



In any case I don't have time to track down why and I can't check this in until
that issue is solved

So, at the moment this is in limbo

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLAcquireSharedResourceCallback.idl (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLAcquireSharedResourceCallback.idl:27: boolean
handleEvent(WebGLSharedObject object);
On 2013/06/19 03:51:24, kbr wrote:
> Is the extension spec correct here? It looks like the object is passed as
> argument to the callback (which seems good), but the extension spec says the
> callback takes no arguments.

No, That's just because I copied it from somewhere else. Removed. There's no
need for it to pass you the resource. It's easy to track that in JavaScript with
closures.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLContextGroup.h:48: // Adds the context to this
group and returns a context index of the context.
On 2013/06/19 03:51:24, kbr wrote:
> Is the context index used anywhere? It looks like it's ignored at the call
site.

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLDebugShaders.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLDebugShaders.cpp:1: /*
On 2013/06/19 03:51:24, kbr wrote:
> Accidental whitespace change.

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.cpp:189: for (int j = 0; location >= 0 && j
< info.size; ++j) {
On 2013/06/19 03:51:24, kbr wrote:
> This inner loop is tricky to understand. Can you add a comment saying that the
> point of the loop is to chain together the locations of uniform sampler
arrays?

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.cpp:234: location = it->value.nextLocation;
On 2013/06/19 03:51:24, kbr wrote:
> This chaining together of the locations of uniform sampler arrays is extremely
> clever. I wonder whether anyone is going to understand it in the future...

added a comment. Or are you suggesting I should use another implementation?

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLProgram.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLProgram.h:84: void
setSampler(WebGLRenderingContext*, GC3Dint, GC3Dsizei, const GC3Dint*);
On 2013/06/19 03:51:24, kbr wrote:
> Would a more descriptive name like "conditionallySetSamplersToTextureUnits" be
> useful here?

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContext.cpp:1673: if
(!object->validate(functionName, this, WebGLSharedObject::Exclusive, true, 0)) {
On 2013/06/19 03:51:24, kbr wrote:
> What happens if the implementation tries to force context loss (because of
page
> navigation, etc.) but the user has messed up the acquire state of the object?
It
> looks like validation will fail and the object won't actually be deleted. Is a
> "force" flag needed?

I don't think so. context lost doesn't go through this code AFAICT

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContext.cpp:4515: return false;
On 2013/06/19 03:51:24, kbr wrote:
> The code associated with resetActiveUnit isn't run in this case; I assume
that's
> OK because the next call with prepareToDraw=false will take care of it?

Yes, added a comment

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:45: case 4:
On 2013/06/19 03:51:24, kbr wrote:
> Should these ints come from WebGLSharedResources.h instead?

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:58:
acquire(WebGLSharedObject::Exclusive, context);
On 2013/06/19 03:51:24, kbr wrote:
> Should there be an ASSERT on the result of this call?

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.cpp:171: // Checks the object belonngs
to this group.
On 2013/06/19 03:51:24, kbr wrote:
> Typo: belonngs

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.h (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.h:55: static bool
IntToAcquireMode(unsigned, AcquireMode*);
On 2013/06/19 03:51:24, kbr wrote:
> nit: I think Blink naming convention is intToAcquireMode.

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.h:143: WebGLRenderingContext*
m_context;
On 2013/06/19 03:51:24, kbr wrote:
> How about a comment: // Weak, but context is kept alive strongly via
> m_sharedObject
> (if that's correct)?

Done.

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLSharedObject.idl (right):

https://codereview.chromium.org/17230006/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLSharedObject.idl:26: interface WebGLSharedObject {
On 2013/06/19 20:30:00, kbr wrote:
> On 2013/06/19 05:57:38, Christophe Dumez wrote:
> > from your link
> >
(http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/%2529:
> > interface WebGLSharedObject : WebGLObject {
> > }
> > 
> > so I still don't understand why this doesn't inherit WebGLObject.
> 
> Thanks for pointing this out; I missed it in my review. It does look like a
> mistake.

Done.