Pass resourceless software mode in BeginFrameArgs

Pass resourceless software mode in SetExternalConstraints

This fixes the bug that resourceless software mode not
dirtying the draw properties.

This allows OutputSurface::ForcedDrawToSoftwareDevice to be
removed. And merged the valid_for_tile_management parameter
into resourceless_software_draw. This effectively reverses
the last bool parameter on SetExternalDrawConstraints.

BUG=391829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282445

[boliu, 2014-07-07 22:21:43]

PS1 adds BeginFrameArgs::allow_separate_surface, and PS2 removes
ForcedDrawToSoftwareDevice on top of PS1. All still work in progress obviously.

Two things:

This CL ended up being bigger than I thought. It's the per-frame variable that's
plumbed this way I think.

I think having the draw mode in BeginFrameArgs is not exactly right. This will
make it impossible for webview to separate BeginFrame and Deadline calls. It's
more like an argument to Deadline, maybe it should be separated out as such.

Thoughts?

[boliu, 2014-07-08 02:21:40]

Actually SetExternalDrawConstaints feels like a better fit than BeginFrameArgs.
Please take a look at PS3.

[boliu, 2014-07-08 20:33:49]

ping?

[brianderson, 2014-07-08 21:26:12]

https://codereview.chromium.org/376683004/diff/80001/cc/output/output_surface.cc
File cc/output/output_surface.cc (left):

https://codereview.chromium.org/376683004/diff/80001/cc/output/output_surface...
cc/output/output_surface.cc:278: #if 0
I'm removing this in https://codereview.chromium.org/323423005. There's a bunch
of other logic that can be removed too. Will rebase that patch.

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1314: resourceless_software_draw_ !=
resourceless_software_draw) {
Is this the part that fixes the bug?

Was the point of removing OutputSurface::ForcedDrawToSoftwareDevice just to
clean up code? Are ForcedDrawToSoftwareDevice and resourceless_software_draw_
equivalent?

[boliu, 2014-07-08 21:31:14]

https://codereview.chromium.org/376683004/diff/80001/cc/output/output_surface.cc
File cc/output/output_surface.cc (left):

https://codereview.chromium.org/376683004/diff/80001/cc/output/output_surface...
cc/output/output_surface.cc:278: #if 0
On 2014/07/08 21:26:12, brianderson wrote:
> I'm removing this in https://codereview.chromium.org/323423005. There's a
bunch
> of other logic that can be removed too. Will rebase that patch.

Yeah presubmit wouldn't let me upload if I don't remove this :/

Looks like yours can go in now? You can cq now and I can rebase after if you
want.

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1314: resourceless_software_draw_ !=
resourceless_software_draw) {
On 2014/07/08 21:26:12, brianderson wrote:
> Is this the part that fixes the bug?
> 

Yes

> Was the point of removing OutputSurface::ForcedDrawToSoftwareDevice just to
> clean up code? Are ForcedDrawToSoftwareDevice and resourceless_software_draw_
> equivalent?

Yes they are meant to be equivalent. It's more of a clean up. See the discussion
in crbug.com/391829

[danakj, 2014-07-09 17:21:07]

https://codereview.chromium.org/376683004/diff/80001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/376683004/diff/80001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:32:
SetExternalDrawConstraints(identity, empty, clip, false, true);
nit: use a temp var to give the boolean literal a name (for the "true" too)?

https://codereview.chromium.org/376683004/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:322: bool resourceless_software_draw =
true;
why true? i just expected false for some reason..

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:553: static DrawMode GetDrawMode(bool
resourceless_software,
how about making this not static, and stop passing things in, then we can make
this method public, and use that from LTI, instead of making the
resourceless_software_mode_ flag public, to avoid different ways of checking for
the same thing?

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:921: if (resourceless_software_draw_)
can you change this to be based on the draw_mode, and can you move this into the
DCHECK so we don't pay for this when DCHECKs are off?

like DCHECK(draw_mode != RESOURCLESS || passes.size() == 1u) << passes.size();

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1471: GetDrawMode(resourceless_software_draw_,
output_surface_.get()),
can you move this GetDrawMode up above the if statement, and use the draw_mode
below instead of resourceless_software_draw_?

[danakj, 2014-07-09 17:22:01]

Do you think the whole external draw constraints could move into the
BeginFrameArgs maybe afterward? I agree it fits well with them, but I wonder if
they could be plumbed through the same path as other args we use to draw a
single frame with?

[brianderson, 2014-07-09 17:29:29]

On 2014/07/09 17:22:01, danakj wrote:
> Do you think the whole external draw constraints could move into the
> BeginFrameArgs maybe afterward? I agree it fits well with them, but I wonder
if
> they could be plumbed through the same path as other args we use to draw a
> single frame with?

Would like to discuss this more and how it would interact with retroactive
BeginFrames. If we get two BeginFrames with different draw constraints before
drawing, should the draw for the first BeginFrame use the newer draw
constraints?

[boliu, 2014-07-09 17:55:12]

On 2014/07/09 17:29:29, brianderson wrote:
> On 2014/07/09 17:22:01, danakj wrote:
> > Do you think the whole external draw constraints could move into the
> > BeginFrameArgs maybe afterward? I agree it fits well with them, but I wonder
> if
> > they could be plumbed through the same path as other args we use to draw a
> > single frame with?
> 
> Would like to discuss this more and how it would interact with retroactive
> BeginFrames. If we get two BeginFrames with different draw constraints before
> drawing, should the draw for the first BeginFrame use the newer draw
> constraints?

One concern I had for webview for moving this to BeginFrameArgs is that we'll
never be able to separate out BeginFrame vs Deadline into separate calls. So
this should all be in "deadline arguments" or "draw arguments" rather than
"begin frame arguments". Not sure if that covers Brian's concern.

Also we are proposing some changes to SetExternalDrawConstraints for
crbug.com/372073. hush@ is writing up a doc on the requirements, I think he'll
send it out later today. Maybe wait until that settles a bit and see where this
stands.

[boliu, 2014-07-09 18:46:58]

Dana, how do you feel about merging resourceless_software_draw and
valid_for_tile_management? Right now:

valid_for_tile_management == !resourceless_software_draw

and I don't think that is going to change any time soon. Depending on how
crbug.com/372073 goes, there might be more stuff in cc that will be conditioned
on resourceless_software_draw, so this is like a start.

[danakj, 2014-07-09 18:52:18]

On 2014/07/09 18:46:58, boliu wrote:
> Dana, how do you feel about merging resourceless_software_draw and
> valid_for_tile_management? Right now:
> 
> valid_for_tile_management == !resourceless_software_draw
> 
> and I don't think that is going to change any time soon. Depending on how
> crbug.com/372073 goes, there might be more stuff in cc that will be
conditioned
> on resourceless_software_draw, so this is like a start.

less variables/code sgtm

[boliu, 2014-07-09 20:59:50]

Merged valid_for_tile_management into resourceless_software_draw, so effectively
reversed the meaning of the bool in SetExternalDrawConstraints. Good thing this
was on top adding a bool, so probalby didn't miss any call sites :)

PTAL

https://codereview.chromium.org/376683004/diff/80001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/376683004/diff/80001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:32:
SetExternalDrawConstraints(identity, empty, clip, false, true);
On 2014/07/09 17:21:07, danakj wrote:
> nit: use a temp var to give the boolean literal a name (for the "true" too)?

Done.

https://codereview.chromium.org/376683004/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:322: bool resourceless_software_draw =
true;
On 2014/07/09 17:21:07, danakj wrote:
> why true? i just expected false for some reason..

Yep, this is false after valid_for_tile_management is merged into
resourceless_software_draw.

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:553: static DrawMode GetDrawMode(bool
resourceless_software,
On 2014/07/09 17:21:07, danakj wrote:
> how about making this not static, and stop passing things in, then we can make
> this method public, and use that from LTI, instead of making the
> resourceless_software_mode_ flag public, to avoid different ways of checking
for
> the same thing?

Done.

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:921: if (resourceless_software_draw_)
On 2014/07/09 17:21:07, danakj wrote:
> can you change this to be based on the draw_mode, and can you move this into
the
> DCHECK so we don't pay for this when DCHECKs are off?
> 
> like DCHECK(draw_mode != RESOURCLESS || passes.size() == 1u) << passes.size();

Done.

https://codereview.chromium.org/376683004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1471: GetDrawMode(resourceless_software_draw_,
output_surface_.get()),
On 2014/07/09 17:21:07, danakj wrote:
> can you move this GetDrawMode up above the if statement, and use the draw_mode
> below instead of resourceless_software_draw_?

Done.

[danakj, 2014-07-10 18:33:59]

https://codereview.chromium.org/376683004/diff/140001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:404: if
(!layer_tree_impl()->resourceless_software_draw()) {
Thanks for making them all resourceless_software_draw instead of making them all
valid_for_tile_management :D

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
cc/output/software_renderer.cc:475: DCHECK(resource_provider_);
is this adding some value that i don't see? i'd just remove this line

https://codereview.chromium.org/376683004/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:682: bool
LayerTreeImpl::resourceless_software_draw() const {
can we get rid of this method, is it used elsewhere? can we just call
GetDrawMode() in UpdateDrawProperties?

[boliu, 2014-07-10 18:40:36]

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
cc/output/software_renderer.cc:475: DCHECK(resource_provider_);
On 2014/07/10 18:33:59, danakj wrote:
> is this adding some value that i don't see? i'd just remove this line

This was supposed to check resourceless software draw never generates tile
quads. I could have added a method to RendererClient just to keep this DCHECK,
but I thought that was overkill, so added the somewhat equivalent
resource_provider_ check.

Umm, maybe just need a comment?

https://codereview.chromium.org/376683004/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:682: bool
LayerTreeImpl::resourceless_software_draw() const {
On 2014/07/10 18:33:59, danakj wrote:
> can we get rid of this method, is it used elsewhere? can we just call
> GetDrawMode() in UpdateDrawProperties?

This is used by PictureLayerImpl, which doesn't need to know the other draw
modes.

But I can pass through GetDrawMode if you prefer?

[danakj, 2014-07-10 18:43:21]

ok cool, a comment on that DCHECK would be cool. LGTM

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/376683004/diff/140001/cc/output/software_rend...
cc/output/software_renderer.cc:475: DCHECK(resource_provider_);
On 2014/07/10 18:40:36, boliu wrote:
> On 2014/07/10 18:33:59, danakj wrote:
> > is this adding some value that i don't see? i'd just remove this line
> 
> This was supposed to check resourceless software draw never generates tile
> quads. I could have added a method to RendererClient just to keep this DCHECK,
> but I thought that was overkill, so added the somewhat equivalent
> resource_provider_ check.
> 
> Umm, maybe just need a comment?

Oh, I see this is NULL in resourceless mode.

[boliu, 2014-07-10 18:50:26]

+sievers for synchronous compositor

[boliu, 2014-07-10 18:50:55]

Oh, added that comment in software_renderer

[no sievers, 2014-07-10 19:13:36]

synchronous compositor changes lgtm

https://codereview.chromium.org/376683004/diff/180001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/376683004/diff/180001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:222:
bool resourceless_software_draw = false;
do we need this variable? :)

[danakj, 2014-07-10 19:14:37]

https://codereview.chromium.org/376683004/diff/180001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/376683004/diff/180001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:222:
bool resourceless_software_draw = false;
On 2014/07/10 19:13:36, sievers wrote:
> do we need this variable? :)

heh, i nitted for it, cuz passing boolean literals to functions is super unclear
what it means >_>

[boliu, 2014-07-10 19:37:25]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-07-10 19:39:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/376683004/180001

[commit-bot: I haz the power, 2014-07-10 22:49:16]

Change committed as 282445