Issue 23455060: mix-blend-mode implementation for accelerated layers

rosca

This is the compositor part of the mix-blend-mode implementation. Please review it.

7 years, 3 months ago (2013-09-24 16:20:21 UTC) #1

rosca

On 2013/09/24 16:24:29, danakj wrote: > Where are the tests? I uploaded this patch as ...

7 years, 3 months ago (2013-09-24 16:52:19 UTC) #3

enne (OOO)

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc#newcode589 cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group()) { Whoa there. According to http://dev.w3.org/fxtf/compositing-1/#csscompositingrules_CSS, an ...

7 years, 3 months ago (2013-09-24 18:05:48 UTC) #4

rosca

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc#newcode589 cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group()) { On 2013/09/24 18:05:48, enne wrote: > ...

7 years, 2 months ago (2013-09-24 21:22:42 UTC) #5

shawnsingh

There are two things i'm concerned about - scalability concerns because of our reliance on ...

7 years, 2 months ago (2013-09-25 05:33:52 UTC) #6

There are two things i'm concerned about - scalability concerns because of our
reliance on render surfaces for implementing this, and the gpu copyTexImage2d
usage.  Especially that second one - as I understand copyTexImage2d, we may not
have guarantees that it will be lightweight enough on all drivers/platforms, and
that is a deal-breaker for the requirements of the compositor impl thread.

Is there any place where the various design options has already been discussed?
I would be interested to hear what options have been considered, and whether
there might be other ways to do this.  It would be nice to hear your
optimization ideas too.

I think now is the time to have that conversation on graphics-dev.

More comments below. =)

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

https://codereview.chromium.org/23455060/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:2609: context_->copyTexSubImage2D(GL_TEXTURE_2D,
Will this be certainly fast on all drivers/hardware?  If not, this could be a
deal breaker since this part of the rendering code needs to be predictable and
fast as much as possible.

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

https://codereview.chromium.org/23455060/diff/1/cc/output/gl_renderer.h#newco...
cc/output/gl_renderer.h:164: bool& background_changed);
We should not use non-const references.  Use a pointer, which is chrome's
convention to clearly show that this is a non-const out argument.

https://codereview.chromium.org/23455060/diff/1/cc/quads/render_pass_draw_quad.h
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/23455060/diff/1/cc/quads/render_pass_draw_qua...
cc/quads/render_pass_draw_quad.h:33: SkXfermode::Mode blend_mode =
SkXfermode::kSrcOver_Mode);
would it make more sense to put this in the SharedQuadState instead?  It won't
change for each quad within one layer.

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:580: if (layer->has_blend_mode()) {
Can we support blend modes for layers when possible, instead of needing to
create a surface?  Ideally we should add "use num_descendants_that_draw_content
> 0" as an additional condition here.  I would expect it may be a common case
that only one leaf node composited layer has a blend mode with no descendants,
and in that case we wouldn't need to create a render surface for it.

Will this greatly affect how the blend modes is actually implemented in the
gl_renderer code?

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group())
{
Enne@ - I disagree about using setForceRenderSurface().  Why should we require
this particular feature to use setForceRenderSurface() while every other reason
has a legitimate condition in this helper function?   The whole purpose of this
helper function is to make an explicit and centralized location to maintain all
the various reasons that we would create a surface.  I feel that
setForceRenderSurface() should remain a debugging/testing tool, and not be used
as a way to bypass putting conditions directly in this function.

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:1535:
render_surface->SetHasBlendMode(layer->has_blend_mode());
FYI, if we do add support for blend modes on layers (as opposed to only for
render surfaces), then this line of code will be dangerous.   We would need to
distinguish between (1) a layer that has its own surface and gives the
blend-mode to the surface, versus (2) a layer that has a blend mode and draws
into a non-blend-mode surface.

rosca

> Is there any place where the various design options has already been discussed? > ...

7 years, 2 months ago (2013-09-25 17:59:59 UTC) #7

> Is there any place where the various design options has already been
discussed?
> I would be interested to hear what options have been considered, and whether
> there might be other ways to do this.  It would be nice to hear your
> optimization ideas too.
> 
> I think now is the time to have that conversation on graphics-dev.

I started a conversation on graphics-dev before working on it:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/_Rc2Cv-ODq4....
I was asking mainly if Skia with its hardware back-end would be a viable option
for implementing blending in the compositor. The alternative was to use pixel
programs implementing blending formulas. Both these options implied copying the
backdrop to another texture to have it as an input for either SkBitmap or for
pixel programs.

Regarding optimizations, I think we can minimize the number of surfaces we
create to keep mix-blend-mode working correctly:
1) If the blending layer is a leaf in the layer tree, it will not require a
separate surface, as it's already isolated. This needs to implement blending for
regular texture too (now we have it only for rendering surfaces).
2) If (1) all the blending layers within a stacking context (isolated group) are
fully contained inside the root layer of that stacking context (no one
overflows) AND (2) the stacking context is fully opaque, then a separate surface
is not required (as blending layers don't have access any color information
outside their stacking context anyways).

I think in the most common use case - with a background picture to blend with,
and one or more blending child elements inside - no additional rendering surface
should be created.

Another optimization that has to be done is for copyTexSubImage2D.
https://bugs.webkit.org/show_bug.cgi?id=80870 is about the same thing and it
appears to be fixed. I'm going to look into it and see how should we fix the
mix-blend-mode case. It would be very helpful if you could provide any pointers
here.

Thanks,
Ion.

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

https://codereview.chromium.org/23455060/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:2609: context_->copyTexSubImage2D(GL_TEXTURE_2D,
On 2013/09/25 05:33:53, shawnsingh wrote:
> Will this be certainly fast on all drivers/hardware?  If not, this could be a
> deal breaker since this part of the rendering code needs to be predictable and
> fast as much as possible.

Are you referring to the copyTexSubImage2D function or to copying pixels from
one texture to another in general?
Afaik there are performance concerns regarding copyTexSubImage2D on some mobile
GPUs (https://bugs.webkit.org/show_bug.cgi?id=80870). I should dig into how this
has been worked around and implement GetFramebufferTexture in another, more
reliable way.

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

https://codereview.chromium.org/23455060/diff/1/cc/output/gl_renderer.h#newco...
cc/output/gl_renderer.h:164: bool& background_changed);
On 2013/09/25 05:33:53, shawnsingh wrote:
> We should not use non-const references.  Use a pointer, which is chrome's
> convention to clearly show that this is a non-const out argument.

Done.

https://codereview.chromium.org/23455060/diff/1/cc/quads/render_pass_draw_quad.h
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/23455060/diff/1/cc/quads/render_pass_draw_qua...
cc/quads/render_pass_draw_quad.h:33: SkXfermode::Mode blend_mode =
SkXfermode::kSrcOver_Mode);
On 2013/09/25 05:33:53, shawnsingh wrote:
> would it make more sense to put this in the SharedQuadState instead?  It won't
> change for each quad within one layer.

Done.

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:580: if (layer->has_blend_mode()) {
On 2013/09/25 05:33:53, shawnsingh wrote:
> Can we support blend modes for layers when possible, instead of needing to
> create a surface?  Ideally we should add "use
num_descendants_that_draw_content
> > 0" as an additional condition here.  I would expect it may be a common case
> that only one leaf node composited layer has a blend mode with no descendants,
> and in that case we wouldn't need to create a render surface for it.
This is one of the optimizations I was thinking of. I added a comment in here to
make things clear.

> 
> Will this greatly affect how the blend modes is actually implemented in the
> gl_renderer code?

Now blending code in gl_renderer is ready only for RenderPassQuad. The same idea
should work for other quad types and this has to be implemented.
Creating surfaces for each blending layer and their stacking contexts makes
mix-blend-mode work well with this patch. As the css property is under the
experimental runtime flag, I would prefer to move forward this way first and add
improvements afterwards. What do you think of this?

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:1535:
render_surface->SetHasBlendMode(layer->has_blend_mode());
On 2013/09/25 05:33:53, shawnsingh wrote:
> FYI, if we do add support for blend modes on layers (as opposed to only for
> render surfaces), then this line of code will be dangerous.   We would need to
> distinguish between (1) a layer that has its own surface and gives the
> blend-mode to the surface, versus (2) a layer that has a blend mode and draws
> into a non-blend-mode surface.

I removed the has_blend_mode property from RenderSurface. It will ask
render_surface->owning_layer for the blend_mode property.

shawnsingh

Forgive me for the dumb question about shared contexts and the gles api... could we ...

7 years, 2 months ago (2013-09-25 23:55:30 UTC) #8

rosca

On 2013/09/25 23:55:30, shawnsingh wrote: > Forgive me for the dumb question about shared contexts ...

7 years, 2 months ago (2013-09-26 09:59:22 UTC) #9

enne (OOO)

On 2013/09/25 17:59:59, Rosca wrote: > I started a conversation on graphics-dev before working on ...

7 years, 2 months ago (2013-09-26 16:25:12 UTC) #10

On 2013/09/25 17:59:59, Rosca wrote:

> I started a conversation on graphics-dev before working on it:
>
https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/_Rc2Cv-OD....
> I was asking mainly if Skia with its hardware back-end would be a viable
option
> for implementing blending in the compositor. The alternative was to use pixel
> programs implementing blending formulas. Both these options implied copying
the
> backdrop to another texture to have it as an input for either SkBitmap or for
> pixel programs.

I think background filters are going to be slow, but also a lot less complexity
in the compositor.  Implementing blending formulas in pixel shaders is going to
be a giant pain without a rewrite of how we generate shaders, since it will
cause an explosion of our already overflowing quantity of shaders.

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group())
{
On 2013/09/25 05:33:53, shawnsingh wrote:
> Enne@ - I disagree about using setForceRenderSurface().  Why should we require
> this particular feature to use setForceRenderSurface() while every other
reason
> has a legitimate condition in this helper function?   The whole purpose of
this
> helper function is to make an explicit and centralized location to maintain
all
> the various reasons that we would create a surface.  I feel that
> setForceRenderSurface() should remain a debugging/testing tool, and not be
used
> as a way to bypass putting conditions directly in this function.

I think there's sort of two approaches you can take here.  One is to make the
compositor not be tightly coupled to Blink features, and have Blink do the
translation from its needs to existing already exposed compositor features,
reusing them where it can.  That's what I was trying to suggest above.  This
keeps the compositor more dumb and pushes complication out to the embedder.

I could buy the argument that maybe Blink shouldn't have to think about render
surfaces, so maybe we should expose blend mode semantics.

One improvement to this code would be to have the pre-recursion count whether a
given layer has mixed blend mode descendants and only create surfaces for those
cases rather than always creating one for isolated groups.

https://codereview.chromium.org/23455060/diff/11001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23455060/diff/11001/cc/layers/layer.h#newcode127
cc/layers/layer.h:127: bool has_blend_mode() const {
bikeshed: has_blend_mode is a little bit ambiguous.  Layers always have a blend
mode.  Maybe has_blend_mode => uses_default_blend_mode()?

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty()
&&
How does this TODO not apply to this path as well? Can you add a test for blend
modes on a layer whose target is a non-root (i.e. transparent) render surface?

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:784: GetFramebufferTextureSubImage(lock.texture_id(),
This new function seems unnecessary if you are always passing gfx::Point() to
it.

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:890: background_texture =
CreateBackgroundTextureWithFilters(
This function seems incorrectly named at this point.

https://codereview.chromium.org/23455060/diff/11001/cc/quads/shared_quad_stat...
File cc/quads/shared_quad_state.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/quads/shared_quad_stat...
cc/quads/shared_quad_state.cc:59: value->SetInteger("blend_mode", blend_mode);
This should probably be a string to make it readable.

rosca

I uploaded a new patch with the changes you've suggested. I'm going to add some ...

7 years, 2 months ago (2013-09-27 10:39:05 UTC) #11

I uploaded a new patch with the changes you've suggested. I'm going to add some
tests in the next few days.
About the implementation - is it ok to move forward with this patch or should I
add first some of the improvements/optimizations we've been talking about? I
would prefer to submit it without improvements, so that other people could try
and test the feature.

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group())
{
On 2013/09/26 16:25:13, enne wrote:
> One improvement to this code would be to have the pre-recursion count whether
a
> given layer has mixed blend mode descendants and only create surfaces for
those
> cases rather than always creating one for isolated groups.

I added a TODO comment with this improvement in the fallowing patch. Thanks.

https://codereview.chromium.org/23455060/diff/11001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23455060/diff/11001/cc/layers/layer.h#newcode127
cc/layers/layer.h:127: bool has_blend_mode() const {
On 2013/09/26 16:25:13, enne wrote:
> bikeshed: has_blend_mode is a little bit ambiguous.  Layers always have a
blend
> mode.  Maybe has_blend_mode => uses_default_blend_mode()?

Done.

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty()
&&
On 2013/09/26 16:25:13, enne wrote:
> How does this TODO not apply to this path as well? Can you add a test for
blend
> modes on a layer whose target is a non-root (i.e. transparent) render surface?
Blend modes should work with transparent pixels. Blending any color with a
transparent pixel will not affect the initial color. I will add a test for this
case. Thanks.

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:784: GetFramebufferTextureSubImage(lock.texture_id(),
On 2013/09/26 16:25:13, enne wrote:
> This new function seems unnecessary if you are always passing gfx::Point() to
> it.

The reason I added this function besides GetFramebufferTexture was to be able to
copy RGBA content. The resource provider allocate rgba textures using
testStorage2DEXT, which has immutable internal format, and
GetFramebufferTexture(copyTexImage2D) tries to change the the internal format,
so it's going to fail. GetFramebufferTextureSubImage(copyTexSubImage2D) seams to
work as long as it just copies the pixels without touching the internal format.
Please correct me if I'm wrong. Actually, this was discussed in this thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/_Rc2Cv-ODq4/5CojB...
Though, I removed the offset parameter as it's not used yet.

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:890: background_texture =
CreateBackgroundTextureWithFilters(
On 2013/09/26 16:25:13, enne wrote:
> This function seems incorrectly named at this point.

I changed it to GetBackgroundWithFilters, would this work?

https://codereview.chromium.org/23455060/diff/11001/cc/quads/shared_quad_stat...
File cc/quads/shared_quad_state.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/quads/shared_quad_stat...
cc/quads/shared_quad_state.cc:59: value->SetInteger("blend_mode", blend_mode);
On 2013/09/26 16:25:13, enne wrote:
> This should probably be a string to make it readable.

Done.

enne (OOO)

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc#newcode589 cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group()) { On 2013/09/27 10:39:06, Rosca wrote: > ...

7 years, 2 months ago (2013-09-27 17:42:33 UTC) #12

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group())
{
On 2013/09/27 10:39:06, Rosca wrote:
> On 2013/09/26 16:25:13, enne wrote:
> > One improvement to this code would be to have the pre-recursion count
whether
> a
> > given layer has mixed blend mode descendants and only create surfaces for
> those
> > cases rather than always creating one for isolated groups.
> 
> I added a TODO comment with this improvement in the fallowing patch. Thanks.

Can you file a bug, assign it to yourself, and reference it in the comment?

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty()
&&
On 2013/09/27 10:39:06, Rosca wrote:
> On 2013/09/26 16:25:13, enne wrote:
> > How does this TODO not apply to this path as well? Can you add a test for
> blend
> > modes on a layer whose target is a non-root (i.e. transparent) render
surface?
> Blend modes should work with transparent pixels. Blending any color with a
> transparent pixel will not affect the initial color. I will add a test for
this
> case. Thanks.

Ah, I think there may be some confusion here.  Here's a potential layer tree:

-G (draws green rectangle)
 -S (creates surface)
  -B (some blend mode that uses the background)

G draws into the default root render target.  If you apply the background filter
before drawing B, the current render target is S, which is transparent and has
nothing drawn into it because G drew into the root and not into S.  To blend
correctly, I think B needs to blend with G, yes?

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:890: background_texture =
CreateBackgroundTextureWithFilters(
On 2013/09/27 10:39:06, Rosca wrote:
> On 2013/09/26 16:25:13, enne wrote:
> > This function seems incorrectly named at this point.
> 
> I changed it to GetBackgroundWithFilters, would this work?

It's the "filters" part that seems like the misnomer to me.

rosca

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_common.cc#newcode589 cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group()) { > > I added a TODO ...

7 years, 2 months ago (2013-09-30 17:28:36 UTC) #13

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:589: if (layer->is_root_for_isolated_group())
{
> > I added a TODO comment with this improvement in the fallowing patch. Thanks.
> 
> Can you file a bug, assign it to yourself, and reference it in the comment?
Here is the bug: http://crbug.com/301738. Unfortunately I cannot assign it, I
don't have the rights. Can you assign it to me?

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty()
&&
On 2013/09/27 17:42:33, enne wrote:
> Ah, I think there may be some confusion here.  Here's a potential layer tree:
> 
> -G (draws green rectangle)
>  -S (creates surface)
>   -B (some blend mode that uses the background)
> 
> G draws into the default root render target.  If you apply the background
filter
> before drawing B, the current render target is S, which is transparent and has
> nothing drawn into it because G drew into the root and not into S.  To blend
> correctly, I think B needs to blend with G, yes?

According to the spec, an element having a blend mode must blend with all the
underlying content of the stacking context that element belongs to. So, for B in
this case, the underlying content is just a transparent layer, and blending with
a transparent layer will have no effect on the result. Just to make sure I got
it right, I made this codepen: http://codepen.io/anon/pen/glCqo.

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:890: background_texture =
CreateBackgroundTextureWithFilters(
On 2013/09/27 17:42:33, enne wrote:
> On 2013/09/27 10:39:06, Rosca wrote:
> > On 2013/09/26 16:25:13, enne wrote:
> > > This function seems incorrectly named at this point.
> > 
> > I changed it to GetBackgroundWithFilters, would this work?
> 
> It's the "filters" part that seems like the misnomer to me.

Do you mean that I should make it something like
"GetBackgroundWithBackgroundFilters"? I think it's obvious that filters are
those, applied to the background. What would be your suggestion?

enne (OOO)

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#newcode759 cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty() && On 2013/09/30 17:28:36, Rosca ...

7 years, 2 months ago (2013-09-30 21:26:52 UTC) #14

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/11001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:759: bool apply_background_filters = !filters.IsEmpty()
&&
On 2013/09/30 17:28:36, Rosca wrote:
> On 2013/09/27 17:42:33, enne wrote:
> > Ah, I think there may be some confusion here.  Here's a potential layer
tree:
> > 
> > -G (draws green rectangle)
> >  -S (creates surface)
> >   -B (some blend mode that uses the background)
> > 
> > G draws into the default root render target.  If you apply the background
> filter
> > before drawing B, the current render target is S, which is transparent and
has
> > nothing drawn into it because G drew into the root and not into S.  To blend
> > correctly, I think B needs to blend with G, yes?
> 
> According to the spec, an element having a blend mode must blend with all the
> underlying content of the stacking context that element belongs to. So, for B
in
> this case, the underlying content is just a transparent layer, and blending
with
> a transparent layer will have no effect on the result. Just to make sure I got
> it right, I made this codepen: http://codepen.io/anon/pen/glCqo.

I think that is true only if S is also a stacking context.  What if G is the
parent stacking context for B?  Not every layer or surface in the compositor
corresponds to a stacking context in Blink.  I think your example would blend
incorrectly when S is not a stacking context but G and B are.

(Also, in your example, .surface only creates a layer and not a surface.  If you
turn on composited layer borders, orange is the color for a layer whereas purple
is the color for a surface.)

In your example, the div for B is a layer with a stacking context (because it
has a blend mode) and by the blending spec it must blend with everything in its
parent stacking context.  G is a non-composited child of the root layer, which
is a stacking context here, so that seems fine.  This all seems good.

However, to be a proper example, S needs to be changed to be promoted to being a
layer without being a stacking context *and* creating a surface while not
creating a stacking context.  Most CSS properties that do either of these things
are also stacking contexts (for good reason), but I don't think it's correct to
assume that all of them do (or will in the future).  It's also not correct to
assume that the compositor will only create surfaces for stacking contexts as
well.

Here are some suggestions off the top of my head to modify your example,
although my confidence level as to whether these are stacking contexts is not
100%:

Two ways you could make S be promoted to a composited layer without being a
stacking context:
1) CompositingReasonOverlap: Have S draw some content and have it overlap some
other layer below it.
2) CompositingReasonBackfaceVisibilityHidden: Give S the backface-visibility:
hidden style.

Two ways you could create a surface without being a stacking context:
1) Give S a parent that has preserves-3d, give S no transform-style, and give S
two layer children that both draw content.
2) Give S a parent with a transform that does not preserve axis alignment
(rotation/perspective) and give S some clip.

shawnsingh

I agree with Enne's concern, but actually I think we're just barely OK... It seems ...

7 years, 2 months ago (2013-09-30 22:15:10 UTC) #15

shawnsingh

I have to take back what I just said =) Enne pointed out to me ...

7 years, 2 months ago (2013-09-30 22:29:46 UTC) #16

rosca

> Two ways you could create a surface without being a stacking context: > 1) ...

7 years, 2 months ago (2013-10-02 22:10:38 UTC) #17

> Two ways you could create a surface without being a stacking context:
> 1) Give S a parent that has preserves-3d, give S no transform-style, and give
S
> two layer children that both draw content.
> 2) Give S a parent with a transform that does not preserve axis alignment
> (rotation/perspective) and give S some clip.
I tried the cases Enne has suggested and he is right about surfaces created
without being stacking contexts. But, these cases doesn't seem to be a problem
for blending, or at least. This is because blending forces the stacking context
layer to create a render surface too. Let's take the examples where surfaces
should be created without being stacking contexts:

*1) Give S a parent that has preserves-3d, give S no transform-style, and give S
two layer children that both draw content.
I couldn't make S create a surface in this precise configuration. I should have
added some transform to S to turn it into a render surface, which implies a
stacking context for it. When forcing a surface for G (S' parent), S doesn't
create a surface anymore and B will blend with both S and G. Here is an example:
http://codepen.io/rosca/pen/ordcF, I forced the surface using a -webkit-filter.

*2) Give S a parent with a transform that does not preserve axis alignment
(rotation/perspective) and give S some clip.
I managed to make S creating a surface with no stacking context by using both
the transform and the clip upon G. The good think is that, creating a surface
for G, the surface for S goes away and B will blend with both S and G.
The example is here: http://codepen.io/rosca/pen/gduHz

I think this happens because the render surfaces forced for G take off some of
the 3d responsibilities for their descendants and they don't require the new
surface.

> So yeah, it does seem like more complicated read-back patterns in the
compositor
> would be necessary to support blend-mode for all possible tree structures. 
> Perhaps we can think of something crazy about how Blink sets up the composited
> layer tree hierarchy to guarantee that the blending layer is an immediate
> descendant of it's stacking context, and could be guaranteed to be siblings to
> any content that renders into that subsequent surface... but it's not clear to
> me whether that's even possible much less how it would work.
Yeah, this seems challenging. I will think on this. Though, I would like to find
an use case proving that we really need it. I should look for a render surface
without being a stacking context, where the stacking context it belongs to, is
also a render surface. Please tell me if you have some ideas. Even if we don't
find this case, there is no guarantee that in the future it will be the same,
but maybe it's enough to DCHECK it.

danakj

On Wed, Oct 2, 2013 at 6:10 PM, <rosca@adobe.com> wrote: > Two ways you could ...

7 years, 2 months ago (2013-10-02 22:19:59 UTC) #18

On Wed, Oct 2, 2013 at 6:10 PM, <rosca@adobe.com> wrote:

> Two ways you could create a surface without being a stacking context:
>> 1) Give S a parent that has preserves-3d, give S no transform-style, and
>> give
>>
> S
>
>> two layer children that both draw content.
>> 2) Give S a parent with a transform that does not preserve axis alignment
>> (rotation/perspective) and give S some clip.
>>
> I tried the cases Enne has suggested and he is right about surfaces created
> without being stacking contexts. But, these cases doesn't seem to be a
> problem
> for blending, or at least. This is because blending forces the stacking
> context
> layer to create a render surface too. Let's take the examples where
> surfaces
> should be created without being stacking contexts:
>
> *1) Give S a parent that has preserves-3d, give S no transform-style, and
> give S
>
> two layer children that both draw content.
> I couldn't make S create a surface in this precise configuration. I should
> have
> added some transform to S to turn it into a render surface, which implies a
> stacking context for it. When forcing a surface for G (S' parent), S
> doesn't
> create a surface anymore and B will blend with both S and G. Here is an
> example:
> http://codepen.io/rosca/pen/**ordcF <http://codepen.io/rosca/pen/ordcF>,
> I forced the surface using a -webkit-filter.
>
> *2) Give S a parent with a transform that does not preserve axis alignment
>
> (rotation/perspective) and give S some clip.
> I managed to make S creating a surface with no stacking context by using
> both
> the transform and the clip upon G. The good think is that, creating a
> surface
> for G, the surface for S goes away and B will blend with both S and G.
> The example is here:
http://codepen.io/rosca/pen/**gduHz<http://codepen.io/rosca/pen/gduHz>
>
> I think this happens because the render surfaces forced for G take off
> some of
> the 3d responsibilities for their descendants and they don't require the
> new
> surface.


What if you have more decendants of S than G? Are you hitting a special
case because G is the only child?


>
>
>  So yeah, it does seem like more complicated read-back patterns in the
>>
> compositor
>
>> would be necessary to support blend-mode for all possible tree structures.
>> Perhaps we can think of something crazy about how Blink sets up the
>> composited
>> layer tree hierarchy to guarantee that the blending layer is an immediate
>> descendant of it's stacking context, and could be guaranteed to be
>> siblings to
>> any content that renders into that subsequent surface... but it's not
>> clear to
>> me whether that's even possible much less how it would work.
>>
> Yeah, this seems challenging. I will think on this. Though, I would like
> to find
> an use case proving that we really need it. I should look for a render
> surface
> without being a stacking context, where the stacking context it belongs
> to, is
> also a render surface. Please tell me if you have some ideas. Even if we
> don't
> find this case, there is no guarantee that in the future it will be the
> same,
> but maybe it's enough to DCHECK it.
>
>
>
https://codereview.chromium.**org/23455060/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

rosca

> > What if you have more decendants of S than G? Are you hitting ...

7 years, 2 months ago (2013-10-02 22:51:47 UTC) #19

enne (OOO)

On 2013/10/02 22:10:38, Rosca wrote: > I tried the cases Enne has suggested and he ...

7 years, 2 months ago (2013-10-02 23:36:33 UTC) #20

On 2013/10/02 22:10:38, Rosca wrote:

> I tried the cases Enne has suggested and he is right about surfaces created
> without being stacking contexts. But, these cases doesn't seem to be a problem
> for blending, or at least. This is because blending forces the stacking
context
> layer to create a render surface too.

It looks to me like example #1 in http://codepen.io/rosca/pen/gduHz demonstrates
what I'm trying to get at exactly.  G is a composited layer with a stacking
context drawing into the root surface.  Because of the clip and the rotation, G
creates a clip layer that gets its own surface.  S's cyan content is a
composited layer child of this clip layer without a surface.  The two red
blending children both get composited layers but no surfaces.

To verify, here's what a hacked version of about://tracing (to include
hasSurface) says about that example:

The tree structure:

  cc::PictureLayerImpl 18
    cc::LayerImpl 34
      cc::PictureLayerImpl 19
      cc::PictureLayerImpl 20
      cc::PictureLayerImpl 21

Layer info:

Layer 18 // G, back rotated clip
{bounds: {height: 200,
          width: 200},
 compositingReasons: ["Has a 3d Transform",
                      "Has backface-visibility: hidden",
                      "Has transform needed by a composited descendant",
                      "Clips a composited descendant"],
 drawsContent: 1,
 hasSurface: false,

Layer 34 // clip from G
{bounds: {height: 200,
          width: 200},
 compositingReasons: ["Convenience layer, to clip subtree"],
 drawsContent: 0,
 hasSurface: true,

Layer 19 // cyan div, middle
{bounds: {height: 150,
          width: 150},
 compositingReasons: ["Has backface-visibility: hidden"],
 drawsContent: 1,
 hasSurface: false,

Layer 20 // red div 1, front blending
{bounds: {height: 20,
          width: 200},
 compositingReasons: ["Has backface-visibility: hidden",
                      "Might overlap a composited animation"],
 drawsContent: 1,
 hasSurface: false,

Layer 21 // red div 2, front blending
{bounds: {height: 20,
          width: 200},
 compositingReasons: ["Has backface-visibility: hidden",
                      "Might overlap a composited animation"],
 drawsContent: 1,
 hasSurface: false,

If I understand the blending spec correctly, those red divs should be able to
blend with both S (cyan) and G (green) even though S and G are drawn into
different render surfaces.  Is that correct?  The current background filter code
only reads back from one render target (hence the TODO) which doesn't support
this, which is what I'm trying to get at.

Is there a Blink-side patch that you're testing these blend modes with?

(Also, for what it's worth, s/he is/they are/)

enne (OOO)

On 2013/10/02 22:10:38, Rosca wrote: > I tried the cases Enne has suggested and he ...

7 years, 2 months ago (2013-10-02 23:36:33 UTC) #21

On 2013/10/02 22:10:38, Rosca wrote:

> I tried the cases Enne has suggested and he is right about surfaces created
> without being stacking contexts. But, these cases doesn't seem to be a problem
> for blending, or at least. This is because blending forces the stacking
context
> layer to create a render surface too.

It looks to me like example #1 in http://codepen.io/rosca/pen/gduHz demonstrates
what I'm trying to get at exactly.  G is a composited layer with a stacking
context drawing into the root surface.  Because of the clip and the rotation, G
creates a clip layer that gets its own surface.  S's cyan content is a
composited layer child of this clip layer without a surface.  The two red
blending children both get composited layers but no surfaces.

To verify, here's what a hacked version of about://tracing (to include
hasSurface) says about that example:

The tree structure:

  cc::PictureLayerImpl 18
    cc::LayerImpl 34
      cc::PictureLayerImpl 19
      cc::PictureLayerImpl 20
      cc::PictureLayerImpl 21

Layer info:

Layer 18 // G, back rotated clip
{bounds: {height: 200,
          width: 200},
 compositingReasons: ["Has a 3d Transform",
                      "Has backface-visibility: hidden",
                      "Has transform needed by a composited descendant",
                      "Clips a composited descendant"],
 drawsContent: 1,
 hasSurface: false,

Layer 34 // clip from G
{bounds: {height: 200,
          width: 200},
 compositingReasons: ["Convenience layer, to clip subtree"],
 drawsContent: 0,
 hasSurface: true,

Layer 19 // cyan div, middle
{bounds: {height: 150,
          width: 150},
 compositingReasons: ["Has backface-visibility: hidden"],
 drawsContent: 1,
 hasSurface: false,

Layer 20 // red div 1, front blending
{bounds: {height: 20,
          width: 200},
 compositingReasons: ["Has backface-visibility: hidden",
                      "Might overlap a composited animation"],
 drawsContent: 1,
 hasSurface: false,

Layer 21 // red div 2, front blending
{bounds: {height: 20,
          width: 200},
 compositingReasons: ["Has backface-visibility: hidden",
                      "Might overlap a composited animation"],
 drawsContent: 1,
 hasSurface: false,

If I understand the blending spec correctly, those red divs should be able to
blend with both S (cyan) and G (green) even though S and G are drawn into
different render surfaces.  Is that correct?  The current background filter code
only reads back from one render target (hence the TODO) which doesn't support
this, which is what I'm trying to get at.

Is there a Blink-side patch that you're testing these blend modes with?

(Also, for what it's worth, s/he is/they are/)

rosca

On 2013/10/02 23:36:33, enne wrote: > If I understand the blending spec correctly, those red ...

7 years, 2 months ago (2013-10-03 05:48:26 UTC) #22

enne (OOO)

Ok, quite right. In your other patch you were proposing to create surfaces for any ...

7 years, 2 months ago (2013-10-11 18:14:35 UTC) #23

rosca

Thanks a lot for your comments! I uploaded a new patch with all them addressed. ...

7 years, 2 months ago (2013-10-16 14:54:47 UTC) #24

enne (OOO)

On 2013/10/16 14:54:47, Rosca wrote: > Regarding the render surfaces that might not be stacking ...

7 years, 2 months ago (2013-10-21 19:35:28 UTC) #25

On 2013/10/16 14:54:47, Rosca wrote:

> Regarding the render surfaces that might not be stacking contexts, I started
> working a few months ago on clipping rounded corners (border-radius property)
> for accelerated layers. I've chosen this issue to learn more about graphics. I
> submitted a patch [1] for clipping elements with accelerated content like
video
> and canvas and my solution was to create a mask being painted in software
which
> masks the corners specified by border-radius. I thought this solution would
also
> work for clipping an accelerated layer and its accelerated descendants [2],
but
> applying a mask on a layer will create a render surface. On the other hand
> border-radius property does not require a stacking context for that element.
> There has been discussions on w3 mailing list about the border-radius and if
it
> should create stacking context or not and apparently they agreed that it
> shouldn't [3].
>
> I didn't submit this patch [4] yet, and I will not do that at all. Instead, I
> plan to find an alternative solution for the border-radius problem [2]. The
> funny thing is that I'm the one who is trying to push 2 patches that may not
> live together :D.

I have a lot of mixed feelings.  border-radius not working in some cases seems
like a real bug that should be fixed, and is some evidence that this blend mode
patch unnecessarily constrains the compositor even in practice.  It seems to me
like the compositor should be able to create surfaces whenever and wherever it
wants (possibly for performance/caching reasons) and it should be entirely
invisible to any embedder.  It also means that we have to be really careful in
the future when adding any new feature in Blink that might create a surface
without a stacking context and this DCHECK there creates a landmine where adding
something new might break blending.

I think I'd be ok with landing this patch if you could help me see a reasonable
path to having correct blending even when there are surfaces between the blend
source and blend destination surfaces.  It doesn't have to be in this patch, but
I don't know that I have any vision of how to move forward here cleanly. 
Without that path, this patch encumbers the compositor and Blink more than I'm
really comfortable with.

rosca

> I have a lot of mixed feelings. border-radius not working in some cases seems ...

7 years, 1 month ago (2013-10-24 21:39:55 UTC) #26

> I have a lot of mixed feelings.  border-radius not working in some cases seems
> like a real bug that should be fixed, and is some evidence that this blend
mode
> patch unnecessarily constrains the compositor even in practice.  It seems to
me
> like the compositor should be able to create surfaces whenever and wherever it
> wants (possibly for performance/caching reasons) and it should be entirely
> invisible to any embedder.  It also means that we have to be really careful in
> the future when adding any new feature in Blink that might create a surface
> without a stacking context and this DCHECK there creates a landmine where
adding
> something new might break blending.
> 
> I think I'd be ok with landing this patch if you could help me see a
reasonable
> path to having correct blending even when there are surfaces between the blend
> source and blend destination surfaces.  It doesn't have to be in this patch,
but
> I don't know that I have any vision of how to move forward here cleanly. 
> Without that path, this patch encumbers the compositor and Blink more than I'm
> really comfortable with.

I've been thinking about this and I have a couple of ideas about how to move
forward with blending. I will list them in brief, with more details coming
bellow.
   A) The embedder (blink, CompositedLayerMapping) will not give to compositor
any reason for creating render surfaces between the blend source and blend
destination surfaces.
   B) The compositor will be able to gather the backdrop information from
multiple surfaces at different levels in the layer tree and provide it to the
blending layer so that is could blend correctly.

For the first option - A, the assumption is that the compositor can create
render surfaces for:
   1) any stacking context coming from CSS (masks, transforms, filters, …). This
case doesn’t affect blending.
   2) masking when it is used as a clipping technique (border-radius, and maybe
other cases in the future) without asking for a stacking context. I think I
found a solution for this case, I will describe it below. 
   3) any other reason like optimizations/cache (unrelated to stacking
contexts). It might be an option just to “turn off” such optimizations over the
layers that are placed in between the blend source and the blend destination
surfaces in the layer tree.

Here is the solution I was thinking of for avoiding the rendering surface
creation when clipping layers (2). Lets use this simple use case:
<div class="A accelerated stacking-context border-radius overflow-hidden">
    <div class="B accelerated mix-blend-mode bigger-then-A"></div>
    <div class="C accelerated bigger-then-A"></div>
  </div>
Blink will create a childContaintainmentLayer under A and will apply a mask to
it in order to clip its descendants B and D. So we have A -> ClipLayer -> B + C,
where ClipLayer will be turned into a render surface and will prevents B from
blending correctly with A.
The fix was to propagate the ClipLayer with the mask to each of its children - B
and C as ancestor clipping layers. Now B will move the blendMode property up its
ancestor clipping layer that holds the mask now, and both blending and clipping
will work correctly. This is what I’ve actually tried locally and it works. I
can upload my patch with these changes if you want to see it.
The more complex case is when B is not a direct descendant of  A, so there is
another composited element X between them. In this case I can crop the mask that
was painted for A and apply it directly on the layer X. This should work,
because X is not transformed relatively to A, since it’s not a stacking context.

The second option - B - is about gathering the backdrop from multiple surfaces.
The idea was to determine the isolated groups containing blending layers, walk
bottom-up the subtrees corresponding to each isolated group and draw everything
inside this group starting from the blend source surface (excluding it) up to
the root of the isolated group and store the result to use it later as backdrop
for the blending layer. So, this option involves walking and drawing portion of
the trees before the actual drawing step starts.
I’m not sure this is possible to implement, but if it could be possible, it
would probably be less performant and would bring more complexity in the
compositor.

What do you think of these options? Which of them do you think might be better
to move forward with, or do you see other options?

enne (OOO)

On 2013/10/24 21:39:55, Rosca wrote: > I've been thinking about this and I have a ...

7 years, 1 month ago (2013-10-29 18:29:14 UTC) #27

On 2013/10/24 21:39:55, Rosca wrote:

> I've been thinking about this and I have a couple of ideas about how to move
> forward with blending. I will list them in brief, with more details coming
> bellow.
>    A) The embedder (blink, CompositedLayerMapping) will not give to compositor
> any reason for creating render surfaces between the blend source and blend
> destination surfaces.

This is a path that I am not comfortable with.  Even if it were possible to make
all embedders of cc (which is more than just Blink) not give the compositor a
reason to create surfaces, it still hamstrings the compositor from making
optimizations in the future and adds a lot of (mental) complexity around surface
creation.  As I said previously, I think the compositor should be able to create
arbitrary surfaces and that the existence of surfaces should be completely
invisible to the embedder.  To be most blunt, I think this option is a
non-starter.

>    B) The compositor will be able to gather the backdrop information from
> multiple surfaces at different levels in the layer tree and provide it to the
> blending layer so that is could blend correctly.

Yeah, it's this step that I kind of wanted a little bit more detail on because
it's complex and I'm not sure I yet see the whole picture for how this should be
done.  How would Direct/GL/SoftwareRenderer need to change to support this? 
Currently, the pass "tree" is drawn depth first, with the root render pass being
drawn last, to minimize FBO changes.  To generate the surface for an isolated
group, it seems like you have to draw some number of partial passes (since there
could be something that draws into the isolated group's surface but comes after
the blending layer).  Do you think this could be done as a FrameData pre-pass,
to break up render passes into these partial passes, thereby keeping most of the
complexity out of the renderers (and maybe being able to reuse the partial
render passes)?

> What do you think of these options? Which of them do you think might be better
> to move forward with, or do you see other options?

I don't see any other options than these, which both have their downsides. 
(That's mostly why I'm asking this.)

Rik

On 2013/10/29 18:29:14, enne wrote: > This is a path that I am not comfortable ...

7 years, 1 month ago (2013-10-29 22:57:10 UTC) #28

enne (OOO)

On 2013/10/29 22:57:10, Rik wrote: > On 2013/10/29 18:29:14, enne wrote: > > This is ...

7 years, 1 month ago (2013-10-29 23:09:13 UTC) #29

Rik

On 2013/10/29 23:09:13, enne wrote: > > Yes, we could. I'm not saying it's not ...

7 years, 1 month ago (2013-10-30 00:03:36 UTC) #30

enne (OOO)

On 2013/10/30 00:03:36, Rik wrote: > On 2013/10/29 23:09:13, enne wrote: > > > > ...

7 years, 1 month ago (2013-10-30 00:31:48 UTC) #31

On 2013/10/30 00:03:36, Rik wrote:
> On 2013/10/29 23:09:13, enne wrote:
> > 
> > Yes, we could.  I'm not saying it's not possible; I'm just saying I don't
want
> > to deal with the complexity that that creates.  It would be a burden for
> future
> > Blink/compositor features to have to work around this new problem of when
it's
> > not ok to create a surface.
> 
> How difficult would that be and how much maintenance would that generate? 
> I realize that we're asking you to maintain this code for years to come so I
> understand your hesitation.

The problem here is that between the layer that creates an isolated group and
the layer with the blend mode, given this current implementation, there can be
no intervening layer that creates a surface without creating a stacking context
(and thus becoming an isolated group itself).  Rosca mentions in comment #24
that one solution for fixing border-radius *already* ran into this limitation,
so it seems like a non-theoretical limitation to me.

When I worry about complexity, here are some of the thoughts I have: we have
some surface triggers that don't have anything to do with Blink (e.g. copy
requests) and now if some consumer of cc tries to screenshot some subset of the
page, then it's going to break and look wrong if there's blending there.  Any
change to Blink's compositor triggers needs to now consider whether or not that
compositing trigger is a stacking context or not (for instance, overflow divs
may become a composited layer without being a stacking context, and so blending
probably will probably already not work with one of these in the middle). 
LayerTreeHostCommon (which decides what can/should be surfaces or not) is
probably one of the most complicated parts of cc (and really needs some major
refactoring for performance) and adding this wrinkle will only make it worse.

I totally agree that as it stands today, this patch will probably work in all
current circumstances with our default shipping configurations.  However, the
path forward for this patch is to assume that it is not desirable to prevent
future edge cases from breaking this patch (Rosca's solution A), and instead to
have some plan in place for what can be done to handle those cases (Rosca's
solution B).  That work doesn't have to be done in this patch or be written
before this patch lands, but I want confidence that a reasonable fix exists that
can be done as a followup to this work.

> FWIW it's highly likely that other features in the web platforms are going to
> need access to the backdrop. SVG filters have background-image and
> background-alpha which access the same backdrop as blending; we've had several
> requests to have blending on drop shadows which we will put in a future
version.

I'm not sure I understand the argument you're trying to make here.  Do these
future features not run into the same problem I describe above?

Rik

On 2013/10/30 00:31:48, enne wrote: > On 2013/10/30 00:03:36, Rik wrote: > > > > ...

7 years, 1 month ago (2013-10-30 16:59:19 UTC) #32

On 2013/10/30 00:31:48, enne wrote:
> On 2013/10/30 00:03:36, Rik wrote:
> > 
> > How difficult would that be and how much maintenance would that generate? 
> > I realize that we're asking you to maintain this code for years to come so I
> > understand your hesitation.
> 
> The problem here is that between the layer that creates an isolated group and
> the layer with the blend mode, given this current implementation, there can be
> no intervening layer that creates a surface without creating a stacking
context
> (and thus becoming an isolated group itself).  Rosca mentions in comment #24
> that one solution for fixing border-radius *already* ran into this limitation,
> so it seems like a non-theoretical limitation to me.

Let's go over that scenario (and please let me know if I'm completely wrong)
Let's say you have the following markup:
  <div opacity=.5>
    <div>
      <blend=multiply>

The tree will look like
  Layer with opacity
    \- Layer with blend

Now change the markup:
  <div opacity=.5>
    <div border radius + overflow clip>
      <blend=multiply>

With Ion's border patch that becomes:
   Layer with opacity
     \-Layer with clip
          \-Layer with blend
and blending is broken.

Ion's proposal is to change the tree to this instead:
   Layer with opacity
     \-Layer with clip
     \-Layer with blend + clip
Which *might* actually be faster (and is similar to a proposal from Shawn where
he'd put the text in another layer)

> 
> When I worry about complexity, here are some of the thoughts I have: we have
> some surface triggers that don't have anything to do with Blink (e.g. copy
> requests) and now if some consumer of cc tries to screenshot some subset of
the
> page, then it's going to break and look wrong if there's blending there.  

What is a copy request? I'm unsure why blending would be different than opacity.

> Any
> change to Blink's compositor triggers needs to now consider whether or not
that
> compositing trigger is a stacking context or not (for instance, overflow divs
> may become a composited layer without being a stacking context, and so
blending
> probably will probably already not work with one of these in the middle). 

Is that 'overflow:scroll'?

> LayerTreeHostCommon (which decides what can/should be surfaces or not) is
> probably one of the most complicated parts of cc (and really needs some major
> refactoring for performance) and adding this wrinkle will only make it worse.
> 
> I totally agree that as it stands today, this patch will probably work in all
> current circumstances with our default shipping configurations.  However, the
> path forward for this patch is to assume that it is not desirable to prevent
> future edge cases from breaking this patch (Rosca's solution A), and instead
to
> have some plan in place for what can be done to handle those cases (Rosca's
> solution B).  That work doesn't have to be done in this patch or be written
> before this patch lands, but I want confidence that a reasonable fix exists
that
> can be done as a followup to this work.

That sounds very reasonable. Ion is investigating that now.
The compositor is a complex beast! Thanks for helping us out! Hopefully we can
return the favor.

> 
> > FWIW it's highly likely that other features in the web platforms are going
to
> > need access to the backdrop. SVG filters have background-image and
> > background-alpha which access the same backdrop as blending; we've had
several
> > requests to have blending on drop shadows which we will put in a future
> version.
> 
> I'm not sure I understand the argument you're trying to make here.  Do these
> future features not run into the same problem I describe above?

Yes, they have the same problem which means that you will have to tackle this
problem sooner or later.

enne (OOO)

tl;dr: I answered my own question for how cc needs to handle this problem, so ...

7 years, 1 month ago (2013-10-30 19:49:15 UTC) #33

tl;dr: I answered my own question for how cc needs to handle this problem, so
I'll take another look at this patch.

On 2013/10/30 16:59:19, Rik wrote:
> With Ion's border patch that becomes:
>    Layer with opacity
>      \-Layer with clip
>           \-Layer with blend
> and blending is broken.
>
> Ion's proposal is to change the tree to this instead:
>    Layer with opacity
>      \-Layer with clip
>      \-Layer with blend + clip
> Which *might* actually be faster (and is similar to a proposal from Shawn
where
> he'd put the text in another layer)

Sorry for my skepticism, but I'm not convinced that a reparenting-style solution
is generalizable.

Here's a tree of layers, some of which create surfaces.  Assume no 3d sorting,
so paint order is a depth-first top-to-bottom traversal.  Layers draw into the
nearest ancestor layer that creates a surface, and I've labelled which target
that is for convenience.  For simplicity, assume surface layers don't draw
content.

  root (surface)
    C0 (content layer, target root)
    G (surface, isolated group, target root)
      C1 (content layer, target G)
      S (surface layer, target G)
          C2 (content layer, target S)
          B (surface, blending, target S)
            C3 (content layer, target B)
          C4 (content layer, target S)
      C5 (content layer, target G)
    C6 (content layer, target root)

This is essentially an isolated group G, an intermediate surface S, and a
blending layer B, with a sandwich of content layers around everything. 
Theoretically there could be indeterminate numbers of intermediate surface
layers like S, but having just one demonstrates the problem well enough.

By my understanding of the blend-mode spec, B needs to blend C3 with only C1 and
C2 because these all come before it in draw order into the isolated group.

The hard part is that these two layers draw into different target surfaces. 
When the cc::Renderer is processing surface B for blending, the current target
is S and G does not exist yet.  In other words, the root of the problem is that
S is a surface but is not the isolated group.  So, any feature that creates such
surface will interfere with blending.

Currently, the set of passes for such a tree looks like this:

  target: list of layers to draw in order
  ---------------------------------------
  (1) B: C3
  (2) S: C2, B, C4       <- blend B against target S, in this patch
  (3) G: C1, S, C5
  (4) root: C0, G, C6

In order to blend B against C1 and C2, you'd have to do something more like
this:

  target: list of layers to draw in order
  ---------------------------------------
  (1) B: C3
  (2) S: C2               <- only draw layers before B
  (3) G: C1, S            <- Do the readback from here
  (4) S (cont.): B, C4    <- blend previous readback with B here
  (5) G (cont.): S, C5    <- reapplying S seems problematic
  (6) root: C0, G, C6

To make cc be able to do that, you'd probably have to do some of this work:

* Process RSLL or FrameData to generate such a new list
* Readback becomes a bit more complicated with transform spaces, as you need B's
transform into G's space (or the reverse, really)
* cc::DirectRenderer would need to be able to reuse a preexisting FBO (as an
optimization)
* If C2 has transparency, G in this example needs to be entirely redrawn from
scratch because you can't redraw S into G as that would blend C2 on top of
itself.  You'd instead have to clear G and then draw C1, S, and C5 into it again
now that S was complete.

This is why the current implementation of background filters had the
limitation/simplification that background filters could only be applied to the
root layer.  :)

This is the best solution that I can come up with.  And, I guess having sat down
and explained this, I think I've answered my own question for what the plan is
for solving this problem more generally.

> > When I worry about complexity, here are some of the thoughts I have: we have
> > some surface triggers that don't have anything to do with Blink (e.g. copy
> > requests) and now if some consumer of cc tries to screenshot some subset of
> the
> > page, then it's going to break and look wrong if there's blending there.  
> 
> What is a copy request? I'm unsure why blending would be different than
opacity.

See cc::Layer::RequestCopyOfOutput.  It's a feature that does a readback of some
subtree.  The problem is that this creates a surface, so can interfere with
blending.

> > Any
> > change to Blink's compositor triggers needs to now consider whether or not
> that
> > compositing trigger is a stacking context or not (for instance, overflow
divs
> > may become a composited layer without being a stacking context, and so
> blending
> > probably will probably already not work with one of these in the middle). 
> 
> Is that 'overflow:scroll'?

Also, overflow:auto.

Rik

On 2013/10/30 19:49:15, enne wrote: > tl;dr: I answered my own question for how cc ...

7 years, 1 month ago (2013-10-31 22:36:24 UTC) #34

enne (OOO)

On 2013/10/31 22:36:24, Rik wrote: > Thanks for this great feedback. > This new information ...

7 years, 1 month ago (2013-11-01 18:49:01 UTC) #35

On 2013/10/31 22:36:24, Rik wrote:

> Thanks for this great feedback.
> This new information gave us an opportunity to brainstorm an improved solution
> that doesn't require changing the tree or render to a surface in different
> stages (like step 4 and 5).
> 
> The trick is that B could be a shader that just applies the raw blend formula
to
> the surface and its backdrop. Compositing can still happen as usual.
> The formula is described here: http://dev.w3.org/fxtf/compositing-1/#blending
> where it says: "The value of the new color becomes"

The problem I described above is mostly about how to read the backdrop when an
isolated group ends up being split over multiple surfaces.  I am not sure how a
shader can fix this since it's really about generating the texture input to a
shader, but I am interested in hearing about any improved solution.

https://codereview.chromium.org/23455060/diff/45001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23455060/diff/45001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:128: if (host && (!filters_.IsEmpty() ||
!background_filters_.IsEmpty() ||
Can you combine all these conditions in a single "SetNeedsFilterContextIfNeeded"
helper that SetLayerTreeHost, all the set filter, and SetBlendMode can call?
That way all of these conditions don't have to be spread around everywhere?

https://codereview.chromium.org/23455060/diff/45001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/45001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2560: void
GLRenderer::GetFramebufferTextureSubImage(unsigned texture_id,
I still feel a little bit uneasy about this API.  It seems pretty non-obvious at
callsites why this function gets called vs the other one.

Is there some way to make this more clear? Is there any easy way to combine this
with GetFramebufferTexture and have it be more smart, picking the more efficient
copyTexImage2D version when possible and the other copyTexSubImage2D version
when needed?

https://codereview.chromium.org/23455060/diff/45001/cc/quads/shared_quad_stat...
File cc/quads/shared_quad_state.cc (right):

https://codereview.chromium.org/23455060/diff/45001/cc/quads/shared_quad_stat...
cc/quads/shared_quad_state.cc:15: const char* BlendModeToString(SkXfermode::Mode
blendMode) {
blend_mode

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1884: // either root_for_isolated_group, or
the root of the layer tree.
Can you also mention that if this is not true, then the problem will be that
blending will be incorrect?

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_pixeltest_blending.cc:26: unsigned const
blend_modes_count = arraysize(blend_modes);
unsigned const => const int, here and elsewhere.  Chromium style is to only use
unsigned for bitmasks, object counts, and array indices.

Also, if you're making a const, it should be kConstStyle not hacker_style. 
(Sorry.)

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_pixeltest_blending.cc:37: for (unsigned i = 0; i <
blend_modes_count; ++i) {
I like these these tests a lot, thanks!

https://codereview.chromium.org/23455060/diff/45001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_pixeltest_blending.cc:81: unsigned const lane_hight =
blend_modes_count * lane_width;
hight => height

https://codereview.chromium.org/23455060/diff/111001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23455060/diff/111001/cc/layers/layer.h#newcod...
cc/layers/layer.h:129: void SetBlendMode(SkXfermode::Mode blend_mode);
Can you help my understanding of the blend mode spec here? It is the case here
that there are a number of SkXfermode enums that aren't web-accessible via the
mix-blend-mode attribute, right?

The compositor invariant that I'd like to maintain is that creating a render
surface at any layer should always create the same resulting pixels.

In particular, there are some problematic SkXfermodes that I am concerned about.
 To list just two: kSrc_Mode which I think corresponds to Porter Duff Copy and
kClear_Mode which corresponds to Porter Duff Clear.

These two blend modes listed above (among others) are problematic in that even
for pixels where source alpha is 0, applying that blend mode will clobber the
destination alpha.

Since render surfaces are just arbitrarily sized to be the union of all the
layers that draw into them, any alpha=0 pixels between layers in that
arbitrarily-sized surface could affect destination alpha with certain blend
modes.  And, therefore the existence and presence of render surfaces will change
the final rendering because that will change that shape.

One solution would be to mask to bounds for any of these types.  Another
short-term solution would be to only support the web-accessible blend modes on
cc::Layer and ignore/DCHECK others.  What do you think?

(This came up yesterday in the context of http://crbug.com/313494 where there's
some need to make layers be able to do something like blend with kSrc_Mode and
so it intersects with this work.)

Rik

On 2013/11/01 18:49:01, enne wrote: > > https://codereview.chromium.org/23455060/diff/111001/cc/layers/layer.h#newcode129 > cc/layers/layer.h:129: void SetBlendMode(SkXfermode::Mode blend_mode); > Can ...

7 years, 1 month ago (2013-11-01 20:46:19 UTC) #36

achicu

I agree we should only implement the color blending for now. Color blending is not ...

7 years, 1 month ago (2013-11-01 22:13:56 UTC) #37

I agree we should only implement the color blending for now. Color blending is
not affecting the alpha channel, so it makes it a lot easier to implement. Also,
it has the most appealing/useful visual results.
 
Certain Porter Duff compositing modes (such as ‘clear’) are harder to implement
using the GPU as we need to figure out how to transport the “punched” pixels
into their parent surfaces. Traditionally, the software compositors would not
require the concept of RenderSurface, so that makes the implementation much
straightforward for the software path.
 
This solution will focus mainly on the “color blend modes”. It should be general
enough to cover arbitrary isolated-groups, meaning that any surface can become a
non-isolated group without limiting the implementation.
 
I think that there’s a bit of confusion on how the backdrop of the blending
operation should be computed. There’s a special case when there’s an
non-isolated group involved. The backdrop is defined as the result of rendering
all the previous layers in paint-order. But, there’s a catch. Non-isolating
groups (like the surface S below), will not apply their effects on the contents.
That means, that any masking, opacity or filter should be ignored when painting
the previous siblings of the “Blended layer”. In the example below, the Backdrop
will be defined as L1 + L2. Note that there’s no masking applied on L2. That’s
why “read-back” might not generate the right results.
 
Let’s take an example, to make it easier to follow:
 
Surface G - isolated group
            Layer L1
            Surface S - non-isolated group, overflow: hidden
                        Layer L2
                        Blending surface B
                                    Layer L3 - Layer with color blending mode
                        Layer L4
            Layer L5
 
The simple implementation would be to prepare a RenderPass that will iterate on
all the previous layers and paint them. We are ignoring any effects (opacity,
masking, clipping) applied on child surfaces, so no extra surfaces would be
required. Only the transforms of the “non-isolated” groups will be considered
when rendering their children.
The texture of the RenderPass will then be passed to the shader that draws the
blending surface. It will be used to determine the actual color of the result.
We will also need an inverse transformation matrix. That’s because the backdrop
RenderPass is rendered in the coordinate space of the “parent isolated group”.
 
So the draw order will look like this:
B = L3
BackdropB = L1 + L2 (including the transform of S, but no opacity or any other
effect)
S = L2 + blend(BackdropB, B) + L4
G = L1 + S + L4
 
Obviously that seems like a lot of drawing, as it will render same layers
multiple times. In this case, BackdropB is actually a bit of G and a bit of S
blended together.
Now the problem is that the RenderPasses are processed in reverse dependency
order. That means that at the moment when the “blending backdrop” is required,
we don’t have access to the pixels behind. In this case the containing
RenderPass didn’t even start rendering yet. In the draw order above, the blend()
function comes before G even started to execute.
 
So the solution would be to just execute the “parent” RenderPasses up to the
point where all the previous layers are rendered. That's easy to do if we make
the Renderer stop when it reaches a “drawSurfaceQuad” operation that references
a quad that is not finished yet. Because of the way the render passes are
sorted, the first nested surface that is not finished yet is exactly the surface
that requires the blending. In the example above, the RenderPass for surface G
will stop just before rendering the quad for surface S. The Renderer will save
the position, so that it can continue at a later time. I will refer to this
position as the play-head of the RenderPass.
 
The draw order will now become like this:
 
B = L3
S = L2
G = L1
BackdropB = G, S (painting S without any filters or opacity)
S += blend(BackdropB, B) + L4
G += S + L4
 
After all the “parent RenderPasses” advanced their play-head, they will have
partial results in their backing textures. We can blend all these RenderPasses
into a separate texture that we can use as the “backdrop” texture. In order to
make this easy in the CC architecture, we can create a new “RenderPass” and add
quads for each of the parent render passes.
 
In this particular example, there’s no advantage to having partial renderings
for “S” and “G” as there is only one layers that needs the blending. In
practice, there will be at least one more layer in the parent surfaces that will
make the render target switch worth the effort.
 
Let’s introduce another non-isolated group, like in the example below:
Surface G - isolated group
            Layer L1
            Surface S1 - non-isolated group, overflow: hidden
                        Layer L2
                        Surface S2 - non-isolated group, overflow: hidden
                                    Layer L3
                                    Blending surface B1
                                                Layer L4 - Layer with color
blending mode
                                    Blending surface B2
                                                Layer L5 - Layer with color
blending mode
                        Layer L6
            Layer L7
 
The draw order will become:
 
B1 = L4
B2 = L5
S2 = L3
S1 = L2
G = L1
BackdropB1 = G, S1, S2 (painting S* without any filters or opacity)
S2 += blend(BackdropB1, B1)
BackdropB2 = G, S1, S2 (painting S* without any filters or opacity)
S2 += blend(BackdropB2, B2) + L6
S1 += S2 + L6
G += S1 + L4
 
There are other optimizations that can be done here:
1. First we can clip the BackdropB1 to the absolute bounding box of the blending
layer L4. We don’t need any of the pixels outside that quad. The same for B2.
2. We can reuse backdrops across multiple layers in the same non-isolated group.
In this case, BackdropB1 and BackdropB2 are pretty similar. It’s just S2 that
mutates from one use to the next one, but G and S1 are the same. We can group G
and S1 in an extra surface.
3. We can completely remove BackdropB1 and BackdropB2 if we use the OpenGL
extensions to read back from the current render buffer (GPU Skia uses this
internally when available). This way we can render “G + S1” once for both blend
operations and combine it with S2 in the shader itself as we are actually
drawing into S2 at that time anyway.
 
Here’s how it could simplify the drawing order:
 
B1 = L4
B2 = L5
S2 = L3
S1 = L2
G = L1
BackdropS2 = G, S1
S2 += blend(BackdropS2 + renderBufferColor, B1) 
S2 += blend(BackdropS2 + renderBufferColor, B2) + L6
S1 += S2 + L6
G += S1 + L4
 
What do you think? I can start a wiki page and dump these thoughts in there.
 
Here are some assumptions I have taken into account. Please correct me if I’m
wrong.
 
RenderPass -> A texture used as a render target and that accumulates
RenderQuads.
RenderQuad -> A quad that is rendered into a parent RenderPass.
 
1. The compositor has a tree of layers maintained by the client (Blink).
2. Layers are never rendered directly to screen.
3. Layers have no “paint method”.
4. The only way a Layer is rendered is through “RenderQuads”.
5. Each layer can generate as many render quads as it needs.
6. RenderQuads have an associated material.
7. RenderPasses have an associated rendering target texture.
8. Some layers become Surface Layers (overflow: hidden, opacity, filters,
non-3d-preserving transforms, etc).
9. Each surface has a RenderPass.
10. RenderPasses can be nested. The RenderPass resulting texture can be used as
a material of a RenderQuad to paint into a different (parent) RenderPass.
11. All the children layers of a Surface Layer will render into the RenderPass
of the surface.
12. This is the important part: All the RenderPasses are arranged and computed
in reverse dependency order so that a pass can be painted without interruption.
This is needed to avoid switching the render target too many times (a big no-no
in OpenGL world).

enne (OOO)

On 2013/11/01 22:13:56, achicu wrote: > I agree we should only implement the color blending ...

7 years, 1 month ago (2013-11-02 00:10:22 UTC) #38

On 2013/11/01 22:13:56, achicu wrote:
> I agree we should only implement the color blending for now. Color blending is
> not affecting the alpha channel, so it makes it a lot easier to implement.
Also,
> it has the most appealing/useful visual results.

Sounds good.  So this patch needs to just DCHECK and then ignore unsupported
blend modes.

> Certain Porter Duff compositing modes (such as ‘clear’) are harder to
implement
> using the GPU as we need to figure out how to transport the “punched” pixels
> into their parent surfaces. Traditionally, the software compositors would not
> require the concept of RenderSurface, so that makes the implementation much
> straightforward for the software path.

Among other reasons, since the opacity property on a layer creates a knockout
group so to speak, our software compositor has to handle render surfaces too.

> I think that there’s a bit of confusion on how the backdrop of the blending
> operation should be computed. There’s a special case when there’s an
> non-isolated group involved. The backdrop is defined as the result of
rendering
> all the previous layers in paint-order. But, there’s a catch. Non-isolating
> groups (like the surface S below), will not apply their effects on the
contents.
> That means, that any masking, opacity or filter should be ignored when
painting
> the previous siblings of the “Blended layer”.

Wait, what??  Why will non-isolating groups not apply their effects?  That's
really surprising to me.  It seems like it would make a solution more
complicated to ignore effects.

> In the example below, the Backdrop
> will be defined as L1 + L2. Note that there’s no masking applied on L2. That’s
> why “read-back” might not generate the right results.

Ignoring my confusion above about effects, I think the more critical problem is
really that in our current system if you do a readback right before you draw the
blended layer, then the current target will be S, and so you will only readback
what L2 drew and will miss L1 entirely.

> What do you think? I can start a wiki page and dump these thoughts in there.

I think we are coming at the same solution (reorder/duplicate draws) from
different sides, but are largely converging on a similar solution.  You're
suggesting making the renderer smarter in how it iterates through passes and I'm
suggesting that we reorder the passes before we get to the renderer.  I think we
are also suggesting drawing things in different orders.  I'm suggesting doing
the readback from the isolated group, and you seem to be suggesting
reverse-projecting everything into the blending layer's target surface.

I think the renderers are harder to test (pixel test vs unit test) and also are
spread across multiple classes (HW, SW, shared base class).  I'd personally
prefer to handle this as a filtering operation on FrameData, although I think
there are different ways to come at this.  That said, I don't think we need to
fully hash this out here.  I feel confident enough that there's *some*
reasonable solution here.

> Here are some assumptions I have taken into account. Please correct me if I’m
> wrong.

Here's some technical quibbles that may help your understanding.  :)

> RenderPass -> A texture used as a render target and that accumulates
> RenderQuads.

A RenderPass is associated with a texture.  That pass has-a list of quads.  I
use RenderSurface and RenderPass synonymously since they have a 1-1
relationship.  Really, a RenderSurface is owned by a Layer and that surface
has-a list of Layers that draw into it.  A RenderPass gets created from (but
doesn't own nor is owned by) a RenderSurface and a has-a list of DrawQuads that
draw into it, and the DrawQuads get generated by the Layers that were in the
RenderSurface's list.  That we have these two steps is somewhat historic and
somewhat of an artifact of needing to call CalcDrawProperties on both of our
threads against Layer and LayerImpl.

I would recommend danakj's
https://docs.google.com/a/chromium.org/presentation/d/11f3A8cdfSSKmYazetxy9oc...
presentation for some more details.

> RenderQuad -> A quad that is rendered into a parent RenderPass.

DrawQuad is the base type here, and the RenderPass owns list of them. 
RenderPassDrawQuad is the specialized quad type for drawing one RenderPass's
output into another.

> 1. The compositor has a tree of layers maintained by the client (Blink).
> 2. Layers are never rendered directly to screen.
> 3. Layers have no “paint method”.
> 4. The only way a Layer is rendered is through “RenderQuads”.
> 5. Each layer can generate as many render quads as it needs.
> 6. RenderQuads have an associated material.

(Yes to all of the above.)

> 7. RenderPasses have an associated rendering target texture.

The root layer in the tree gets a RenderSurface (and a RenderPass), but it
doesn't have a render target and is the default backbuffer.

> 8. Some layers become Surface Layers (overflow: hidden, opacity, filters,
> non-3d-preserving transforms, etc).

"Become" is too strong of a word here.  Layers can create and own
RenderSurfaces.  In these cases they can still draw content.

> 9. Each surface has a RenderPass.

"Has a" is too strong here.  Each RenderSurface is 1-1 with a RenderPass, but
they aren't associated directly.

> 10. RenderPasses can be nested. The RenderPass resulting texture can be used
as
> a material of a RenderQuad to paint into a different (parent) RenderPass.

Logically nested, yes.  The layer tree is a tree, but by the time you get to
passes it's just an ordered list.

> 11. All the children layers of a Surface Layer will render into the RenderPass
> of the surface.

A layer draws into the surface of the closest ancestor (including itself) that
has a surface.  A surface gets drawn into the surface of the closest ancestor of
its owning layer's *parent* (including that parent) that has a surface.  The
problematic part here is that a layer with a surface that draws content draws
twice (in some senses): once as the layer into the surface, and once as the
surface into some other surface.

> 12. This is the important part: All the RenderPasses are arranged and computed
> in reverse dependency order so that a pass can be painted without
interruption.
> This is needed to avoid switching the render target too many times (a big
no-no
> in OpenGL world).

This is an optimization that we're currently making, yeah.

rosca

Thanks for your comments. I uploaded a new patch addressing your comments. There is one ...

7 years, 1 month ago (2013-11-04 17:14:34 UTC) #39

enne (OOO)

https://codereview.chromium.org/23455060/diff/421001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/23455060/diff/421001/cc/layers/layer.cc#newcode523 cc/layers/layer.cc:523: bool is_blend_mode = (blend_mode == SkXfermode::kSrcOver_Mode || naming: is_blend_mode ...

7 years, 1 month ago (2013-11-04 22:38:15 UTC) #40

enne (OOO)

https://codereview.chromium.org/23455060/diff/421001/cc/trees/layer_tree_host_pixeltest_blending.cc File cc/trees/layer_tree_host_pixeltest_blending.cc (right): https://codereview.chromium.org/23455060/diff/421001/cc/trees/layer_tree_host_pixeltest_blending.cc#newcode46 cc/trees/layer_tree_host_pixeltest_blending.cc:46: RunPixelTest(GL_WITH_BITMAP, On 2013/11/04 22:38:16, enne wrote: > I'm assuming ...

7 years, 1 month ago (2013-11-04 23:24:35 UTC) #41

rosca

I uploaded a new patch. Besides the changes you requested, I added to delegated_renderer_layer.cc the ...

7 years, 1 month ago (2013-11-05 19:14:09 UTC) #42

I uploaded a new patch. Besides the changes you requested, I added to
delegated_renderer_layer.cc the condition for creating offscreen context when
there are render passes having a blend mode.

Thanks,

https://codereview.chromium.org/23455060/diff/421001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23455060/diff/421001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:523: bool is_blend_mode = (blend_mode ==
SkXfermode::kSrcOver_Mode ||
On 2013/11/04 22:38:16, enne wrote:
> naming: is_blend_mode => supported_blend_mode or even just supported.  All the
> SkXfermodes are technically blend modes.
We call the other modes "Compositing operators", but that's not very clear from
skia declaration of SkXfermodes.

https://codereview.chromium.org/23455060/diff/421001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:525: blend_mode <= SkXfermode::kLastMode));
On 2013/11/04 22:38:16, enne wrote:
> This doesn't seem very futureproof.  Could you just make this a switch
statement
> (without default) and enumerate what's supported and what isn't?

Done.

https://codereview.chromium.org/23455060/diff/421001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/421001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:743: is_using_texture_storage_ ? RGBA_4444 : RGBA_8888;
On 2013/11/04 22:38:16, enne wrote:
> I am probably missing something, but can you help me understand how this
avoids
> copyTexSubImage2D and lets you use copyTexImage2D in all cases?
When GL is capable of texture storage, resource provider (RP) uses
texStorage2DEXT to allocate the RGBA/BGRA_8888 textures [1]. This makes
texture's structure immutable and copyTexImage2D will fail when called on that
texture. The RGBA_4444 format is not supported by texture storage, so when I ask
RP to allocate a RGBA_4444 texture, it will use texImage2D and copyTexImage2D
will just work for all the cases.
On trunk, background filters are using RGBA_8888 with copyTexImage2D and it
fails on my windows box for the same reason. Before the RGBA_8888 format being
added, background filters were using RGB format and this was working well with
RP and copyTexImage.

However, I've changed my mind about RGBA_4444, I found that Android WebView
doesn't support this format [2] :(.

This time I added an extra bool argument to GetFramebufferTexture:
is_texture_storage, which should be true if the texture has been allocated using
texture storage. GetFramebufferTexture will call either copyTexImage2D, or
copyTexSubImage2D, depending on that argument.

Another option to avoid using copyTexSubImage2D would be to ask ResourceProvider
allocate the texture using texImage2D, event if it's capable to use texture
storage. What do you think?

> (Also, could you just ask for
resource_provider_->memory_efficient_texture_format() rather than
> adding another flag? 
I still need is_using_texture_storage_ flag. I can just expose this flag from
ResourceProvider and call it here. What do you think?

[1]https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resource_provider.cc&q=texStorage2DEXT&sq=package:chromium&type=cs&l=1609
[2]https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/gpu/render_widget_compositor.cc&q=use_rgba_4444_textures&sq=package:chromium&type=cs&l=295

https://codereview.chromium.org/23455060/diff/421001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/23455060/diff/421001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_blending.cc:46: RunPixelTest(GL_WITH_BITMAP,
On 2013/11/04 23:24:36, enne wrote:
> On 2013/11/04 22:38:16, enne wrote:
> > I'm assuming that you will add cc::SoftwareRenderer support for this in a
> > follow-up patch?  I don't think we have background filter support for the
> > SoftwareRenderer at all, so this will probably be a good chunk of work.
> 
> I filed http://crbug.com/314865 and http://crbug.com/314864 to address this.

Thanks for filing these bugs. We plan to add functionality for SoftwareRenderer
as well.

enne (OOO)

A few more small comments, mostly about cleaning up the texture storage-related code. https://codereview.chromium.org/23455060/diff/561001/cc/layers/layer_impl.cc File ...

7 years, 1 month ago (2013-11-06 22:40:18 UTC) #43

rosca

Thanks, uploaded another patch. I removed webkit/renderer/compositor_bindings files from this review, as they have dependencies ...

7 years, 1 month ago (2013-11-07 01:58:15 UTC) #45

enne (OOO)

cdn: owners for content/common/cc_messages* lgtm from me. Before landing this, please let danakj have a ...

7 years, 1 month ago (2013-11-07 22:24:31 UTC) #46

danakj

https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_reasons.h File cc/layers/compositing_reasons.h (right): https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_reasons.h#newcode56 cc/layers/compositing_reasons.h:56: const uint64 kCompositingReasonIsolateCompositedDescendants = GG_UINT64_C(1) nit: this line wrapping ...

7 years, 1 month ago (2013-11-13 21:02:02 UTC) #47

Cris Neckar

On 2013/11/13 21:02:02, danakj wrote: > https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_reasons.h > File cc/layers/compositing_reasons.h (right): > > https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_reasons.h#newcode56 > ...

7 years, 1 month ago (2013-11-14 02:08:45 UTC) #48

rosca

Thanks for your comments. I uploaded a new patch addressing most of the comments in ...

7 years, 1 month ago (2013-11-14 21:56:32 UTC) #49

Thanks for your comments.
I uploaded a new patch addressing most of the comments in #47. There are 2 more
unit tests for occlusion I should add.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_re...
File cc/layers/compositing_reasons.h (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/compositing_re...
cc/layers/compositing_reasons.h:56: const uint64
kCompositingReasonIsolateCompositedDescendants = GG_UINT64_C(1)
On 2013/11/13 21:02:03, danakj wrote:
> nit: this line wrapping differs from the rest of this file, was this via
> clang-format? if not, can you match the others?
Yes, it's clang-format suggesting this, but I think we should keep them
consistent.
Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:110: static bool
FrameDataRequiresOffscreenContext(const DelegatedFrameData* frame) {
On 2013/11/13 21:02:03, danakj wrote:
> bikeshed: RequiresFilterContext()? this would match the function name on
> LayerTreeHost.
It matches with the getter's name "needs_offscreen_context" in LayerTreeHost.
Setter uses "filter_context" and I'm not sure they were intended to be
different, it's probably called so because the first time it's been used for
filters only. The context provider is called "offscreen context provider" as
well.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:120:
render_pass_quad->shared_quad_state->blend_mode !=
On 2013/11/13 21:02:03, danakj wrote:
> IIUC there is a plan to support blend mode for any quad, not just RPDQ? Is
there
> a reason to limit the checks here to RPDQ? Can we look at all the shared quad
> states for this blend mode?
Yes, I should have checked blend mode for all types of draw quads. Done.

> (It'd be faster to iterate the shared_quad_state_list than the quad_list,
since
> multiple quads can point to the same SQS.)
You're right, but  it will iterate through all the quads anyway to check for
filters. If we iterate SQSs separately it will bring better readability, but
more steps to iterate.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:140: if (layer_tree_host() &&
!layer_tree_host()->needs_offscreen_context() &&
On 2013/11/13 21:02:03, danakj wrote:
> nit: no real need to check needs_offscreen_context() here, it's just calling a
> setter.
I checked this to limit the number of FrameDataRequiresOffscreenContext calls,
which seems to be expensive enough to be called so often.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:557: DCHECK(false);
On 2013/11/13 21:02:03, danakj wrote:
> NOTREACHED() instead of DCHECK(false)

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:566: void Layer::SetIsRootForIsolatedGroup(bool root) {
On 2013/11/13 21:02:03, danakj wrote:
> Can you add these 2 new Set* methods to layer_unittest.cc to verify they cause
a
> commit?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.h#newcode31
cc/layers/layer.h:31: #include "third_party/skia/include/core/SkPicture.h"
On 2013/11/13 21:02:03, danakj wrote:
> Can we include SkXfermode.h in this file since we're using it here?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.h#newcod...
cc/layers/layer.h:136: void SetIsRootForIsolatedGroup(bool root);
On 2013/11/13 21:02:03, danakj wrote:
> Can you put a comment explaining what this means?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer.h#newcod...
cc/layers/layer.h:474: // Called when the blend mode or filters have been
changed
On 2013/11/13 21:02:03, danakj wrote:
> period at the end of sentences

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:884: void LayerImpl::SetBlendMode(SkXfermode::Mode
blend_mode) {
On 2013/11/13 21:02:03, danakj wrote:
> Can you add these 2 new Set* methods to layer_impl_unittest.cc to verify their
> side effects on damage and needs update draw props?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:896: is_root_for_isolated_group_ = root;
On 2013/11/13 21:02:03, danakj wrote:
> It seems to be this should be causing damage. If this never changes how things
> are displayed there'd be no point of it existing, so it must be changing how
> things look sometimes.
> 
> From discussing with enne@ it sounds like this should be doing
> NoteLayerPropertyForSubtree().
I think it would be more accurate to call
NoteLayerPropertyChangedForDescendants, because only children may change how
they look due to isolation, not the layer itself. I've changed it, please
correct me if I'm wrong.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:739: ResourceProvider::TextureUsageFramebuffer,
On 2013/11/13 21:02:03, danakj wrote:
> Why this change? We don't bind this texture to be a FBO do we? We're binding
the
> |background_texture| below which is a different texture. I see us copying this
> texture into the FBO, or using it as an input to ApplyImageFilter, but not
> binding it as an FBO?
The device_background_texture's format has been recently changed from GL_RGB to
RGBA_8888, and this change has broken background filters on windows. RGBA_8888
textures are supported by texture storage, therefore ResourceProvider uses
texStorage2DEXT to allocate that texture. The actual failure happened on
GetFramebufferTexture/copyTexImage2D call, which is not compatible with storage
textures.
So, I changed the texture usage hint, as enne@ suggested
(https://codereview.chromium.org/23455060/diff/561001/cc/output/gl_renderer.cc...).
If we Allocate() the texture using TextureUsageFramebuffer hint,
ResourcePrivider will not use storage2d for this texture and copyTexImage2D will
work fine.
I agree that the hint doesn't say what it actually does. Should I add another
hint?
> If this is not needed like it seems, then the ResourceProvider change could go
> away as well..

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:750: int filtered_device_background_texture_id = 0;
On 2013/11/13 21:02:03, danakj wrote:
> move this down to above line 765?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:755: if (apply_background_filters)
On 2013/11/13 21:02:03, danakj wrote:
> need {}

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:762: if (background_changed)
On 2013/11/13 21:02:03, danakj wrote:
> why make this optional? i'd prefer to just always have the caller pass in a
> bool. it seems like it already does?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:845: bool apply_blend_mode =
On 2013/11/13 21:02:03, danakj wrote:
> can this be
> 
> bool need_background_texture =
>   blend_mode != SrcOver ||
>   !background_filters.IsEmpty();
> 
> ...
> 
> if (need_background_texture) {
> }

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:910: // Draw the background texture if there is one.
On 2013/11/13 21:02:03, danakj wrote:
> update this comment

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:592: // types of quads than RenderPassQuad.
Layers having descendants that draw
On 2013/11/13 21:02:03, danakj wrote:
> RenderPassDrawQuad

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:606: // (layer has transparent background or
descendants overflow)
On 2013/11/13 21:02:03, danakj wrote:
> period at the end.

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:613: return true;
On 2013/11/13 21:02:03, danakj wrote:
> Can you move this down before the if (is_root) case to around line 665 if this
> one is allowed on the root layer?

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2082: DCHECK(layer->uses_default_blend_mode()
|| !layer->parent() ||
On 2013/11/13 21:02:03, danakj wrote:
> Can you add DCHECK(...) << each << of << the << variables << in << this <<
> dcheck after it, so we can tell what failed when it does?
They are disjunctions, so it will fail only when all of them are false. Maybe I
misunderstood your request.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_blending.cc:16: SkXfermode::Mode const
blend_modes[] = {
On 2013/11/13 21:02:03, danakj wrote:
> style: compile-time constants should be kBlendModes

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_blending.cc:29: const int lane_width = 15;
On 2013/11/13 21:02:03, danakj wrote:
> style: compile-time constants should be kLaneWidth

Done.

rosca

https://codereview.chromium.org/23455060/diff/681001/cc/trees/occlusion_tracker.cc File cc/trees/occlusion_tracker.cc (right): https://codereview.chromium.org/23455060/diff/681001/cc/trees/occlusion_tracker.cc#newcode238 cc/trees/occlusion_tracker.cc:238: !finished_target->uses_default_blend_mode() || On 2013/11/13 21:02:03, danakj wrote: > Can ...

7 years, 1 month ago (2013-11-18 10:01:34 UTC) #50

danakj

Thanks this looks good, I have a few last comments. https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_renderer_layer.cc File cc/layers/delegated_renderer_layer.cc (right): https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_renderer_layer.cc#newcode110 ...

7 years, 1 month ago (2013-11-20 03:32:49 UTC) #51

Thanks this looks good, I have a few last comments.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:110: static bool
FrameDataRequiresOffscreenContext(const DelegatedFrameData* frame) {
On 2013/11/14 21:56:34, Rosca wrote:
> On 2013/11/13 21:02:03, danakj wrote:
> > bikeshed: RequiresFilterContext()? this would match the function name on
> > LayerTreeHost.
> It matches with the getter's name "needs_offscreen_context" in LayerTreeHost.
> Setter uses "filter_context" and I'm not sure they were intended to be
> different, it's probably called so because the first time it's been used for
> filters only. The context provider is called "offscreen context provider" as
> well.

The idea was the setter has some more context about "what you're tying to do" ie
I want to use filters. Whereas the getter is disconnected from use cases, and
just says that we need to make an offscreen context.

If there were other ways to do filters, then maybe a different getter would
return true and the needs_offscreen_context would not. That's the idea here. So
where we are dealing with filters (ie outside of the code that actually is
worrying about how we implement the filters) i would prefer that we don't tie
ourselves to "OffscreenContext".

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:140: if (layer_tree_host() &&
!layer_tree_host()->needs_offscreen_context() &&
On 2013/11/14 21:56:34, Rosca wrote:
> On 2013/11/13 21:02:03, danakj wrote:
> > nit: no real need to check needs_offscreen_context() here, it's just calling
a
> > setter.
> I checked this to limit the number of FrameDataRequiresOffscreenContext calls,
> which seems to be expensive enough to be called so often.

This will only avoid the call when we've already seen a filter, which is by far
the minority of cases, so generally we'll be doing this call every time we get a
new frame anyways. If the check is a bottleneck we should make it faster. I'd
prefer to avoid the if(needs_offscreen_context) here for that reason.

By the way, this code moved to Update() and I should have removed the
layer_tree_host() null check. There's no way LTH can be null inside Update(), we
shouldn't be checking for it here.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:896: is_root_for_isolated_group_ = root;
On 2013/11/14 21:56:34, Rosca wrote:
> On 2013/11/13 21:02:03, danakj wrote:
> > It seems to be this should be causing damage. If this never changes how
things
> > are displayed there'd be no point of it existing, so it must be changing how
> > things look sometimes.
> > 
> > From discussing with enne@ it sounds like this should be doing
> > NoteLayerPropertyForSubtree().
> I think it would be more accurate to call
> NoteLayerPropertyChangedForDescendants, because only children may change how
> they look due to isolation, not the layer itself. I've changed it, please
> correct me if I'm wrong.

I'd like enne@ to double check this. I would expect isolation to affect all
layers that draw into the surface equally, not just descendants of the owning
layer? But I don't think I have the same understanding of this part of the CL..

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:739: ResourceProvider::TextureUsageFramebuffer,
On 2013/11/14 21:56:34, Rosca wrote:
> On 2013/11/13 21:02:03, danakj wrote:
> > Why this change? We don't bind this texture to be a FBO do we? We're binding
> the
> > |background_texture| below which is a different texture. I see us copying
this
> > texture into the FBO, or using it as an input to ApplyImageFilter, but not
> > binding it as an FBO?
> The device_background_texture's format has been recently changed from GL_RGB
to
> RGBA_8888, and this change has broken background filters on windows. RGBA_8888
> textures are supported by texture storage, therefore ResourceProvider uses
> texStorage2DEXT to allocate that texture. The actual failure happened on
> GetFramebufferTexture/copyTexImage2D call, which is not compatible with
storage
> textures.
> So, I changed the texture usage hint, as enne@ suggested
>
(https://codereview.chromium.org/23455060/diff/561001/cc/output/gl_renderer.cc...).
> If we Allocate() the texture using TextureUsageFramebuffer hint,
> ResourcePrivider will not use storage2d for this texture and copyTexImage2D
will
> work fine.
> I agree that the hint doesn't say what it actually does. Should I add another
> hint?
> > If this is not needed like it seems, then the ResourceProvider change could
go
> > away as well..
> 

If this is fixing background filters entirely, then it should be a separate CL
from this. We do have pixel tests for background filters and they have not
failed on windows, do you know why that is? Is it because you're adding a test
that does a background filter on a non-root surface?

I'd like to see this change with a test as a separate CL if possible. Adding a
comment to explain this here may be sufficient rather than another hint flag..
I'd be okay with either way.

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2082: DCHECK(layer->uses_default_blend_mode()
|| !layer->parent() ||
On 2013/11/14 21:56:34, Rosca wrote:
> On 2013/11/13 21:02:03, danakj wrote:
> > Can you add DCHECK(...) << each << of << the << variables << in << this <<
> > dcheck after it, so we can tell what failed when it does?
> They are disjunctions, so it will fail only when all of them are false. Maybe
I
> misunderstood your request.

Oh, no you're right :) I just knee-jerk when I see DCHECK() with multiple tests
inside.

rosca

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_renderer_layer.cc File cc/layers/delegated_renderer_layer.cc (right): https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_renderer_layer.cc#newcode110 cc/layers/delegated_renderer_layer.cc:110: static bool FrameDataRequiresOffscreenContext(const DelegatedFrameData* frame) { On 2013/11/20 03:32:50, ...

7 years, 1 month ago (2013-11-20 21:52:02 UTC) #52

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:110: static bool
FrameDataRequiresOffscreenContext(const DelegatedFrameData* frame) {
On 2013/11/20 03:32:50, danakj wrote:
> The idea was the setter has some more context about "what you're tying to do"
ie
> I want to use filters. Whereas the getter is disconnected from use cases, and
> just says that we need to make an offscreen context.
> 
> If there were other ways to do filters, then maybe a different getter would
> return true and the needs_offscreen_context would not. That's the idea here.
So
> where we are dealing with filters (ie outside of the code that actually is
> worrying about how we implement the filters) i would prefer that we don't tie
> ourselves to "OffscreenContext".
It makes sense now, thanks! Done.

https://codereview.chromium.org/23455060/diff/681001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.cc:140: if (layer_tree_host() &&
!layer_tree_host()->needs_offscreen_context() &&
On 2013/11/20 03:32:50, danakj wrote:
> This will only avoid the call when we've already seen a filter, which is by
far
> the minority of cases, so generally we'll be doing this call every time we get
a
> new frame anyways. If the check is a bottleneck we should make it faster. I'd
> prefer to avoid the if(needs_offscreen_context) here for that reason.
> 
> By the way, this code moved to Update() and I should have removed the
> layer_tree_host() null check. There's no way LTH can be null inside Update(),
we
> shouldn't be checking for it here.

Done.

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/23455060/diff/681001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:739: ResourceProvider::TextureUsageFramebuffer,
On 2013/11/20 03:32:50, danakj wrote:
> If this is fixing background filters entirely, then it should be a separate CL
> from this. We do have pixel tests for background filters and they have not
> failed on windows, do you know why that is? Is it because you're adding a test
> that does a background filter on a non-root surface?
I run into this problem by forcing some background filters in the code, I don't
have a real use case for it. Pixel tests use mesa gl library, which does not
support texture storage, so it will not have this problem. Angle on windows does
support texture storage.
> 
> I'd like to see this change with a test as a separate CL if possible. Adding a
> comment to explain this here may be sufficient rather than another hint flag..
> I'd be okay with either way.
I added a new CL with a ResourceProvider unittest:
https://codereview.chromium.org/79603002/

danakj

Thank you for your patience. This needs to be rebased on top of https://codereview.chromium.org/79603002/ I ...

7 years, 1 month ago (2013-11-21 00:22:35 UTC) #53

rosca

Thanks for your comments. I rebased it on top of https://codereview.chromium.org/79603002/. enne: I have one ...

7 years, 1 month ago (2013-11-21 13:04:44 UTC) #54

enne (OOO)

Thanks for your patience through this review process. This all still lgtm. https://codereview.chromium.org/23455060/diff/1541001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc ...

7 years, 1 month ago (2013-11-21 18:15:57 UTC) #55

danakj

On Fri, Nov 22, 2013 at 3:15 AM, <enne@chromium.org> wrote: > Thanks for your patience ...

7 years, 1 month ago (2013-11-22 01:46:53 UTC) #56

enne (OOO)

On 2013/11/22 01:46:53, danakj wrote: > > Hmm, on second thought this should be removed. ...

7 years ago (2013-11-25 20:19:44 UTC) #57

danakj

OK sounds good then. On Nov 26, 2013 5:19 AM, <enne@chromium.org> wrote: > On 2013/11/22 ...

7 years ago (2013-11-25 20:51:22 UTC) #58

commit-bot: I haz the power

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

7 years ago (2013-11-25 21:20:20 UTC) #59

commit-bot: I haz the power

Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=37997

7 years ago (2013-11-25 21:43:23 UTC) #60

jam

On 2013/11/25 21:43:23, I haz the power (commit-bot) wrote: > Retried try job too often ...

7 years ago (2013-11-25 21:52:12 UTC) #61

commit-bot: I haz the power

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

7 years ago (2013-11-26 08:28:46 UTC) #62

Message was sent while issue was closed.

Change committed as 237295

Issue 23455060: mix-blend-mode implementation for accelerated layers (Closed)

Description

Patch Set 1 #

Patch Set 2 : moving blend_mode property to SharedQuadState #

Patch Set 3 : has_blend_mode -> !uses_default_blend_mode and other small changes #

Patch Set 4 : Added compositor_bindungs #

Patch Set 5 : Adding compositor pixel tests, clang-format #

Patch Set 6 : rebase #

Patch Set 7 : Addressing comments #

Patch Set 8 : Addressing comments #40 #

Patch Set 9 : removed src/webkit/renderer/compositor_bindings #

Patch Set 10 : Addressing comments #47 #

Patch Set 11 : Added tests for occlusion #

Patch Set 12 : Small changes to cc blending pixeltests #

Patch Set 13 : Addressing comments #51 #

Patch Set 14 : Rebase #

Patch Set 15 : #

Patch Set 16 : unittests fixed #

Messages