Add support for all blendmodes if we have GL_KHR_blend_equation_advanced.

Add support for all blendmodes if we have GL_KHR_blend_equation_advanced.

BUG=243223
Committed: https://crrev.com/9198b422852a9640e47c2851679b0c69be3eac68
Cr-Commit-Position: refs/heads/master@{#300887}

[Erik Dahlström (inactive), 2014-10-16 12:05:12]

ed@opera.com changed reviewers:
+ enne@chromium.org, jbauman@chromium.org, mkwst@chromium.org,
piman@chromium.org

[Erik Dahlström (inactive), 2014-10-16 12:05:12]

ed@opera.com changed required reviewers:
+ enne@chromium.org

[Erik Dahlström (inactive), 2014-10-16 12:05:13]

PTAL

[piman, 2014-10-16 20:13:29]

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:718: case SkXfermode::kScreen_Mode:
nit: indent style (run git cl format?)

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1261: GLC(gl_, gl_->BlendBarrierNV());
Is it worth skipping this if we have GL_*_blend_equation_advanced_coherent?

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:1557: 'gl_test_func':
'glBlendBarrierNV',
At the command buffer level, I would prefer that we expose the vendor-neutral
KHR_blend_equation_advanced, which is a subset but looks enough for what we use
in cc. In the back-end, we can then use either KHR_blend_equation_advanced or
NV_blend_equation_advanced

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:858: const GLenum equations[] = {
nit: indent looks wrong.

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:873: GL_HSL_LUMINOSITY_NV
If we don't support the whole range of enums, we shouldn't claim support for the
extension. That's also why I prefer exposing KHR_blend_equation_advanced instead
of NV_blend_equation_advanced at the command-buffer level.

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:876: for (const GLenum& equation :
equations)
nit: const & superfluous?

https://codereview.chromium.org/643373003/diff/20001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/643373003/diff/20001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:875: 'extensions': ['GL_NV_blend_equation_advanced']
}],
If we enable the extension for either GL_NV_blend_equation_advanced or
GL_KHR_blend_equation_advanced, we need bindings for both entrypoints (triggered
on each extension).
Also, note that the extension is available for GL2 as well as GLES2, so we
shouldn't limit to gl4.

[Mike West, 2014-10-17 07:56:48]

IPC LGTM.

[Erik Dahlström (inactive), 2014-10-17 15:33:18]

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:718: case SkXfermode::kScreen_Mode:
On 2014/10/16 20:13:28, piman (Very slow to review) wrote:
> nit: indent style (run git cl format?)

Done.

https://codereview.chromium.org/643373003/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1261: GLC(gl_, gl_->BlendBarrierNV());
On 2014/10/16 20:13:29, piman (Very slow to review) wrote:
> Is it worth skipping this if we have GL_*_blend_equation_advanced_coherent?

Perhaps, my current card/driver setup doesn't support the coherent mode so I
can't test if that's better/faster.

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:1557: 'gl_test_func':
'glBlendBarrierNV',
On 2014/10/16 20:13:29, piman (Very slow to review) wrote:
> At the command buffer level, I would prefer that we expose the vendor-neutral
> KHR_blend_equation_advanced, which is a subset but looks enough for what we
use
> in cc. In the back-end, we can then use either KHR_blend_equation_advanced or
> NV_blend_equation_advanced

Agreed, will fix. Note that this also means I'll have to update
third_party/khronos since the current gl2ext.h there only has the NV extension,
not the KHR one.

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:873: GL_HSL_LUMINOSITY_NV
On 2014/10/16 20:13:29, piman (Very slow to review) wrote:
> If we don't support the whole range of enums, we shouldn't claim support for
the
> extension. That's also why I prefer exposing KHR_blend_equation_advanced
instead
> of NV_blend_equation_advanced at the command-buffer level.

Done.

https://codereview.chromium.org/643373003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:876: for (const GLenum& equation :
equations)
On 2014/10/16 20:13:29, piman (Very slow to review) wrote:
> nit: const & superfluous?

Done.

[piman, 2014-10-17 21:36:29]

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

https://codereview.chromium.org/643373003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:882:
AddExtensionString("GL_KHR_blend_equation_advanced_coherent");
nit: indent wrong (should be +2)

https://codereview.chromium.org/643373003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:884:
AddExtensionString("GL_KHR_blend_equation_advanced");
We should add this one either way.

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:874: 'gl_versions': ['es2', 'gl2'],
If you list versions, you have to list explicitly all later versions (i.e. es3,
gl3, gl4 etc.)

I think you don't need to restrict it though. We should be able to assume that
the driver will only expose the extension on a version of the driver that
supports it (either way, we fail early if we don't have es2/gl2).
Just leaving out the gl_versions line should work ok.

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:877: 'gl_versions': ['es2', 'gl2'],
(same here).

[Erik Dahlström (inactive), 2014-10-20 15:32:21]

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

https://codereview.chromium.org/643373003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:882:
AddExtensionString("GL_KHR_blend_equation_advanced_coherent");
On 2014/10/17 21:36:29, piman (Very slow to review) wrote:
> nit: indent wrong (should be +2)

Done.

https://codereview.chromium.org/643373003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/feature_info.cc:884:
AddExtensionString("GL_KHR_blend_equation_advanced");
On 2014/10/17 21:36:29, piman (Very slow to review) wrote:
> We should add this one either way.

Done.

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:874: 'gl_versions': ['es2', 'gl2'],
On 2014/10/17 21:36:29, piman (Very slow to review) wrote:
> If you list versions, you have to list explicitly all later versions (i.e.
es3,
> gl3, gl4 etc.)
> 
> I think you don't need to restrict it though. We should be able to assume that
> the driver will only expose the extension on a version of the driver that
> supports it (either way, we fail early if we don't have es2/gl2).
> Just leaving out the gl_versions line should work ok.

Done.

https://codereview.chromium.org/643373003/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:877: 'gl_versions': ['es2', 'gl2'],
On 2014/10/17 21:36:29, piman (Very slow to review) wrote:
> (same here).

Done.

[piman, 2014-10-21 19:38:19]

lgtm

[Erik Dahlström (inactive), 2014-10-22 10:48:35]

ed@opera.com changed required reviewers:
- enne@chromium.org

[Erik Dahlström (inactive), 2014-10-22 12:03:04]

Thanks for the review.

I'll try to land this as soon as https://codereview.chromium.org/637953007/ has
landed.

[Erik Dahlström (inactive), 2014-10-23 14:07:58]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-10-23 14:08:21]

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

[commit-bot: I haz the power, 2014-10-23 15:01:48]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-23 15:02:31]

Patchset 8 (id:??) landed as
https://crrev.com/9198b422852a9640e47c2851679b0c69be3eac68
Cr-Commit-Position: refs/heads/master@{#300887}