Implement mix-blend-mode in GL renderer using shaders.

Implement mix-blend-mode in GL renderer using shaders.

This implementation brings in several improvements for blending:
- it's done faster, due to reducing the number of readbacks.
- the backdrop doesn't suffer any transformation, so the result is more
  correct.
- the results are similar to the ones obtained using the software paths.
- blending is always applied after filters, even when the filters
  can be expressed using a color matrix.

The initial experiment: https://codereview.chromium.org/555133002/.

BUG=243223
Committed: https://crrev.com/7b73f83a1d6c212c3f4bc915f495baf1b4671dbf
Cr-Commit-Position: refs/heads/master@{#300657}

[rosca, 2014-10-14 11:07:56]

rosca@adobe.com changed reviewers:
+ enne@chromium.org

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

ed@opera.com changed reviewers:
+ ed@opera.com

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

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:47: static const SkXfermode::Mode kLastBlendMode =
SkXfermode::kLuminosity_Mode;
why not use SkXfermode::kLastMode?

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:48: static const int kNumBlendModes = kLastBlendMode -
kFirstBlendMode + 2;
Deserves a comment to explain the +2.

[rosca, 2014-10-15 15:33:55]

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:47: static const SkXfermode::Mode kLastBlendMode =
SkXfermode::kLuminosity_Mode;
On 2014/10/14 13:56:07, Erik Dahlström wrote:
> why not use SkXfermode::kLastMode?
I thought kLastMode might not necessarily refer to a color blend mode. However,
I doubt they will ever change, so I can use it.

https://codereview.chromium.org/658483003/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:48: static const int kNumBlendModes = kLastBlendMode -
kFirstBlendMode + 2;
On 2014/10/14 13:56:07, Erik Dahlström wrote:
> Deserves a comment to explain the +2.
I will, thanks.

[enne (OOO), 2014-10-15 19:18:22]

Just so I understand the ordering here, I'm expecting that the unit test patch
is going to land first, before this?

[rosca, 2014-10-15 19:36:47]

On 2014/10/15 19:18:22, enne wrote:
> Just so I understand the ordering here, I'm expecting that the unit test patch
> is going to land first, before this?

If the unit tests land first, we need to create separate expectations for GL and
software rendering for the current implementation.
With this patch, we can have the same expectations for both rendering paths.
It would be easier to submit this patch first and the unit tests after, or just
add the tests to this patch. What do you think?

[enne (OOO), 2014-10-16 01:41:25]

If the unit tests can't come before, I think they'd be better as a part of this
patch, rather than after.

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

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:48: static const int kNumBlendModes = kLastBlendMode -
kFirstBlendMode + 2;
Would it make sense to add these to Skia?

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:73: for (SkXfermode::Mode mode =
kDefaultBlendMode; mode <= kLastBlendMode;) {
This is weird to read.  I would expect loops to go first to last? Can you come
up with some way to not need the weird for loop increment dance below? You have
a blend mode to index mapping.  Maybe you just need the reverse and iterate from
0 to num blend modes and do index_to_blend_mode?

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:280: int bm = index_for_blend_mode(mode);
nit: please don't abbreviate bm; make it blend_mode_index or some such.

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:632: #define UNUSED_BLEND_MODE_UNIFORMS
(is_default_blend_mode() ? 2 : 0)
This doesn't need to be a macro, I think.

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:673: return SHADER0(precision mediump float;)
SHADER0 was intended as a way to make a big block of shader code look more
legible.  I don't know that it makes things better as a one-liner.  Maybe break
up each of these things into separate strings (maybe using Shader0 for the
larger chunk) and then put them together?

[rosca, 2014-10-16 16:14:56]

Adrienne, thanks for the review.
I merged the pixel tests from https://codereview.chromium.org/598593005/ into
this patch.

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

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:48: static const int kNumBlendModes = kLastBlendMode -
kFirstBlendMode + 2;
On 2014/10/16 01:41:25, enne wrote:
> Would it make sense to add these to Skia?
Actually, we don't need coeff blend modes for shaders. I made kFirstBlendMode =
SkXfermode::kLastCoeffMode + 1 and kLastBlendMode = SkXfermode::kLastMode. Given
this, do you think we should add to kFirstBlendMode and kLastBlendMode to Skia?

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:73: for (SkXfermode::Mode mode =
kDefaultBlendMode; mode <= kLastBlendMode;) {
On 2014/10/16 01:41:25, enne wrote:
> This is weird to read.  I would expect loops to go first to last? Can you come
> up with some way to not need the weird for loop increment dance below? You
have
> a blend mode to index mapping.  Maybe you just need the reverse and iterate
from
> 0 to num blend modes and do index_to_blend_mode?

Done.

https://codereview.chromium.org/658483003/diff/20001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:280: int bm = index_for_blend_mode(mode);
On 2014/10/16 01:41:25, enne wrote:
> nit: please don't abbreviate bm; make it blend_mode_index or some such.

Done.

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:632: #define UNUSED_BLEND_MODE_UNIFORMS
(is_default_blend_mode() ? 2 : 0)
On 2014/10/16 01:41:25, enne wrote:
> This doesn't need to be a macro, I think.
It looks weird to see this expression inline. It is tied to what is in
BLEND_MODE_UNIFORMS. Other options would be either to move all of them inline,
or to add a BLEND_MODE_UNIFORMS_COUNT and move UNUSED_BLEND_MODE_UNIFORMS and
BLEND_MODE_SET_LOCATIONS as methods in FragmentTexBlendMode. Wdyt?

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:673: return SHADER0(precision mediump float;)
On 2014/10/16 01:41:25, enne wrote:
> SHADER0 was intended as a way to make a big block of shader code look more
> legible.  I don't know that it makes things better as a one-liner.  Maybe
break
> up each of these things into separate strings (maybe using Shader0 for the
> larger chunk) and then put them together?

I replaced SHADER0 with quotes for one liners. Do you want me to remove it from
other places as well? The other occurrences look helpful to me. Maybe we can
make git cl format to be aware of this macro and skip fixing indentation.

[rosca, 2014-10-16 17:25:18]

rosca@adobe.com changed reviewers:
+ danakj@chromium.org

[rosca, 2014-10-16 17:25:19]

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_filters.cc:140: float
percentage_pixels_large_error = 0.435f;  // 174px / (200*200)
This test requires minor adjustments. I will try it on mac and will see if it
requires a new png.
The change is caused by the backdrop which is 1 pixel wider when aa is on.
Visually, I cannot see any difference on the results, however, 24 more pixels
are different:
http://codepen.io/rosca/full/AEqaG

[danakj, 2014-10-16 17:27:00]

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_filters.cc:140: float
percentage_pixels_large_error = 0.435f;  // 174px / (200*200)
On 2014/10/16 17:25:19, rosca wrote:
> This test requires minor adjustments. I will try it on mac and will see if it
> requires a new png.
> The change is caused by the backdrop which is 1 pixel wider when aa is on.
> Visually, I cannot see any difference on the results, however, 24 more pixels
> are different:
> http://codepen.io/rosca/full/AEqaG

Try linux too please. Generally we take our baselines from there since we work
there most of the time.

[rosca, 2014-10-17 13:55:55]

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/658483003/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:673: return SHADER0(precision mediump float;)
On 2014/10/16 01:41:25, enne wrote:
> SHADER0 was intended as a way to make a big block of shader code look more
> legible.
I removed SHADER0 from small blocks of shader code and run cl format on big
blocks.

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/658483003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_filters.cc:140: float
percentage_pixels_large_error = 0.435f;  // 174px / (200*200)
On 2014/10/16 17:27:00, danakj wrote:
> Try linux too please. Generally we take our baselines from there since we work
> there most of the time.

After regenerating pixels from linux, the test passes on windows without my
changes, so I could revert this file.

[enne (OOO), 2014-10-17 19:16:16]

This looks good in general to me.  Thanks for adding the force antialiasing
flag.

https://codereview.chromium.org/658483003/diff/100001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/658483003/diff/100001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:62: static_assert(kNumBlendModes == 15,
It's definitely good to document these assumptions, but it's not clear to me how
to not to make this a giant pain to folks rolling in changes to Skia.  Adding a
new xfermode seems like it shouldn't break the roll or require dependent patches
that land simultaneously.  That's kind of why I was asking if this should be put
into Skia.  (Also, somebody could remove and add a new-but-unsupported xfermode
and not trigger these asserts...)

Who were you working with on the Skia side about blend modes? Can you CC them
here and get their input?

One option that exchanges efficiency for easier rolling would just be a function
with a switch statement that turned Skia xfermodes that had supported shader
versions into some index (or some new cc-side enum).  Then new xfer modes would
be unsupported by default without breaking anything, and only the removal of an
xfermode would cause a problem.

[rosca, 2014-10-20 10:02:58]

rosca@adobe.com changed reviewers:
+ bsalomon@chromium.org

[rosca, 2014-10-20 12:07:37]

https://codereview.chromium.org/658483003/diff/100001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/658483003/diff/100001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:62: static_assert(kNumBlendModes == 15,
On 2014/10/17 19:16:15, enne wrote:
> It's definitely good to document these assumptions, but it's not clear to me
how
> to not to make this a giant pain to folks rolling in changes to Skia.  Adding
a
> new xfermode seems like it shouldn't break the roll or require dependent
patches
> that land simultaneously.  That's kind of why I was asking if this should be
put
> into Skia.  (Also, somebody could remove and add a new-but-unsupported
xfermode
> and not trigger these asserts...)
> 
> Who were you working with on the Skia side about blend modes? Can you CC them
> here and get their input?
> 
> One option that exchanges efficiency for easier rolling would just be a
function
> with a switch statement that turned Skia xfermodes that had supported shader
> versions into some index (or some new cc-side enum).  Then new xfer modes
would
> be unsupported by default without breaking anything, and only the removal of
an
> xfermode would cause a problem.

I added a shader specific BlendModes enum and a conversion function with a
switch statement.

bsalomon1, could you give your input on this?

[rosca, 2014-10-21 13:28:22]

rosca@adobe.com changed reviewers:
+ reed@google.com

[rosca, 2014-10-21 13:48:13]

reed, bsalomon: please take a look at how we use Xfermode modes in this patch
wrt integrating new skia rolls in Chromium (see comment #14 & #16).

[reed1, 2014-10-21 13:55:59]

On 2014/10/21 13:48:13, rosca wrote:
> reed, bsalomon: please take a look at how we use Xfermode modes in this patch
> wrt integrating new skia rolls in Chromium (see comment #14 & #16).

The separate enum and helper-conversion functions sgtm.

[enne (OOO), 2014-10-21 18:13:30]

lgtm, thanks!

https://codereview.chromium.org/658483003/diff/120001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/658483003/diff/120001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:88: default:
style nit: Don't use default, since all the enum values need to be handled. 
That way when somebody if/when somebody adds a new enum the compiler will
complain that this switch statement needs to be updated.  It does mean that you
need to add a case for NumBlendModes, but you can NOTREACHED that one.

https://codereview.chromium.org/658483003/diff/120001/cc/output/shader.h
File cc/output/shader.h (right):

https://codereview.chromium.org/658483003/diff/120001/cc/output/shader.h#newc...
cc/output/shader.h:42: BlendModeNormal = 0,
No need to assign values here.

[rosca, 2014-10-21 20:32:32]

Thanks!

https://codereview.chromium.org/658483003/diff/120001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/658483003/diff/120001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:88: default:
On 2014/10/21 18:13:30, enne wrote:
> style nit: Don't use default, since all the enum values need to be handled. 
> That way when somebody if/when somebody adds a new enum the compiler will
> complain that this switch statement needs to be updated.  It does mean that
you
> need to add a case for NumBlendModes, but you can NOTREACHED that one.

Done.

https://codereview.chromium.org/658483003/diff/120001/cc/output/shader.h
File cc/output/shader.h (right):

https://codereview.chromium.org/658483003/diff/120001/cc/output/shader.h#newc...
cc/output/shader.h:42: BlendModeNormal = 0,
On 2014/10/21 18:13:30, enne wrote:
> No need to assign values here.

Done.

[rosca, 2014-10-22 06:06:29]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-10-22 06:07:40]

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

[commit-bot: I haz the power, 2014-10-22 07:13:05]

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

[commit-bot: I haz the power, 2014-10-22 07:13:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rosca, 2014-10-22 07:36:40]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-10-22 07:37:39]

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

[rosca, 2014-10-22 07:55:13]

The CQ bit was unchecked by rosca@adobe.com

[rosca, 2014-10-22 08:54:26]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-10-22 08:55:07]

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

[commit-bot: I haz the power, 2014-10-22 09:54:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-10-22 09:56:17]

Patchset 9 (id:??) landed as
https://crrev.com/7b73f83a1d6c212c3f4bc915f495baf1b4671dbf
Cr-Commit-Position: refs/heads/master@{#300657}

[enne (OOO), 2014-10-22 19:00:19]

https://codereview.chromium.org/658483003/diff/160001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/658483003/diff/160001/cc/output/shader.cc#new...
cc/output/shader.cc:712: );
Did the presubmit linter not yell at you for about whitespace and parentheses?

[rosca, 2014-10-22 19:35:02]

https://codereview.chromium.org/658483003/diff/160001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/658483003/diff/160001/cc/output/shader.cc#new...
cc/output/shader.cc:712: );
On 2014/10/22 19:00:18, enne wrote:
> Did the presubmit linter not yell at you for about whitespace and parentheses?

Seemingly not. I should have run it locally. Thanks for fixing it.