Implemented failIfMajorPerformanceCaveat WebGL context creation attribute.

Implemented failIfMajorPerformanceCaveat WebGL context creation attribute.

Currently only fails if using swiftshader.

BUG=319965
R=kbr@chromium.org, piman@chromium.org, zmo@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237661

[bajones, 2013-11-20 22:33:37]

Please take a look

[Ken Russell (switch to Gerrit), 2013-11-20 22:39:01]

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/common/gle...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/common/gle...
gpu/command_buffer/common/gles2_cmd_utils.cc:737: const int32
kFailIfMajorPerfCaveat = 0x10002;
This duplication of constants is really unfortunate. Is there no way to share
the definitions with
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h ? If not,
please at least put a comment here and in
webgraphicscontext3d_command_buffer_impl.h about keeping the values in sync.

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2293: return false;
How has this been tested?

[piman, 2013-11-20 22:44:54]

https://codereview.chromium.org/79123004/diff/1/content/common/gpu/client/web...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/79123004/diff/1/content/common/gpu/client/web...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:665:
FAIL_IF_MAJOR_PERF_CAVEAT = 0x10002
Can you also implement this in the in-process context, in
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.*

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2291: if
(requested_implementation_name == "swiftshader") {
Can we move the logic about "is this swiftshader" into the feature_info.cc,
maybe even feeding this through the bug/workarounds blacklist?

[Zhenyao Mo, 2013-11-20 22:44:56]

LGTM with nits

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/79123004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2288: if
(command_line->HasSwitch(switches::kUseGL)) {
You don't have to check here.  If kUseGL is not there, it simply returns an
empty string.

Also, why don't we move this and the attrib_parser to the top of the function?
so we don't waste work if it's swiftshader.

[bajones, 2013-11-22 22:16:32]

On 2013/11/20 22:39:01, Ken Russell wrote:
> This duplication of constants is really unfortunate. Is there no way to share
> the definitions with
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h ? If not,
> please at least put a comment here and in
> webgraphicscontext3d_command_buffer_impl.h about keeping the values in sync.

Completely agree. I put the comment in place, and spent some time looking at
what it's going to take to 
normalize the enums here. Unfortunately it's something that would quickly
balloon outside the scope of this
ticket, but I'd like to do it separately.

Logged as http://crbug.com/322699  
 
> How has this been tested?

Patchset #1 was tested on my Windows box with a full Chrome (not Chromium)
build. Aggravatingly I have not been able to test the more recent patchset
because of issues building, which I a continuing to work on. The code is posted
for feedback only.

Automatic testing of this feature is difficult, since swiftshader is only
available in official builds.

On 2013/11/20 22:44:54, piman wrote:
> Can you also implement this in the in-process context, in
> webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.*

Done, and thanks for pointing out that it was needed.

> Can we move the logic about "is this swiftshader" into the feature_info.cc,
> maybe even feeding this through the bug/workarounds blacklist?

Done, and better for it. Thanks.

On 2013/11/20 22:44:56, Zhenyao Mo wrote:
> Also, why don't we move this and the attrib_parser to the top of the function?
> so we don't waste work if it's swiftshader.

Done.

[Ken Russell (switch to Gerrit), 2013-11-23 03:07:03]

LGTM with the following comments addressed and if this has been tested manually.
It would be much better if we could figure out a way to test this on the Windows
GPU bots.

https://codereview.chromium.org/79123004/diff/130002/gpu/command_buffer/clien...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/79123004/diff/130002/gpu/command_buffer/clien...
gpu/command_buffer/client/gl_in_process_context.cc:142: const int32
FAIL_IF_MAJOR_PERF_CAVEAT = 0x10002;
Please add a comment about dependencies between this file and
gles2_cmd_utils.cc.

https://codereview.chromium.org/79123004/diff/130002/gpu/command_buffer/commo...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/79123004/diff/130002/gpu/command_buffer/commo...
gpu/command_buffer/common/gles2_cmd_utils.cc:737: const int32
kFailIfMajorPerfCaveat = 0x10002;
Please add a comment about dependencies between this file,
webgraphicscontext3d_command_buffer_impl.h, and gl_in_process_context.cc.

[piman, 2013-11-23 03:29:10]

lgtm

[commit-bot: I haz the power, 2013-11-28 01:21:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/79123004/240001

[commit-bot: I haz the power, 2013-11-28 02:01:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/79123004/240001

[commit-bot: I haz the power, 2013-11-28 02:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/79123004/240001

[commit-bot: I haz the power, 2013-11-28 02:30:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/79123004/240001

[commit-bot: I haz the power, 2013-11-28 03:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/79123004/240001

[bajones, 2013-11-28 03:11:56]

Committed patchset #3 manually as r237661 (presubmit successful).