Add layout qualifiers to compositor shaders

Add layout qualifiers to compositor shaders

KHR_blend_equation_advanced requires layout qualifiers on the out
parameters from fragment shaders.  This also switches from using
gl_FragColor to explicit out parameters everywhere.

This can't be landed without changes further down the shader
compilation pipeline.

BUG=661715
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[enne (OOO), 2016-12-01 19:45:29]

Description was changed from

==========
Add layout qualifiers to compositor shaders

KHR_blend_equation_advanced requires layout qualifiers on the out
parameters from fragment shaders.  This also switches from using
gl_FragColor to explicit out parameters everywhere.

This can't be landed without changes further down the shader
compilation pipeline.

BUG=661715
==========

to

==========
Add layout qualifiers to compositor shaders

KHR_blend_equation_advanced requires layout qualifiers on the out
parameters from fragment shaders.  This also switches from using
gl_FragColor to explicit out parameters everywhere.

This can't be landed without changes further down the shader
compilation pipeline.

BUG=661715
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2016-12-01 19:45:57]

kbr: I think this should do the trick, although it's hard to test since no
shaders compile.  :)

[Ken Russell (switch to Gerrit), 2016-12-02 02:22:21]

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

[Ken Russell (switch to Gerrit), 2016-12-02 02:22:22]

I think there's another problem here.

KHR_blend_equation_advanced requires these new layout qualifiers, but
NV_blend_equation_advanced doesn't know about them. So presumably a system
supporting the NV_ extension and not the KHR_ extension would reject these.

feature_info.cc seems to treat the NV and KHR extensions identically, and
advertise them as the KHR extension to clients like the renderer process.

Presumably these would need to be stripped out in the shader translator if the
NV extension was actually being used? Olli, others at or formerly at NVIDIA,
Brian -- any experience porting between these?

[kkinnunennvidia (do not use), 2016-12-02 06:56:45]

On 2016/12/02 02:22:22, Ken Russell wrote:
> feature_info.cc seems to treat the NV and KHR extensions identically, and
> advertise them as the KHR extension to clients like the renderer process.

No concrete experience -- Chris should have.

Yeah, sounds that strictly speaking the command buffer should emulate the parts
where KHR_bea is not subset of NV_bea.
This means the shader layout qualifiers and the draw time error.

Naturally, the draw time error should also be implemented in ANGLE too, if ANGLE
would expose KHR_bea and implement it with NV_bea.
(In addition to the shader compiler change that would fix the command buffer and
angle).


If it is useful, I can try to ask around if we can estimate the population that
has NV_bea but not KHR_bea, but I don't think we have any exact data..

[Ken Russell (switch to Gerrit), 2016-12-02 08:04:11]

On 2016/12/02 06:56:45, kkinnunennvidia (do not use) wrote:
> On 2016/12/02 02:22:22, Ken Russell wrote:
> > feature_info.cc seems to treat the NV and KHR extensions identically, and
> > advertise them as the KHR extension to clients like the renderer process.
> 
> No concrete experience -- Chris should have.
> 
> Yeah, sounds that strictly speaking the command buffer should emulate the
parts
> where KHR_bea is not subset of NV_bea.
> This means the shader layout qualifiers and the draw time error.
> 
> Naturally, the draw time error should also be implemented in ANGLE too, if
ANGLE
> would expose KHR_bea and implement it with NV_bea.
> (In addition to the shader compiler change that would fix the command buffer
and
> angle).
> 
> 
> If it is useful, I can try to ask around if we can estimate the population
that
> has NV_bea but not KHR_bea, but I don't think we have any exact data..

Would you please do that? I'd really appreciate it. Emulating the draw call
checks sounds like a lot of work, and a potential performance hit. Unless
there's a strong objection, I think the best solution for Chrome is to require
KHR_bea to be supported by the driver in order to get support for these blend
equations. (My Linux workstation with NVIDIA GPU and 367.57 driver supports both
extensions, for what it's worth.)

Brian, do you have any feedback about this? Does Skia support both extensions,
and only emit the layout qualifiers when KHR_bea is in use?

I think we also need to emit an #extension directive at the top of these
shaders:

#extension GL_KHR_blend_equation_advanced : require

Enne, possible for you to handle that in this patch too? I don't know where it
would go. Thanks.

[bsalomon, 2016-12-02 14:21:46]

On 2016/12/02 08:04:11, Ken Russell wrote:
> On 2016/12/02 06:56:45, kkinnunennvidia (do not use) wrote:
> > On 2016/12/02 02:22:22, Ken Russell wrote:
> > > feature_info.cc seems to treat the NV and KHR extensions identically, and
> > > advertise them as the KHR extension to clients like the renderer process.
> > 
> > No concrete experience -- Chris should have.
> > 
> > Yeah, sounds that strictly speaking the command buffer should emulate the
> parts
> > where KHR_bea is not subset of NV_bea.
> > This means the shader layout qualifiers and the draw time error.
> > 
> > Naturally, the draw time error should also be implemented in ANGLE too, if
> ANGLE
> > would expose KHR_bea and implement it with NV_bea.
> > (In addition to the shader compiler change that would fix the command buffer
> and
> > angle).
> > 
> > 
> > If it is useful, I can try to ask around if we can estimate the population
> that
> > has NV_bea but not KHR_bea, but I don't think we have any exact data..
> 
> Would you please do that? I'd really appreciate it. Emulating the draw call
> checks sounds like a lot of work, and a potential performance hit. Unless
> there's a strong objection, I think the best solution for Chrome is to require
> KHR_bea to be supported by the driver in order to get support for these blend
> equations. (My Linux workstation with NVIDIA GPU and 367.57 driver supports
both
> extensions, for what it's worth.)
> 
> Brian, do you have any feedback about this? Does Skia support both extensions,
> and only emit the layout qualifiers when KHR_bea is in use?

This is indeed what we do. I believe we supported the NV variant before the KHR
was available. I don't have a strong opinion about whether Chrome supports the
NV extension, though. If the population of people that have NV but not KHR is
very small then it seems fine to drop it. The only real consequence would be
some extra shader compilations.

> 
> I think we also need to emit an #extension directive at the top of these
> shaders:
> 
> #extension GL_KHR_blend_equation_advanced : require
> 
> Enne, possible for you to handle that in this patch too? I don't know where it
> would go. Thanks.

[enne (OOO), 2016-12-02 18:25:33]

> > I think we also need to emit an #extension directive at the top of these
> > shaders:
> > 
> > #extension GL_KHR_blend_equation_advanced : require
> > 
> > Enne, possible for you to handle that in this patch too? I don't know where
it
> > would go. Thanks.

Sure sure, done.  I'm going on vacation shortly.  If you need more work, the
diffs here should hopefully be pretty self-explanatory.  You can also run
cc_unittests --gtest_filter=*ShadersCompile and it will compile every compositor
shader and print out errors and the shader text for it if it fails.

Also, looking at the build logs:
  Error compiling shader: ERROR: 0:4: 'out' : storage qualifier supported in
GLSL ES 3.00 and above only
  Error compiling shader: ERROR: 0:8: 'layout' : syntax error

I knew 'layout' needed some work to be supported here, but it sounds like the
compositor needs to be switched to use es3 as well in order to use out with that
layout qualifier?

[Ken Russell (switch to Gerrit), 2016-12-05 05:44:29]

On 2016/12/02 18:25:33, enne_out_til_january wrote:
> > > I think we also need to emit an #extension directive at the top of these
> > > shaders:
> > > 
> > > #extension GL_KHR_blend_equation_advanced : require
> > > 
> > > Enne, possible for you to handle that in this patch too? I don't know
where
> it
> > > would go. Thanks.
> 
> Sure sure, done.  I'm going on vacation shortly.  If you need more work, the
> diffs here should hopefully be pretty self-explanatory.  You can also run
> cc_unittests --gtest_filter=*ShadersCompile and it will compile every
compositor
> shader and print out errors and the shader text for it if it fails.
> 
> Also, looking at the build logs:
>   Error compiling shader: ERROR: 0:4: 'out' : storage qualifier supported in
> GLSL ES 3.00 and above only
>   Error compiling shader: ERROR: 0:8: 'layout' : syntax error
> 
> I knew 'layout' needed some work to be supported here, but it sounds like the
> compositor needs to be switched to use es3 as well in order to use out with
that
> layout qualifier?

Argh. Yes, that was an oversight on my part. I don't see any way to make this
work if the compositor is still outputting ESSL 1.00 (OpenGL ES 2.0) shaders.
The reason is that those shaders will need to be compiled to backends like
desktop OpenGL, which require the layout qualifiers, and it's not feasible to
infer their need from an ESSL 1.00 shader which can't use the layout qualifiers.

[Ken Russell (switch to Gerrit), 2016-12-05 05:44:33]

https://codereview.chromium.org/2549623002/diff/40001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2549623002/diff/40001/cc/output/shader.cc#new...
cc/output/shader.cc:824: "#extension GL_KHR_blend_equation_advanced : require\n"
I'm pretty sure this extension directive has to come at the beginning of the
shader. Will that happen with the code as written?