Move tile-free-software render surface disabling to UpdateDrawProperties.

Move tile-free-software render surface disabling to UpdateDrawProperties.

Currently when DrawProperties decides a subtree needs a render surface,
Android WebView software mode just drops it at CalculateRenderPasses
time and draws nothing.  If we instead suppress the render surface
decision at an earlier stage, we're more likely to get a halfway
reasonable result.

The motivating example was a page setting preserve-3d where the render
surface didn't affect the output result anyway.

New test LayerTreeHostCommonTest.CanRenderToSeparateSurface.

NOTRY=true
BUG=297299
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225637

[aelias_OOO_until_Jul13, 2013-09-23 23:47:50]

PTAL, another incremental workaround for lack of intermediate buffers in
software WebView.

[danakj, 2013-09-24 14:47:04]

I have two reservations about this CL:

1) Making the RSLL diverge greatly on the main thread side produces wildly
different views of the world. Maybe that's just fine under impl painting,
however, so this isn't a big deal?


2) By just ignoring surfaces, you can end up with radically incorrect web page
output.. In the case where the surface has no effect.. sure.. what about cases
where it is supposed to have an effect? Maybe missing content is better than
crazy wrong content that ends up covering things it should not, etc?

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

https://codereview.chromium.org/24280009/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:1456: if
(globals.can_render_to_separate_surface)
{}

https://codereview.chromium.org/24280009/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/24280009/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:882: // Should only have one render pass in
forced-draw mode.
in resourceless software mode.

[enne (OOO), 2013-09-24 15:40:07]

I think the preserves-3d creating unnecessary surfaces bug could be fixed
separately as a performance optimization.  That seems like it would affect other
cases too.

Am I remembering correctly that the reason the resourceless software renderer
can't handle surfaces is that DirectRenderer tries to ask ResourceProvider to
create bitmap "surfaces" rather than doing SkCanvas calls to
saveLayer/restoreLayer?

On 2013/09/24 14:47:04, danakj wrote:

> 1) Making the RSLL diverge greatly on the main thread side produces wildly
> different views of the world. Maybe that's just fine under impl painting,
> however, so this isn't a big deal?

I think it'll mostly be ok.  Aside from target differences causing different
draw transforms, the visible content rects will still be the same for these
layers modulo floating point precision.  I think most of the difference that
subtrees can't be skipped on the main thread due to animating opacity.  I don't
like having this extra divergence that much because it's just additional
complication to remember, but I don't think it's that problematic in this case
in practice.

> 2) By just ignoring surfaces, you can end up with radically incorrect web page
> output.. In the case where the surface has no effect.. sure.. what about cases
> where it is supposed to have an effect? Maybe missing content is better than
> crazy wrong content that ends up covering things it should not, etc?

What does the legacy WebView do for these cases?

[aelias_OOO_until_Jul13, 2013-09-24 18:47:51]

On 2013/09/24 15:40:07, enne wrote:
> I think the preserves-3d creating unnecessary surfaces bug could be fixed
> separately as a performance optimization.  That seems like it would affect
other
> cases too.
> 

Hmm, this is a good idea.  Would it be correct to not have a render surface if
the child subtree has no perspective transforms?

> Am I remembering correctly that the reason the resourceless software renderer
> can't handle surfaces is that DirectRenderer tries to ask ResourceProvider to
> create bitmap "surfaces" rather than doing SkCanvas calls to
> saveLayer/restoreLayer?
> 

Correct.

> On 2013/09/24 14:47:04, danakj wrote:
> > 2) By just ignoring surfaces, you can end up with radically incorrect web
page
> > output.. In the case where the surface has no effect.. sure.. what about
cases
> > where it is supposed to have an effect? Maybe missing content is better than
> > crazy wrong content that ends up covering things it should not, etc?
> 
> What does the legacy WebView do for these cases?

I don't know what the legacy WebView software mode does in most of the cases, as
apps usually don't try to use the features that aren't supported.  It would be
some kind of degradation.

In my opinion doing a best-effort imperfect rendering is more "webby" than
dropping it entirely.  Rendering nothing reminds me of XHTML that turns your
site into an error page when you make a typo.

[shawnsingh, 2013-09-24 18:58:34]

On 2013/09/24 18:47:51, aelias wrote:
> On 2013/09/24 15:40:07, enne wrote:
> > I think the preserves-3d creating unnecessary surfaces bug could be fixed
> > separately as a performance optimization.  That seems like it would affect
> other
> > cases too.
> > 
> 
> Hmm, this is a good idea.  Would it be correct to not have a render surface if
> the child subtree has no perspective transforms?

What is the preserve-3d issue?  I'm not seeing any mention of it in this
discussion or on the bug...  I'm also not seeing where we create unnecessary
surfaces due to preserve-3d?

[aelias_OOO_until_Jul13, 2013-09-24 19:36:12]

On 2013/09/24 18:58:34, shawnsingh wrote:
> What is the preserve-3d issue?  I'm not seeing any mention of it in this
> discussion or on the bug...  I'm also not seeing where we create unnecessary
> surfaces due to preserve-3d?

The unit test in this patch demonstrates the scenario.

Here is the code that causes the extra surface:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

[shawnsingh, 2013-09-24 20:51:59]

On 2013/09/24 19:36:12, aelias wrote:
> On 2013/09/24 18:58:34, shawnsingh wrote:
> > What is the preserve-3d issue?  I'm not seeing any mention of it in this
> > discussion or on the bug...  I'm also not seeing where we create unnecessary
> > surfaces due to preserve-3d?
> 
> The unit test in this patch demonstrates the scenario.
> 
> Here is the code that causes the extra surface:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Could we use setForceRenderSurface(true) for this scenario, and make sure to
define that SubtreeGlobals::can_render_to_separate_surface takes precedence over
Layer::force_render_surface?

If we want to be more aggressive about not-creating-surfaces-for-preserve3d,
perspective is not enough of a condition.  It's tricker than that. 
"perserve-3d" is a misnomer from spec.  What it really should be called is
"isIn3dRenderingContext".  There are some complexities to doing this that we can
discuss when this comes up.  =)

[enne (OOO), 2013-09-26 18:53:34]

lgtm, modulo danakj's comments

Filed issue 299105 for optimizing this behavior.

[aelias_OOO_until_Jul13, 2013-09-27 01:57:38]

Thanks.  Nits applied, landing.

[commit-bot: I haz the power, 2013-09-27 01:58:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/24280009/15001

[commit-bot: I haz the power, 2013-09-27 02:46:50]

Change committed as 225637