Draw omnibox shadow with a ninebox layer.

Draw omnibox shadow with a ninebox layer.

This will tile the shadow image in hardware rather than software. The
visual result is unchanged.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[Evan Stade, 2016-12-01 02:30:40]

Description was changed from

==========
Draw omnibox shadow with a ninebox layer.

This will tile the shadow image in hardware rather than software. The
visual result is unchanged.

BUG=
==========

to

==========
Draw omnibox shadow with a ninebox layer.

This will tile the shadow image in hardware rather than software. The
visual result is unchanged.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Evan Stade, 2016-12-01 02:39:01]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org

[Evan Stade, 2016-12-01 02:39:01]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org

[Evan Stade, 2016-12-01 02:39:02]

Hi Sadrul, could you take a look? I think this is along the lines of what we
discussed while I was in WAT. I have a couple questions inline, plus these:

a) is there a bug for this work? should we file one to track it?
b) is there a handy way to measure perf impact or any guidelines about how we'd
do that? We have some omnibox time-to-repaint UMA but it would be nice to try to
predict perf changes before committing (or needing to write a finch experiment).

https://codereview.chromium.org/2543473005/diff/1/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/2543473005/diff/1/cc/layers/nine_patch_layer....
cc/layers/nine_patch_layer.h:30: // TODO(estade): this really ought to be a
gfx::Insets and not a gfx::Rect. To
does this make sense? This API was a bit of a head scratcher for me and it's
pretty hard to debug when you can't figure out what the rect is supposed to be.

In fact, I'm not sure why we even need SetBorder. It seems like it's redundant
once you know the occlusion and the aperture.

In fact^2, it seems like aperture and layer bounds should be enough. Why do we
even need occlusion? Seems like we should just assume the outer edges of the
ninebox align to the edges of the layer's bounds.

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:290: // parented.
I'm not entirely sure why this is necessary --- it took me a while to figure out
the layer wasn't being properly reparented and I didn't track down why.

[Evan Stade, 2016-12-01 02:39:02]

Hi Sadrul, could you take a look? I think this is along the lines of what we
discussed while I was in WAT. I have a couple questions inline, plus these:

a) is there a bug for this work? should we file one to track it?
b) is there a handy way to measure perf impact or any guidelines about how we'd
do that? We have some omnibox time-to-repaint UMA but it would be nice to try to
predict perf changes before committing (or needing to write a finch experiment).

https://codereview.chromium.org/2543473005/diff/1/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/2543473005/diff/1/cc/layers/nine_patch_layer....
cc/layers/nine_patch_layer.h:30: // TODO(estade): this really ought to be a
gfx::Insets and not a gfx::Rect. To
does this make sense? This API was a bit of a head scratcher for me and it's
pretty hard to debug when you can't figure out what the rect is supposed to be.

In fact, I'm not sure why we even need SetBorder. It seems like it's redundant
once you know the occlusion and the aperture.

In fact^2, it seems like aperture and layer bounds should be enough. Why do we
even need occlusion? Seems like we should just assume the outer edges of the
ninebox align to the edges of the layer's bounds.

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:290: // parented.
I'm not entirely sure why this is necessary --- it took me a while to figure out
the layer wasn't being properly reparented and I didn't track down why.

[Evan Stade, 2016-12-01 02:40:08]

oops, sorry for the double send. I also meant to say +pkasting as FYI for now
and review later, but you can feel free to wait until Sadrul provides some
initial feedback.

[Peter Kasting, 2016-12-01 04:43:47]

Does drawing this in hardware get us some power savings (or some other nicety)?

[Evan Stade, 2016-12-01 17:33:21]

On 2016/12/01 04:43:47, Peter Kasting wrote:
> Does drawing this in hardware get us some power savings (or some other
nicety)?

Sadrul can answer this better than I can (this CL was his idea), but to my
understanding, perf improvements both in memory and time. I would imagine that
power savings tend to follow time perf improvements in that fewer cycles use
less power.

[sadrul, 2016-12-06 20:05:21]

sadrul@chromium.org changed reviewers:
+ danakj@chromium.org

[sadrul, 2016-12-06 20:05:22]

[sorry about the late review, I have been out sick for the last few days]

+danakj@ for cc/, and also for the discussion below.

On 2016/12/01 17:33:21, Evan Stade wrote:
> On 2016/12/01 04:43:47, Peter Kasting wrote:
> > Does drawing this in hardware get us some power savings (or some other
> nicety)?
> 
> Sadrul can answer this better than I can (this CL was his idea), but to my
> understanding, perf improvements both in memory and time. I would imagine that
> power savings tend to follow time perf improvements in that fewer cycles use
> less power.

I don't know that this saves us a lot, either in memory or time. It looks like
the popup-view itself also has a new layer after this change, whereas it doesn't
have one now. This would mean we are using more memory.

> a) is there a bug for this work? should we file one to track it?

I haven't filed any bugs for this yet. We probably should have some bugs
tracking the work.

> b) is there a handy way to measure perf impact or any guidelines about how
we'd
> do that? We have some omnibox time-to-repaint UMA but it would be nice to try
to
> predict perf changes before committing (or needing to write a finch
experiment).

https://cs.chromium.org/chromium/src/docs/memory-infra/ has some useful
documents. You could use tracing to see some of the measurements. I also have
some old patches; I will attempt to dig those out.

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:290: // parented.
On 2016/12/01 02:39:02, Evan Stade wrote:
> I'm not entirely sure why this is necessary --- it took me a while to figure
out
> the layer wasn't being properly reparented and I didn't track down why.

Yeah ... this seems weird.

https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:413:
shadow_layer_->UpdateNinePatchOcclusion(shadow_bounds);
ui::Shadow seems to update layer-border/aperture as well. Should this do that
too?

[danakj, 2016-12-06 21:41:44]

On 2016/12/06 20:05:22, sadrul (OOO) wrote:
> [sorry about the late review, I have been out sick for the last few days]
> 
> +danakj@ for cc/, and also for the discussion below.
> 
> On 2016/12/01 17:33:21, Evan Stade wrote:
> > On 2016/12/01 04:43:47, Peter Kasting wrote:
> > > Does drawing this in hardware get us some power savings (or some other
> > nicety)?
> > 
> > Sadrul can answer this better than I can (this CL was his idea), but to my
> > understanding, perf improvements both in memory and time. I would imagine
that
> > power savings tend to follow time perf improvements in that fewer cycles use
> > less power.
> 
> I don't know that this saves us a lot, either in memory or time. It looks like
> the popup-view itself also has a new layer after this change, whereas it
doesn't
> have one now. This would mean we are using more memory.

So this is slightly complicated and need to think about the whole system.

The strength of NinePatchLayer is that it can use UIResources.
https://cs.chromium.org/chromium/src/cc/layers/ui_resource_layer.h?rcl=148104...

UIResources are upload-once-use-many things that cc manages for you. So when you
have 5 NinePatch layers and they all use the same UIResource, you have memory
for the bitmap and the texture only once. If, however, you give a bitmap to
every NinePatch layer (what this patch is doing) it can't tell they are the same
and it will upload it for each layer so you get 1 bitmap 5 textures which is
worse for memory.

If you use a PictureLayer (ie views painting) it can't share anything so you get
1 bitmap (since we share it as a global here) placed into 5 recordings, rastered
into 5 tilings (1 per layer). That's poor for memory *and* has additional
CPU/GPU overhead to raster the bitmap into a tiling in this case.

So if the # of layers is constant the cost is roughly
cpu/gpu: PictureLayer/painting > NinePatch with bitmap = NinePatch with
UIResource
memory: PictureLayer/painting = NinePatch with bitmap > NinePatch with
UIResource

If the number of layers is changing then you have to take into account the
*overlapping* surface area of the layers, and double count that for both costs.
There's also some additional computational overhead per layer but it's small per
layer (adds up for hundreds of layers).

Does this help?

> > a) is there a bug for this work? should we file one to track it?
> 
> I haven't filed any bugs for this yet. We probably should have some bugs
> tracking the work.
> 
> > b) is there a handy way to measure perf impact or any guidelines about how
> we'd
> > do that? We have some omnibox time-to-repaint UMA but it would be nice to
try
> to
> > predict perf changes before committing (or needing to write a finch
> experiment).
> 
> https://cs.chromium.org/chromium/src/docs/memory-infra/ has some useful
> documents. You could use tracing to see some of the measurements. I also have
> some old patches; I will attempt to dig those out.
> 
>
https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):
> 
>
https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:290: //
parented.
> On 2016/12/01 02:39:02, Evan Stade wrote:
> > I'm not entirely sure why this is necessary --- it took me a while to figure
> out
> > the layer wasn't being properly reparented and I didn't track down why.
> 
> Yeah ... this seems weird.
> 
>
https://codereview.chromium.org/2543473005/diff/1/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:413:
> shadow_layer_->UpdateNinePatchOcclusion(shadow_bounds);
> ui::Shadow seems to update layer-border/aperture as well. Should this do that
> too?

[Evan Stade, 2016-12-06 23:25:48]

> So this is slightly complicated and need to think about the whole system.
> 
> The strength of NinePatchLayer is that it can use UIResources.
>
https://cs.chromium.org/chromium/src/cc/layers/ui_resource_layer.h?rcl=148104...
> 
> UIResources are upload-once-use-many things that cc manages for you. So when
you
> have 5 NinePatch layers and they all use the same UIResource, you have memory
> for the bitmap and the texture only once. If, however, you give a bitmap to
> every NinePatch layer (what this patch is doing) it can't tell they are the
same
> and it will upload it for each layer so you get 1 bitmap 5 textures which is
> worse for memory.
> 
> If you use a PictureLayer (ie views painting) it can't share anything so you
get
> 1 bitmap (since we share it as a global here) placed into 5 recordings,
rastered
> into 5 tilings (1 per layer). That's poor for memory *and* has additional
> CPU/GPU overhead to raster the bitmap into a tiling in this case.
> 
> So if the # of layers is constant the cost is roughly
> cpu/gpu: PictureLayer/painting > NinePatch with bitmap = NinePatch with
> UIResource
> memory: PictureLayer/painting = NinePatch with bitmap > NinePatch with
> UIResource
> 
> If the number of layers is changing then you have to take into account the
> *overlapping* surface area of the layers, and double count that for both
costs.
> There's also some additional computational overhead per layer but it's small
per
> layer (adds up for hundreds of layers).
> 
> Does this help?

yes, thanks. So to be clear, the bitmap we're using for the ninebox is 1x11px.
Even if we have to upload that for every omnibox layer, that bitmap in itself
seems like a pretty small memory footprint. The bitmap is tiled many times
horizontally (like 1000 times if your screen is 1000px wide). Are you saying
that this uses up memory proportional to the number of times it's tiled? Before
this patch, i.e. using PictureLayer, I would expect that to be true, but I had
hoped this patch would address that.

How would one use a UIResourceLayer? In codesearch, I don't see any uses outside
of tests of UIResourceLayer::SetUIResourceId.

[danakj, 2016-12-06 23:32:37]

On Tue, Dec 6, 2016 at 3:25 PM, <estade@chromium.org> wrote:

> > So this is slightly complicated and need to think about the whole system.
> >
> > The strength of NinePatchLayer is that it can use UIResources.
> >
> https://cs.chromium.org/chromium/src/cc/layers/ui_resource_
> layer.h?rcl=1481041267&l=33
> >
> > UIResources are upload-once-use-many things that cc manages for you. So
> when
> you
> > have 5 NinePatch layers and they all use the same UIResource, you have
> memory
> > for the bitmap and the texture only once. If, however, you give a bitmap
> to
> > every NinePatch layer (what this patch is doing) it can't tell they are
> the
> same
> > and it will upload it for each layer so you get 1 bitmap 5 textures
> which is
> > worse for memory.
> >
> > If you use a PictureLayer (ie views painting) it can't share anything so
> you
> get
> > 1 bitmap (since we share it as a global here) placed into 5 recordings,
> rastered
> > into 5 tilings (1 per layer). That's poor for memory *and* has additional
> > CPU/GPU overhead to raster the bitmap into a tiling in this case.
> >
> > So if the # of layers is constant the cost is roughly
> > cpu/gpu: PictureLayer/painting > NinePatch with bitmap = NinePatch with
> > UIResource
> > memory: PictureLayer/painting = NinePatch with bitmap > NinePatch with
> > UIResource
> >
> > If the number of layers is changing then you have to take into account
> the
> > *overlapping* surface area of the layers, and double count that for both
> costs.
> > There's also some additional computational overhead per layer but it's
> small
> per
> > layer (adds up for hundreds of layers).
> >
> > Does this help?
>
> yes, thanks. So to be clear, the bitmap we're using for the ninebox is
> 1x11px.
> Even if we have to upload that for every omnibox layer, that bitmap in
> itself
> seems like a pretty small memory footprint. The bitmap is tiled many times
> horizontally (like 1000 times if your screen is 1000px wide). Are you
> saying
> that this uses up memory proportional to the number of times it's tiled?
> Before
> this patch, i.e. using PictureLayer, I would expect that to be true, but I
> had
> hoped this patch would address that.
>

With PictureLayer in the UI we will raster it without applying the layer's
transform. In blink we do differently and raster based on the layer
transform. So these should both look and use the same memory WRT layer
transform/scaling.

However here you're saying tiled, I did not know that UIResourceLayer does
repeating textures which is how i read that, so I wonder if you mean
scaling here or something else. I would expect the shadow to be stretched
not tiled (which results in the same output maybe if its 1px wide but just
so we're speaking the same language).


>
> How would one use a UIResourceLayer? In codesearch, I don't see any uses
> outside
> of tests of UIResourceLayer::SetUIResourceId.
>

Android uses it which doesn't show up in codesearch easily. LayerTreeHost
provides ways to make a UIResource and then u can use it for any layer on
that host.


>
> https://codereview.chromium.org/2543473005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-12-07 01:18:33]

On 2016/12/06 23:32:37, danakj wrote:
> On Tue, Dec 6, 2016 at 3:25 PM, <mailto:estade@chromium.org> wrote:
> 
> > > So this is slightly complicated and need to think about the whole system.
> > >
> > > The strength of NinePatchLayer is that it can use UIResources.
> > >
> > https://cs.chromium.org/chromium/src/cc/layers/ui_resource_
> > layer.h?rcl=1481041267&l=33
> > >
> > > UIResources are upload-once-use-many things that cc manages for you. So
> > when
> > you
> > > have 5 NinePatch layers and they all use the same UIResource, you have
> > memory
> > > for the bitmap and the texture only once. If, however, you give a bitmap
> > to
> > > every NinePatch layer (what this patch is doing) it can't tell they are
> > the
> > same
> > > and it will upload it for each layer so you get 1 bitmap 5 textures
> > which is
> > > worse for memory.
> > >
> > > If you use a PictureLayer (ie views painting) it can't share anything so
> > you
> > get
> > > 1 bitmap (since we share it as a global here) placed into 5 recordings,
> > rastered
> > > into 5 tilings (1 per layer). That's poor for memory *and* has additional
> > > CPU/GPU overhead to raster the bitmap into a tiling in this case.
> > >
> > > So if the # of layers is constant the cost is roughly
> > > cpu/gpu: PictureLayer/painting > NinePatch with bitmap = NinePatch with
> > > UIResource
> > > memory: PictureLayer/painting = NinePatch with bitmap > NinePatch with
> > > UIResource
> > >
> > > If the number of layers is changing then you have to take into account
> > the
> > > *overlapping* surface area of the layers, and double count that for both
> > costs.
> > > There's also some additional computational overhead per layer but it's
> > small
> > per
> > > layer (adds up for hundreds of layers).
> > >
> > > Does this help?
> >
> > yes, thanks. So to be clear, the bitmap we're using for the ninebox is
> > 1x11px.
> > Even if we have to upload that for every omnibox layer, that bitmap in
> > itself
> > seems like a pretty small memory footprint. The bitmap is tiled many times
> > horizontally (like 1000 times if your screen is 1000px wide). Are you
> > saying
> > that this uses up memory proportional to the number of times it's tiled?
> > Before
> > this patch, i.e. using PictureLayer, I would expect that to be true, but I
> > had
> > hoped this patch would address that.
> >
> 
> With PictureLayer in the UI we will raster it without applying the layer's
> transform. In blink we do differently and raster based on the layer
> transform. So these should both look and use the same memory WRT layer
> transform/scaling.
> 
> However here you're saying tiled, I did not know that UIResourceLayer does
> repeating textures which is how i read that, so I wonder if you mean
> scaling here or something else. I would expect the shadow to be stretched
> not tiled (which results in the same output maybe if its 1px wide but just
> so we're speaking the same language).

OK, I guess they're stretched, but tiling and stretching do the same thing for a
properly formatted ninebox, where the top/bottom/left/right tiles should always
be 1px thick. Either way, it seems like ninebox layers should only use as much
memory as the ninebox image itself (in this case, 44 bytes or so). Is that not
the case?

> 
> 
> >
> > How would one use a UIResourceLayer? In codesearch, I don't see any uses
> > outside
> > of tests of UIResourceLayer::SetUIResourceId.
> >
> 
> Android uses it which doesn't show up in codesearch easily. LayerTreeHost
> provides ways to make a UIResource and then u can use it for any layer on
> that host.
> 
> 
> >
> > https://codereview.chromium.org/2543473005/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-12-07 01:32:50]

On Tue, Dec 6, 2016 at 5:18 PM, <estade@chromium.org> wrote:

> On 2016/12/06 23:32:37, danakj wrote:
>
> > On Tue, Dec 6, 2016 at 3:25 PM, <mailto:estade@chromium.org> wrote:
> >
> > > > So this is slightly complicated and need to think about the whole
> system.
> > > >
> > > > The strength of NinePatchLayer is that it can use UIResources.
> > > >
> > > https://cs.chromium.org/chromium/src/cc/layers/ui_resource_
> > > layer.h?rcl=1481041267&l=33
> > > >
> > > > UIResources are upload-once-use-many things that cc manages for you.
> So
> > > when
> > > you
> > > > have 5 NinePatch layers and they all use the same UIResource, you
> have
> > > memory
> > > > for the bitmap and the texture only once. If, however, you give a
> bitmap
> > > to
> > > > every NinePatch layer (what this patch is doing) it can't tell they
> are
> > > the
> > > same
> > > > and it will upload it for each layer so you get 1 bitmap 5 textures
> > > which is
> > > > worse for memory.
> > > >
> > > > If you use a PictureLayer (ie views painting) it can't share
> anything so
> > > you
> > > get
> > > > 1 bitmap (since we share it as a global here) placed into 5
> recordings,
> > > rastered
> > > > into 5 tilings (1 per layer). That's poor for memory *and* has
> additional
> > > > CPU/GPU overhead to raster the bitmap into a tiling in this case.
> > > >
> > > > So if the # of layers is constant the cost is roughly
> > > > cpu/gpu: PictureLayer/painting > NinePatch with bitmap = NinePatch
> with
> > > > UIResource
> > > > memory: PictureLayer/painting = NinePatch with bitmap > NinePatch
> with
> > > > UIResource
> > > >
> > > > If the number of layers is changing then you have to take into
> account
> > > the
> > > > *overlapping* surface area of the layers, and double count that for
> both
> > > costs.
> > > > There's also some additional computational overhead per layer but
> it's
> > > small
> > > per
> > > > layer (adds up for hundreds of layers).
> > > >
> > > > Does this help?
> > >
> > > yes, thanks. So to be clear, the bitmap we're using for the ninebox is
> > > 1x11px.
> > > Even if we have to upload that for every omnibox layer, that bitmap in
> > > itself
> > > seems like a pretty small memory footprint. The bitmap is tiled many
> times
> > > horizontally (like 1000 times if your screen is 1000px wide). Are you
> > > saying
> > > that this uses up memory proportional to the number of times it's
> tiled?
> > > Before
> > > this patch, i.e. using PictureLayer, I would expect that to be true,
> but I
> > > had
> > > hoped this patch would address that.
> > >
> >
> > With PictureLayer in the UI we will raster it without applying the
> layer's
> > transform. In blink we do differently and raster based on the layer
> > transform. So these should both look and use the same memory WRT layer
> > transform/scaling.
> >
> > However here you're saying tiled, I did not know that UIResourceLayer
> does
> > repeating textures which is how i read that, so I wonder if you mean
> > scaling here or something else. I would expect the shadow to be stretched
> > not tiled (which results in the same output maybe if its 1px wide but
> just
> > so we're speaking the same language).
>
> OK, I guess they're stretched, but tiling and stretching do the same thing
> for a
> properly formatted ninebox, where the top/bottom/left/right tiles should
> always
> be 1px thick. Either way, it seems like ninebox layers should only use as
> much
> memory as the ninebox image itself (in this case, 44 bytes or so). Is that
> not
> the case?
>

Yep that's correct if I'm understanding everything right. You can use
about:tracing to record memory if you want to measure thing btw.
(documented in
https://cs.chromium.org/chromium/src/docs/memory-infra/probe-cc.md?q=/docs/me...
)


>
>
> >
> >
> > >
> > > How would one use a UIResourceLayer? In codesearch, I don't see any
> uses
> > > outside
> > > of tests of UIResourceLayer::SetUIResourceId.
> > >
> >
> > Android uses it which doesn't show up in codesearch easily. LayerTreeHost
> > provides ways to make a UIResource and then u can use it for any layer on
> > that host.
> >
> >
> > >
> > > https://codereview.chromium.org/2543473005/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2543473005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.