cc: Toggle LCD text at raster time instead of record time.

cc: Toggle LCD text at raster time instead of record time.

Always tell blink that it can use LCD text (in future this can stop
being passed at all). And at raster time:

- If LCD text is allowed initialize SkSurfaceProps with the LegacyHost
parameter which will cause skia to pick an LCD text format matching
current behaviour.
- If LCD is not allowed initialize SkSurfaceProps with an "unknown
pixelformat" causing LCD text to not be used.

This also removes the need for SK_SUPPORT_LEGACY_TEXTRENDERMODE from
resource_provider.cc.

R=enne,reveman
BUG=430617
Committed: https://crrev.com/ad672f645e82ca677978b097a170c908c614d184
Cr-Commit-Position: refs/heads/master@{#304486}

Committed: https://crrev.com/3f76acec92ff4dbd8bc46e7add2f8f7a2cbacfb0
Cr-Commit-Position: refs/heads/master@{#304623}

[danakj, 2014-11-11 18:07:14]

danakj@chromium.org changed reviewers:
+ vmpstr@chromium.org
- enne@chromium.org

[danakj, 2014-11-11 18:28:53]

Removed the DidChangeLCDText from the ContentLayerClient and updated the unit
test for it.

[reveman, 2014-11-11 21:16:49]

https://codereview.chromium.org/684543006/diff/100001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/684543006/diff/100001/cc/layers/content_layer...
cc/layers/content_layer.h:73: bool ignore_lcd_text_change_;
Suggestion that you can ignore if you like as this is not something new to this
patch:

We could use an enum here instead of two bools. ie. "enum {
CAN_USE_LCD_TEXT_UNKNOWN, CAN_USE_LCD_TEXT_TRUE, CAN_USE_LCD_TEXT_FALSE }". I
think that will make it easier to see what state transitions are allowed.

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1109: // recreate the surface within the
resource.
this comment needs an update.

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1111: if (!create_surface) {
this conditional seems overkill. Temporary variables for
surface_uses_distance_field_text and surface_can_use_lcd_text is nice but
create_surface is not needed imo.

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1153: resource_->sk_surface =
skia::AdoptRef(SkSurface::NewRenderTargetDirect(
Please avoid duplication of this line. I had to spend some time to determine
that it is the same as above. Maybe make this more consistent with your change
to RWP.

[danakj, 2014-11-11 21:56:44]

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1111: if (!create_surface) {
On 2014/11/11 21:16:49, reveman wrote:
> this conditional seems overkill. Temporary variables for
> surface_uses_distance_field_text and surface_can_use_lcd_text is nice but
> create_surface is not needed imo.

It avoids 2 virtual function calls per resource in the common case though?

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1153: resource_->sk_surface =
skia::AdoptRef(SkSurface::NewRenderTargetDirect(
On 2014/11/11 21:16:49, reveman wrote:
> Please avoid duplication of this line. I had to spend some time to determine
> that it is the same as above. Maybe make this more consistent with your change
> to RWP.

OK. I had it that way first but it looked really ugly with the flags here. I'll
try again.

[danakj, 2014-11-12 17:22:11]

PTAL

https://codereview.chromium.org/684543006/diff/100001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/684543006/diff/100001/cc/layers/content_layer...
cc/layers/content_layer.h:73: bool ignore_lcd_text_change_;
On 2014/11/11 21:16:49, reveman wrote:
> Suggestion that you can ignore if you like as this is not something new to
this
> patch:
> 
> We could use an enum here instead of two bools. ie. "enum {
> CAN_USE_LCD_TEXT_UNKNOWN, CAN_USE_LCD_TEXT_TRUE, CAN_USE_LCD_TEXT_FALSE }". I
> think that will make it easier to see what state transitions are allowed.

Done. Though poking around here I realized content layer doesn't actually do
anything with this LCD flag anymore (ui::Layer doesn't take any flag for it). So
I removed it from here, but I did the enum thing in PictureLayer.

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1109: // recreate the surface within the
resource.
On 2014/11/11 21:16:49, reveman wrote:
> this comment needs an update.

Done.

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1153: resource_->sk_surface =
skia::AdoptRef(SkSurface::NewRenderTargetDirect(
On 2014/11/11 21:56:44, danakj wrote:
> On 2014/11/11 21:16:49, reveman wrote:
> > Please avoid duplication of this line. I had to spend some time to determine
> > that it is the same as above. Maybe make this more consistent with your
change
> > to RWP.
> 
> OK. I had it that way first but it looked really ugly with the flags here.
I'll
> try again. 

Done.

[danakj, 2014-11-12 21:13:24]

I have restored the method.

[danakj, 2014-11-12 21:15:00]

On 2014/11/12 21:13:24, danakj wrote:
> I have restored the method.

(re: https://codereview.chromium.org/684653004/#msg16)

[danakj, 2014-11-12 21:26:05]

enne pointed out that we can just use a bool and start with layers allowing LCD
text instead of this unknown state to start. changed to that, updated test to
not expect an invalidation of the layer on the first paint.

[enne (OOO), 2014-11-12 21:47:29]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2014-11-12 21:47:30]

https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:130: can_use_lcd_text_for_update_ ==
LCD_TEXT_STATE_ALLOWED;
Er, isn't can_use_lcd_text_for_update_ now a bool?

[danakj, 2014-11-12 21:47:52]

https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:130: can_use_lcd_text_for_update_ ==
LCD_TEXT_STATE_ALLOWED;
On 2014/11/12 21:47:30, enne wrote:
> Er, isn't can_use_lcd_text_for_update_ now a bool?

Er, how did this compile?

[danakj, 2014-11-12 21:48:43]

On 2014/11/12 21:47:52, danakj wrote:
>
https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer.cc
> File cc/layers/picture_layer.cc (right):
> 
>
https://codereview.chromium.org/684543006/diff/220001/cc/layers/picture_layer...
> cc/layers/picture_layer.cc:130: can_use_lcd_text_for_update_ ==
> LCD_TEXT_STATE_ALLOWED;
> On 2014/11/12 21:47:30, enne wrote:
> > Er, isn't can_use_lcd_text_for_update_ now a bool?
> 
> Er, how did this compile?

Oops, left behind the enum. Thanks :D Tests were passing though.. haha :)

[enne (OOO), 2014-11-12 22:19:43]

Am I right in reading this patch that this removes the lcd text notification
from ContentLayer and sets ContentLayer to always turn on lcd text?  The first
is maybe ok in practice given impl-side painting going away (but is kind of
weirdly asymmetric in the short term), but is the second an ok change?

[danakj, 2014-11-12 22:29:36]

On Wed, Nov 12, 2014 at 5:19 PM, <enne@chromium.org> wrote:

> Am I right in reading this patch that this removes the lcd text
> notification
> from ContentLayer and sets ContentLayer to always turn on lcd text?  The
> first
> is maybe ok in practice given impl-side painting going away (but is kind of
> weirdly asymmetric in the short term), but is the second an ok change?
>

Yes you are. ContentLayer's LCD state would never do anything with
ui::Layers since they ignore it. Blink doesn't hook up ContentLayers to
WebContentLayerImpl except in layout tests.

So I guess this would change layout tests to always tell blink it can use
LCD text until we fix that. But no layout test expectations changed from
this CL when I hardcoded passing true to blink for supporting LCD text. So
the ContentLayer's flag is basically doing nothing anymore AIUI.

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

[enne (OOO), 2014-11-12 22:41:47]

Can you remind me what the ui::Compositor lcd text situation is?  I know that
previously ui::Layer would ignore the DidChange message, but would it initially
paint one way or the other?  Or would it always set lcd=true?

[danakj, 2014-11-12 22:48:02]

On Wed, Nov 12, 2014 at 5:41 PM, <enne@chromium.org> wrote:

> Can you remind me what the ui::Compositor lcd text situation is?  I know
> that
> previously ui::Layer would ignore the DidChange message, but would it
> initially
> paint one way or the other?  Or would it always set lcd=true?


I had no idea how UI works for LCD, so I had to look it up. Since it is a
ContentLayerClient, not a WebContentLayerClient, the PaintContents() method
does not take a |can_use_lcd_text| flag, meaning there's currently no such
plumbing out of cc to tell ui/ whether it can use LCD on a layer or not.
So, it just chooses to use it or not on its own.

SkiaTextRenderer constructor always turns on LCD text.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...

And disables it over here if it has a non-opaque background.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...

So I think we'll stop using LCD in the UI when we shouldn't be using it
when we turn on Impl-side painting there. For now we use it regardless of
what cc thinks based on transforms etc.

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

[danakj, 2014-11-12 22:57:52]

enne, you previously asked me to restore the DidChangeLayerCanUseLcdText method
because blink wants to invalidate everything but ui doesn't.. after looking at
the ui code I'm starting to question this logic, even for blink.

In a world where blink always records with LCD text enabled, cc changing that
state on a layer does not change the inputs to blink at all. Invalidating the
recordings does nothing except make blink re-record the same thing again.

Rather, we need to drop rastered tiles, so we need to insert synthetic
invalidation on update for the entire layer to drop anything rastered. But we
should do that for the ui/ too, since otherwise we can end up with 1 tile in LCD
text and 1 not?

[enne (OOO), 2014-11-12 23:04:48]

On 2014/11/12 at 22:57:52, danakj wrote:
> enne, you previously asked me to restore the DidChangeLayerCanUseLcdText
method because blink wants to invalidate everything but ui doesn't.. after
looking at the ui code I'm starting to question this logic, even for blink.
> 
> In a world where blink always records with LCD text enabled, cc changing that
state on a layer does not change the inputs to blink at all. Invalidating the
recordings does nothing except make blink re-record the same thing again.
> 
> Rather, we need to drop rastered tiles, so we need to insert synthetic
invalidation on update for the entire layer to drop anything rastered. But we
should do that for the ui/ too, since otherwise we can end up with 1 tile in LCD
text and 1 not?

I'd ask piman's opinion.  My memory is that this was hooked up this way because
many ui::Layers expected to paint once and then drop whatever resources were
used to raster them because they'd never get invalidated again.  I'm not sure if
ui::Layer wants to reraster if it ever transforms, or if it wants to be more in
control of that too.

I think you are right in the short term that PictureLayer could just add a
synthetic invalidation because ui::Layer isn't using it (yet).  If the longer
term answer is that it wants to behave differently, then this client call that
you have now is good enough.

[reveman, 2014-11-13 17:23:16]

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1111: if (!create_surface) {
On 2014/11/11 21:56:44, danakj wrote:
> On 2014/11/11 21:16:49, reveman wrote:
> > this conditional seems overkill. Temporary variables for
> > surface_uses_distance_field_text and surface_can_use_lcd_text is nice but
> > create_surface is not needed imo.
> 
> It avoids 2 virtual function calls per resource in the common case though?

What are the virtual function calls we avoid here? Even if that's the case,
avoiding 2 virtual function calls per gpu rasterized tile seems like a premature
micro-optimization that I have a hard time to imagine makes a difference. I
prefer if we keep this as simple and clean as possible for now.

[danakj, 2014-11-13 17:30:03]

On 2014/11/12 23:04:48, enne wrote:
> On 2014/11/12 at 22:57:52, danakj wrote:
> > enne, you previously asked me to restore the DidChangeLayerCanUseLcdText
> method because blink wants to invalidate everything but ui doesn't.. after
> looking at the ui code I'm starting to question this logic, even for blink.
> > 
> > In a world where blink always records with LCD text enabled, cc changing
that
> state on a layer does not change the inputs to blink at all. Invalidating the
> recordings does nothing except make blink re-record the same thing again.
> > 
> > Rather, we need to drop rastered tiles, so we need to insert synthetic
> invalidation on update for the entire layer to drop anything rastered. But we
> should do that for the ui/ too, since otherwise we can end up with 1 tile in
LCD
> text and 1 not?
> 
> I'd ask piman's opinion.  My memory is that this was hooked up this way
because
> many ui::Layers expected to paint once and then drop whatever resources were
> used to raster them because they'd never get invalidated again.  I'm not sure
if
> ui::Layer wants to reraster if it ever transforms, or if it wants to be more
in
> control of that too.

For a ContentLayer re-raster requires us going to the client and asking for
resources etc. But with impl-side re-rastering doesn't require that. The UI code
can throw away whatever it wants and cc will hold onto a recording for it to do
those re-rasters with. So this doesn't seem to violate that requirement.

> I think you are right in the short term that PictureLayer could just add a
> synthetic invalidation because ui::Layer isn't using it (yet).  If the longer
> term answer is that it wants to behave differently, then this client call that
> you have now is good enough.

I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything for blink
since we always tell it true now, and I'll add invalidations in
PictureLayer::Update to pass to the impl thread. Does that sounds good to you?

But I'm not sure what use an always-empty client function is at that point
though. We can add it back later if we need to?

[danakj, 2014-11-13 17:30:31]

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

[danakj, 2014-11-13 17:30:32]

On 2014/11/13 17:30:03, danakj wrote:
> On 2014/11/12 23:04:48, enne wrote:
> > On 2014/11/12 at 22:57:52, danakj wrote:
> > > enne, you previously asked me to restore the DidChangeLayerCanUseLcdText
> > method because blink wants to invalidate everything but ui doesn't.. after
> > looking at the ui code I'm starting to question this logic, even for blink.
> > > 
> > > In a world where blink always records with LCD text enabled, cc changing
> that
> > state on a layer does not change the inputs to blink at all. Invalidating
the
> > recordings does nothing except make blink re-record the same thing again.
> > > 
> > > Rather, we need to drop rastered tiles, so we need to insert synthetic
> > invalidation on update for the entire layer to drop anything rastered. But
we
> > should do that for the ui/ too, since otherwise we can end up with 1 tile in
> LCD
> > text and 1 not?
> > 
> > I'd ask piman's opinion.  My memory is that this was hooked up this way
> because
> > many ui::Layers expected to paint once and then drop whatever resources were
> > used to raster them because they'd never get invalidated again.  I'm not
sure
> if
> > ui::Layer wants to reraster if it ever transforms, or if it wants to be more
> in
> > control of that too.
> 
> For a ContentLayer re-raster requires us going to the client and asking for
> resources etc. But with impl-side re-rastering doesn't require that. The UI
code
> can throw away whatever it wants and cc will hold onto a recording for it to
do
> those re-rasters with. So this doesn't seem to violate that requirement.
> 
> > I think you are right in the short term that PictureLayer could just add a
> > synthetic invalidation because ui::Layer isn't using it (yet).  If the
longer
> > term answer is that it wants to behave differently, then this client call
that
> > you have now is good enough.
> 
> I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything for
blink
> since we always tell it true now, and I'll add invalidations in
> PictureLayer::Update to pass to the impl thread. Does that sounds good to you?
> 
> But I'm not sure what use an always-empty client function is at that point
> though. We can add it back later if we need to?

+sky for comment about ui::Layer and LCD text

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1111: if (!create_surface) {
On 2014/11/13 17:23:15, reveman wrote:
> On 2014/11/11 21:56:44, danakj wrote:
> > On 2014/11/11 21:16:49, reveman wrote:
> > > this conditional seems overkill. Temporary variables for
> > > surface_uses_distance_field_text and surface_can_use_lcd_text is nice but
> > > create_surface is not needed imo.
> > 
> > It avoids 2 virtual function calls per resource in the common case though?
> 
> What are the virtual function calls we avoid here? Even if that's the case,
> avoiding 2 virtual function calls per gpu rasterized tile seems like a
premature
> micro-optimization that I have a hard time to imagine makes a difference. I
> prefer if we keep this as simple and clean as possible for now.

Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.

[sky, 2014-11-13 18:29:24]

On 2014/11/13 17:30:32, danakj wrote:
> On 2014/11/13 17:30:03, danakj wrote:
> > On 2014/11/12 23:04:48, enne wrote:
> > > On 2014/11/12 at 22:57:52, danakj wrote:
> > > > enne, you previously asked me to restore the DidChangeLayerCanUseLcdText
> > > method because blink wants to invalidate everything but ui doesn't.. after
> > > looking at the ui code I'm starting to question this logic, even for
blink.
> > > > 
> > > > In a world where blink always records with LCD text enabled, cc changing
> > that
> > > state on a layer does not change the inputs to blink at all. Invalidating
> the
> > > recordings does nothing except make blink re-record the same thing again.
> > > > 
> > > > Rather, we need to drop rastered tiles, so we need to insert synthetic
> > > invalidation on update for the entire layer to drop anything rastered. But
> we
> > > should do that for the ui/ too, since otherwise we can end up with 1 tile
in
> > LCD
> > > text and 1 not?
> > > 
> > > I'd ask piman's opinion.  My memory is that this was hooked up this way
> > because
> > > many ui::Layers expected to paint once and then drop whatever resources
were
> > > used to raster them because they'd never get invalidated again.  I'm not
> sure
> > if
> > > ui::Layer wants to reraster if it ever transforms, or if it wants to be
more
> > in
> > > control of that too.
> > 
> > For a ContentLayer re-raster requires us going to the client and asking for
> > resources etc. But with impl-side re-rastering doesn't require that. The UI
> code
> > can throw away whatever it wants and cc will hold onto a recording for it to
> do
> > those re-rasters with. So this doesn't seem to violate that requirement.
> > 
> > > I think you are right in the short term that PictureLayer could just add a
> > > synthetic invalidation because ui::Layer isn't using it (yet).  If the
> longer
> > > term answer is that it wants to behave differently, then this client call
> that
> > > you have now is good enough.
> > 
> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything for
> blink
> > since we always tell it true now, and I'll add invalidations in
> > PictureLayer::Update to pass to the impl thread. Does that sounds good to
you?
> > 
> > But I'm not sure what use an always-empty client function is at that point
> > though. We can add it back later if we need to?
> 
> +sky for comment about ui::Layer and LCD text

Sorry, what comment is that? (too long a thread).

  -Scott

>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> cc/resources/resource_provider.cc:1111: if (!create_surface) {
> On 2014/11/13 17:23:15, reveman wrote:
> > On 2014/11/11 21:56:44, danakj wrote:
> > > On 2014/11/11 21:16:49, reveman wrote:
> > > > this conditional seems overkill. Temporary variables for
> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is nice
but
> > > > create_surface is not needed imo.
> > > 
> > > It avoids 2 virtual function calls per resource in the common case though?
> > 
> > What are the virtual function calls we avoid here? Even if that's the case,
> > avoiding 2 virtual function calls per gpu rasterized tile seems like a
> premature
> > micro-optimization that I have a hard time to imagine makes a difference. I
> > prefer if we keep this as simple and clean as possible for now.
> 
> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.

[danakj, 2014-11-13 18:38:36]

On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:

> On 2014/11/13 17:30:32, danakj wrote:
>
>> On 2014/11/13 17:30:03, danakj wrote:
>> > On 2014/11/12 23:04:48, enne wrote:
>> > > On 2014/11/12 at 22:57:52, danakj wrote:
>> > > > enne, you previously asked me to restore the
>> DidChangeLayerCanUseLcdText
>> > > method because blink wants to invalidate everything but ui doesn't..
>> after
>> > > looking at the ui code I'm starting to question this logic, even for
>>
> blink.
>
>> > > >
>> > > > In a world where blink always records with LCD text enabled, cc
>> changing
>> > that
>> > > state on a layer does not change the inputs to blink at all.
>> Invalidating
>> the
>> > > recordings does nothing except make blink re-record the same thing
>> again.
>> > > >
>> > > > Rather, we need to drop rastered tiles, so we need to insert
>> synthetic
>> > > invalidation on update for the entire layer to drop anything
>> rastered. But
>> we
>> > > should do that for the ui/ too, since otherwise we can end up with 1
>> tile
>>
> in
>
>> > LCD
>> > > text and 1 not?
>> > >
>> > > I'd ask piman's opinion.  My memory is that this was hooked up this
>> way
>> > because
>> > > many ui::Layers expected to paint once and then drop whatever
>> resources
>>
> were
>
>> > > used to raster them because they'd never get invalidated again.  I'm
>> not
>> sure
>> > if
>> > > ui::Layer wants to reraster if it ever transforms, or if it wants to
>> be
>>
> more
>
>> > in
>> > > control of that too.
>> >
>> > For a ContentLayer re-raster requires us going to the client and asking
>> for
>> > resources etc. But with impl-side re-rastering doesn't require that.
>> The UI
>> code
>> > can throw away whatever it wants and cc will hold onto a recording for
>> it to
>> do
>> > those re-rasters with. So this doesn't seem to violate that requirement.
>> >
>> > > I think you are right in the short term that PictureLayer could just
>> add a
>> > > synthetic invalidation because ui::Layer isn't using it (yet).  If the
>> longer
>> > > term answer is that it wants to behave differently, then this client
>> call
>> that
>> > > you have now is good enough.
>> >
>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything
>> for
>> blink
>> > since we always tell it true now, and I'll add invalidations in
>> > PictureLayer::Update to pass to the impl thread. Does that sounds good
>> to
>>
> you?
>
>> >
>> > But I'm not sure what use an always-empty client function is at that
>> point
>> > though. We can add it back later if we need to?
>>
>
>  +sky for comment about ui::Layer and LCD text
>>
>
> Sorry, what comment is that? (too long a thread).


Np, I will try to resummarize.

Currently when a layer gets a transform where LCD text would look bad, ui
compositor doesn't do anything with the notification, so it doesn't
invalidate the contents and will continue to draw the LCD text anyways.

With impl-side painting we can behave differently if desirable, trying to
discern what is best:

Since we take a recording of the layer contents when we paint, we can
re-raster it without involving the ui::Layer at all. So when a layer would
not display LCD text we can make it fall back to non-LCD text and reraster
its contents to do so. With blink once a layer does this it stays non-LCD
for the rest of its lifetime currently.

Alternately, we could make cc always allow LCD text in the ui compositor,
which would more closely match what it does right now. But enne's comment
was that they believe it is this way now just to avoid invalidating the
ui::Layer. This isn't a concern anymore with impl-side painting though.



>
>
>   -Scott
>
>
>
> https://codereview.chromium.org/684543006/diff/100001/cc/
> resources/resource_provider.cc
>
>> File cc/resources/resource_provider.cc (right):
>>
>
>
> https://codereview.chromium.org/684543006/diff/100001/cc/
> resources/resource_provider.cc#newcode1111
>
>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
>> On 2014/11/13 17:23:15, reveman wrote:
>> > On 2014/11/11 21:56:44, danakj wrote:
>> > > On 2014/11/11 21:16:49, reveman wrote:
>> > > > this conditional seems overkill. Temporary variables for
>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is
>> nice
>>
> but
>
>> > > > create_surface is not needed imo.
>> > >
>> > > It avoids 2 virtual function calls per resource in the common case
>> though?
>> >
>> > What are the virtual function calls we avoid here? Even if that's the
>> case,
>> > avoiding 2 virtual function calls per gpu rasterized tile seems like a
>> premature
>> > micro-optimization that I have a hard time to imagine makes a
>> difference. I
>> > prefer if we keep this as simple and clean as possible for now.
>>
>
>  Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
>>
>
>
>
> https://codereview.chromium.org/684543006/
>

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

[danakj, 2014-11-13 19:20:50]

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1111: if (!create_surface) {
On 2014/11/13 17:30:32, danakj wrote:
> On 2014/11/13 17:23:15, reveman wrote:
> > On 2014/11/11 21:56:44, danakj wrote:
> > > On 2014/11/11 21:16:49, reveman wrote:
> > > > this conditional seems overkill. Temporary variables for
> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is nice
but
> > > > create_surface is not needed imo.
> > > 
> > > It avoids 2 virtual function calls per resource in the common case though?
> > 
> > What are the virtual function calls we avoid here? Even if that's the case,
> > avoiding 2 virtual function calls per gpu rasterized tile seems like a
> premature
> > micro-optimization that I have a hard time to imagine makes a difference. I
> > prefer if we keep this as simple and clean as possible for now.
> 
> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.

OK I split this into a ShouldCreateSurface() function so we can return true
instead of setting create_surface=true in multiple places.

[sky, 2014-11-13 20:15:02]

Thanks for the summary.

Under what conditions would a transform result in looking bad? Is it
only during animation? Screen rotation? Scaling for high dpi?

  -Scott

On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org> wrote:
> On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
>>
>> On 2014/11/13 17:30:32, danakj wrote:
>>>
>>> On 2014/11/13 17:30:03, danakj wrote:
>>> > On 2014/11/12 23:04:48, enne wrote:
>>> > > On 2014/11/12 at 22:57:52, danakj wrote:
>>> > > > enne, you previously asked me to restore the
>>> > > > DidChangeLayerCanUseLcdText
>>> > > method because blink wants to invalidate everything but ui doesn't..
>>> > > after
>>> > > looking at the ui code I'm starting to question this logic, even for
>>
>> blink.
>>>
>>> > > >
>>> > > > In a world where blink always records with LCD text enabled, cc
>>> > > > changing
>>> > that
>>> > > state on a layer does not change the inputs to blink at all.
>>> > > Invalidating
>>> the
>>> > > recordings does nothing except make blink re-record the same thing
>>> > > again.
>>> > > >
>>> > > > Rather, we need to drop rastered tiles, so we need to insert
>>> > > > synthetic
>>> > > invalidation on update for the entire layer to drop anything
>>> > > rastered. But
>>> we
>>> > > should do that for the ui/ too, since otherwise we can end up with 1
>>> > > tile
>>
>> in
>>>
>>> > LCD
>>> > > text and 1 not?
>>> > >
>>> > > I'd ask piman's opinion.  My memory is that this was hooked up this
>>> > > way
>>> > because
>>> > > many ui::Layers expected to paint once and then drop whatever
>>> > > resources
>>
>> were
>>>
>>> > > used to raster them because they'd never get invalidated again.  I'm
>>> > > not
>>> sure
>>> > if
>>> > > ui::Layer wants to reraster if it ever transforms, or if it wants to
>>> > > be
>>
>> more
>>>
>>> > in
>>> > > control of that too.
>>> >
>>> > For a ContentLayer re-raster requires us going to the client and asking
>>> > for
>>> > resources etc. But with impl-side re-rastering doesn't require that.
>>> > The UI
>>> code
>>> > can throw away whatever it wants and cc will hold onto a recording for
>>> > it to
>>> do
>>> > those re-rasters with. So this doesn't seem to violate that
>>> > requirement.
>>> >
>>> > > I think you are right in the short term that PictureLayer could just
>>> > > add a
>>> > > synthetic invalidation because ui::Layer isn't using it (yet).  If
>>> > > the
>>> longer
>>> > > term answer is that it wants to behave differently, then this client
>>> > > call
>>> that
>>> > > you have now is good enough.
>>> >
>>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything
>>> > for
>>> blink
>>> > since we always tell it true now, and I'll add invalidations in
>>> > PictureLayer::Update to pass to the impl thread. Does that sounds good
>>> > to
>>
>> you?
>>>
>>> >
>>> > But I'm not sure what use an always-empty client function is at that
>>> > point
>>> > though. We can add it back later if we need to?
>>
>>
>>> +sky for comment about ui::Layer and LCD text
>>
>>
>> Sorry, what comment is that? (too long a thread).
>
>
> Np, I will try to resummarize.
>
> Currently when a layer gets a transform where LCD text would look bad, ui
> compositor doesn't do anything with the notification, so it doesn't
> invalidate the contents and will continue to draw the LCD text anyways.
>
> With impl-side painting we can behave differently if desirable, trying to
> discern what is best:
>
> Since we take a recording of the layer contents when we paint, we can
> re-raster it without involving the ui::Layer at all. So when a layer would
> not display LCD text we can make it fall back to non-LCD text and reraster
> its contents to do so. With blink once a layer does this it stays non-LCD
> for the rest of its lifetime currently.
>
> Alternately, we could make cc always allow LCD text in the ui compositor,
> which would more closely match what it does right now. But enne's comment
> was that they believe it is this way now just to avoid invalidating the
> ui::Layer. This isn't a concern anymore with impl-side painting though.
>
>
>>
>>
>>
>>   -Scott
>>
>>
>>
>>
>>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>>>
>>> File cc/resources/resource_provider.cc (right):
>>
>>
>>
>>
>>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>>>
>>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
>>> On 2014/11/13 17:23:15, reveman wrote:
>>> > On 2014/11/11 21:56:44, danakj wrote:
>>> > > On 2014/11/11 21:16:49, reveman wrote:
>>> > > > this conditional seems overkill. Temporary variables for
>>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is
>>> > > > nice
>>
>> but
>>>
>>> > > > create_surface is not needed imo.
>>> > >
>>> > > It avoids 2 virtual function calls per resource in the common case
>>> > > though?
>>> >
>>> > What are the virtual function calls we avoid here? Even if that's the
>>> > case,
>>> > avoiding 2 virtual function calls per gpu rasterized tile seems like a
>>> premature
>>> > micro-optimization that I have a hard time to imagine makes a
>>> > difference. I
>>> > prefer if we keep this as simple and clean as possible for now.
>>
>>
>>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
>>
>>
>>
>>
>> https://codereview.chromium.org/684543006/
>
>

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

[chromium-reviews, 2014-11-13 20:20:58]

almost any transform (except possible vertical mirror)
almost any imagefilter

On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org> wrote:

> Thanks for the summary.
>
> Under what conditions would a transform result in looking bad? Is it
> only during animation? Screen rotation? Scaling for high dpi?
>
>   -Scott
>
> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
> >>
> >> On 2014/11/13 17:30:32, danakj wrote:
> >>>
> >>> On 2014/11/13 17:30:03, danakj wrote:
> >>> > On 2014/11/12 23:04:48, enne wrote:
> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
> >>> > > > enne, you previously asked me to restore the
> >>> > > > DidChangeLayerCanUseLcdText
> >>> > > method because blink wants to invalidate everything but ui
> doesn't..
> >>> > > after
> >>> > > looking at the ui code I'm starting to question this logic, even
> for
> >>
> >> blink.
> >>>
> >>> > > >
> >>> > > > In a world where blink always records with LCD text enabled, cc
> >>> > > > changing
> >>> > that
> >>> > > state on a layer does not change the inputs to blink at all.
> >>> > > Invalidating
> >>> the
> >>> > > recordings does nothing except make blink re-record the same thing
> >>> > > again.
> >>> > > >
> >>> > > > Rather, we need to drop rastered tiles, so we need to insert
> >>> > > > synthetic
> >>> > > invalidation on update for the entire layer to drop anything
> >>> > > rastered. But
> >>> we
> >>> > > should do that for the ui/ too, since otherwise we can end up with
> 1
> >>> > > tile
> >>
> >> in
> >>>
> >>> > LCD
> >>> > > text and 1 not?
> >>> > >
> >>> > > I'd ask piman's opinion.  My memory is that this was hooked up this
> >>> > > way
> >>> > because
> >>> > > many ui::Layers expected to paint once and then drop whatever
> >>> > > resources
> >>
> >> were
> >>>
> >>> > > used to raster them because they'd never get invalidated again.
> I'm
> >>> > > not
> >>> sure
> >>> > if
> >>> > > ui::Layer wants to reraster if it ever transforms, or if it wants
> to
> >>> > > be
> >>
> >> more
> >>>
> >>> > in
> >>> > > control of that too.
> >>> >
> >>> > For a ContentLayer re-raster requires us going to the client and
> asking
> >>> > for
> >>> > resources etc. But with impl-side re-rastering doesn't require that.
> >>> > The UI
> >>> code
> >>> > can throw away whatever it wants and cc will hold onto a recording
> for
> >>> > it to
> >>> do
> >>> > those re-rasters with. So this doesn't seem to violate that
> >>> > requirement.
> >>> >
> >>> > > I think you are right in the short term that PictureLayer could
> just
> >>> > > add a
> >>> > > synthetic invalidation because ui::Layer isn't using it (yet).  If
> >>> > > the
> >>> longer
> >>> > > term answer is that it wants to behave differently, then this
> client
> >>> > > call
> >>> that
> >>> > > you have now is good enough.
> >>> >
> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything
> >>> > for
> >>> blink
> >>> > since we always tell it true now, and I'll add invalidations in
> >>> > PictureLayer::Update to pass to the impl thread. Does that sounds
> good
> >>> > to
> >>
> >> you?
> >>>
> >>> >
> >>> > But I'm not sure what use an always-empty client function is at that
> >>> > point
> >>> > though. We can add it back later if we need to?
> >>
> >>
> >>> +sky for comment about ui::Layer and LCD text
> >>
> >>
> >> Sorry, what comment is that? (too long a thread).
> >
> >
> > Np, I will try to resummarize.
> >
> > Currently when a layer gets a transform where LCD text would look bad, ui
> > compositor doesn't do anything with the notification, so it doesn't
> > invalidate the contents and will continue to draw the LCD text anyways.
> >
> > With impl-side painting we can behave differently if desirable, trying to
> > discern what is best:
> >
> > Since we take a recording of the layer contents when we paint, we can
> > re-raster it without involving the ui::Layer at all. So when a layer
> would
> > not display LCD text we can make it fall back to non-LCD text and
> reraster
> > its contents to do so. With blink once a layer does this it stays non-LCD
> > for the rest of its lifetime currently.
> >
> > Alternately, we could make cc always allow LCD text in the ui compositor,
> > which would more closely match what it does right now. But enne's comment
> > was that they believe it is this way now just to avoid invalidating the
> > ui::Layer. This isn't a concern anymore with impl-side painting though.
> >
> >
> >>
> >>
> >>
> >>   -Scott
> >>
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >>>
> >>> File cc/resources/resource_provider.cc (right):
> >>
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >>>
> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
> >>> On 2014/11/13 17:23:15, reveman wrote:
> >>> > On 2014/11/11 21:56:44, danakj wrote:
> >>> > > On 2014/11/11 21:16:49, reveman wrote:
> >>> > > > this conditional seems overkill. Temporary variables for
> >>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is
> >>> > > > nice
> >>
> >> but
> >>>
> >>> > > > create_surface is not needed imo.
> >>> > >
> >>> > > It avoids 2 virtual function calls per resource in the common case
> >>> > > though?
> >>> >
> >>> > What are the virtual function calls we avoid here? Even if that's the
> >>> > case,
> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems like
> a
> >>> premature
> >>> > micro-optimization that I have a hard time to imagine makes a
> >>> > difference. I
> >>> > prefer if we keep this as simple and clean as possible for now.
> >>
> >>
> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
> >>
> >>
> >>
> >>
> >> https://codereview.chromium.org/684543006/
> >
> >
>

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

[danakj, 2014-11-13 20:30:37]

On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org> wrote:

> Thanks for the summary.
>
> Under what conditions would a transform result in looking bad? Is it
> only during animation? Screen rotation? Scaling for high dpi?
>

Transforms with non-integer translations, ie the origin doesn't align with
a pixel. Or rotations, etc.

On high-dpi we should still be putting stuff on pixels not between them so
it's okay in that state. Screen rotation will make LCD text not work
correctly at all, yah.


>
>   -Scott
>
> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
> >>
> >> On 2014/11/13 17:30:32, danakj wrote:
> >>>
> >>> On 2014/11/13 17:30:03, danakj wrote:
> >>> > On 2014/11/12 23:04:48, enne wrote:
> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
> >>> > > > enne, you previously asked me to restore the
> >>> > > > DidChangeLayerCanUseLcdText
> >>> > > method because blink wants to invalidate everything but ui
> doesn't..
> >>> > > after
> >>> > > looking at the ui code I'm starting to question this logic, even
> for
> >>
> >> blink.
> >>>
> >>> > > >
> >>> > > > In a world where blink always records with LCD text enabled, cc
> >>> > > > changing
> >>> > that
> >>> > > state on a layer does not change the inputs to blink at all.
> >>> > > Invalidating
> >>> the
> >>> > > recordings does nothing except make blink re-record the same thing
> >>> > > again.
> >>> > > >
> >>> > > > Rather, we need to drop rastered tiles, so we need to insert
> >>> > > > synthetic
> >>> > > invalidation on update for the entire layer to drop anything
> >>> > > rastered. But
> >>> we
> >>> > > should do that for the ui/ too, since otherwise we can end up with
> 1
> >>> > > tile
> >>
> >> in
> >>>
> >>> > LCD
> >>> > > text and 1 not?
> >>> > >
> >>> > > I'd ask piman's opinion.  My memory is that this was hooked up this
> >>> > > way
> >>> > because
> >>> > > many ui::Layers expected to paint once and then drop whatever
> >>> > > resources
> >>
> >> were
> >>>
> >>> > > used to raster them because they'd never get invalidated again.
> I'm
> >>> > > not
> >>> sure
> >>> > if
> >>> > > ui::Layer wants to reraster if it ever transforms, or if it wants
> to
> >>> > > be
> >>
> >> more
> >>>
> >>> > in
> >>> > > control of that too.
> >>> >
> >>> > For a ContentLayer re-raster requires us going to the client and
> asking
> >>> > for
> >>> > resources etc. But with impl-side re-rastering doesn't require that.
> >>> > The UI
> >>> code
> >>> > can throw away whatever it wants and cc will hold onto a recording
> for
> >>> > it to
> >>> do
> >>> > those re-rasters with. So this doesn't seem to violate that
> >>> > requirement.
> >>> >
> >>> > > I think you are right in the short term that PictureLayer could
> just
> >>> > > add a
> >>> > > synthetic invalidation because ui::Layer isn't using it (yet).  If
> >>> > > the
> >>> longer
> >>> > > term answer is that it wants to behave differently, then this
> client
> >>> > > call
> >>> that
> >>> > > you have now is good enough.
> >>> >
> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do anything
> >>> > for
> >>> blink
> >>> > since we always tell it true now, and I'll add invalidations in
> >>> > PictureLayer::Update to pass to the impl thread. Does that sounds
> good
> >>> > to
> >>
> >> you?
> >>>
> >>> >
> >>> > But I'm not sure what use an always-empty client function is at that
> >>> > point
> >>> > though. We can add it back later if we need to?
> >>
> >>
> >>> +sky for comment about ui::Layer and LCD text
> >>
> >>
> >> Sorry, what comment is that? (too long a thread).
> >
> >
> > Np, I will try to resummarize.
> >
> > Currently when a layer gets a transform where LCD text would look bad, ui
> > compositor doesn't do anything with the notification, so it doesn't
> > invalidate the contents and will continue to draw the LCD text anyways.
> >
> > With impl-side painting we can behave differently if desirable, trying to
> > discern what is best:
> >
> > Since we take a recording of the layer contents when we paint, we can
> > re-raster it without involving the ui::Layer at all. So when a layer
> would
> > not display LCD text we can make it fall back to non-LCD text and
> reraster
> > its contents to do so. With blink once a layer does this it stays non-LCD
> > for the rest of its lifetime currently.
> >
> > Alternately, we could make cc always allow LCD text in the ui compositor,
> > which would more closely match what it does right now. But enne's comment
> > was that they believe it is this way now just to avoid invalidating the
> > ui::Layer. This isn't a concern anymore with impl-side painting though.
> >
> >
> >>
> >>
> >>
> >>   -Scott
> >>
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >>>
> >>> File cc/resources/resource_provider.cc (right):
> >>
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >>>
> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
> >>> On 2014/11/13 17:23:15, reveman wrote:
> >>> > On 2014/11/11 21:56:44, danakj wrote:
> >>> > > On 2014/11/11 21:16:49, reveman wrote:
> >>> > > > this conditional seems overkill. Temporary variables for
> >>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is
> >>> > > > nice
> >>
> >> but
> >>>
> >>> > > > create_surface is not needed imo.
> >>> > >
> >>> > > It avoids 2 virtual function calls per resource in the common case
> >>> > > though?
> >>> >
> >>> > What are the virtual function calls we avoid here? Even if that's the
> >>> > case,
> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems like
> a
> >>> premature
> >>> > micro-optimization that I have a hard time to imagine makes a
> >>> > difference. I
> >>> > prefer if we keep this as simple and clean as possible for now.
> >>
> >>
> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
> >>
> >>
> >>
> >>
> >> https://codereview.chromium.org/684543006/
> >
> >
>

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

[sky, 2014-11-13 20:50:29]

This will likely effect chromeos mode on windows when scaled and
chromeos on devices with non integer scales (AFAIK there is only one
device ever shipped like this).

Lets say you're on a chromeos device with a non-integer scale and drag
a window around. Which means you're dragging a layer around and
because it's a non-integer scale the layer is going to bounce between
landing on a pixel and not landing on pixels. If we did flip back and
forth between lcd and non-lcd won't the text change quite a bit?

  -Scott

On Thu, Nov 13, 2014 at 12:30 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Thanks for the summary.
>>
>> Under what conditions would a transform result in looking bad? Is it
>> only during animation? Screen rotation? Scaling for high dpi?
>
>
> Transforms with non-integer translations, ie the origin doesn't align with a
> pixel. Or rotations, etc.
>
> On high-dpi we should still be putting stuff on pixels not between them so
> it's okay in that state. Screen rotation will make LCD text not work
> correctly at all, yah.
>
>>
>>
>>   -Scott
>>
>> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
>> wrote:
>> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
>> >>
>> >> On 2014/11/13 17:30:32, danakj wrote:
>> >>>
>> >>> On 2014/11/13 17:30:03, danakj wrote:
>> >>> > On 2014/11/12 23:04:48, enne wrote:
>> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
>> >>> > > > enne, you previously asked me to restore the
>> >>> > > > DidChangeLayerCanUseLcdText
>> >>> > > method because blink wants to invalidate everything but ui
>> >>> > > doesn't..
>> >>> > > after
>> >>> > > looking at the ui code I'm starting to question this logic, even
>> >>> > > for
>> >>
>> >> blink.
>> >>>
>> >>> > > >
>> >>> > > > In a world where blink always records with LCD text enabled, cc
>> >>> > > > changing
>> >>> > that
>> >>> > > state on a layer does not change the inputs to blink at all.
>> >>> > > Invalidating
>> >>> the
>> >>> > > recordings does nothing except make blink re-record the same thing
>> >>> > > again.
>> >>> > > >
>> >>> > > > Rather, we need to drop rastered tiles, so we need to insert
>> >>> > > > synthetic
>> >>> > > invalidation on update for the entire layer to drop anything
>> >>> > > rastered. But
>> >>> we
>> >>> > > should do that for the ui/ too, since otherwise we can end up with
>> >>> > > 1
>> >>> > > tile
>> >>
>> >> in
>> >>>
>> >>> > LCD
>> >>> > > text and 1 not?
>> >>> > >
>> >>> > > I'd ask piman's opinion.  My memory is that this was hooked up
>> >>> > > this
>> >>> > > way
>> >>> > because
>> >>> > > many ui::Layers expected to paint once and then drop whatever
>> >>> > > resources
>> >>
>> >> were
>> >>>
>> >>> > > used to raster them because they'd never get invalidated again.
>> >>> > > I'm
>> >>> > > not
>> >>> sure
>> >>> > if
>> >>> > > ui::Layer wants to reraster if it ever transforms, or if it wants
>> >>> > > to
>> >>> > > be
>> >>
>> >> more
>> >>>
>> >>> > in
>> >>> > > control of that too.
>> >>> >
>> >>> > For a ContentLayer re-raster requires us going to the client and
>> >>> > asking
>> >>> > for
>> >>> > resources etc. But with impl-side re-rastering doesn't require that.
>> >>> > The UI
>> >>> code
>> >>> > can throw away whatever it wants and cc will hold onto a recording
>> >>> > for
>> >>> > it to
>> >>> do
>> >>> > those re-rasters with. So this doesn't seem to violate that
>> >>> > requirement.
>> >>> >
>> >>> > > I think you are right in the short term that PictureLayer could
>> >>> > > just
>> >>> > > add a
>> >>> > > synthetic invalidation because ui::Layer isn't using it (yet).  If
>> >>> > > the
>> >>> longer
>> >>> > > term answer is that it wants to behave differently, then this
>> >>> > > client
>> >>> > > call
>> >>> that
>> >>> > > you have now is good enough.
>> >>> >
>> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do
>> >>> > anything
>> >>> > for
>> >>> blink
>> >>> > since we always tell it true now, and I'll add invalidations in
>> >>> > PictureLayer::Update to pass to the impl thread. Does that sounds
>> >>> > good
>> >>> > to
>> >>
>> >> you?
>> >>>
>> >>> >
>> >>> > But I'm not sure what use an always-empty client function is at that
>> >>> > point
>> >>> > though. We can add it back later if we need to?
>> >>
>> >>
>> >>> +sky for comment about ui::Layer and LCD text
>> >>
>> >>
>> >> Sorry, what comment is that? (too long a thread).
>> >
>> >
>> > Np, I will try to resummarize.
>> >
>> > Currently when a layer gets a transform where LCD text would look bad,
>> > ui
>> > compositor doesn't do anything with the notification, so it doesn't
>> > invalidate the contents and will continue to draw the LCD text anyways.
>> >
>> > With impl-side painting we can behave differently if desirable, trying
>> > to
>> > discern what is best:
>> >
>> > Since we take a recording of the layer contents when we paint, we can
>> > re-raster it without involving the ui::Layer at all. So when a layer
>> > would
>> > not display LCD text we can make it fall back to non-LCD text and
>> > reraster
>> > its contents to do so. With blink once a layer does this it stays
>> > non-LCD
>> > for the rest of its lifetime currently.
>> >
>> > Alternately, we could make cc always allow LCD text in the ui
>> > compositor,
>> > which would more closely match what it does right now. But enne's
>> > comment
>> > was that they believe it is this way now just to avoid invalidating the
>> > ui::Layer. This isn't a concern anymore with impl-side painting though.
>> >
>> >
>> >>
>> >>
>> >>
>> >>   -Scott
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>> >>>
>> >>> File cc/resources/resource_provider.cc (right):
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>> >>>
>> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
>> >>> On 2014/11/13 17:23:15, reveman wrote:
>> >>> > On 2014/11/11 21:56:44, danakj wrote:
>> >>> > > On 2014/11/11 21:16:49, reveman wrote:
>> >>> > > > this conditional seems overkill. Temporary variables for
>> >>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text is
>> >>> > > > nice
>> >>
>> >> but
>> >>>
>> >>> > > > create_surface is not needed imo.
>> >>> > >
>> >>> > > It avoids 2 virtual function calls per resource in the common case
>> >>> > > though?
>> >>> >
>> >>> > What are the virtual function calls we avoid here? Even if that's
>> >>> > the
>> >>> > case,
>> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems like
>> >>> > a
>> >>> premature
>> >>> > micro-optimization that I have a hard time to imagine makes a
>> >>> > difference. I
>> >>> > prefer if we keep this as simple and clean as possible for now.
>> >>
>> >>
>> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
>> >>
>> >>
>> >>
>> >>
>> >> https://codereview.chromium.org/684543006/
>> >
>> >
>
>

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

[danakj, 2014-11-13 21:30:03]

On Thu, Nov 13, 2014 at 3:50 PM, Scott Violet <sky@chromium.org> wrote:

> This will likely effect chromeos mode on windows when scaled and
> chromeos on devices with non integer scales (AFAIK there is only one
> device ever shipped like this).
>
> Lets say you're on a chromeos device with a non-integer scale and drag
> a window around. Which means you're dragging a layer around and
> because it's a non-integer scale the layer is going to bounce between
> landing on a pixel and not landing on pixels. If we did flip back and
> forth between lcd and non-lcd won't the text change quite a bit?
>

Ideally we'd be pixel snapping the transform at each step or the text is
going to look awful as you're moving it, and/or at the new position. Surely
we already do this to avoid bad text..? If not it would look better to have
grayscale AA text than to have LCD text that is not aligning to the pixels.

I don't see anything in the ui code to turn off LCD text except in a
transparent-background case. So I assume we're pixel snapping so things
don't look weird. But I'm not sure where that would be happening..


>
>   -Scott
>
> On Thu, Nov 13, 2014 at 12:30 PM, Dana Jansens <danakj@chromium.org>
> wrote:
> > On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> Thanks for the summary.
> >>
> >> Under what conditions would a transform result in looking bad? Is it
> >> only during animation? Screen rotation? Scaling for high dpi?
> >
> >
> > Transforms with non-integer translations, ie the origin doesn't align
> with a
> > pixel. Or rotations, etc.
> >
> > On high-dpi we should still be putting stuff on pixels not between them
> so
> > it's okay in that state. Screen rotation will make LCD text not work
> > correctly at all, yah.
> >
> >>
> >>
> >>   -Scott
> >>
> >> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
> >> wrote:
> >> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
> >> >>
> >> >> On 2014/11/13 17:30:32, danakj wrote:
> >> >>>
> >> >>> On 2014/11/13 17:30:03, danakj wrote:
> >> >>> > On 2014/11/12 23:04:48, enne wrote:
> >> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
> >> >>> > > > enne, you previously asked me to restore the
> >> >>> > > > DidChangeLayerCanUseLcdText
> >> >>> > > method because blink wants to invalidate everything but ui
> >> >>> > > doesn't..
> >> >>> > > after
> >> >>> > > looking at the ui code I'm starting to question this logic, even
> >> >>> > > for
> >> >>
> >> >> blink.
> >> >>>
> >> >>> > > >
> >> >>> > > > In a world where blink always records with LCD text enabled,
> cc
> >> >>> > > > changing
> >> >>> > that
> >> >>> > > state on a layer does not change the inputs to blink at all.
> >> >>> > > Invalidating
> >> >>> the
> >> >>> > > recordings does nothing except make blink re-record the same
> thing
> >> >>> > > again.
> >> >>> > > >
> >> >>> > > > Rather, we need to drop rastered tiles, so we need to insert
> >> >>> > > > synthetic
> >> >>> > > invalidation on update for the entire layer to drop anything
> >> >>> > > rastered. But
> >> >>> we
> >> >>> > > should do that for the ui/ too, since otherwise we can end up
> with
> >> >>> > > 1
> >> >>> > > tile
> >> >>
> >> >> in
> >> >>>
> >> >>> > LCD
> >> >>> > > text and 1 not?
> >> >>> > >
> >> >>> > > I'd ask piman's opinion.  My memory is that this was hooked up
> >> >>> > > this
> >> >>> > > way
> >> >>> > because
> >> >>> > > many ui::Layers expected to paint once and then drop whatever
> >> >>> > > resources
> >> >>
> >> >> were
> >> >>>
> >> >>> > > used to raster them because they'd never get invalidated again.
> >> >>> > > I'm
> >> >>> > > not
> >> >>> sure
> >> >>> > if
> >> >>> > > ui::Layer wants to reraster if it ever transforms, or if it
> wants
> >> >>> > > to
> >> >>> > > be
> >> >>
> >> >> more
> >> >>>
> >> >>> > in
> >> >>> > > control of that too.
> >> >>> >
> >> >>> > For a ContentLayer re-raster requires us going to the client and
> >> >>> > asking
> >> >>> > for
> >> >>> > resources etc. But with impl-side re-rastering doesn't require
> that.
> >> >>> > The UI
> >> >>> code
> >> >>> > can throw away whatever it wants and cc will hold onto a recording
> >> >>> > for
> >> >>> > it to
> >> >>> do
> >> >>> > those re-rasters with. So this doesn't seem to violate that
> >> >>> > requirement.
> >> >>> >
> >> >>> > > I think you are right in the short term that PictureLayer could
> >> >>> > > just
> >> >>> > > add a
> >> >>> > > synthetic invalidation because ui::Layer isn't using it (yet).
> If
> >> >>> > > the
> >> >>> longer
> >> >>> > > term answer is that it wants to behave differently, then this
> >> >>> > > client
> >> >>> > > call
> >> >>> that
> >> >>> > > you have now is good enough.
> >> >>> >
> >> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do
> >> >>> > anything
> >> >>> > for
> >> >>> blink
> >> >>> > since we always tell it true now, and I'll add invalidations in
> >> >>> > PictureLayer::Update to pass to the impl thread. Does that sounds
> >> >>> > good
> >> >>> > to
> >> >>
> >> >> you?
> >> >>>
> >> >>> >
> >> >>> > But I'm not sure what use an always-empty client function is at
> that
> >> >>> > point
> >> >>> > though. We can add it back later if we need to?
> >> >>
> >> >>
> >> >>> +sky for comment about ui::Layer and LCD text
> >> >>
> >> >>
> >> >> Sorry, what comment is that? (too long a thread).
> >> >
> >> >
> >> > Np, I will try to resummarize.
> >> >
> >> > Currently when a layer gets a transform where LCD text would look bad,
> >> > ui
> >> > compositor doesn't do anything with the notification, so it doesn't
> >> > invalidate the contents and will continue to draw the LCD text
> anyways.
> >> >
> >> > With impl-side painting we can behave differently if desirable, trying
> >> > to
> >> > discern what is best:
> >> >
> >> > Since we take a recording of the layer contents when we paint, we can
> >> > re-raster it without involving the ui::Layer at all. So when a layer
> >> > would
> >> > not display LCD text we can make it fall back to non-LCD text and
> >> > reraster
> >> > its contents to do so. With blink once a layer does this it stays
> >> > non-LCD
> >> > for the rest of its lifetime currently.
> >> >
> >> > Alternately, we could make cc always allow LCD text in the ui
> >> > compositor,
> >> > which would more closely match what it does right now. But enne's
> >> > comment
> >> > was that they believe it is this way now just to avoid invalidating
> the
> >> > ui::Layer. This isn't a concern anymore with impl-side painting
> though.
> >> >
> >> >
> >> >>
> >> >>
> >> >>
> >> >>   -Scott
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >> >>>
> >> >>> File cc/resources/resource_provider.cc (right):
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >> >>>
> >> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
> >> >>> On 2014/11/13 17:23:15, reveman wrote:
> >> >>> > On 2014/11/11 21:56:44, danakj wrote:
> >> >>> > > On 2014/11/11 21:16:49, reveman wrote:
> >> >>> > > > this conditional seems overkill. Temporary variables for
> >> >>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text
> is
> >> >>> > > > nice
> >> >>
> >> >> but
> >> >>>
> >> >>> > > > create_surface is not needed imo.
> >> >>> > >
> >> >>> > > It avoids 2 virtual function calls per resource in the common
> case
> >> >>> > > though?
> >> >>> >
> >> >>> > What are the virtual function calls we avoid here? Even if that's
> >> >>> > the
> >> >>> > case,
> >> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems
> like
> >> >>> > a
> >> >>> premature
> >> >>> > micro-optimization that I have a hard time to imagine makes a
> >> >>> > difference. I
> >> >>> > prefer if we keep this as simple and clean as possible for now.
> >> >>
> >> >>
> >> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> https://codereview.chromium.org/684543006/
> >> >
> >> >
> >
> >
>

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

[danakj, 2014-11-13 21:50:23]

On 2014/11/13 21:30:03, danakj wrote:
> On Thu, Nov 13, 2014 at 3:50 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> > This will likely effect chromeos mode on windows when scaled and
> > chromeos on devices with non integer scales (AFAIK there is only one
> > device ever shipped like this).
> >
> > Lets say you're on a chromeos device with a non-integer scale and drag
> > a window around. Which means you're dragging a layer around and
> > because it's a non-integer scale the layer is going to bounce between
> > landing on a pixel and not landing on pixels. If we did flip back and
> > forth between lcd and non-lcd won't the text change quite a bit?
> >
> 
> Ideally we'd be pixel snapping the transform at each step or the text is
> going to look awful as you're moving it, and/or at the new position. Surely
> we already do this to avoid bad text..? If not it would look better to have
> grayscale AA text than to have LCD text that is not aligning to the pixels.
> 
> I don't see anything in the ui code to turn off LCD text except in a
> transparent-background case. So I assume we're pixel snapping so things
> don't look weird. But I'm not sure where that would be happening..

enne points out that on chromeos there are window scale animations and these
would destroy lcd text.

So I'll propose that I add a setting that ui compositor sets that makes cc let
all layers use LCD text as specified by the ui::Layer painting, and never tries
to disable it itself. This should match or improve current behaviour.

[enne (OOO), 2014-11-13 21:52:32]

Yeah.  In the short term in this patch, I think PictureLayerImpl can just add
its own synthetic invalidations.  (And, ContentLayer can just do what
ui::Compositor needs asymmetrically.)  In the long term, we need an additional
setting for force LCD test and if that setting is on, we can not synthetically
invalidate and always raster with lcd text on.

[enne (OOO), 2014-11-13 21:53:08]

Er, *PictureLayer* can add its own invalidations.

[sky, 2014-11-13 21:57:13]

I believe we snap the layer for the renderer to pixels, but not other
layers. Some of these other layers do draw text, such as the tabstrip.

I don't know what the right answer is, I just want to avoid things
looking crap:)

  -Scott

On Thu, Nov 13, 2014 at 1:29 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Thu, Nov 13, 2014 at 3:50 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> This will likely effect chromeos mode on windows when scaled and
>> chromeos on devices with non integer scales (AFAIK there is only one
>> device ever shipped like this).
>>
>> Lets say you're on a chromeos device with a non-integer scale and drag
>> a window around. Which means you're dragging a layer around and
>> because it's a non-integer scale the layer is going to bounce between
>> landing on a pixel and not landing on pixels. If we did flip back and
>> forth between lcd and non-lcd won't the text change quite a bit?
>
>
> Ideally we'd be pixel snapping the transform at each step or the text is
> going to look awful as you're moving it, and/or at the new position. Surely
> we already do this to avoid bad text..? If not it would look better to have
> grayscale AA text than to have LCD text that is not aligning to the pixels.
>
> I don't see anything in the ui code to turn off LCD text except in a
> transparent-background case. So I assume we're pixel snapping so things
> don't look weird. But I'm not sure where that would be happening..
>
>>
>>
>>   -Scott
>>
>> On Thu, Nov 13, 2014 at 12:30 PM, Dana Jansens <danakj@chromium.org>
>> wrote:
>> > On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org> wrote:
>> >>
>> >> Thanks for the summary.
>> >>
>> >> Under what conditions would a transform result in looking bad? Is it
>> >> only during animation? Screen rotation? Scaling for high dpi?
>> >
>> >
>> > Transforms with non-integer translations, ie the origin doesn't align
>> > with a
>> > pixel. Or rotations, etc.
>> >
>> > On high-dpi we should still be putting stuff on pixels not between them
>> > so
>> > it's okay in that state. Screen rotation will make LCD text not work
>> > correctly at all, yah.
>> >
>> >>
>> >>
>> >>   -Scott
>> >>
>> >> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
>> >> wrote:
>> >> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
>> >> >>
>> >> >> On 2014/11/13 17:30:32, danakj wrote:
>> >> >>>
>> >> >>> On 2014/11/13 17:30:03, danakj wrote:
>> >> >>> > On 2014/11/12 23:04:48, enne wrote:
>> >> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
>> >> >>> > > > enne, you previously asked me to restore the
>> >> >>> > > > DidChangeLayerCanUseLcdText
>> >> >>> > > method because blink wants to invalidate everything but ui
>> >> >>> > > doesn't..
>> >> >>> > > after
>> >> >>> > > looking at the ui code I'm starting to question this logic,
>> >> >>> > > even
>> >> >>> > > for
>> >> >>
>> >> >> blink.
>> >> >>>
>> >> >>> > > >
>> >> >>> > > > In a world where blink always records with LCD text enabled,
>> >> >>> > > > cc
>> >> >>> > > > changing
>> >> >>> > that
>> >> >>> > > state on a layer does not change the inputs to blink at all.
>> >> >>> > > Invalidating
>> >> >>> the
>> >> >>> > > recordings does nothing except make blink re-record the same
>> >> >>> > > thing
>> >> >>> > > again.
>> >> >>> > > >
>> >> >>> > > > Rather, we need to drop rastered tiles, so we need to insert
>> >> >>> > > > synthetic
>> >> >>> > > invalidation on update for the entire layer to drop anything
>> >> >>> > > rastered. But
>> >> >>> we
>> >> >>> > > should do that for the ui/ too, since otherwise we can end up
>> >> >>> > > with
>> >> >>> > > 1
>> >> >>> > > tile
>> >> >>
>> >> >> in
>> >> >>>
>> >> >>> > LCD
>> >> >>> > > text and 1 not?
>> >> >>> > >
>> >> >>> > > I'd ask piman's opinion.  My memory is that this was hooked up
>> >> >>> > > this
>> >> >>> > > way
>> >> >>> > because
>> >> >>> > > many ui::Layers expected to paint once and then drop whatever
>> >> >>> > > resources
>> >> >>
>> >> >> were
>> >> >>>
>> >> >>> > > used to raster them because they'd never get invalidated again.
>> >> >>> > > I'm
>> >> >>> > > not
>> >> >>> sure
>> >> >>> > if
>> >> >>> > > ui::Layer wants to reraster if it ever transforms, or if it
>> >> >>> > > wants
>> >> >>> > > to
>> >> >>> > > be
>> >> >>
>> >> >> more
>> >> >>>
>> >> >>> > in
>> >> >>> > > control of that too.
>> >> >>> >
>> >> >>> > For a ContentLayer re-raster requires us going to the client and
>> >> >>> > asking
>> >> >>> > for
>> >> >>> > resources etc. But with impl-side re-rastering doesn't require
>> >> >>> > that.
>> >> >>> > The UI
>> >> >>> code
>> >> >>> > can throw away whatever it wants and cc will hold onto a
>> >> >>> > recording
>> >> >>> > for
>> >> >>> > it to
>> >> >>> do
>> >> >>> > those re-rasters with. So this doesn't seem to violate that
>> >> >>> > requirement.
>> >> >>> >
>> >> >>> > > I think you are right in the short term that PictureLayer could
>> >> >>> > > just
>> >> >>> > > add a
>> >> >>> > > synthetic invalidation because ui::Layer isn't using it (yet).
>> >> >>> > > If
>> >> >>> > > the
>> >> >>> longer
>> >> >>> > > term answer is that it wants to behave differently, then this
>> >> >>> > > client
>> >> >>> > > call
>> >> >>> that
>> >> >>> > > you have now is good enough.
>> >> >>> >
>> >> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do
>> >> >>> > anything
>> >> >>> > for
>> >> >>> blink
>> >> >>> > since we always tell it true now, and I'll add invalidations in
>> >> >>> > PictureLayer::Update to pass to the impl thread. Does that sounds
>> >> >>> > good
>> >> >>> > to
>> >> >>
>> >> >> you?
>> >> >>>
>> >> >>> >
>> >> >>> > But I'm not sure what use an always-empty client function is at
>> >> >>> > that
>> >> >>> > point
>> >> >>> > though. We can add it back later if we need to?
>> >> >>
>> >> >>
>> >> >>> +sky for comment about ui::Layer and LCD text
>> >> >>
>> >> >>
>> >> >> Sorry, what comment is that? (too long a thread).
>> >> >
>> >> >
>> >> > Np, I will try to resummarize.
>> >> >
>> >> > Currently when a layer gets a transform where LCD text would look
>> >> > bad,
>> >> > ui
>> >> > compositor doesn't do anything with the notification, so it doesn't
>> >> > invalidate the contents and will continue to draw the LCD text
>> >> > anyways.
>> >> >
>> >> > With impl-side painting we can behave differently if desirable,
>> >> > trying
>> >> > to
>> >> > discern what is best:
>> >> >
>> >> > Since we take a recording of the layer contents when we paint, we can
>> >> > re-raster it without involving the ui::Layer at all. So when a layer
>> >> > would
>> >> > not display LCD text we can make it fall back to non-LCD text and
>> >> > reraster
>> >> > its contents to do so. With blink once a layer does this it stays
>> >> > non-LCD
>> >> > for the rest of its lifetime currently.
>> >> >
>> >> > Alternately, we could make cc always allow LCD text in the ui
>> >> > compositor,
>> >> > which would more closely match what it does right now. But enne's
>> >> > comment
>> >> > was that they believe it is this way now just to avoid invalidating
>> >> > the
>> >> > ui::Layer. This isn't a concern anymore with impl-side painting
>> >> > though.
>> >> >
>> >> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>   -Scott
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>> >> >>>
>> >> >>> File cc/resources/resource_provider.cc (right):
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
>> >> >>>
>> >> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
>> >> >>> On 2014/11/13 17:23:15, reveman wrote:
>> >> >>> > On 2014/11/11 21:56:44, danakj wrote:
>> >> >>> > > On 2014/11/11 21:16:49, reveman wrote:
>> >> >>> > > > this conditional seems overkill. Temporary variables for
>> >> >>> > > > surface_uses_distance_field_text and surface_can_use_lcd_text
>> >> >>> > > > is
>> >> >>> > > > nice
>> >> >>
>> >> >> but
>> >> >>>
>> >> >>> > > > create_surface is not needed imo.
>> >> >>> > >
>> >> >>> > > It avoids 2 virtual function calls per resource in the common
>> >> >>> > > case
>> >> >>> > > though?
>> >> >>> >
>> >> >>> > What are the virtual function calls we avoid here? Even if that's
>> >> >>> > the
>> >> >>> > case,
>> >> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems
>> >> >>> > like
>> >> >>> > a
>> >> >>> premature
>> >> >>> > micro-optimization that I have a hard time to imagine makes a
>> >> >>> > difference. I
>> >> >>> > prefer if we keep this as simple and clean as possible for now.
>> >> >>
>> >> >>
>> >> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops. OK.
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> https://codereview.chromium.org/684543006/
>> >> >
>> >> >
>> >
>> >
>
>

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

[chromium-reviews, 2014-11-13 21:59:18]

Just ship more high-res screens, and then we can turn off LCD globally and
it will still look nice (and blit faster!)

On Thu, Nov 13, 2014 at 4:57 PM, Scott Violet <sky@chromium.org> wrote:

> I believe we snap the layer for the renderer to pixels, but not other
> layers. Some of these other layers do draw text, such as the tabstrip.
>
> I don't know what the right answer is, I just want to avoid things
> looking crap:)
>
>   -Scott
>
> On Thu, Nov 13, 2014 at 1:29 PM, Dana Jansens <danakj@chromium.org> wrote:
> > On Thu, Nov 13, 2014 at 3:50 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> This will likely effect chromeos mode on windows when scaled and
> >> chromeos on devices with non integer scales (AFAIK there is only one
> >> device ever shipped like this).
> >>
> >> Lets say you're on a chromeos device with a non-integer scale and drag
> >> a window around. Which means you're dragging a layer around and
> >> because it's a non-integer scale the layer is going to bounce between
> >> landing on a pixel and not landing on pixels. If we did flip back and
> >> forth between lcd and non-lcd won't the text change quite a bit?
> >
> >
> > Ideally we'd be pixel snapping the transform at each step or the text is
> > going to look awful as you're moving it, and/or at the new position.
> Surely
> > we already do this to avoid bad text..? If not it would look better to
> have
> > grayscale AA text than to have LCD text that is not aligning to the
> pixels.
> >
> > I don't see anything in the ui code to turn off LCD text except in a
> > transparent-background case. So I assume we're pixel snapping so things
> > don't look weird. But I'm not sure where that would be happening..
> >
> >>
> >>
> >>   -Scott
> >>
> >> On Thu, Nov 13, 2014 at 12:30 PM, Dana Jansens <danakj@chromium.org>
> >> wrote:
> >> > On Thu, Nov 13, 2014 at 3:15 PM, Scott Violet <sky@chromium.org>
> wrote:
> >> >>
> >> >> Thanks for the summary.
> >> >>
> >> >> Under what conditions would a transform result in looking bad? Is it
> >> >> only during animation? Screen rotation? Scaling for high dpi?
> >> >
> >> >
> >> > Transforms with non-integer translations, ie the origin doesn't align
> >> > with a
> >> > pixel. Or rotations, etc.
> >> >
> >> > On high-dpi we should still be putting stuff on pixels not between
> them
> >> > so
> >> > it's okay in that state. Screen rotation will make LCD text not work
> >> > correctly at all, yah.
> >> >
> >> >>
> >> >>
> >> >>   -Scott
> >> >>
> >> >> On Thu, Nov 13, 2014 at 10:38 AM, Dana Jansens <danakj@chromium.org>
> >> >> wrote:
> >> >> > On Thu, Nov 13, 2014 at 1:29 PM, <sky@chromium.org> wrote:
> >> >> >>
> >> >> >> On 2014/11/13 17:30:32, danakj wrote:
> >> >> >>>
> >> >> >>> On 2014/11/13 17:30:03, danakj wrote:
> >> >> >>> > On 2014/11/12 23:04:48, enne wrote:
> >> >> >>> > > On 2014/11/12 at 22:57:52, danakj wrote:
> >> >> >>> > > > enne, you previously asked me to restore the
> >> >> >>> > > > DidChangeLayerCanUseLcdText
> >> >> >>> > > method because blink wants to invalidate everything but ui
> >> >> >>> > > doesn't..
> >> >> >>> > > after
> >> >> >>> > > looking at the ui code I'm starting to question this logic,
> >> >> >>> > > even
> >> >> >>> > > for
> >> >> >>
> >> >> >> blink.
> >> >> >>>
> >> >> >>> > > >
> >> >> >>> > > > In a world where blink always records with LCD text
> enabled,
> >> >> >>> > > > cc
> >> >> >>> > > > changing
> >> >> >>> > that
> >> >> >>> > > state on a layer does not change the inputs to blink at all.
> >> >> >>> > > Invalidating
> >> >> >>> the
> >> >> >>> > > recordings does nothing except make blink re-record the same
> >> >> >>> > > thing
> >> >> >>> > > again.
> >> >> >>> > > >
> >> >> >>> > > > Rather, we need to drop rastered tiles, so we need to
> insert
> >> >> >>> > > > synthetic
> >> >> >>> > > invalidation on update for the entire layer to drop anything
> >> >> >>> > > rastered. But
> >> >> >>> we
> >> >> >>> > > should do that for the ui/ too, since otherwise we can end up
> >> >> >>> > > with
> >> >> >>> > > 1
> >> >> >>> > > tile
> >> >> >>
> >> >> >> in
> >> >> >>>
> >> >> >>> > LCD
> >> >> >>> > > text and 1 not?
> >> >> >>> > >
> >> >> >>> > > I'd ask piman's opinion.  My memory is that this was hooked
> up
> >> >> >>> > > this
> >> >> >>> > > way
> >> >> >>> > because
> >> >> >>> > > many ui::Layers expected to paint once and then drop whatever
> >> >> >>> > > resources
> >> >> >>
> >> >> >> were
> >> >> >>>
> >> >> >>> > > used to raster them because they'd never get invalidated
> again.
> >> >> >>> > > I'm
> >> >> >>> > > not
> >> >> >>> sure
> >> >> >>> > if
> >> >> >>> > > ui::Layer wants to reraster if it ever transforms, or if it
> >> >> >>> > > wants
> >> >> >>> > > to
> >> >> >>> > > be
> >> >> >>
> >> >> >> more
> >> >> >>>
> >> >> >>> > in
> >> >> >>> > > control of that too.
> >> >> >>> >
> >> >> >>> > For a ContentLayer re-raster requires us going to the client
> and
> >> >> >>> > asking
> >> >> >>> > for
> >> >> >>> > resources etc. But with impl-side re-rastering doesn't require
> >> >> >>> > that.
> >> >> >>> > The UI
> >> >> >>> code
> >> >> >>> > can throw away whatever it wants and cc will hold onto a
> >> >> >>> > recording
> >> >> >>> > for
> >> >> >>> > it to
> >> >> >>> do
> >> >> >>> > those re-rasters with. So this doesn't seem to violate that
> >> >> >>> > requirement.
> >> >> >>> >
> >> >> >>> > > I think you are right in the short term that PictureLayer
> could
> >> >> >>> > > just
> >> >> >>> > > add a
> >> >> >>> > > synthetic invalidation because ui::Layer isn't using it
> (yet).
> >> >> >>> > > If
> >> >> >>> > > the
> >> >> >>> longer
> >> >> >>> > > term answer is that it wants to behave differently, then this
> >> >> >>> > > client
> >> >> >>> > > call
> >> >> >>> that
> >> >> >>> > > you have now is good enough.
> >> >> >>> >
> >> >> >>> > I'll get rid of the SetNeedsDisplay() for now, it doesn't do
> >> >> >>> > anything
> >> >> >>> > for
> >> >> >>> blink
> >> >> >>> > since we always tell it true now, and I'll add invalidations in
> >> >> >>> > PictureLayer::Update to pass to the impl thread. Does that
> sounds
> >> >> >>> > good
> >> >> >>> > to
> >> >> >>
> >> >> >> you?
> >> >> >>>
> >> >> >>> >
> >> >> >>> > But I'm not sure what use an always-empty client function is at
> >> >> >>> > that
> >> >> >>> > point
> >> >> >>> > though. We can add it back later if we need to?
> >> >> >>
> >> >> >>
> >> >> >>> +sky for comment about ui::Layer and LCD text
> >> >> >>
> >> >> >>
> >> >> >> Sorry, what comment is that? (too long a thread).
> >> >> >
> >> >> >
> >> >> > Np, I will try to resummarize.
> >> >> >
> >> >> > Currently when a layer gets a transform where LCD text would look
> >> >> > bad,
> >> >> > ui
> >> >> > compositor doesn't do anything with the notification, so it doesn't
> >> >> > invalidate the contents and will continue to draw the LCD text
> >> >> > anyways.
> >> >> >
> >> >> > With impl-side painting we can behave differently if desirable,
> >> >> > trying
> >> >> > to
> >> >> > discern what is best:
> >> >> >
> >> >> > Since we take a recording of the layer contents when we paint, we
> can
> >> >> > re-raster it without involving the ui::Layer at all. So when a
> layer
> >> >> > would
> >> >> > not display LCD text we can make it fall back to non-LCD text and
> >> >> > reraster
> >> >> > its contents to do so. With blink once a layer does this it stays
> >> >> > non-LCD
> >> >> > for the rest of its lifetime currently.
> >> >> >
> >> >> > Alternately, we could make cc always allow LCD text in the ui
> >> >> > compositor,
> >> >> > which would more closely match what it does right now. But enne's
> >> >> > comment
> >> >> > was that they believe it is this way now just to avoid invalidating
> >> >> > the
> >> >> > ui::Layer. This isn't a concern anymore with impl-side painting
> >> >> > though.
> >> >> >
> >> >> >
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>   -Scott
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >> >> >>>
> >> >> >>> File cc/resources/resource_provider.cc (right):
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
>
https://codereview.chromium.org/684543006/diff/100001/cc/resources/resource_p...
> >> >> >>>
> >> >> >>> cc/resources/resource_provider.cc:1111: if (!create_surface) {
> >> >> >>> On 2014/11/13 17:23:15, reveman wrote:
> >> >> >>> > On 2014/11/11 21:56:44, danakj wrote:
> >> >> >>> > > On 2014/11/11 21:16:49, reveman wrote:
> >> >> >>> > > > this conditional seems overkill. Temporary variables for
> >> >> >>> > > > surface_uses_distance_field_text and
> surface_can_use_lcd_text
> >> >> >>> > > > is
> >> >> >>> > > > nice
> >> >> >>
> >> >> >> but
> >> >> >>>
> >> >> >>> > > > create_surface is not needed imo.
> >> >> >>> > >
> >> >> >>> > > It avoids 2 virtual function calls per resource in the common
> >> >> >>> > > case
> >> >> >>> > > though?
> >> >> >>> >
> >> >> >>> > What are the virtual function calls we avoid here? Even if
> that's
> >> >> >>> > the
> >> >> >>> > case,
> >> >> >>> > avoiding 2 virtual function calls per gpu rasterized tile seems
> >> >> >>> > like
> >> >> >>> > a
> >> >> >>> premature
> >> >> >>> > micro-optimization that I have a hard time to imagine makes a
> >> >> >>> > difference. I
> >> >> >>> > prefer if we keep this as simple and clean as possible for now.
> >> >> >>
> >> >> >>
> >> >> >>> Mmh.. oh I was thinking of the CanUseLcdText function.. woops.
> OK.
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> https://codereview.chromium.org/684543006/
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

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

[enne (OOO), 2014-11-13 21:59:31]

On 2014/11/13 at 21:57:13, sky wrote:
> I believe we snap the layer for the renderer to pixels, but not other
> layers. Some of these other layers do draw text, such as the tabstrip.
> 
> I don't know what the right answer is, I just want to avoid things
> looking crap:)

There really isn't a right answer.  I think ui::Compositor needs to retain
control of performance vs. correctness tradeoffs (e.g. window scale animations
should be fast).  Blink doesn't have that control, so needs automatic
re-rastering.

[danakj, 2014-11-13 23:27:17]

Patchset #17 (id:320001) has been deleted

[danakj, 2014-11-13 23:27:28]

Patchset #17 (id:340001) has been deleted

[danakj, 2014-11-13 23:32:41]

New patch set does:

- Adds a layers_always_allowed_lcd_text to LayerTreeSettings, and passes true
for ui compositor.
- Uses the setting to always allow lcd text in CalcDrawProps.
- Adds test coverage for CalcDrawProps with this setting.
- Makes picture_pile add synthetic invalidation when lcd text state changes.
- Makes picture_layer not early out of update when lcd text state changes.
- Updates the LTH test coverage for LCD text for new expectations that we don't
paint/record but we do raster when LCD text state changes.

[enne (OOO), 2014-11-14 00:05:46]

lgtm

https://codereview.chromium.org/684543006/diff/380001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/684543006/diff/380001/cc/resources/raster_sou...
cc/resources/raster_source.h:97: virtual bool CanUseLcdText() const = 0;
Lcd => LCD

https://codereview.chromium.org/684543006/diff/380001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/684543006/diff/380001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2396:
SINGLE_AND_MULTI_THREAD_IMPL_TEST_F(LayerTreeHostTestLcdChange);
Nice try, but Lcd => LCD

$ git grep Lcd cc | wc -l
0

$ git grep LCD cc | wc -l
63

https://codereview.chromium.org/684543006/diff/380001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/684543006/diff/380001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:57: bool layers_always_allowed_lcd_text;
I wasn't sure about this being a layer tree setting vs a per layer setting, but
I guess the way it is used now is as if it were a layer tree setting.  If
ui::Compositor ever wants a per layer setting, we can change this.

https://codereview.chromium.org/684543006/diff/380001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/684543006/diff/380001/ui/compositor/composito...
ui/compositor/compositor.cc:93: settings.layers_always_allowed_lcd_text = true;
Would it make sense to leave a comment here about this not doing anything until
impl-side painting is enabled? It's a little weird to have a setting that
doesn't do anything, even if the implicit behavior in ContentLayer is to do what
this setting does when true.

[danakj, 2014-11-14 15:50:17]

Thanks, all done.

[f(malita), 2014-11-14 15:51:11]

On 2014/11/13 23:32:41, danakj wrote:
> New patch set does:
> 
> - Adds a layers_always_allowed_lcd_text to LayerTreeSettings, and passes true
> for ui compositor.
> - Uses the setting to always allow lcd text in CalcDrawProps.
> - Adds test coverage for CalcDrawProps with this setting.
> - Makes picture_pile add synthetic invalidation when lcd text state changes.
> - Makes picture_layer not early out of update when lcd text state changes.
> - Updates the LTH test coverage for LCD text for new expectations that we
don't
> paint/record but we do raster when LCD text state changes.

Something I just ran into: while use_lcd/GC::shouldSmoothFonts() plumbing is a
big piece of Blink's LCD logic, there's also
GC::isCertainlyOpaque()/GC::setCertainlyOpaque(). I'm still figuring how that is
being pushed into Blink, but there's significant machinery in place to get it
into GC where all it does is get &&ed with shouldSmoothFonts() to disable LCD
(at record time) for non-opaque surfaces :(

I don't think we should delay this CL, but it certainly seems that if opaqueness
is known in CC (I assume it must be) then we should take that into consideration
when deciding whether or not to disable LCD on the surface. Then Blink (or at
least GC) can be oblivious of target canvas transparency.

[danakj, 2014-11-14 15:55:30]

On Fri, Nov 14, 2014 at 10:51 AM, <fmalita@chromium.org> wrote:

> On 2014/11/13 23:32:41, danakj wrote:
>
>> New patch set does:
>>
>
>  - Adds a layers_always_allowed_lcd_text to LayerTreeSettings, and passes
>> true
>> for ui compositor.
>> - Uses the setting to always allow lcd text in CalcDrawProps.
>> - Adds test coverage for CalcDrawProps with this setting.
>> - Makes picture_pile add synthetic invalidation when lcd text state
>> changes.
>> - Makes picture_layer not early out of update when lcd text state changes.
>> - Updates the LTH test coverage for LCD text for new expectations that we
>>
> don't
>
>> paint/record but we do raster when LCD text state changes.
>>
>
> Something I just ran into: while use_lcd/GC::shouldSmoothFonts() plumbing
> is a
> big piece of Blink's LCD logic, there's also
> GC::isCertainlyOpaque()/GC::setCertainlyOpaque(). I'm still figuring how
> that is
> being pushed into Blink, but there's significant machinery in place to get
> it
> into GC where all it does is get &&ed with shouldSmoothFonts() to disable
> LCD
> (at record time) for non-opaque surfaces :(
>
> I don't think we should delay this CL, but it certainly seems that if
> opaqueness
> is known in CC (I assume it must be) then we should take that into
> consideration
> when deciding whether or not to disable LCD on the surface. Then Blink (or
> at
> least GC) can be oblivious of target canvas transparency.
>

Certainly opaque comes from GraphicsLayer right now. It's passed to cc/ and
to the contents layer delegate:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

When contents opaqueness changes, we'll have to re-record, so it might make
sense to change LCD text on/off in blink when it does that recording. We
could make that decision else-where too, if you think that makes more
sense. I'd suggest doing that independently from this CL.

Right now if cc/ turns off LCD text on a layer it leaves it off forever.
Whereas if blink changes its background to be opaque or not, each recording
can change the LCD text, and blink knows the bounds to invalidate in order
to do so. Those are a couple reasons to leave it in blink, I think.

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

[danakj, 2014-11-14 15:56:09]

On Fri, Nov 14, 2014 at 10:55 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Nov 14, 2014 at 10:51 AM, <fmalita@chromium.org> wrote:
>
>> On 2014/11/13 23:32:41, danakj wrote:
>>
>>> New patch set does:
>>>
>>
>>  - Adds a layers_always_allowed_lcd_text to LayerTreeSettings, and passes
>>> true
>>> for ui compositor.
>>> - Uses the setting to always allow lcd text in CalcDrawProps.
>>> - Adds test coverage for CalcDrawProps with this setting.
>>> - Makes picture_pile add synthetic invalidation when lcd text state
>>> changes.
>>> - Makes picture_layer not early out of update when lcd text state
>>> changes.
>>> - Updates the LTH test coverage for LCD text for new expectations that we
>>>
>> don't
>>
>>> paint/record but we do raster when LCD text state changes.
>>>
>>
>> Something I just ran into: while use_lcd/GC::shouldSmoothFonts()
>> plumbing is a
>> big piece of Blink's LCD logic, there's also
>> GC::isCertainlyOpaque()/GC::setCertainlyOpaque(). I'm still figuring how
>> that is
>> being pushed into Blink, but there's significant machinery in place to
>> get it
>> into GC where all it does is get &&ed with shouldSmoothFonts() to disable
>> LCD
>> (at record time) for non-opaque surfaces :(
>>
>> I don't think we should delay this CL, but it certainly seems that if
>> opaqueness
>> is known in CC (I assume it must be) then we should take that into
>> consideration
>> when deciding whether or not to disable LCD on the surface. Then Blink
>> (or at
>> least GC) can be oblivious of target canvas transparency.
>>
>
> Certainly opaque comes from GraphicsLayer right now.
>

Er, Currently, I mean.

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

[danakj, 2014-11-14 15:57:15]

LGTY reveman?

[f(malita), 2014-11-14 17:09:10]

On 2014/11/14 15:55:30, danakj wrote:
> When contents opaqueness changes, we'll have to re-record, so it might make
> sense to change LCD text on/off in blink when it does that recording. We
> could make that decision else-where too, if you think that makes more
> sense. I'd suggest doing that independently from this CL.

Absolutely, let's take the discussion off this thread.

[reveman, 2014-11-14 17:19:16]

lgtm with some more nits

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1102: static bool
ShouldCreateSurface(SkSurface* surface,
nit: can you move this to anonymous namespace and remove "static" keyword?

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1105: if (!surface)
nit: hm, maybe it's better to make this a private member function and remove the
|surface| parameter instead? or this utility function can be renamed to
something like "SurfaceHasMatchingProperties" and assume that |surface| is never
null and instead have the caller handle that case. I find it awkward to pass a
parameter to a function and first thing in the function check if it's null and
return early...

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1124: bool create_surface =
ShouldCreateSurface(
nit: does this temporary variable really help in terms of readability? if you
want to keep it, please rename it to "should_create_surface" unless there's a
good reason for it to not be consistent with the name of the utility function.

[danakj, 2014-11-17 18:19:58]

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1102: static bool
ShouldCreateSurface(SkSurface* surface,
On 2014/11/14 17:19:16, reveman wrote:
> nit: can you move this to anonymous namespace and remove "static" keyword?

Done.

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1105: if (!surface)
On 2014/11/14 17:19:16, reveman wrote:
> nit: hm, maybe it's better to make this a private member function and remove
the
> |surface| parameter instead? or this utility function can be renamed to
> something like "SurfaceHasMatchingProperties" and assume that |surface| is
never
> null and instead have the caller handle that case. I find it awkward to pass a
> parameter to a function and first thing in the function check if it's null and
> return early...

Done.

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1124: bool create_surface =
ShouldCreateSurface(
On 2014/11/14 17:19:16, reveman wrote:
> nit: does this temporary variable really help in terms of readability? if you
> want to keep it, please rename it to "should_create_surface" unless there's a
> good reason for it to not be consistent with the name of the utility function.

it makes the line wrapping easier to follow.

if (foo) {  // this is easier to read than

if (foo(bar,
        baz,
        bam)) {  //this

so i like having the var.

[danakj, 2014-11-17 18:20:44]

I will have to wait for https://codereview.chromium.org/732503005/ to land
before this, as it breaks/more-flakes the Pinch/ContinuousDraw tests without
that CL.

[danakj, 2014-11-17 19:22:11]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-11-17 19:23:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684543006/420001

[reveman, 2014-11-17 19:46:02]

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1102: static bool
ShouldCreateSurface(SkSurface* surface,
On 2014/11/17 18:19:59, danakj wrote:
> On 2014/11/14 17:19:16, reveman wrote:
> > nit: can you move this to anonymous namespace and remove "static" keyword?
> 
> Done.

I don't see this in latest patch. Did you forget to upload?

https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1105: if (!surface)
On 2014/11/17 18:19:59, danakj wrote:
> On 2014/11/14 17:19:16, reveman wrote:
> > nit: hm, maybe it's better to make this a private member function and remove
> the
> > |surface| parameter instead? or this utility function can be renamed to
> > something like "SurfaceHasMatchingProperties" and assume that |surface| is
> never
> > null and instead have the caller handle that case. I find it awkward to pass
a
> > parameter to a function and first thing in the function check if it's null
and
> > return early...
> 
> Done.

also missing in latest patch

[danakj, 2014-11-17 19:48:02]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-11-17 19:49:41]

On 2014/11/17 19:46:02, reveman wrote:
>
https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
> cc/resources/resource_provider.cc:1102: static bool
> ShouldCreateSurface(SkSurface* surface,
> On 2014/11/17 18:19:59, danakj wrote:
> > On 2014/11/14 17:19:16, reveman wrote:
> > > nit: can you move this to anonymous namespace and remove "static" keyword?
> > 
> > Done.
> 
> I don't see this in latest patch. Did you forget to upload?
> 
>
https://codereview.chromium.org/684543006/diff/400001/cc/resources/resource_p...
> cc/resources/resource_provider.cc:1105: if (!surface)
> On 2014/11/17 18:19:59, danakj wrote:
> > On 2014/11/14 17:19:16, reveman wrote:
> > > nit: hm, maybe it's better to make this a private member function and
remove
> > the
> > > |surface| parameter instead? or this utility function can be renamed to
> > > something like "SurfaceHasMatchingProperties" and assume that |surface| is
> > never
> > > null and instead have the caller handle that case. I find it awkward to
pass
> a
> > > parameter to a function and first thing in the function check if it's null
> and
> > > return early...
> > 
> > Done.
> 
> also missing in latest patch

Argh, ya I stashed them somehow.

[danakj, 2014-11-17 19:51:06]

Patchset #21 (id:440001) has been deleted

[danakj, 2014-11-17 19:53:44]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-11-17 19:54:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684543006/460001

[danakj, 2014-11-17 20:34:10]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-11-17 20:34:25]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-11-17 20:34:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684543006/460001

[commit-bot: I haz the power, 2014-11-17 21:48:09]

Committed patchset #21 (id:460001)

[commit-bot: I haz the power, 2014-11-17 21:48:47]

Patchset 21 (id:??) landed as
https://crrev.com/ad672f645e82ca677978b097a170c908c614d184
Cr-Commit-Position: refs/heads/master@{#304486}

[Jeffrey Yasskin, 2014-11-18 01:19:52]

jyasskin@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Jeffrey Yasskin, 2014-11-18 01:19:55]

This may have broken most of the MSan bots:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...,
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...,
and
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...
show traces like:

==9== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7ffe6939a3ce in
cc::PicturePile::UpdateAndExpandInvalidation(cc::ContentLayerClient*,
cc::Region*, unsigned int, bool, bool, bool, gfx::Size const&, gfx::Rect const&,
int, cc::Picture::RecordingMode) cc/resources/picture_pile.cc:215:7
    #1 0x7ffe692dc068 in cc::PictureLayer::Update(cc::ResourceUpdateQueue*,
cc::OcclusionTracker<cc::Layer> const*) cc/layers/picture_layer.cc:133:14
    #2 0x7ffe69424ea5 in
cc::LayerTreeHost::PaintLayerContents(cc::RenderSurfaceLayerList const&,
cc::ResourceUpdateQueue*, bool*, bool*) cc/trees/layer_tree_host.cc:1049:29
    #3 0x7ffe6942343d in cc::LayerTreeHost::UpdateLayers(cc::Layer*,
cc::ResourceUpdateQueue*) cc/trees/layer_tree_host.cc:886:3
    #4 0x7ffe6942260c in
cc::LayerTreeHost::UpdateLayers(cc::ResourceUpdateQueue*)
cc/trees/layer_tree_host.cc:758:17
    #5 0x7ffe694aa463 in
cc::ThreadProxy::BeginMainFrame(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)
cc/trees/thread_proxy.cc:779:18
    #6 0x7ffe694b6cc9 in Run base/bind_internal.h:190:12
    #7 0x7ffe694b6cc9 in base::internal::InvokeHelper<true, void,
base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>, void
(base::WeakPtr<cc::ThreadProxy> const&,
scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState>
>)>::MakeItSo(base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>,
base::WeakPtr<cc::ThreadProxy> const&,
scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)
base/bind_internal.h:909:0
    #8 0x7ffe694b6a55 in base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>, void
(cc::ThreadProxy*, scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >), void
(base::WeakPtr<cc::ThreadProxy>,
base::internal::PassedWrapper<scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> > >)>, void
(cc::ThreadProxy*, scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState>
>)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1248:12
    #9 0x7ffe5eb6ec33 in Run base/callback.h:401:12
    #10 0x7ffe5eb6ec33 in base::debug::TaskAnnotator::RunTask(char const*, char
const*, base::PendingTask const&) base/debug/task_annotator.cc:63:0
    #11 0x7ffe5eaa968e in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:448:3
    #12 0x7ffe5eaaa4ea in DeferOrRunPendingTask
base/message_loop/message_loop.cc:458:5
    #13 0x7ffe5eaaa4ea in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:567:0
    #14 0x7ffe5eab1132 in
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:32:21
    #15 0x7ffe5eadcc3f in base::RunLoop::Run() base/run_loop.cc:55:3
    #16 0x7ffe5eaa7d29 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:310:3
    #17 0x7ffe6acdf3c8 in content::RendererMain(content::MainFunctionParams
const&) content/renderer/renderer_main.cc:235:7
    #18 0x7ffe6a6f4b00 in content::RunZygote(content::MainFunctionParams const&,
content::ContentMainDelegate*) content/app/content_main_runner.cc:347:14
    #19 0x7ffe6a6f5e8a in
content::RunNamedProcessTypeMain(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&, content::ContentMainDelegate*)
content/app/content_main_runner.cc:431:12
    #20 0x7ffe6a6f7307 in content::ContentMainRunnerImpl::Run()
content/app/content_main_runner.cc:789:12
    #21 0x7ffe6a6f3d35 in content::ContentMain(content::ContentMainParams
const&) content/app/content_main.cc:19:15
    #22 0x7ffe6883fc4c in content::LaunchTests(content::TestLauncherDelegate*,
int, int, char**) content/public/test/test_launcher.cc:474:12
    #23 0x7ffe5e9addea in LaunchChromeTests(int, ChromeTestSuiteRunner*, int,
char**) chrome/test/base/chrome_test_launcher.cc:125:10
    #24 0x7ffe5d9b8870 in main chrome/test/base/browser_tests_main.cc:21:10
    #25 0x7ffe5180d76c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226:0
    #26 0x7ffe5bd34c28 in _start ??:0:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value ??:0 ??

I'm going to see if reverting locally fixes those, and then send a revert
through the CQ, unless I get a reply that someone's fixing it before me.

[Jeffrey Yasskin, 2014-11-18 01:43:36]

A revert of this CL (patchset #21 id:460001) has been created in
https://codereview.chromium.org/733233003/ by jyasskin@chromium.org.

The reason for reverting is: Broke several MSan bots:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...,
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...,
and
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Brows...
show traces like:

==9== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7ffe6939a3ce in
cc::PicturePile::UpdateAndExpandInvalidation(cc::ContentLayerClient*,
cc::Region*, unsigned int, bool, bool, bool, gfx::Size const&, gfx::Rect const&,
int, cc::Picture::RecordingMode) cc/resources/picture_pile.cc:215:7
    #1 0x7ffe692dc068 in cc::PictureLayer::Update(cc::ResourceUpdateQueue*,
cc::OcclusionTracker<cc::Layer> const*) cc/layers/picture_layer.cc:133:14
    #2 0x7ffe69424ea5 in
cc::LayerTreeHost::PaintLayerContents(cc::RenderSurfaceLayerList const&,
cc::ResourceUpdateQueue*, bool*, bool*) cc/trees/layer_tree_host.cc:1049:29
    #3 0x7ffe6942343d in cc::LayerTreeHost::UpdateLayers(cc::Layer*,
cc::ResourceUpdateQueue*) cc/trees/layer_tree_host.cc:886:3
    #4 0x7ffe6942260c in
cc::LayerTreeHost::UpdateLayers(cc::ResourceUpdateQueue*)
cc/trees/layer_tree_host.cc:758:17
    #5 0x7ffe694aa463 in
cc::ThreadProxy::BeginMainFrame(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)
cc/trees/thread_proxy.cc:779:18
    #6 0x7ffe694b6cc9 in Run base/bind_internal.h:190:12
    #7 0x7ffe694b6cc9 in base::internal::InvokeHelper<true, void,
base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>, void
(base::WeakPtr<cc::ThreadProxy> const&,
scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState>
)>::MakeItSo(base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>,
base::WeakPtr<cc::ThreadProxy> const&,
scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)
base/bind_internal.h:909:0
    #8 0x7ffe694b6a55 in base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(cc::ThreadProxy::*)(scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >)>, void
(cc::ThreadProxy*, scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> >), void
(base::WeakPtr<cc::ThreadProxy>,
base::internal::PassedWrapper<scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState> > >)>, void
(cc::ThreadProxy*, scoped_ptr<cc::ThreadProxy::BeginMainFrameAndCommitState,
base::DefaultDeleter<cc::ThreadProxy::BeginMainFrameAndCommitState>
)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1248:12
    #9 0x7ffe5eb6ec33 in Run base/callback.h:401:12
    #10 0x7ffe5eb6ec33 in base::debug::TaskAnnotator::RunTask(char const*, char
const*, base::PendingTask const&) base/debug/task_annotator.cc:63:0
    #11 0x7ffe5eaa968e in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:448:3
    #12 0x7ffe5eaaa4ea in DeferOrRunPendingTask
base/message_loop/message_loop.cc:458:5
    #13 0x7ffe5eaaa4ea in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:567:0
    #14 0x7ffe5eab1132 in
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:32:21
    #15 0x7ffe5eadcc3f in base::RunLoop::Run() base/run_loop.cc:55:3
    #16 0x7ffe5eaa7d29 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:310:3
    #17 0x7ffe6acdf3c8 in content::RendererMain(content::MainFunctionParams
const&) content/renderer/renderer_main.cc:235:7
    #18 0x7ffe6a6f4b00 in content::RunZygote(content::MainFunctionParams const&,
content::ContentMainDelegate*) content/app/content_main_runner.cc:347:14
    #19 0x7ffe6a6f5e8a in
content::RunNamedProcessTypeMain(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&, content::ContentMainDelegate*)
content/app/content_main_runner.cc:431:12
    #20 0x7ffe6a6f7307 in content::ContentMainRunnerImpl::Run()
content/app/content_main_runner.cc:789:12
    #21 0x7ffe6a6f3d35 in content::ContentMain(content::ContentMainParams
const&) content/app/content_main.cc:19:15
    #22 0x7ffe6883fc4c in content::LaunchTests(content::TestLauncherDelegate*,
int, int, char**) content/public/test/test_launcher.cc:474:12
    #23 0x7ffe5e9addea in LaunchChromeTests(int, ChromeTestSuiteRunner*, int,
char**) chrome/test/base/chrome_test_launcher.cc:125:10
    #24 0x7ffe5d9b8870 in main chrome/test/base/browser_tests_main.cc:21:10
    #25 0x7ffe5180d76c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226:0
    #26 0x7ffe5bd34c28 in _start ??:0:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value ??:0 ??

See http://www.chromium.org/developers/testing/memorysanitizer to test locally
under msan..

[danakj, 2014-11-18 15:59:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-11-18 16:01:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684543006/500001

[commit-bot: I haz the power, 2014-11-18 16:56:10]

Committed patchset #23 (id:500001)

[commit-bot: I haz the power, 2014-11-18 16:57:41]

Patchset 23 (id:??) landed as
https://crrev.com/3f76acec92ff4dbd8bc46e7add2f8f7a2cbacfb0
Cr-Commit-Position: refs/heads/master@{#304623}

[danakj, 2014-11-18 18:37:56]

The msan bot is green with this patch now. Yay.

[tfarina, 2014-11-26 15:11:55]

https://codereview.chromium.org/684543006/diff/500001/cc/blink/web_content_la...
File cc/blink/web_content_layer_impl.h (right):

https://codereview.chromium.org/684543006/diff/500001/cc/blink/web_content_la...
cc/blink/web_content_layer_impl.h:50: bool can_use_lcd_text_;
you can remove this member variable now, you made it a local variable in
PaintContents().