Skip drawing unsupported layers in forced software mode

Skip drawing unsupported layers in forced software mode

Only draw PictureLayer, SolidColorLayer, and ScrollbarLayer
if using solid color scrollbars.

Changes contract of LayerImpl WillDraw/AppendQuads/DidDraw.
Now WillDraw returns a bool, and AppendQuads/DidDraw will be
skipped if it returns false.

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

[boliu, 2013-05-29 22:02:12]

Alex for first pass. Test forthcoming

[aelias_OOO_until_Jul13, 2013-05-29 22:30:47]

Also, let's add it to ScrollbarLayerImpl and there return "return
layer_tree_impl()->settings().solid_color_scrollbars;".

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

https://codereview.chromium.org/16211002/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:642: } else if
(!output_surface_->ForcedDrawToSoftwareDevice() ||
Seems like an arbitrary place to bail out... I suggest early-returning in
AppendQuadsForLayer() and AppendQuadsForRenderSurfaceLayer() based on the value
of append_quads_data.allow_tile_draw_quads .  (Alternatively we could exclude
this entire loop contents with a "continue" above occlusion_tracker.EnterLayer,
but I'm thinking the finer-grained early return would be easier to reason
about.)

[boliu, 2013-05-30 00:01:28]

Added test. ScrollbarLayerImpl change done.

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

https://codereview.chromium.org/16211002/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:642: } else if
(!output_surface_->ForcedDrawToSoftwareDevice() ||
On 2013/05/29 22:30:47, aelias wrote:
> Seems like an arbitrary place to bail out... I suggest early-returning in
> AppendQuadsForLayer() and AppendQuadsForRenderSurfaceLayer() based on the
value
> of append_quads_data.allow_tile_draw_quads .  (Alternatively we could exclude
> this entire loop contents with a "continue" above
occlusion_tracker.EnterLayer,
> but I'm thinking the finer-grained early return would be easier to reason
> about.)

Went with option 2 stead, skip the whole loop body if draw not supported. This
is because WillDraw/DidDraw call pairs might be doing somethings that are kind
of heavy (and possibly not supported?) in this mode. Eg, video layer acquires a
lock, texture layer creates resource, etc. Probably good to skip those as well.

[aelias_OOO_until_Jul13, 2013-05-30 00:46:51]

Looks fine to me, adding enne@ for OWNERS.

https://codereview.chromium.org/16211002/diff/5001/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16211002/diff/5001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:616: append_quads_data.allow_tile_draw_quads =
false;
Nit: I suggest moving up the AppendQuadsData to the top of the loop and merging
this into the same if() block.

https://codereview.chromium.org/16211002/diff/5001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:649: 
Nit: newline

[enne (OOO), 2013-05-30 17:04:37]

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

https://codereview.chromium.org/16211002/diff/11001/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:422: virtual bool CanDrawInTilelessSoftwareMode() const;
I am generally averse to yet more virtual boolean pseudo-RTTI functions on
LayerImpl, in part because it creates distance between the flag-checking code,
the code that's avoided, and this flag function itself.

I think I'd prefer making other layer types behave like PictureLayerImpl in that
you ask the layers and they make some decision to behave differently if needed.

What do you think about having WillDraw/AppendQuadsData take an explicit enum of
{ hardware, software, resourceless } and each layer type could do its best to
meet that request? WillDraw could even return a boolean to imply that the layer
can't support that mode.

That would also make testing easier.  You could have all layer types (there's a
test helper for this) crossed with all drawing modes so it'd be clear that all
layer types were safe in all modes.

https://codereview.chromium.org/16211002/diff/11001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16211002/diff/11001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:610: append_quads_data.allow_tile_draw_quads =
false;
This line is a little weird.  Shouldn't allow_tile_draw_quads be set every time
ForcedDrawToSoftwareDevice is on and not just when you first encounter a layer
that can draw?

[boliu, 2013-05-30 20:32:51]

First cut. WillDraw takes a DrawMode enum and returns a bool. If it returns
false, AppendQuads and DidDraw will not be called.

Had to implement WillDraw in almost all layers to skip resourceless software
mode. Saved the current mode in a protector var in LayerImpl. Does these
decisions feel reasonable?

Have not fixed tests yet. Still has lint errors, some leftover TODOs, require
other clean ups.

[enne (OOO), 2013-05-30 22:09:09]

Thanks.  It's a bit more verbose, but I like this direction a lot better.  Those
decisions seem reasonable.

[piman, 2013-05-30 22:18:17]

https://codereview.chromium.org/16211002/diff/16001/cc/layers/texture_layer_i...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/16001/cc/layers/texture_layer_i...
cc/layers/texture_layer_impl.cc:70: return false;
if uses_mailbox_ and we're not in DRAW_MODE_RESOURCELESS_SOFTWARE we still want
to draw (we just want to skip the CreateResourceFromExternalTexture)

[boliu, 2013-05-31 00:09:07]

Went through all the layers and tried to move early outs in AppendQuads into
WillDraw to return false. Please go through these and make sure I did the right
thing (piman already caught one mistake)

enne: could you point me to the test helper for going through all layers you
mentioned?

https://codereview.chromium.org/16211002/diff/16001/cc/layers/texture_layer_i...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/16001/cc/layers/texture_layer_i...
cc/layers/texture_layer_impl.cc:70: return false;
On 2013/05/30 22:18:18, piman wrote:
> if uses_mailbox_ and we're not in DRAW_MODE_RESOURCELESS_SOFTWARE we still
want
> to draw (we just want to skip the CreateResourceFromExternalTexture)

Done.

[enne (OOO), 2013-05-31 00:24:37]

On 2013/05/31 00:09:07, boliu wrote:
> Went through all the layers and tried to move early outs in AppendQuads into
> WillDraw to return false. Please go through these and make sure I did the
right
> thing (piman already caught one mistake)

I couldn't see any other mistakes.

> enne: could you point me to the test helper for going through all layers you
> mentioned?

I apologize--I was mistaken.  We have a "create all quads" function, but not a
"create all LayerImpl" one.  Sorry about that.

[piman, 2013-05-31 00:48:14]

https://codereview.chromium.org/16211002/diff/25001/cc/layers/texture_layer_i...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/25001/cc/layers/texture_layer_i...
cc/layers/texture_layer_impl.cc:70: return false;
still nak, because if uses_mailbox_, then texture_id_ will be 0.

Basically you have 2 modes (mailbox and non-mailbox).

In the mailbox mode, the resource is created in DidBecomeActive, if the mailbox
is not empty. If it's empty, we leave the resource as 0 and we skip drawing in
AppendQuads.

In the texture mode, the resource is created here if the texture is non-0,
otherwise we leave the resource as 0 and skip drawing in AppendQuads

You can refactor this as:
if (draw_mode == DRAW_MODE_RESOURCELESS_SOFTWARE)
  return false;
if (!uses_mailbox_) {
  DCHECK(!external_texture_resource_);
  external_texture_resource_ =
resource_provider->CreateResourceFromExternalTexture(
          GL_TEXTURE_2D,
          texture_id_);
}
return LayerImpl::WillDraw(draw_mode, resource_provider) &&
!!external_texture_resource_;

And then leave out the early return in AppendQuads.

[boliu, 2013-05-31 02:50:07]

Added tests. Ready for more thorough review now.

https://codereview.chromium.org/16211002/diff/25001/cc/layers/texture_layer_i...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/25001/cc/layers/texture_layer_i...
cc/layers/texture_layer_impl.cc:70: return false;
On 2013/05/31 00:48:15, piman wrote:
> still nak, because if uses_mailbox_, then texture_id_ will be 0.
> 
> Basically you have 2 modes (mailbox and non-mailbox).
> 
> In the mailbox mode, the resource is created in DidBecomeActive, if the
mailbox
> is not empty. If it's empty, we leave the resource as 0 and we skip drawing in
> AppendQuads.
> 
> In the texture mode, the resource is created here if the texture is non-0,
> otherwise we leave the resource as 0 and skip drawing in AppendQuads
> 
> You can refactor this as:
> if (draw_mode == DRAW_MODE_RESOURCELESS_SOFTWARE)
>   return false;
> if (!uses_mailbox_) {
>   DCHECK(!external_texture_resource_);
>   external_texture_resource_ =
> resource_provider->CreateResourceFromExternalTexture(
>           GL_TEXTURE_2D,
>           texture_id_);
> }
> return LayerImpl::WillDraw(draw_mode, resource_provider) &&
> !!external_texture_resource_;
> 
> And then leave out the early return in AppendQuads.

Second try! Should be fixed now. The second if condition should be
(!uses_mailbox_ && texture_id_). Also added a test just for this.

[enne (OOO), 2013-05-31 16:58:08]

lgtm

[boliu, 2013-05-31 17:55:41]

piman: Could you take a look at texture layer again?

[piman, 2013-05-31 18:08:41]

LGTM, thanks for the extra tests!

[commit-bot: I haz the power, 2013-05-31 18:18:15]

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

[boliu, 2013-05-31 23:34:47]

Alex pointed out that GetDraw had a mistake. ForcedDrawToSoftwareDevice() need
to override context3d() and software_device(). There is no other change from
Patch Set 8.

As a follow up refactor, probably should move that logic to a method on
OutputSurface, replacing ForcedDrawToSoftwareDevice.

[aelias_OOO_until_Jul13, 2013-05-31 23:37:21]

New GetDrawMode() logic LGTM.

[commit-bot: I haz the power, 2013-05-31 23:37:39]

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

[commit-bot: I haz the power, 2013-06-02 19:54:30]

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

[commit-bot: I haz the power, 2013-06-02 23:48:00]

Change committed as 203637

[danakj, 2013-06-04 15:30:43]

Nice change! I think a couple layers got missed in the later refactors of the CL
though?

https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer...
File cc/layers/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_impl.cc:79: LayerImpl::WillDraw(draw_mode,
resource_provider);
This call should only be made when returning true right? And it should early-out
before it when the return will be false?

https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_imp...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_imp...
cc/layers/video_layer_impl.cc:94: LayerImpl::WillDraw(draw_mode,
resource_provider);
In other layers we always return this value. Should we return false here if this
returns false?

[boliu, 2013-06-04 15:39:08]

On 2013/06/04 15:30:43, danakj wrote:
> Nice change! I think a couple layers got missed in the later refactors of the
CL
> though?
> 
>
https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer...
> File cc/layers/scrollbar_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer...
> cc/layers/scrollbar_layer_impl.cc:79: LayerImpl::WillDraw(draw_mode,
> resource_provider);
> This call should only be made when returning true right? And it should
early-out
> before it when the return will be false?
> 
>
https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_imp...
> File cc/layers/video_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_imp...
> cc/layers/video_layer_impl.cc:94: LayerImpl::WillDraw(draw_mode,
> resource_provider);
> In other layers we always return this value. Should we return false here if
this
> returns false?

These two are semi-sketchy dependency on the fact that LayerImpl::WillDraw is
essentially no-op and always returns true. I thought about make it a protected
helper method and make WillDraw pure virtual in LayerImpl, but LayerImpl is a
concrete class.

I can send a fix up to remove this dependency.

[danakj, 2013-06-04 15:45:04]

On Tue, Jun 4, 2013 at 11:39 AM, <boliu@chromium.org> wrote:

> On 2013/06/04 15:30:43, danakj wrote:
>
>> Nice change! I think a couple layers got missed in the later refactors of
>> the
>>
> CL
>
>> though?
>>
>
>
> https://codereview.chromium.**org/16211002/diff/74001/cc/**
>
layers/scrollbar_layer_impl.cc<https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer_impl.cc>
>
>> File cc/layers/scrollbar_layer_**impl.cc (right):
>>
>
>
> https://codereview.chromium.**org/16211002/diff/74001/cc/**
>
layers/scrollbar_layer_impl.**cc#newcode79<https://codereview.chromium.org/16211002/diff/74001/cc/layers/scrollbar_layer_impl.cc#newcode79>
>
>> cc/layers/scrollbar_layer_**impl.cc:79: LayerImpl::WillDraw(draw_mode,
>> resource_provider);
>> This call should only be made when returning true right? And it should
>>
> early-out
>
>> before it when the return will be false?
>>
>
>
> https://codereview.chromium.**org/16211002/diff/74001/cc/**
>
layers/video_layer_impl.cc<https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_impl.cc>
>
>> File cc/layers/video_layer_impl.cc (right):
>>
>
>
> https://codereview.chromium.**org/16211002/diff/74001/cc/**
>
layers/video_layer_impl.cc#**newcode94<https://codereview.chromium.org/16211002/diff/74001/cc/layers/video_layer_impl.cc#newcode94>
>
>> cc/layers/video_layer_impl.cc:**94: LayerImpl::WillDraw(draw_mode,
>> resource_provider);
>> In other layers we always return this value. Should we return false here
>> if
>>
> this
>
>> returns false?
>>
>
> These two are semi-sketchy dependency on the fact that LayerImpl::WillDraw
> is
> essentially no-op and always returns true. I thought about make it a
> protected
> helper method and make WillDraw pure virtual in LayerImpl, but LayerImpl
> is a
> concrete class.
>

Yeh. I'm just thinking about the fact that it could now return false one
day for some reason we don't forsee, and then these would break.


>
> I can send a fix up to remove this dependency.
>
>
https://codereview.chromium.**org/16211002/<https://codereview.chromium.org/1...
>