Adding support for blending in cc::SoftwareRenderer

Adding support for blending in cc::SoftwareRenderer

GLRenderer already can handle blend modes, so it should be handled for
SoftwareRenderer too.
Blending layers get their own rendering surface for the time being,
so blending should be applied only on render pass quads.

BUG=314865
Committed: https://crrev.com/50f0dca8adabc08a73d2e534085525ed301c8704
Cr-Commit-Position: refs/heads/master@{#293972}

[rosca, 2014-09-04 08:43:19]

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

[rosca, 2014-09-04 08:43:20]

Hi Adrienne, can you review this patch?

[Erik Dahlström (inactive), 2014-09-05 09:19:22]

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

[Erik Dahlström (inactive), 2014-09-05 09:19:23]

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
It's a little confusing that xfermode is set in DoDrawQuad above too, with this
overriding it (could lead to bugs if code is moved around).

[rosca, 2014-09-05 09:35:31]

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
On 2014/09/05 09:19:23, Erik Dahlström wrote:
> It's a little confusing that xfermode is set in DoDrawQuad above too, with
this
> overriding it (could lead to bugs if code is moved around).

Indeed, DoDrawQuad would be the right place where we should set the xfermode,
but for now we should only apply blend mode for render passes. Blend mode is
also set on the owning layer of the render pass and we should not apply it
there.
I will move it to DoDrawQuad, but I should remove blend modes for the quads of
the owning layers first.

[enne (OOO), 2014-09-05 17:49:08]

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
On 2014/09/05 at 09:35:31, Rosca wrote:
> On 2014/09/05 09:19:23, Erik Dahlström wrote:
> > It's a little confusing that xfermode is set in DoDrawQuad above too, with
this
> > overriding it (could lead to bugs if code is moved around).
> 
> Indeed, DoDrawQuad would be the right place where we should set the xfermode,
but for now we should only apply blend mode for render passes. Blend mode is
also set on the owning layer of the render pass and we should not apply it
there.
> I will move it to DoDrawQuad, but I should remove blend modes for the quads of
the owning layers first.

It seems like maybe the owning layer shouldn't have blend_mode set for its
shared quad state, then.

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_blending.cc:28: class
FuzzyPixelOffByOneComparator : public FuzzyPixelComparator {
I feel like this is duplicated elsewhere.  Can you move this to some common
location?

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_blending.cc:54: if (type ==
SOFTWARE_WITH_BITMAP)
I'd be ok with making all the blending tests use an off-by-one comparator.  It's
not clear to me that the mesa version is right and the software renderer version
is wrong.

[rosca, 2014-09-05 20:52:28]

Thanks for the review:

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
On 2014/09/05 17:49:08, enne wrote:
> On 2014/09/05 at 09:35:31, Rosca wrote:
> > On 2014/09/05 09:19:23, Erik Dahlström wrote:
> > > It's a little confusing that xfermode is set in DoDrawQuad above too, with
> this
> > > overriding it (could lead to bugs if code is moved around).
> > 
> > Indeed, DoDrawQuad would be the right place where we should set the
xfermode,
> but for now we should only apply blend mode for render passes. Blend mode is
> also set on the owning layer of the render pass and we should not apply it
> there.
> > I will move it to DoDrawQuad, but I should remove blend modes for the quads
of
> the owning layers first.
> 
> It seems like maybe the owning layer shouldn't have blend_mode set for its
> shared quad state, then.

I'll do it with another patch, including some unit tests. I think it's suitable
to add a blend_mode field in draw_properties, which could be different from
LayerType::blend_mode.

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_blending.cc:28: class
FuzzyPixelOffByOneComparator : public FuzzyPixelComparator {
On 2014/09/05 17:49:08, enne wrote:
> I feel like this is duplicated elsewhere.  Can you move this to some common
> location?

Done.

https://codereview.chromium.org/532003003/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_blending.cc:54: if (type ==
SOFTWARE_WITH_BITMAP)
On 2014/09/05 17:49:08, enne wrote:
> I'd be ok with making all the blending tests use an off-by-one comparator. 
It's
> not clear to me that the mesa version is right and the software renderer
version
> is wrong.

Done.

[enne (OOO), 2014-09-05 21:03:04]

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
On 2014/09/05 20:52:27, Rosca wrote:
> On 2014/09/05 17:49:08, enne wrote:
> > On 2014/09/05 at 09:35:31, Rosca wrote:
> > > On 2014/09/05 09:19:23, Erik Dahlström wrote:
> > > > It's a little confusing that xfermode is set in DoDrawQuad above too,
with
> > this
> > > > overriding it (could lead to bugs if code is moved around).
> > > 
> > > Indeed, DoDrawQuad would be the right place where we should set the
> xfermode,
> > but for now we should only apply blend mode for render passes. Blend mode is
> > also set on the owning layer of the render pass and we should not apply it
> > there.
> > > I will move it to DoDrawQuad, but I should remove blend modes for the
quads
> of
> > the owning layers first.
> > 
> > It seems like maybe the owning layer shouldn't have blend_mode set for its
> > shared quad state, then.
> 
> I'll do it with another patch, including some unit tests. I think it's
suitable
> to add a blend_mode field in draw_properties, which could be different from
> LayerType::blend_mode.

DrawProperties doesn't seem like the right place for this.  Those are all
computed properties based on tree hierarchies.

Can you help me understand why the blend mode on SharedQuadState is not
sufficient? The owning layer's SharedQuadState for drawing as itself and the
render pass's SharedQuadState for representing a contributing render surfaces
when it draws are not the same.  Could you not just set blend mode properly on
the right SharedQuadStates and then have DoDrawQuad always read it and set it
from there?

[enne (OOO), 2014-09-05 23:37:52]

enne@chromium.org changed reviewers:
+ danakj@chromium.org
- enne@chromium.org

[enne (OOO), 2014-09-05 23:38:17]

(I'm out for two weeks, so am CCing danakj to finish reviewing this.)

[rosca, 2014-09-06 00:25:47]

Thanks @enne for letting me know that you'll be out, and hi @danakj!

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/532003003/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:514:
current_paint_.setXfermodeMode(quad->shared_quad_state->blend_mode);
On 2014/09/05 21:03:04, enne_OOO_until_sep_22 wrote:
> On 2014/09/05 20:52:27, Rosca wrote:
> > On 2014/09/05 17:49:08, enne wrote:
> > > On 2014/09/05 at 09:35:31, Rosca wrote:
> > > > On 2014/09/05 09:19:23, Erik Dahlström wrote:
> > > > > It's a little confusing that xfermode is set in DoDrawQuad above too,
> with
> > > this
> > > > > overriding it (could lead to bugs if code is moved around).
> > > > 
> > > > Indeed, DoDrawQuad would be the right place where we should set the
> > xfermode,
> > > but for now we should only apply blend mode for render passes. Blend mode
is
> > > also set on the owning layer of the render pass and we should not apply it
> > > there.
> > > > I will move it to DoDrawQuad, but I should remove blend modes for the
> quads
> > of
> > > the owning layers first.
> > > 
> > > It seems like maybe the owning layer shouldn't have blend_mode set for its
> > > shared quad state, then.
> > 
> > I'll do it with another patch, including some unit tests. I think it's
> suitable
> > to add a blend_mode field in draw_properties, which could be different from
> > LayerType::blend_mode.
> 
> DrawProperties doesn't seem like the right place for this.  Those are all
> computed properties based on tree hierarchies.
> 
> Can you help me understand why the blend mode on SharedQuadState is not
> sufficient? The owning layer's SharedQuadState for drawing as itself and the
> render pass's SharedQuadState for representing a contributing render surfaces
> when it draws are not the same.  Could you not just set blend mode properly on
> the right SharedQuadStates and then have DoDrawQuad always read it and set it
> from there?

Now, all the blending layers get their own render surfaces, so we need to set
blend_mode only on the render pass's SharedQuadState(SQS). What happens in the
code is that we set blend_mode on render surfaces' SQS and on each of the
layers' SQS, even if the latter is not used at the moment.
I think that we should keep setting blend mode on layer's SQS, as we are going
to support blending for layers without separate rendering surfaces. The only
case where we should avoid setting blend_mode in layer's SQS is when the layer
has its own rendering surface which will become responsible for blending the
entire subtree as a group.

In this context blend_mode looks like a property based on tree hierarchy, this
is why I was thinking off adding it to draw_properties. However, I can compute
the blend mode for layers right before calling shared_quad_state->SetAll(), by
looking for the rendering surface existence. Do you think this would be better?

I uploaded my changes with draw_properties here:
https://codereview.chromium.org/549683002.

[danakj, 2014-09-09 16:04:39]

LGTM

[danakj, 2014-09-09 16:04:45]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-09 16:07:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rosca@adobe.com/532003003/20001

[commit-bot: I haz the power, 2014-09-09 18:37:57]

Committed patchset #2 (id:20001) as 54383d1eb6e24b6bb68a630d44d16181c4bed18e

[commit-bot: I haz the power, 2014-09-10 03:54:25]

Patchset 2 (id:??) landed as
https://crrev.com/50f0dca8adabc08a73d2e534085525ed301c8704
Cr-Commit-Position: refs/heads/master@{#293972}