Prepare WebGL contexts for resource sharing with the compositor context.

Add an additional feature to GLES2DecoderImpl that is used to force the context to obey WebGL feature semantics.

The 'force_webgl_glsl_validation' flag is used to override the FeatureInfo parameters of the context group.

This change enables all extensions for WebGL contexts, in preparation for allowing resource sharing with the compositor.

BUG=96605
TEST=https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/webgl-conformance-tests.html

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103724

[Jeff Timanus, 2011-09-07 20:45:20]

Please take a look.

Not much to see.  I'm concerned that this approach will incorrectly expose
functionality to WebGL.

Do we need a better approach?

[apatrick_chromium, 2011-09-07 20:48:03]

+kbr, will this break our WebGL semantics?

[Ken Russell (switch to Gerrit), 2011-09-08 03:22:17]

Yes, this will break WebGL conformance. Hopefully
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
would catch the failure.

I've replied to the email thread; a better fix, if possible, would be to have
WebGL contexts use their own validators, rather than those of the share group.

[Jeff Timanus, 2011-09-08 18:21:20]

On 2011/09/08 03:22:17, kbr wrote:
> Yes, this will break WebGL conformance. Hopefully
>
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
> would catch the failure.

I ran this test both with and without my change, and it passed.

Offline, you mentioned that the proper test to try is the following:  
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...

It also explicitly passes with my changes - we still fail to create BGRA
textures.  This doesn't make sense to me, as I explicitly enabled that
extension.  I will track down the cause.

> 
> I've replied to the email thread; a better fix, if possible, would be to have
> WebGL contexts use their own validators, rather than those of the share group.

I'm investigating the feasibility of what you propose.

[Ken Russell (switch to Gerrit), 2011-09-08 18:38:31]

On 2011/09/08 18:21:20, Jeff Timanus wrote:
> On 2011/09/08 03:22:17, kbr wrote:
> > Yes, this will break WebGL conformance. Hopefully
> >
>
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
> > would catch the failure.
> 
> I ran this test both with and without my change, and it passed.
> 
> Offline, you mentioned that the proper test to try is the following:  
>
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
> 
> It also explicitly passes with my changes - we still fail to create BGRA
> textures.  This doesn't make sense to me, as I explicitly enabled that
> extension.  I will track down the cause.

I looked through WebGLRenderingContext::validateTexFuncFormatAndType
(Source/WebCore/html/canvas/WebGLRenderingContext.cpp in the WebKit tree) and it
is what is catching the invalid usage. So for practical purposes the WebGL
implementation will catch this particular issue.

I'm still concerned that the compositor generally enables "all" extensions and,
depending on the order of initialization, WebGL might see many more features
than it expects. I really think the more robust fix to allow WebGL to share
resources with the compositor is to give it independent validator instances.

> > 
> > I've replied to the email thread; a better fix, if possible, would be to
have
> > WebGL contexts use their own validators, rather than those of the share
group.
> 
> I'm investigating the feasibility of what you propose.

[Jeff Timanus, 2011-09-15 20:44:05]

On 2011/09/08 18:38:31, kbr wrote:
> On 2011/09/08 18:21:20, Jeff Timanus wrote:
> > On 2011/09/08 03:22:17, kbr wrote:
> > > Yes, this will break WebGL conformance. Hopefully
> > >
> >
>
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
> > > would catch the failure.
> > 
> > I ran this test both with and without my change, and it passed.
> > 
> > Offline, you mentioned that the proper test to try is the following:  
> >
>
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/confo...
> > 
> > It also explicitly passes with my changes - we still fail to create BGRA
> > textures.  This doesn't make sense to me, as I explicitly enabled that
> > extension.  I will track down the cause.
> 
> I looked through WebGLRenderingContext::validateTexFuncFormatAndType
> (Source/WebCore/html/canvas/WebGLRenderingContext.cpp in the WebKit tree) and
it
> is what is catching the invalid usage. So for practical purposes the WebGL
> implementation will catch this particular issue.
> 
> I'm still concerned that the compositor generally enables "all" extensions
and,
> depending on the order of initialization, WebGL might see many more features
> than it expects. I really think the more robust fix to allow WebGL to share
> resources with the compositor is to give it independent validator instances.
> 
> > > 
> > > I've replied to the email thread; a better fix, if possible, would be to
> have
> > > WebGL contexts use their own validators, rather than those of the share
> group.
> > 
> > I'm investigating the feasibility of what you propose.

Patch #3 implements a rough first pass at providing an individual set of
validators for all contexts requesting only a subset of extensions.

Because the sharing group is initialized when the first context is constructed,
we have no way of knowing which set of extensions will be requested.  This is
why the context group is constructed with all extensions.

With this change, I was able to enable resource sharing for WebGL contexts
without observing failures in the compositor.

Will update further once I've run all of the tests.

[Ken Russell (switch to Gerrit), 2011-09-15 23:57:27]

http://codereview.chromium.org/7782038/diff/14001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/14001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1753: if (0 ==
strcmp(allowed_extensions, "*")) {
Shouldn't this comparison be "0 != strcmp(...)"? We only want the local
FeatureInfo if the allowed extensions *aren't* "*".

[Jeff Timanus, 2011-09-16 15:00:17]

http://codereview.chromium.org/7782038/diff/14001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/14001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1753: if (0 ==
strcmp(allowed_extensions, "*")) {
On 2011/09/15 23:57:27, kbr wrote:
> Shouldn't this comparison be "0 != strcmp(...)"? We only want the local
> FeatureInfo if the allowed extensions *aren't* "*".

Yes!

Done.

[apatrick_chromium, 2011-09-16 18:03:33]

I have reservations about this. Is there a bug assigned to fix this at the
WebKit level and get rid of the local feature info?

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1311: const Validators*
validators_;
For a context with local feature into, this could point to
local_feature_info_.validators() rather than the share groups validators, that
it shouldn't ever use.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1317: scoped_ptr<FeatureInfo>
local_feature_info_;
Then this could be FeatureInfo rather than scoped_ptr<FeatureInfo>.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1320: const Validators*
validators() const {
And this function wouldn't be needed, nor the change to the python script.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1754:
local_feature_info_.reset(new FeatureInfo());
You could modify validators_ to point to the local validators here.

[Jeff Timanus, 2011-09-16 18:31:24]

On 2011/09/16 18:03:33, apatrick_chromium wrote:
> I have reservations about this. Is there a bug assigned to fix this at the
> WebKit level and get rid of the local feature info?

Yes, there is a bug assigned to Ken to remove the need for the local feature
info.  See crbug.com/96605

> 
>
http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1311: const Validators*
> validators_;
> For a context with local feature into, this could point to
> local_feature_info_.validators() rather than the share groups validators, that
> it shouldn't ever use.
> 
>
http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1317: scoped_ptr<FeatureInfo>
> local_feature_info_;
> Then this could be FeatureInfo rather than scoped_ptr<FeatureInfo>.
> 
>
http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1320: const Validators*
> validators() const {
> And this function wouldn't be needed, nor the change to the python script.
> 
>
http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1754:
> local_feature_info_.reset(new FeatureInfo());
> You could modify validators_ to point to the local validators here.

[Jeff Timanus, 2011-09-16 21:25:00]

I'm looking into the failures reported by the try bots on patch #4.

A point against my current design is that it is asymmetrical: The feature_info_
that is used is that of the ContextGroup, which is always initialized with all
extensions, while the validators point to the local feature info, which is
initialized according to the client's request.  

Can we also replace the feature_info_ alias with direct reference to
local_feature_info_?  I do not yet know this code well enough to determine if
this is reasonable.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1311: const Validators*
validators_;
On 2011/09/16 18:03:34, apatrick_chromium wrote:
> For a context with local feature into, this could point to
> local_feature_info_.validators() rather than the share groups validators, that
> it shouldn't ever use.

Done.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1317: scoped_ptr<FeatureInfo>
local_feature_info_;
On 2011/09/16 18:03:34, apatrick_chromium wrote:
> Then this could be FeatureInfo rather than scoped_ptr<FeatureInfo>.

Done.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1320: const Validators*
validators() const {
On 2011/09/16 18:03:34, apatrick_chromium wrote:
> And this function wouldn't be needed, nor the change to the python script.

Done.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1754:
local_feature_info_.reset(new FeatureInfo());
On 2011/09/16 18:03:34, apatrick_chromium wrote:
> You could modify validators_ to point to the local validators here.
Since validators_ can now always point to the local_feature_info_, I assign it
at GLES2DecoderImpl construction time.

[apatrick_chromium, 2011-09-16 21:39:20]

I think having feature_info_ point to local_feature_info_ might be right (for
some approximation of right!) I would need to inspect all the code to be sure
though.

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1754:
local_feature_info_.reset(new FeatureInfo());
On 2011/09/16 21:25:01, Jeff Timanus wrote:
> On 2011/09/16 18:03:34, apatrick_chromium wrote:
> > You could modify validators_ to point to the local validators here.
> Since validators_ can now always point to the local_feature_info_, I assign it
> at GLES2DecoderImpl construction time.

But if allowed_extensions is "*", we don't want local validators, no? I think we
might as well be correct in that case at least. The compositor context and the
accelerated 2d canvas are much more promiscuous about which textures they share
with each other and we don't want their validators to get out of sync. Or am I
missing something?

[Jeff Timanus, 2011-09-16 22:01:51]

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/18001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1754:
local_feature_info_.reset(new FeatureInfo());
On 2011/09/16 21:39:20, apatrick_chromium wrote:
> On 2011/09/16 21:25:01, Jeff Timanus wrote:
> > On 2011/09/16 18:03:34, apatrick_chromium wrote:
> > > You could modify validators_ to point to the local validators here.
> > Since validators_ can now always point to the local_feature_info_, I assign
it
> > at GLES2DecoderImpl construction time.
> 
> But if allowed_extensions is "*", we don't want local validators, no? I think
we
> might as well be correct in that case at least. The compositor context and the
> accelerated 2d canvas are much more promiscuous about which textures they
share
> with each other and we don't want their validators to get out of sync. Or am I
> missing something?

It's possible I'm missing something, but here's my understanding:

If allowed extensions is *, then local_feature_info_ will be initialized as *,
the same as the context group.  Because of this, the value of the validators
will be exactly the same as if they pointed to those of the context group.

This is why I used a pointer in my first implementation - so that we could
bypass the initialization and allocation of the local feature instance entirely
when all extensions are requested.  Reverting to the scoped_ptr version seems
like a slightly better approach to me.

Is it possible for extension support to be added post-initialization?  My
understanding is that once a context is built, new extensions cannot be added.

[apatrick_chromium, 2011-09-16 22:12:14]

I did some digging. If you look at TextureManager::SetLevelInfo, it's keeping
track of how many unrenderable textures there are in the share group. We
definitely don't want that to get out of sync by having multiple FeatureInfos.
So on that basis I think don't make feature_info_ point to local_feature_info_.
Have you tried putting a local_validators_ in the decoder rather than
local_feature_info_?

[Jeff Timanus, 2011-09-20 17:58:06]

On 2011/09/16 22:12:14, apatrick_chromium wrote:
> I did some digging. If you look at TextureManager::SetLevelInfo, it's keeping
> track of how many unrenderable textures there are in the share group. We
> definitely don't want that to get out of sync by having multiple FeatureInfos.
> So on that basis I think don't make feature_info_ point to
local_feature_info_.
> Have you tried putting a local_validators_ in the decoder rather than
> local_feature_info_?

Patch #8 implements this approach.  I construct a temporary FeatureInfo
instance, and copy its validators into a validator instance owned by the
GLES2DecoderImpl.  This bypasses the dangers of having multiple FeatureInfo
instances living throughout the duration of the decoder.

Again, I bypass pointing validators to the context group because the local ones
will always be constructed 'correctly', as discussed above.

I had to modify the testing framework to add new expectations for the
construction of the temporary FeatureInfo.

[greggman, 2011-09-20 18:06:40]

I'm frustrated by this CL. I feel like my POV has been completely ignored.

The whole point of the command buffer is SECURITY. The service prevents the
client from being able to do anything bad.

This CL breaks that contract. Before this CL the client could screw up but
nothing the client did could ever make the service mess up.

After this CL the client now has the ability to mess up the service's internal
bookkeeping.

Please DO NOT DO THIS!

http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6665:
validators_.vertex_attrib_type.AddValue(GL_FIXED);
The reason validators are CONST is because NO other code should be able to
change the validation. Do not make the validators non-const!

[Jeff Timanus, 2011-09-20 19:15:59]

On 2011/09/20 18:06:40, greggman wrote:
> I'm frustrated by this CL. I feel like my POV has been completely ignored.
> 
> The whole point of the command buffer is SECURITY. The service prevents the
> client from being able to do anything bad.
> 
> This CL breaks that contract. Before this CL the client could screw up but
> nothing the client did could ever make the service mess up.
> 
> After this CL the client now has the ability to mess up the service's internal
> bookkeeping.
> 
> Please DO NOT DO THIS!
> 
>
http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:6665:
> validators_.vertex_attrib_type.AddValue(GL_FIXED);
> The reason validators are CONST is because NO other code should be able to
> change the validation. Do not make the validators non-const!

I just had a chat with Gregg about his concerns.

He strongly proposed the following:
#1 Change kWebGLPreferredGLExtensions to "*" in WebGraphics3DCommandBufferImp
#2 Add a case for GLES2DecoderImpl::HandleEnableFeatureCHROMIUM to check for
"webgl_enable_glsl_webgl_validation"  and set a flag in GLES2DecoderImpl that it
should use webgl_glsl_valdiation in gles2_cmd_decoder.cc

Gregg felt strongly that this wouldn't expose unwanted features to WebGL, and
would only be necessary until we have a resource sharing path via
EGLImage/EGLSurface.

I do admit that this changes does feel like it may be breaking the assumptions
made by a lot of the decoder code.

[Ken Russell (switch to Gerrit), 2011-09-20 20:31:32]

On 2011/09/20 19:15:59, Jeff Timanus wrote:
> On 2011/09/20 18:06:40, greggman wrote:
> > I'm frustrated by this CL. I feel like my POV has been completely ignored.
> > 
> > The whole point of the command buffer is SECURITY. The service prevents the
> > client from being able to do anything bad.
> > 
> > This CL breaks that contract. Before this CL the client could screw up but
> > nothing the client did could ever make the service mess up.
> > 
> > After this CL the client now has the ability to mess up the service's
internal
> > bookkeeping.
> > 
> > Please DO NOT DO THIS!
> > 
> >
>
http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
http://codereview.chromium.org/7782038/diff/23001/gpu/command_buffer/service/...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:6665:
> > validators_.vertex_attrib_type.AddValue(GL_FIXED);
> > The reason validators are CONST is because NO other code should be able to
> > change the validation. Do not make the validators non-const!
> 
> I just had a chat with Gregg about his concerns.
> 
> He strongly proposed the following:
> #1 Change kWebGLPreferredGLExtensions to "*" in WebGraphics3DCommandBufferImp
> #2 Add a case for GLES2DecoderImpl::HandleEnableFeatureCHROMIUM to check for
> "webgl_enable_glsl_webgl_validation"  and set a flag in GLES2DecoderImpl that
it
> should use webgl_glsl_valdiation in gles2_cmd_decoder.cc
> 
> Gregg felt strongly that this wouldn't expose unwanted features to WebGL, and
> would only be necessary until we have a resource sharing path via
> EGLImage/EGLSurface.
> 
> I do admit that this changes does feel like it may be breaking the assumptions
> made by a lot of the decoder code.

I'm certainly in favor of making the more correct change.

You should be able to request the "GL_CHROMIUM_webglsl" extension from WebKit
code. However I am not 100% sure that that extension is reported as
"requestable" on the WebKit side, so I don't think you can make the request from
WebGLRenderingContext.cpp. Try adding code like the following to
GraphicsContext3DPrivate::createGraphicsContextFromWebContext just before
returning:

if (attrs.noExtensions) {
  ASSERT(threadUsage == ForUseOnThisThread);
  webContext->requestExtensionCHROMIUM("GL_CHROMIUM_webglsl");
}

Then try taking out the kWebGLPreferredExtensions special case from
WebGraphicsContext3DCommandBufferImpl and run the WebGL conformance suite.

Note, there are existing conformance test failures. See Issue 97267. We need to
be *very* careful that those failures do not mask any new failures caused by
these changes.

[Jeff Timanus, 2011-09-21 15:20:35]

On 2011/09/20 19:15:59, Jeff Timanus wrote:

> He strongly proposed the following:
> #1 Change kWebGLPreferredGLExtensions to "*" in WebGraphics3DCommandBufferImp

For reference, enabling * as the WebGL extensions produces the following
conformance test failures (on Linux@101752).  

I find these failures a bit surprising, because passing * should also enable
WebGL shader validation, which is a current bug.  See crbug.com/97312.  

Investigating further.

conformance/context/premultiplyalpha-test.html (42 of 43 passed)
failed: should draw with 128,128,128,255

conformance/extensions/oes-standard-derivatives.html (19 of 22 passed)
failed: GL_OES_standard_derivatives defined in shaders when extension is
disabled
failed: Shader built-ins compiled successfully when extension disabled
failed: Shader built-ins allowed without #extension pragma

conformance/glsl/glsl-conformance.html (118 of 120 passed)
failed: [vshader/fshaderWithdFdx]: fragment shader that uses dFdx should fail
failed: [vshader/fshaderWithdFdxNoExt]: fragment shader that uses dFdx without
#extension should fail

conformance/glsl/glsl-feature-normalize.html (9 of 73 passed)
failed: *** Error compiling shader '[object WebGLShader]':ERROR: 0:8:
'normalize_base' : no matching overloaded function found
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw

conformance/glsl/glsl-feature-reflect.html (9 of 73 passed)
failed: *** Error compiling shader '[object WebGLShader]':ERROR: 0:8: 'value' :
undeclared identifier ERROR: 0:8: 'reflect_base' : no matching overloaded
function found
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw
failed: *** Error loading shader '[object WebGLShader]':INVALID_OPERATION
failed: *** Error loading shader '[object WebGLShader]':INVALID_VALUE
failed: Error in program linking:missing shaders
failed: getError expected: NO_ERROR. Was INVALID_VALUE : no errors from draw

conformance/misc/uninitialized-test.html (8 of 19 passed)
failed: gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
failed: getError expected: NO_ERROR. Was INVALID_FRAMEBUFFER_OPERATION :
failed: gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
failed: non-zero pixel values are wrong
failed: getError expected: NO_ERROR. Was INVALID_FRAMEBUFFER_OPERATION :
failed: gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
failed: non-zero pixel values are wrong
failed: getError expected: NO_ERROR. Was INVALID_FRAMEBUFFER_OPERATION :
failed: gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
failed: non-zero pixel values are wrong
failed: getError expected: NO_ERROR. Was INVALID_FRAMEBUFFER_OPERATION :

conformance/more/conformance/webGLArrays.html(*timeout*)


> #2 Add a case for GLES2DecoderImpl::HandleEnableFeatureCHROMIUM to check for
> "webgl_enable_glsl_webgl_validation"  and set a flag in GLES2DecoderImpl that
it
> should use webgl_glsl_valdiation in gles2_cmd_decoder.cc
> 
> Gregg felt strongly that this wouldn't expose unwanted features to WebGL, and
> would only be necessary until we have a resource sharing path via
> EGLImage/EGLSurface.

[Jeff Timanus, 2011-09-21 21:14:31]

On 2011/09/21 15:20:35, Jeff Timanus wrote:
> On 2011/09/20 19:15:59, Jeff Timanus wrote:
> 
> > He strongly proposed the following:
> > #1 Change kWebGLPreferredGLExtensions to "*" in
WebGraphics3DCommandBufferImp
> 
> For reference, enabling * as the WebGL extensions produces the following
> conformance test failures (on Linux@101752).  
> 
> I find these failures a bit surprising, because passing * should also enable
> WebGL shader validation, which is a current bug.  See crbug.com/97312.  
> 
> Investigating further.
> 

This failure (below) is a known issue.  See crbug.com/97468.

> conformance/context/premultiplyalpha-test.html (42 of 43 passed)
> failed: should draw with 128,128,128,255
> 

These failures are a result of my change.  The problem is that for WebGL
contexts, we need the GL_CHROMIUM_webglsl extension but NOT the
GL_OES_standard_derivatives extension.  Passing * means that we by default get
these extensions.

Do I need to address crbug.com/97312 to correctly perform this change?  Should
some extensions not be enabled when requesting *, and still rely on an explicit
request mechanism?

> conformance/extensions/oes-standard-derivatives.html (19 of 22 passed)
> failed: GL_OES_standard_derivatives defined in shaders when extension is
> disabled
> failed: Shader built-ins compiled successfully when extension disabled
> failed: Shader built-ins allowed without #extension pragma
> conformance/glsl/glsl-conformance.html (118 of 120 passed)
> failed: [vshader/fshaderWithdFdx]: fragment shader that uses dFdx should fail
> failed: [vshader/fshaderWithdFdxNoExt]: fragment shader that uses dFdx without
> #extension should fail

The other failures were a result of a stale driver on my Linux machine.

> 
> 
> > #2 Add a case for GLES2DecoderImpl::HandleEnableFeatureCHROMIUM to check for
> > "webgl_enable_glsl_webgl_validation"  and set a flag in GLES2DecoderImpl
that
> it
> > should use webgl_glsl_valdiation in gles2_cmd_decoder.cc
> > 
> > Gregg felt strongly that this wouldn't expose unwanted features to WebGL,
and
> > would only be necessary until we have a resource sharing path via
> > EGLImage/EGLSurface.

[Ken Russell (switch to Gerrit), 2011-09-21 21:31:00]

On 2011/09/21 21:14:31, Jeff Timanus wrote:
> These failures are a result of my change.  The problem is that for WebGL
> contexts, we need the GL_CHROMIUM_webglsl extension but NOT the
> GL_OES_standard_derivatives extension.  Passing * means that we by default get
> these extensions.
> 
> Do I need to address crbug.com/97312 to correctly perform this change?  Should
> some extensions not be enabled when requesting *, and still rely on an
explicit
> request mechanism?

One way to fix this would be: when GL_CHROMIUM_webglsl is enabled, then if
GL_OES_standard_derivatives has already been enabled, disable it. That way WebGL
contexts wanting GL_OES_standard_derivatives would once again need to explicitly
enable it. (The shader translator instances are recreated whenever any of these
bits affecting its behavior change.)

This doesn't fix the problem of non-WebGL contexts enforcing
GL_CHROMIUM_webglsl, though.

Maybe the better fix is to make those two extensions not affected by requesting
"*".

[Jeff Timanus, 2011-09-22 19:56:31]

On 2011/09/21 21:31:00, kbr wrote:
> On 2011/09/21 21:14:31, Jeff Timanus wrote:
> > These failures are a result of my change.  The problem is that for WebGL
> > contexts, we need the GL_CHROMIUM_webglsl extension but NOT the
> > GL_OES_standard_derivatives extension.  Passing * means that we by default
get
> > these extensions.
> > 
> > Do I need to address crbug.com/97312 to correctly perform this change? 
Should
> > some extensions not be enabled when requesting *, and still rely on an
> explicit
> > request mechanism?
> 
> One way to fix this would be: when GL_CHROMIUM_webglsl is enabled, then if
> GL_OES_standard_derivatives has already been enabled, disable it. That way
WebGL
> contexts wanting GL_OES_standard_derivatives would once again need to
explicitly
> enable it. (The shader translator instances are recreated whenever any of
these
> bits affecting its behavior change.)
> 
> This doesn't fix the problem of non-WebGL contexts enforcing
> GL_CHROMIUM_webglsl, though.
> 
> Maybe the better fix is to make those two extensions not affected by
requesting
> "*".

Patch #10 implements an approach that I think is the best of all worlds.  Users
of FeatureInfo can now pass *, along with extra extensions the need to be
explicitly enabled.

WebGL contexts now enable all extensions, and explicitly enable
GL_CHROMIUM_webglsl.  All other contexts enable all extensions, and explicitly
enable GL_OES_standard_derivatives.  This preserves the existing behaviour for
all non-WebGL contexts.

Note:  All conformance tests pass with this change.

Please take another look.

[greggman, 2011-09-22 20:19:38]

Sorry to be so picky but this doesn't really solve the issue and in fact seem to
only be working through luck. 

You have 2 settings that are mutually exclusive

#1) GL_CHROMIUM_webglsl.

This should only be true for WebGL contexts

#2) GL_OES_standard_derivatives

This should be on for everything BUT WebGL contexts (skia wants this if
available doesn't it?) (I see it being used in GrGLProgram.cpp)

This is why I suggested just always pass in "*" and then add a case for
glCHROMIUMEnableFeature (or some other method of your choice) to set flags in a
specfic context (GLES2DecoderImpl) specifically for WebGL.

That flag or flags should tell the shader validator to disallow
GL_OES_standard_derivatives and to tell the shader validator to use the WebGL
rules instead of the OpenGL ES rules.


..

Other ways of possibly setting those flags

1) Add some more initialization attributes. 

That sounds like more work?

2) Parse the desired_features string in GLES2DecoderImpl. 

That sounds like overloading the purpose of something that shouldn't be
overloaded.

[Jeff Timanus, 2011-09-22 20:45:23]

On 2011/09/22 20:19:38, greggman wrote:
> Sorry to be so picky but this doesn't really solve the issue and in fact seem
to
> only be working through luck. 
> 
> You have 2 settings that are mutually exclusive
> 
> #1) GL_CHROMIUM_webglsl.
> 
> This should only be true for WebGL contexts
> 
> #2) GL_OES_standard_derivatives
> 
> This should be on for everything BUT WebGL contexts (skia wants this if
> available doesn't it?) (I see it being used in GrGLProgram.cpp)
> 
> This is why I suggested just always pass in "*" and then add a case for
> glCHROMIUMEnableFeature (or some other method of your choice) to set flags in
a
> specfic context (GLES2DecoderImpl) specifically for WebGL.
> 
> That flag or flags should tell the shader validator to disallow
> GL_OES_standard_derivatives and to tell the shader validator to use the WebGL
> rules instead of the OpenGL ES rules.

I hadn't realized that there is a separate shader validator per context, even
among shared contexts.  Thanks for pointing out the flaw in my design.

Will upload a patch implementing the suggested approach.

> 
> 
> ..
> 
> Other ways of possibly setting those flags
> 
> 1) Add some more initialization attributes. 
> 
> That sounds like more work?
> 
> 2) Parse the desired_features string in GLES2DecoderImpl. 
> 
> That sounds like overloading the purpose of something that shouldn't be
> overloaded.

[Jeff Timanus, 2011-09-27 01:49:23]

On 2011/09/22 20:45:23, Jeff Timanus wrote:
> On 2011/09/22 20:19:38, greggman wrote:
> > Sorry to be so picky but this doesn't really solve the issue and in fact
seem
> to
> > only be working through luck. 
> > 
> > You have 2 settings that are mutually exclusive
> > 
> > #1) GL_CHROMIUM_webglsl.
> > 
> > This should only be true for WebGL contexts
> > 
> > #2) GL_OES_standard_derivatives
> > 
> > This should be on for everything BUT WebGL contexts (skia wants this if
> > available doesn't it?) (I see it being used in GrGLProgram.cpp)
> > 
> > This is why I suggested just always pass in "*" and then add a case for
> > glCHROMIUMEnableFeature (or some other method of your choice) to set flags
in
> a
> > specfic context (GLES2DecoderImpl) specifically for WebGL.
> > 
> > That flag or flags should tell the shader validator to disallow
> > GL_OES_standard_derivatives and to tell the shader validator to use the
WebGL
> > rules instead of the OpenGL ES rules.
> 
> I hadn't realized that there is a separate shader validator per context, even
> among shared contexts.  Thanks for pointing out the flaw in my design.
> 
> Will upload a patch implementing the suggested approach.

Patch #14 implements the design, as suggested.

I had to insert a few seams to properly handle the on-demand capability to
enable OES derivatives.  

In the longer term, I think a better solution is required than what is presented
here, as it is a little fragile.

> 
> > 
> > 
> > ..
> > 
> > Other ways of possibly setting those flags
> > 
> > 1) Add some more initialization attributes. 
> > 
> > That sounds like more work?
> > 
> > 2) Parse the desired_features string in GLES2DecoderImpl. 
> > 
> > That sounds like overloading the purpose of something that shouldn't be
> > overloaded.

[Jeff Timanus, 2011-09-29 21:45:06]

If possible, could a reviewer please comment on the feasibility of this change?

Thanks,

Jeff

On 2011/09/27 01:49:23, Jeff Timanus wrote:
> On 2011/09/22 20:45:23, Jeff Timanus wrote:
> > On 2011/09/22 20:19:38, greggman wrote:
> > > Sorry to be so picky but this doesn't really solve the issue and in fact
> seem
> > to
> > > only be working through luck. 
> > > 
> > > You have 2 settings that are mutually exclusive
> > > 
> > > #1) GL_CHROMIUM_webglsl.
> > > 
> > > This should only be true for WebGL contexts
> > > 
> > > #2) GL_OES_standard_derivatives
> > > 
> > > This should be on for everything BUT WebGL contexts (skia wants this if
> > > available doesn't it?) (I see it being used in GrGLProgram.cpp)
> > > 
> > > This is why I suggested just always pass in "*" and then add a case for
> > > glCHROMIUMEnableFeature (or some other method of your choice) to set flags
> in
> > a
> > > specfic context (GLES2DecoderImpl) specifically for WebGL.
> > > 
> > > That flag or flags should tell the shader validator to disallow
> > > GL_OES_standard_derivatives and to tell the shader validator to use the
> WebGL
> > > rules instead of the OpenGL ES rules.
> > 
> > I hadn't realized that there is a separate shader validator per context,
even
> > among shared contexts.  Thanks for pointing out the flaw in my design.
> > 
> > Will upload a patch implementing the suggested approach.
> 
> Patch #14 implements the design, as suggested.
> 
> I had to insert a few seams to properly handle the on-demand capability to
> enable OES derivatives.  
> 
> In the longer term, I think a better solution is required than what is
presented
> here, as it is a little fragile.
> 
> > 
> > > 
> > > 
> > > ..
> > > 
> > > Other ways of possibly setting those flags
> > > 
> > > 1) Add some more initialization attributes. 
> > > 
> > > That sounds like more work?
> > > 
> > > 2) Parse the desired_features string in GLES2DecoderImpl. 
> > > 
> > > That sounds like overloading the purpose of something that shouldn't be
> > > overloaded.

[Ken Russell (switch to Gerrit), 2011-09-29 21:58:12]

These changes look good to me; they seem pretty minimal. Are any WebKit changes
needed, or does Chrome still pass the WebGL conformance suite (at least as well
as it did previously)?

[greggman, 2011-09-29 22:15:06]

http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been enabled.
Is this necessary? 

It seems like the model we want is

1) WebGL (which is implemented in WebKit) thinks it's running on top of OpenGL
ES 2.0
2) In WebKit (ie, Safari and Chrome) it will always see the string
"GL_OES_stardard_derivatives"
3) WebGL would normally do its own GLSL validation, rejecting shaders that
attempt to use GL_OES_standard_derivatives
4) in Chrome, we want to add the option to tell the command buffer to do that
validation for WebGL since it's already doing some validation.

So, no reason to pull out GL_OES_standard_derivatives here

Instead, make it possible for WebGL to request that validation.

I would suggest that calling glEnableFeatureCHROMIUM with
"webgl_enable_glsl_webgl_validation" should set flags to make glsl validation as
strict as possible and something like
"webgl_enable_glsl_oes_standard_derivatives" should relax one that one
restriction.

[Ken Russell (switch to Gerrit), 2011-09-29 22:22:45]

On 2011/09/29 22:15:06, greggman wrote:
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been enabled.
> Is this necessary? 
> 
> It seems like the model we want is
> 
> 1) WebGL (which is implemented in WebKit) thinks it's running on top of OpenGL
> ES 2.0
> 2) In WebKit (ie, Safari and Chrome) it will always see the string
> "GL_OES_stardard_derivatives"
> 3) WebGL would normally do its own GLSL validation, rejecting shaders that
> attempt to use GL_OES_standard_derivatives
> 4) in Chrome, we want to add the option to tell the command buffer to do that
> validation for WebGL since it's already doing some validation.
> 
> So, no reason to pull out GL_OES_standard_derivatives here
> 
> Instead, make it possible for WebGL to request that validation.
> 
> I would suggest that calling glEnableFeatureCHROMIUM with
> "webgl_enable_glsl_webgl_validation" should set flags to make glsl validation
as
> strict as possible and something like
> "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> restriction.

In the Chrome WebGL implementation the shader compiler instances are currently
kept solely in the command buffer implementation. This avoids double validation
and also makes ANGLE rolls *much* easier since we don't have to worry about
conflicts with code upstream in WebKit, just in Chrome. It would be a *major*
engineering inconvenience to make Chrome's WebKit code depend on ANGLE.

Please continue to make it possible for WebGL contexts to selectively enable
GL_OES_standard_derivatives in the command buffer code.

[greggman, 2011-09-29 22:31:05]

On 2011/09/29 22:22:45, kbr wrote:
> On 2011/09/29 22:15:06, greggman wrote:
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been enabled.
> > Is this necessary? 
> > 
> > It seems like the model we want is
> > 
> > 1) WebGL (which is implemented in WebKit) thinks it's running on top of
OpenGL
> > ES 2.0
> > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > "GL_OES_stardard_derivatives"
> > 3) WebGL would normally do its own GLSL validation, rejecting shaders that
> > attempt to use GL_OES_standard_derivatives
> > 4) in Chrome, we want to add the option to tell the command buffer to do
that
> > validation for WebGL since it's already doing some validation.
> > 
> > So, no reason to pull out GL_OES_standard_derivatives here
> > 
> > Instead, make it possible for WebGL to request that validation.
> > 
> > I would suggest that calling glEnableFeatureCHROMIUM with
> > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
validation
> as
> > strict as possible and something like
> > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > restriction.
> 
> In the Chrome WebGL implementation the shader compiler instances are currently
> kept solely in the command buffer implementation. This avoids double
validation
> and also makes ANGLE rolls *much* easier since we don't have to worry about
> conflicts with code upstream in WebKit, just in Chrome. It would be a *major*
> engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> 
> Please continue to make it possible for WebGL contexts to selectively enable
> GL_OES_standard_derivatives in the command buffer code.

my suggestion still keeps validation in the cmd buf

[Jeff Timanus, 2011-09-29 22:38:27]

On 2011/09/29 21:58:12, kbr wrote:
> These changes look good to me; they seem pretty minimal. Are any WebKit
changes
> needed, or does Chrome still pass the WebGL conformance suite (at least as
well
> as it did previously)?

No, there are no webkit changes.  Also, these changes do not introduce any
regressions to the WebGL conformance tests.  (At least of the last time I
checked.)

[Ken Russell (switch to Gerrit), 2011-09-29 22:42:04]

On 2011/09/29 22:31:05, greggman wrote:
> On 2011/09/29 22:22:45, kbr wrote:
> > On 2011/09/29 22:15:06, greggman wrote:
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been enabled.
> > > Is this necessary? 
> > > 
> > > It seems like the model we want is
> > > 
> > > 1) WebGL (which is implemented in WebKit) thinks it's running on top of
> OpenGL
> > > ES 2.0
> > > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > > "GL_OES_stardard_derivatives"
> > > 3) WebGL would normally do its own GLSL validation, rejecting shaders that
> > > attempt to use GL_OES_standard_derivatives
> > > 4) in Chrome, we want to add the option to tell the command buffer to do
> that
> > > validation for WebGL since it's already doing some validation.
> > > 
> > > So, no reason to pull out GL_OES_standard_derivatives here
> > > 
> > > Instead, make it possible for WebGL to request that validation.
> > > 
> > > I would suggest that calling glEnableFeatureCHROMIUM with
> > > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
> validation
> > as
> > > strict as possible and something like
> > > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > > restriction.
> > 
> > In the Chrome WebGL implementation the shader compiler instances are
currently
> > kept solely in the command buffer implementation. This avoids double
> validation
> > and also makes ANGLE rolls *much* easier since we don't have to worry about
> > conflicts with code upstream in WebKit, just in Chrome. It would be a
*major*
> > engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> > 
> > Please continue to make it possible for WebGL contexts to selectively enable
> > GL_OES_standard_derivatives in the command buffer code.
> 
> my suggestion still keeps validation in the cmd buf

I don't see how it would work. If "GL_OES_standard_derivatives" is reported to
Chromium's WebKit code then it will think that this extension is already enabled
and will not have the ability to enable it later.

[Ken Russell (switch to Gerrit), 2011-09-29 22:42:23]

On 2011/09/29 22:38:27, Jeff Timanus wrote:
> On 2011/09/29 21:58:12, kbr wrote:
> > These changes look good to me; they seem pretty minimal. Are any WebKit
> changes
> > needed, or does Chrome still pass the WebGL conformance suite (at least as
> well
> > as it did previously)?
> 
> No, there are no webkit changes.  Also, these changes do not introduce any
> regressions to the WebGL conformance tests.  (At least of the last time I
> checked.)

In that case LGTM. Thanks for pushing this through.

[greggman, 2011-09-29 22:58:05]

On 2011/09/29 22:42:04, kbr wrote:
> On 2011/09/29 22:31:05, greggman wrote:
> > On 2011/09/29 22:22:45, kbr wrote:
> > > On 2011/09/29 22:15:06, greggman wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been
enabled.
> > > > Is this necessary? 
> > > > 
> > > > It seems like the model we want is
> > > > 
> > > > 1) WebGL (which is implemented in WebKit) thinks it's running on top of
> > OpenGL
> > > > ES 2.0
> > > > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > > > "GL_OES_stardard_derivatives"
> > > > 3) WebGL would normally do its own GLSL validation, rejecting shaders
that
> > > > attempt to use GL_OES_standard_derivatives
> > > > 4) in Chrome, we want to add the option to tell the command buffer to do
> > that
> > > > validation for WebGL since it's already doing some validation.
> > > > 
> > > > So, no reason to pull out GL_OES_standard_derivatives here
> > > > 
> > > > Instead, make it possible for WebGL to request that validation.
> > > > 
> > > > I would suggest that calling glEnableFeatureCHROMIUM with
> > > > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
> > validation
> > > as
> > > > strict as possible and something like
> > > > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > > > restriction.
> > > 
> > > In the Chrome WebGL implementation the shader compiler instances are
> currently
> > > kept solely in the command buffer implementation. This avoids double
> > validation
> > > and also makes ANGLE rolls *much* easier since we don't have to worry
about
> > > conflicts with code upstream in WebKit, just in Chrome. It would be a
> *major*
> > > engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> > > 
> > > Please continue to make it possible for WebGL contexts to selectively
enable
> > > GL_OES_standard_derivatives in the command buffer code.
> > 
> > my suggestion still keeps validation in the cmd buf
> 
> I don't see how it would work. If "GL_OES_standard_derivatives" is reported to
> Chromium's WebKit code then it will think that this extension is already
enabled
> and will not have the ability to enable it later.

Those 2 things are being conflated.

In REAL OpenGL ES 2.0, which the WebKit code is designed to run on, there is no
turning off features. All there is is querying which features there are.

Separate from that, WebGL needs to disallow GL_OES_standard_derivatives features
in GLSL.

How it does that is irrelevant to whether or not the feature is enabled in
OpenGL ES 2.0

In a "normal" WebGL implementation WebGL would have to validate the shaders
before passing them to OpenGL ES

In Chrome, the only difference is you want to ask the command buffer to do that
validation. Asking it to do that validation is orthogonal to whether or not
OpenGL ES 2.0 reports that GL_OES_standard_derivities is enabled.

[Jeff Timanus, 2011-09-29 23:00:25]

On 2011/09/29 22:31:05, greggman wrote:
> On 2011/09/29 22:22:45, kbr wrote:
> > On 2011/09/29 22:15:06, greggman wrote:
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been enabled.
> > > Is this necessary? 

The change was necessary because of the logic involved in enabling extensions. 
Previous to this change, WebGL contexts would never see the derivatives
extension unless it had been explicitly enabled.  

The logic that enables the extension only follows the enabling path, if the
extension was not yet in the extensions string.  See
GraphicsContext3DPrivate::ensureExtensionEnabled.

> > > 
> > > It seems like the model we want is
> > > 
> > > 1) WebGL (which is implemented in WebKit) thinks it's running on top of
> OpenGL
> > > ES 2.0
> > > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > > "GL_OES_stardard_derivatives"
This is not how the system works at the moment.  OES derivatives are only seen
after explicitly enabling the extension.
> > > 3) WebGL would normally do its own GLSL validation, rejecting shaders that
> > > attempt to use GL_OES_standard_derivatives
As Ken mentioned, WebGL does not do any validation outside of the command
buffers and translators.

> > > 4) in Chrome, we want to add the option to tell the command buffer to do
> that
> > > validation for WebGL since it's already doing some validation.
> > > 
> > > So, no reason to pull out GL_OES_standard_derivatives here
> > > 
> > > Instead, make it possible for WebGL to request that validation.
> > > 
> > > I would suggest that calling glEnableFeatureCHROMIUM with
> > > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
> validation
> > as
> > > strict as possible and something like
> > > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > > restriction.
> > 
> > In the Chrome WebGL implementation the shader compiler instances are
currently
> > kept solely in the command buffer implementation. This avoids double
> validation
> > and also makes ANGLE rolls *much* easier since we don't have to worry about
> > conflicts with code upstream in WebKit, just in Chrome. It would be a
*major*
> > engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> > 
> > Please continue to make it possible for WebGL contexts to selectively enable
> > GL_OES_standard_derivatives in the command buffer code.
> 
> my suggestion still keeps validation in the cmd buf

[greggman, 2011-09-29 23:22:51]

On 2011/09/29 23:00:25, Jeff Timanus wrote:
> On 2011/09/29 22:31:05, greggman wrote:
> > On 2011/09/29 22:22:45, kbr wrote:
> > > On 2011/09/29 22:15:06, greggman wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been
enabled.
> > > > Is this necessary? 
> 
> The change was necessary because of the logic involved in enabling extensions.

> Previous to this change, WebGL contexts would never see the derivatives
> extension unless it had been explicitly enabled.  
> 
> The logic that enables the extension only follows the enabling path, if the
> extension was not yet in the extensions string.  See
> GraphicsContext3DPrivate::ensureExtensionEnabled.


The entire point of these changes is that because contexts are now sharing
resources THERE IS NO WAY TO INDIVIDUALLY ENABLE FEATURES anymore. That entire
code path, the ability to select features, is pointless now that we're using
shared resources.

So, you have to do it a new way. You have 2 choices

#1) Stop sharing resources

This way you have it the way it was and can individually select features

#2) Change WebGL so it expects all features are on

Then, where possible, add some options to increase validation.

I'm trying to get to #2 since I'm assuming we're not going back to non-shared
resources anytime soon.





> 
> > > > 
> > > > It seems like the model we want is
> > > > 
> > > > 1) WebGL (which is implemented in WebKit) thinks it's running on top of
> > OpenGL
> > > > ES 2.0
> > > > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > > > "GL_OES_stardard_derivatives"
> This is not how the system works at the moment.  OES derivatives are only seen
> after explicitly enabling the extension.
> > > > 3) WebGL would normally do its own GLSL validation, rejecting shaders
that
> > > > attempt to use GL_OES_standard_derivatives
> As Ken mentioned, WebGL does not do any validation outside of the command
> buffers and translators.
> 
> > > > 4) in Chrome, we want to add the option to tell the command buffer to do
> > that
> > > > validation for WebGL since it's already doing some validation.
> > > > 
> > > > So, no reason to pull out GL_OES_standard_derivatives here
> > > > 
> > > > Instead, make it possible for WebGL to request that validation.
> > > > 
> > > > I would suggest that calling glEnableFeatureCHROMIUM with
> > > > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
> > validation
> > > as
> > > > strict as possible and something like
> > > > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > > > restriction.
> > > 
> > > In the Chrome WebGL implementation the shader compiler instances are
> currently
> > > kept solely in the command buffer implementation. This avoids double
> > validation
> > > and also makes ANGLE rolls *much* easier since we don't have to worry
about
> > > conflicts with code upstream in WebKit, just in Chrome. It would be a
> *major*
> > > engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> > > 
> > > Please continue to make it possible for WebGL contexts to selectively
enable
> > > GL_OES_standard_derivatives in the command buffer code.
> > 
> > my suggestion still keeps validation in the cmd buf

[Ken Russell (switch to Gerrit), 2011-09-29 23:43:05]

On 2011/09/29 23:22:51, greggman wrote:
> On 2011/09/29 23:00:25, Jeff Timanus wrote:
> > On 2011/09/29 22:31:05, greggman wrote:
> > > On 2011/09/29 22:22:45, kbr wrote:
> > > > On 2011/09/29 22:15:06, greggman wrote:
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7782038/diff/50001/gpu/command_buffer/service/...
> > > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5506: // not been
> enabled.
> > > > > Is this necessary? 
> > 
> > The change was necessary because of the logic involved in enabling
extensions.
> 
> > Previous to this change, WebGL contexts would never see the derivatives
> > extension unless it had been explicitly enabled.  
> > 
> > The logic that enables the extension only follows the enabling path, if the
> > extension was not yet in the extensions string.  See
> > GraphicsContext3DPrivate::ensureExtensionEnabled.
> 
> 
> The entire point of these changes is that because contexts are now sharing
> resources THERE IS NO WAY TO INDIVIDUALLY ENABLE FEATURES anymore. That entire
> code path, the ability to select features, is pointless now that we're using
> shared resources.
> 
> So, you have to do it a new way. You have 2 choices
> 
> #1) Stop sharing resources
> 
> This way you have it the way it was and can individually select features
> 
> #2) Change WebGL so it expects all features are on
> 
> Then, where possible, add some options to increase validation.
> 
> I'm trying to get to #2 since I'm assuming we're not going back to non-shared
> resources anytime soon.

This patch gets us very close to what you want. The only wart is the removal of
advertising of the OES_standard_derivatives extension to WebGL contexts. We will
need to modify the WebKit code in WebGL in order to request different strings to
enable stricter shader validation. That will require several back-and-forth
commits to WebKit and Chrome. In the meantime this patch implements basically
the same idea. I think that we should land this, unblock Jeff's work, and in
parallel fix how WebGL enables its stricter shader validation and
OES_standard_derivatives.

Still LGTM.



> 
> 
> 
> 
> 
> > 
> > > > > 
> > > > > It seems like the model we want is
> > > > > 
> > > > > 1) WebGL (which is implemented in WebKit) thinks it's running on top
of
> > > OpenGL
> > > > > ES 2.0
> > > > > 2) In WebKit (ie, Safari and Chrome) it will always see the string
> > > > > "GL_OES_stardard_derivatives"
> > This is not how the system works at the moment.  OES derivatives are only
seen
> > after explicitly enabling the extension.
> > > > > 3) WebGL would normally do its own GLSL validation, rejecting shaders
> that
> > > > > attempt to use GL_OES_standard_derivatives
> > As Ken mentioned, WebGL does not do any validation outside of the command
> > buffers and translators.
> > 
> > > > > 4) in Chrome, we want to add the option to tell the command buffer to
do
> > > that
> > > > > validation for WebGL since it's already doing some validation.
> > > > > 
> > > > > So, no reason to pull out GL_OES_standard_derivatives here
> > > > > 
> > > > > Instead, make it possible for WebGL to request that validation.
> > > > > 
> > > > > I would suggest that calling glEnableFeatureCHROMIUM with
> > > > > "webgl_enable_glsl_webgl_validation" should set flags to make glsl
> > > validation
> > > > as
> > > > > strict as possible and something like
> > > > > "webgl_enable_glsl_oes_standard_derivatives" should relax one that one
> > > > > restriction.
> > > > 
> > > > In the Chrome WebGL implementation the shader compiler instances are
> > currently
> > > > kept solely in the command buffer implementation. This avoids double
> > > validation
> > > > and also makes ANGLE rolls *much* easier since we don't have to worry
> about
> > > > conflicts with code upstream in WebKit, just in Chrome. It would be a
> > *major*
> > > > engineering inconvenience to make Chrome's WebKit code depend on ANGLE.
> > > > 
> > > > Please continue to make it possible for WebGL contexts to selectively
> enable
> > > > GL_OES_standard_derivatives in the command buffer code.
> > > 
> > > my suggestion still keeps validation in the cmd buf

[commit-bot: I haz the power, 2011-10-03 15:23:28]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/7782038/50001

[commit-bot: I haz the power, 2011-10-03 16:31:07]

Change committed as 103724