Background filters are affected by masks

Background filters are affected by masks

At first, mask didn't affect on background, if filter was applied on it.
This bug occurred because of loss of information about
original background texture. So I added necessary information to shader
and fixed this bug.

R=danakj@chromium.org, enne@chromium.org

TEST=LayerTreeHostMasksForBackgroundFiltersPixelTest.MaskOfLayerWithBackgroundFilter

Committed: https://crrev.com/c285abaea4ed9dbe8c9dcc285d6d4acc8676cd2f
Cr-Commit-Position: refs/heads/master@{#319323}

[enne (OOO), 2015-03-02 23:34:41]

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

https://codereview.chromium.org/959283004/diff/1/cc/output/shader.h#newcode62
cc/output/shader.h:62: struct ShaderLocations {
This seems like a good refactoring.  Can you split this out into a separate
change, and then we can land that first?

[Lof, 2015-03-03 22:39:24]

rebase

[enne (OOO), 2015-03-03 23:27:47]

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

https://codereview.chromium.org/959283004/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:941: // Reset original background texture if there is
no any mask
grammar nit: no => not

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

https://codereview.chromium.org/959283004/diff/20001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:417: [1];
Shouldn't this be [2]?

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

https://codereview.chromium.org/959283004/diff/20001/cc/output/gl_renderer_un...
cc/output/gl_renderer_unittest.cc:281: [0]);
This is just a style nit, but I'm not super keen on 0 and 1, as it's not clear
to the reader what that's supposed to mean in a bunch of these callsites.  I
know there's only two values, but could you make this explicit with a HAS_MASK /
NO_MASK / LAST_MASK_VALUE enum?

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

https://codereview.chromium.org/959283004/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:797: vec4 original_backdrop =
texture2D(s_originalBackdropTexture, bgTexCoord);
Would it be more precise to call this unfiltered, rather than original? And call
the other one filtered?

What happens if there's a blend mode and a mask, but no background filter? (And
thus no original_backdrop?)

https://codereview.chromium.org/959283004/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:1815: ? SHADER0([]() { gl_FragColor =
ApplyBlendModeWithMask(
Can you make this easier to read and reduce the copy and paste boilerplate
somehow?

Maybe you could use mask_for_background() in the same way that has_blend_mode()
works.  Then you could always pass the maskColor.w to ApplyBlendMode but only
conditionally apply it in the SetBlendModeFunctions helper?

[Lof, 2015-03-04 18:04:23]

I fixed remarks.
Also I wrote test for your test case. It works as expected.
Original backdrop is required only if both filtered background and mask are
presented.
In this case mask_for_background in gl_renderer.cc turns into true.
In all other cases mask_for_background will be false and old code path will be
executed.
So I think that “original” is suitable as variable's name.

[enne (OOO), 2015-03-04 19:58:44]

Thanks so much for the test and all the changes! This is great! I have a few
small style nits (sorry!), but otherwise this looks almost good to go.

https://codereview.chromium.org/959283004/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/959283004/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3060: for (int l = 0; l <= 1; ++l) {
Use the enum here?

https://codereview.chromium.org/959283004/diff/40001/cc/output/program_binding.h
File cc/output/program_binding.h (right):

https://codereview.chromium.org/959283004/diff/40001/cc/output/program_bindin...
cc/output/program_binding.h:63: bool mask_for_background = false) {
style nit: no default parameters in Chromium.

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

https://codereview.chromium.org/959283004/diff/40001/cc/output/shader.cc#newc...
cc/output/shader.cc:783: ? SHADER0([]() {
This is super hard to read.  Can you put the two branches of the ternary
expression into temporaries above this, so that there's not stuff like "); } })
) +" that is a bit hard to parse?

https://codereview.chromium.org/959283004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_masks.cc (right):

https://codereview.chromium.org/959283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_masks.cc:361: // GL_GPU_RASTER_2D_DRAW, Gpu
rasterisation has errors in accuracy
Can you use a fuzzy pixel comparator? How inaccurate is it? If it's related to
the circle edges, you could always consider making it a square, instead?

[Lof, 2015-03-04 22:45:46]

Thank you for comments! I fixed all.

[enne (OOO), 2015-03-04 22:47:57]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-03-04 22:47:57]

lgtm

[enne (OOO), 2015-03-04 22:48:06]

Thanks for this!

[commit-bot: I haz the power, 2015-03-04 22:48:31]

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

[commit-bot: I haz the power, 2015-03-05 03:34:46]

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

[commit-bot: I haz the power, 2015-03-05 03:34:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lof, 2015-03-05 17:28:27]

More robust fuzzy comparator

[enne (OOO), 2015-03-05 20:08:33]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-03-05 20:08:33]

lgtm

[commit-bot: I haz the power, 2015-03-05 20:09:21]

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

[commit-bot: I haz the power, 2015-03-05 20:57:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-05 20:58:30]

Patchset 5 (id:??) landed as
https://crrev.com/c285abaea4ed9dbe8c9dcc285d6d4acc8676cd2f
Cr-Commit-Position: refs/heads/master@{#319323}