Teach GPU command buffer whether a context is webgl.

Teach GPU command buffer whether a context is webgl.

Then we initialize it differently. For example, if it's WebGL 1, we disallow
NPOT support. Also, we initialize shader translator correctly once (before
we have to initialize it to the default ES spec and then reset a new one with
WebGL spec), so this is a speedup at WebGL context creation time.

Also, we separate the idea of whether a context is ES2/ES3 or WebGL1/WebGL2.

This is necessary because before we assume ES3==WebGL2, which isn't true, say,
we may always want to use ES3, for WebGL 2 and for compositor, etc.

BUG=497464
TEST=gpu_unittests, webgl_conformance, gl_tests
R=kbr@chromium.org,piman@chromium.org

Committed: https://crrev.com/539d22c09d9ff40d132998b1d955ffc57bbfc05c
Cr-Commit-Position: refs/heads/master@{#333439}

[Zhenyao Mo, 2015-06-07 18:35:07]

kbr, piman: please review.

(Both are required as each of you are only owner of part of the files this CL
touches.)

Others: FYI

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11091:
InitializeShaderTranslator();
This second InitializeShaderTranslator() call per each WebGL context
initialization is eliminated with this refactoring.

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11128:
feature_str.find("GL_CHROMIUM_webglsl") != std::string::npos;
"GL_CHROMIUM_webglsl" was not the correct feature string. This was a bug that
caused InitializeShaderTranslator() to be always called below, which hides
another bug that enabling EXT_shader_texture_lod isn't triggering the reset as
it should be.

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11160: desire_shader_texture_lod
!= shader_texture_lod_explicitly_enabled_) {
This is apparently a bug, see above the reasoning.

[Zhenyao Mo, 2015-06-07 21:42:12]

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:526: if
(!disallowed_features_.npot_support &&
This is where we disable NPOT support for WebGL 1.  Note that handling of
falling back to (0, 0, 0, 1) is emulated in command buffer, not delegating to
underlying driver even when NPOT support isn't reported by the driver.

[piman, 2015-06-08 21:40:21]

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2599: if
(attrib_parser.es3_context_required &&
Don't we want to keep this? I assume we don't want to expose ES3 to clients that
don't request it explicitly?

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.h:49: npot_support(false) {
So, if we add this to the FeatureInfo, that could bring trouble - the
FeatureInfo is shared between all contexts in the same group, but in theory
today one can have a webgl context sharing with a non-webgl context (actually it
used to be that way but we removed it a little while ago).

I think it's a reasonable restriction, but it means we need to validate, when we
create the context, that the context group is compatible, otherwise fail.
Otherwise a NPOT texture would be valid/complete in one context and invalid in
another context and that violates a bunch of assumptions in the TextureManager.

[Zhenyao Mo, 2015-06-08 22:23:22]

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2599: if
(attrib_parser.es3_context_required &&
On 2015/06/08 21:40:21, piman (Very slow to review) wrote:
> Don't we want to keep this? I assume we don't want to expose ES3 to clients
that
> don't request it explicitly?

I am OK with only exposing ES3 for WebGL 2 for now, but at certain point we may
want to expose ES3 by default if it can be supported.

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.h:49: npot_support(false) {
On 2015/06/08 21:40:21, piman (Very slow to review) wrote:
> So, if we add this to the FeatureInfo, that could bring trouble - the
> FeatureInfo is shared between all contexts in the same group, but in theory
> today one can have a webgl context sharing with a non-webgl context (actually
it
> used to be that way but we removed it a little while ago).
> 
> I think it's a reasonable restriction, but it means we need to validate, when
we
> create the context, that the context group is compatible, otherwise fail.
> Otherwise a NPOT texture would be valid/complete in one context and invalid in
> another context and that violates a bunch of assumptions in the
TextureManager.

I remember we decided each WebGL is in it's own context.  In the future we might
want to share WebGL contexts for shared resources extension purpose.  It seems
that we should not share WebGL and WebGL 2 contexts.

I can add the checks in, but should it be a CHECK/DCHECK or an initialization
failure?

[piman, 2015-06-08 22:37:24]

On Mon, Jun 8, 2015 at 3:23 PM, <zmo@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):
>
>
>
https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:2599: if
> (attrib_parser.es3_context_required &&
> On 2015/06/08 21:40:21, piman (Very slow to review) wrote:
>
>> Don't we want to keep this? I assume we don't want to expose ES3 to
>>
> clients that
>
>> don't request it explicitly?
>>
>
> I am OK with only exposing ES3 for WebGL 2 for now, but at certain point
> we may want to expose ES3 by default if it can be supported.


I guess we'd want to make it work like EGL (EGL_CONTEXT_CLIENT_VERSION):
you get the version you ask for, and if you don't you get the base version.

>
>
>
>
https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.h (right):
>
>
>
https://codereview.chromium.org/1143393007/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.h:49: npot_support(false) {
> On 2015/06/08 21:40:21, piman (Very slow to review) wrote:
>
>> So, if we add this to the FeatureInfo, that could bring trouble - the
>> FeatureInfo is shared between all contexts in the same group, but in
>>
> theory
>
>> today one can have a webgl context sharing with a non-webgl context
>>
> (actually it
>
>> used to be that way but we removed it a little while ago).
>>
>
>  I think it's a reasonable restriction, but it means we need to
>>
> validate, when we
>
>> create the context, that the context group is compatible, otherwise
>>
> fail.
>
>> Otherwise a NPOT texture would be valid/complete in one context and
>>
> invalid in
>
>> another context and that violates a bunch of assumptions in the
>>
> TextureManager.
>
> I remember we decided each WebGL is in it's own context.  In the future
> we might want to share WebGL contexts for shared resources extension
> purpose.  It seems that we should not share WebGL and WebGL 2 contexts.
>
> I can add the checks in, but should it be a CHECK/DCHECK or an
> initialization failure?
>

It needs to be an initialization failure, because we can't trust the client
(and we don't want to give the client something it didn't ask for).


>
> https://codereview.chromium.org/1143393007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2015-06-09 01:07:57]

This direction looks good overall. There's no plan to put WebGL contexts back
into client-side share groups with non-WebGL contexts, so having that be a
constraint is fine.

[Zhenyao Mo, 2015-06-09 01:17:36]

Patchset #4 (id:60001) has been deleted

[Zhenyao Mo, 2015-06-09 01:18:14]

Patchset #4 (id:60001) has been deleted

[Zhenyao Mo, 2015-06-09 01:18:24]

Patchset #4 (id:80001) has been deleted

[Zhenyao Mo, 2015-06-09 01:19:13]

Revised. Please take another look.

[piman, 2015-06-09 02:19:37]

lgtm

https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:48: kContextTypeWebGL1,
nit: CONTEXT_TYPE_WEBGL1 etc. per style guide.

[Zhenyao Mo, 2015-06-09 03:50:23]

https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:48: kContextTypeWebGL1,
On 2015/06/09 02:19:37, piman (Very slow to review) wrote:
> nit: CONTEXT_TYPE_WEBGL1 etc. per style guide.

Actually the new style guide prefers the constant style instead of the macro
style for enums:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Enumerator_Names

[Zhenyao Mo, 2015-06-09 03:50:33]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-06-09 03:50:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143393007/100001

[commit-bot: I haz the power, 2015-06-09 04:14:30]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-06-09 04:15:25]

Patchset 4 (id:??) landed as
https://crrev.com/539d22c09d9ff40d132998b1d955ffc57bbfc05c
Cr-Commit-Position: refs/heads/master@{#333439}

[piman, 2015-06-09 15:00:09]

On Mon, Jun 8, 2015 at 8:50 PM, <zmo@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/context_group.h (right):
>
>
>
https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
> gpu/command_buffer/service/context_group.h:48: kContextTypeWebGL1,
> On 2015/06/09 02:19:37, piman (Very slow to review) wrote:
>
>> nit: CONTEXT_TYPE_WEBGL1 etc. per style guide.
>>
>
> Actually the new style guide prefers the constant style instead of the
> macro style for enums:
>
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Enumerator_Names


http://dev.chromium.org/developers/coding-style#Naming overrules this for
Chromium code.

Antoine


>
>
> https://codereview.chromium.org/1143393007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2015-06-09 16:31:27]

On 2015/06/09 15:00:09, piman (Very slow to review) wrote:
> On Mon, Jun 8, 2015 at 8:50 PM, <mailto:zmo@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/context_group.h (right):
> >
> >
> >
>
https://codereview.chromium.org/1143393007/diff/100001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/context_group.h:48: kContextTypeWebGL1,
> > On 2015/06/09 02:19:37, piman (Very slow to review) wrote:
> >
> >> nit: CONTEXT_TYPE_WEBGL1 etc. per style guide.
> >>
> >
> > Actually the new style guide prefers the constant style instead of the
> > macro style for enums:
> >
> >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Enumerator_Names
> 
> 
> http://dev.chromium.org/developers/coding-style#Naming overrules this for
> Chromium code.
> 
> Antoine

Thanks for the info. I'll update it in a followup CL.

> 
> 
> >
> >
> > https://codereview.chromium.org/1143393007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.