command_buffer: Use GLSL core version that matches the OpenGL version

command_buffer: Use GLSL core version that matches the OpenGL version

Use GLSL core version that matches OpenGL version as specified in the
corresponding OpenGL specification.

This makes it simpler to support GL extensions that depend on specific
GLSL feature and are specified to be supported on contexts implementing
the GLSL language version.

Using the specified version as opposed to a hand-picked specific version
should minimize the kind of regressions caused by forcing a specific
core GLSL version in the past.

The idea is that when implementing a client-side feature that requires
a specific service-side GLSL feature, the feature_info can just check
the corresponding GL context version. Typically context version of an
extension is selected so that GLSL features match.

Needed in implementing EXT_blend_func_extended in the client. The
service side will be implemented with ARB_blend_func_extended, which
uses custom outputs. Using compatibility output from ANGLE would result
in gl_ built-ins as well as custom outputs, which is not allowed.

BUG=471960
Committed: https://crrev.com/48854200686afcc6f5ff26b3316cb5ae59c365dc
Cr-Commit-Position: refs/heads/master@{#343172}

[Kimmo Kinnunen, 2015-08-05 11:01:59]

kkinnunen@nvidia.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org

[Kimmo Kinnunen, 2015-08-05 11:02:00]

Would this be ok?

[Ken Russell (switch to Gerrit), 2015-08-05 18:29:37]

kbr@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2015-08-05 18:29:38]

This looks generally OK to me. The biggest risk will be on Linux since we'll
start upgrading even the compositor's shaders to later GLSL versions, even
running on the compatibility profile. The default behavior of Windows and Mac
will be unchanged.

I'm not an OWNER in this directory. CC'ing more OWNERS.

A few comments.

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:221: ShShaderOutput
GetShaderOutputLanguageForContext(
Please figure out some way to test this code. Perhaps you could expose it in
gles2_cmd_decoder.h as a static "ForTesting" method, and add tests of the
version string handling to gles2_cmd_decoder_unittest.cc -- or go through the
whole command decoder initialization (which sounds like overkill).

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
File gpu/command_buffer/service/shader_translator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
gpu/command_buffer/service/shader_translator_unittest.cc:13: class
ShaderTranslatorTest : public testing::Test {
Somewhere there should be some tests of the handling of the various GLSL
versions in the translator. They probably don't belong here, but it looks to me
like more tests are needed in src/tests/compiler_tests/ in ANGLE; please work
with Jamie and Geoff to add them.

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
gpu/command_buffer/service/shader_translator_unittest.cc:31: &resources,
SH_GLSL_150_CORE_OUTPUT,
I'm not sure the 150_CORE_OUTPUT will be the common case -- COMPATIBILITY_OUTPUT
seems more likely.

[Jamie Madill, 2015-08-05 18:35:55]

jmadill@chromium.org changed reviewers:
+ jmadill@chromium.org

[Jamie Madill, 2015-08-05 18:35:56]

Not an owner but the general logic of the patch (use highest available version)
LGTM.

Would there be a theoretical mismatch if the command buffer is running in compat
and we compile core shaders? I'm not sure how different core shaders are from
compat in any case.

[Geoff Lang, 2015-08-05 18:40:55]

On 2015/08/05 18:35:56, Jamie Madill wrote:
> Not an owner but the general logic of the patch (use highest available
version)
> LGTM.
> 
> Would there be a theoretical mismatch if the command buffer is running in
compat
> and we compile core shaders? I'm not sure how different core shaders are from
> compat in any case.

LGTM for the general logic too.  I added the new GLSL output targets so that we
can always target the highest available GLSL version in ANGLE's GL renderer. See
https://chromium.googlesource.com/angle/angle/+/master/src/libANGLE/renderer/...
for a similar usage.

[Zhenyao Mo, 2015-08-05 18:52:22]

LGTM with kbr's comments addressed.

[Ken Russell (switch to Gerrit), 2015-08-06 00:41:00]

On 2015/08/05 18:35:56, Jamie Madill wrote:
> Not an owner but the general logic of the patch (use highest available
version)
> LGTM.
> 
> Would there be a theoretical mismatch if the command buffer is running in
compat
> and we compile core shaders? I'm not sure how different core shaders are from
> compat in any case.

I think this should work. We just need to be careful not to emit a #version
directive higher than the GLSL version supported by the context. The problem
encountered previously on OS X was that the core profile no longer supported
shaders written for pre-OpenGL-3.2 contexts.

[piman, 2015-08-06 19:40:04]

LGTM after Ken's comments are addressed.

[Kimmo Kinnunen, 2015-08-10 08:36:34]

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:221: ShShaderOutput
GetShaderOutputLanguageForContext(
On 2015/08/05 18:29:37, Ken Russell OOO-till-Aug-17 wrote:
> Please figure out some way to test this code. Perhaps you could expose it in
> gles2_cmd_decoder.h as a static "ForTesting" method, and add tests of the
> version string handling to gles2_cmd_decoder_unittest.cc -- or go through the
> whole command decoder initialization (which sounds like overkill).

I struggled a bit adding the return type (ShShaderOutput) to the decoder.h.
Maybe the posted alternative would be ok?

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
File gpu/command_buffer/service/shader_translator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1273693002/diff/1/gpu/command_buffer/s...
gpu/command_buffer/service/shader_translator_unittest.cc:31: &resources,
SH_GLSL_150_CORE_OUTPUT,
On 2015/08/05 18:29:38, Ken Russell OOO-till-Aug-17 wrote:
> I'm not sure the 150_CORE_OUTPUT will be the common case --
COMPATIBILITY_OUTPUT
> seems more likely.

Done.

[Ken Russell (switch to Gerrit), 2015-08-11 16:41:51]

Test looks very good. LGTM

[Kimmo Kinnunen, 2015-08-12 05:16:58]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-08-12 05:16:58]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
zmo@chromium.org, geofflang@chromium.org, jmadill@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1273693002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-08-12 05:17:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273693002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273693002/40001

[commit-bot: I haz the power, 2015-08-12 05:35:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-12 05:35:19]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Kimmo Kinnunen, 2015-08-12 07:31:58]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-08-12 07:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
piman@chromium.org, zmo@chromium.org, geofflang@chromium.org,
jmadill@chromium.org
Link to the patchset: https://codereview.chromium.org/1273693002/#ps60001
(title: "fix android compile problem and rebase")

[commit-bot: I haz the power, 2015-08-12 07:32:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273693002/60001

[commit-bot: I haz the power, 2015-08-12 08:08:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-12 08:08:52]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Kimmo Kinnunen, 2015-08-13 07:56:51]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-08-13 07:57:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273693002/60001

[commit-bot: I haz the power, 2015-08-13 09:41:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-13 09:42:21]

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