Plumb can_use_lcd_text, contents_opaque directly to ContentLayerClient.

Plumb can_use_lcd_text, contents_opaque directly to ContentLayerClient.

Currently can_use_lcd_text is inserted into the WebContentLayerClient
by the WebContentLayerImpl in its ContentLayerClient implementation.
And contents_opaque is plumbed to CC but then not passed to painting
and instead plumbed independently inside blink to a single painting
place to use it.

Unify this plumbing so it all comes through the paintContents()
method in the WebContentLayerClient directly from the cc Layer state.

BUG=

[enne (OOO), 2014-11-05 00:51:08]

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

[enne (OOO), 2014-11-05 00:51:09]

+reed

I'm not sure this is the right thing to do.  I got the impression that Skia
could change these flags post-recording.

I'm not saying that we should change these without a commit (which is what we
used to do), but it seems like it could be possible to keep Blink agnostic about
lcd text, make the decision about it at the cc::Layer level, and then use that
decision at playback time.

This would let us rip all of this out of Blink and keep all the plumbing inside
of cc.

[reed1, 2014-11-05 19:28:09]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-11-05 19:28:10]

+florin

I think this is great. The new feature in SkSurface/SkSurfaceProps is to enable
the surface to disable LCD, antialiasing, dither at runtime, without needing to
change the content/picture. In that model, Blink should just request LCD if that
is its pleasure, and we can disable as needed during playback.

... did I answer your question? :)

[danakj, 2014-11-05 19:58:24]

On 2014/11/05 19:28:10, reed1 wrote:
> +florin
> 
> I think this is great. The new feature in SkSurface/SkSurfaceProps is to
enable
> the surface to disable LCD, antialiasing, dither at runtime, without needing
to
> change the content/picture. In that model, Blink should just request LCD if
that
> is its pleasure, and we can disable as needed during playback.
> 
> ... did I answer your question? :)

I don't see us using SkSurface in our raster code right now. Does that mean we
can't do this yet, or should I be looking for another way to do this?

[danakj, 2014-11-05 20:16:01]

On 2014/11/05 19:58:24, danakj wrote:
> On 2014/11/05 19:28:10, reed1 wrote:
> > +florin
> > 
> > I think this is great. The new feature in SkSurface/SkSurfaceProps is to
> enable
> > the surface to disable LCD, antialiasing, dither at runtime, without needing
> to
> > change the content/picture. In that model, Blink should just request LCD if
> that
> > is its pleasure, and we can disable as needed during playback.
> > 
> > ... did I answer your question? :)
> 
> I don't see us using SkSurface in our raster code right now. Does that mean we
> can't do this yet, or should I be looking for another way to do this?

It seems like surfaces are only for GPU raster. How does this apply for software
raster? I am having trouble finding anything via SkCanvas.

[danakj, 2014-11-05 20:34:50]

On 2014/11/05 20:16:01, danakj wrote:
> On 2014/11/05 19:58:24, danakj wrote:
> > On 2014/11/05 19:28:10, reed1 wrote:
> > > +florin
> > > 
> > > I think this is great. The new feature in SkSurface/SkSurfaceProps is to
> > enable
> > > the surface to disable LCD, antialiasing, dither at runtime, without
needing
> > to
> > > change the content/picture. In that model, Blink should just request LCD
if
> > that
> > > is its pleasure, and we can disable as needed during playback.
> > > 
> > > ... did I answer your question? :)
> > 
> > I don't see us using SkSurface in our raster code right now. Does that mean
we
> > can't do this yet, or should I be looking for another way to do this?
> 
> It seems like surfaces are only for GPU raster. How does this apply for
software
> raster? I am having trouble finding anything via SkCanvas.

Ok I dug some more, it seems like I'd wrap a pixel pointer with a Surface
instead of a Bitmap.

I'm not sure how we do stuff like this with an SkSurface though:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[f(malita), 2014-11-05 20:55:33]

On 2014/11/05 20:34:50, danakj wrote:
> Ok I dug some more, it seems like I'd wrap a pixel pointer with a Surface
> instead of a Bitmap.
> 
> I'm not sure how we do stuff like this with an SkSurface though:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Hmm, I'm guessing what Blink is trying to say is "draw this using LCD if
possible, and if not use A8".

Ben/Mike: is this achievable using (kLCDRenderText_Flag | kGenA8FromLCD_Flag) on
paint + surface LCD props?

[bungeman-skia, 2014-11-05 22:07:58]

On 2014/11/05 20:55:33, Florin Malita wrote:
> On 2014/11/05 20:34:50, danakj wrote:
> > Ok I dug some more, it seems like I'd wrap a pixel pointer with a Surface
> > instead of a Bitmap.
> > 
> > I'm not sure how we do stuff like this with an SkSurface though:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Hmm, I'm guessing what Blink is trying to say is "draw this using LCD if
> possible, and if not use A8".
> 
> Ben/Mike: is this achievable using (kLCDRenderText_Flag | kGenA8FromLCD_Flag)
on
> paint + surface LCD props?

Not sure what the exact question is... but at the moment (kLCDRenderText_Flag |
kGenA8FromLCD_Flag) means, "I wanted to exploit the subpixel geometry to draw,
but it turns out I cannot, please draw the closest possible thing to what you
would have drawn when exploiting the subpixel geometry, but do regular single
channel anti-aliasing." In the future the idea is to mark the surface as having
an "unknown/flat" pixel geometry and then just setting the
lcd/exploit-subpixel-geometry flag on the paint. This will have the same effect,
but is much easier to work with. In other words, 'can_use_lcd_text' lives on the
SkSurface, by setting the SkSurface's pixel geometry to something other than
"unknown".

[f(malita), 2014-11-05 22:34:58]

On 2014/11/05 22:07:58, bungeman1 wrote:
> On 2014/11/05 20:55:33, Florin Malita wrote:
> > On 2014/11/05 20:34:50, danakj wrote:
> > > Ok I dug some more, it seems like I'd wrap a pixel pointer with a Surface
> > > instead of a Bitmap.
> > > 
> > > I'm not sure how we do stuff like this with an SkSurface though:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Hmm, I'm guessing what Blink is trying to say is "draw this using LCD if
> > possible, and if not use A8".
> > 
> > Ben/Mike: is this achievable using (kLCDRenderText_Flag |
kGenA8FromLCD_Flag)
> on
> > paint + surface LCD props?
> 
> Not sure what the exact question is... but at the moment (kLCDRenderText_Flag
|
> kGenA8FromLCD_Flag) means, "I wanted to exploit the subpixel geometry to draw,
> but it turns out I cannot, please draw the closest possible thing to what you
> would have drawn when exploiting the subpixel geometry, but do regular single
> channel anti-aliasing." In the future the idea is to mark the surface as
having
> an "unknown/flat" pixel geometry and then just setting the
> lcd/exploit-subpixel-geometry flag on the paint. This will have the same
effect,
> but is much easier to work with. In other words, 'can_use_lcd_text' lives on
the
> SkSurface, by setting the SkSurface's pixel geometry to something other than
> "unknown".


My interpretation of the comments in FontPlatformDataWin.cpp is that we need a
way for Blink to say "I want to draw this using LCD geometry if the target
surface allows it -- otherwise take the kGenA8FromLCD_Flag path". IOW we want to
defer that decision to Skia, since Blink has no freaking clue about target
surface capabilities at record-time. If I understand your comment correctly, we
currently do not support these semantics (the kGenA8FromLCD_Flag twist in
particular).

[reed1, 2014-11-05 23:54:07]

On 2014/11/05 22:34:58, Florin Malita wrote:
> On 2014/11/05 22:07:58, bungeman1 wrote:
> > On 2014/11/05 20:55:33, Florin Malita wrote:
> > > On 2014/11/05 20:34:50, danakj wrote:
> > > > Ok I dug some more, it seems like I'd wrap a pixel pointer with a
Surface
> > > > instead of a Bitmap.
> > > > 
> > > > I'm not sure how we do stuff like this with an SkSurface though:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Hmm, I'm guessing what Blink is trying to say is "draw this using LCD if
> > > possible, and if not use A8".
> > > 
> > > Ben/Mike: is this achievable using (kLCDRenderText_Flag |
> kGenA8FromLCD_Flag)
> > on
> > > paint + surface LCD props?
> > 
> > Not sure what the exact question is... but at the moment
(kLCDRenderText_Flag
> |
> > kGenA8FromLCD_Flag) means, "I wanted to exploit the subpixel geometry to
draw,
> > but it turns out I cannot, please draw the closest possible thing to what
you
> > would have drawn when exploiting the subpixel geometry, but do regular
single
> > channel anti-aliasing." In the future the idea is to mark the surface as
> having
> > an "unknown/flat" pixel geometry and then just setting the
> > lcd/exploit-subpixel-geometry flag on the paint. This will have the same
> effect,
> > but is much easier to work with. In other words, 'can_use_lcd_text' lives on
> the
> > SkSurface, by setting the SkSurface's pixel geometry to something other than
> > "unknown".
> 
> 
> My interpretation of the comments in FontPlatformDataWin.cpp is that we need a
> way for Blink to say "I want to draw this using LCD geometry if the target
> surface allows it -- otherwise take the kGenA8FromLCD_Flag path". IOW we want
to
> defer that decision to Skia, since Blink has no freaking clue about target
> surface capabilities at record-time. If I understand your comment correctly,
we
> currently do not support these semantics (the kGenA8FromLCD_Flag twist in
> particular).

SkSurface::NewRasterDirect(...) should allow you to wrap anything in a surface
for cpu.

We already set the GenA8 flag when we decided to "disable" and LCD request, so I
think that will come for free.

[reed1, 2014-11-05 23:54:33]

On 2014/11/05 23:54:07, reed1 wrote:
> On 2014/11/05 22:34:58, Florin Malita wrote:
> > On 2014/11/05 22:07:58, bungeman1 wrote:
> > > On 2014/11/05 20:55:33, Florin Malita wrote:
> > > > On 2014/11/05 20:34:50, danakj wrote:
> > > > > Ok I dug some more, it seems like I'd wrap a pixel pointer with a
> Surface
> > > > > instead of a Bitmap.
> > > > > 
> > > > > I'm not sure how we do stuff like this with an SkSurface though:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > Hmm, I'm guessing what Blink is trying to say is "draw this using LCD if
> > > > possible, and if not use A8".
> > > > 
> > > > Ben/Mike: is this achievable using (kLCDRenderText_Flag |
> > kGenA8FromLCD_Flag)
> > > on
> > > > paint + surface LCD props?
> > > 
> > > Not sure what the exact question is... but at the moment
> (kLCDRenderText_Flag
> > |
> > > kGenA8FromLCD_Flag) means, "I wanted to exploit the subpixel geometry to
> draw,
> > > but it turns out I cannot, please draw the closest possible thing to what
> you
> > > would have drawn when exploiting the subpixel geometry, but do regular
> single
> > > channel anti-aliasing." In the future the idea is to mark the surface as
> > having
> > > an "unknown/flat" pixel geometry and then just setting the
> > > lcd/exploit-subpixel-geometry flag on the paint. This will have the same
> > effect,
> > > but is much easier to work with. In other words, 'can_use_lcd_text' lives
on
> > the
> > > SkSurface, by setting the SkSurface's pixel geometry to something other
than
> > > "unknown".
> > 
> > 
> > My interpretation of the comments in FontPlatformDataWin.cpp is that we need
a
> > way for Blink to say "I want to draw this using LCD geometry if the target
> > surface allows it -- otherwise take the kGenA8FromLCD_Flag path". IOW we
want
> to
> > defer that decision to Skia, since Blink has no freaking clue about target
> > surface capabilities at record-time. If I understand your comment correctly,
> we
> > currently do not support these semantics (the kGenA8FromLCD_Flag twist in
> > particular).
> 
> SkSurface::NewRasterDirect(...) should allow you to wrap anything in a surface
> for cpu.
> 
> We already set the GenA8 flag when we decided to "disable" and LCD request, so
I
> think that will come for free.

by "We" I mean the skia code that disables LCD during raster...

[danakj, 2014-11-06 15:14:18]

On Wed, Nov 5, 2014 at 6:54 PM, <reed@google.com> wrote:

> We already set the GenA8 flag when we decided to "disable" and LCD
>> request, so
>>
> I
>
>> think that will come for free.
>>
>
> by "We" I mean the skia code that disables LCD during raster...
>

To be sure I understand: That means the blink recording code doesn't need
to set any flags on the recording? Or it needs to always set the LCD text
flag? Or both the LCD and GenA8?

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

[chromium-reviews, 2014-11-06 17:26:52]

Blink should just set LCD when they want it.
The surfaceprops will disable during raster as requested, and will take
care of getting the "right thing" on GDI (i.e. it will set the GenA8 flag).


On Thu, Nov 6, 2014 at 7:08 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Nov 5, 2014 at 6:54 PM, <reed@google.com> wrote:
>
>> We already set the GenA8 flag when we decided to "disable" and LCD
>>> request, so
>>>
>> I
>>
>>> think that will come for free.
>>>
>>
>> by "We" I mean the skia code that disables LCD during raster...
>>
>
> To be sure I understand: That means the blink recording code doesn't need
> to set any flags on the recording? Or it needs to always set the LCD text
> flag? Or both the LCD and GenA8?
>

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

[danakj, 2014-11-06 17:30:45]

On Thu, Nov 6, 2014 at 12:26 PM, Mike Reed <reed@google.com> wrote:

> Blink should just set LCD when they want it.
> The surfaceprops will disable during raster as requested, and will take
> care of getting the "right thing" on GDI (i.e. it will set the GenA8 flag).
>

Cool, thanks.


>
>
> On Thu, Nov 6, 2014 at 7:08 AM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Wed, Nov 5, 2014 at 6:54 PM, <reed@google.com> wrote:
>>
>>> We already set the GenA8 flag when we decided to "disable" and LCD
>>>> request, so
>>>>
>>> I
>>>
>>>> think that will come for free.
>>>>
>>>
>>> by "We" I mean the skia code that disables LCD during raster...
>>>
>>
>> To be sure I understand: That means the blink recording code doesn't need
>> to set any flags on the recording? Or it needs to always set the LCD text
>> flag? Or both the LCD and GenA8?
>>
>
>

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 18:51:05]

https://codereview.chromium.org/684653004/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (left):

https://codereview.chromium.org/684653004/diff/1/cc/layers/content_layer.cc#o...
cc/layers/content_layer.cc:135: client_->DidChangeLayerCanUseLCDText();
I don't think this can go.  The reason this was implemented in
WebContentLayerImpl and not inside of this function is because Blink wanted
invalidations and ui::Layer did not.  ui::Layer never wants invalidations when
lcd changes.

[danakj, 2014-11-12 18:52:48]

https://codereview.chromium.org/684653004/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (left):

https://codereview.chromium.org/684653004/diff/1/cc/layers/content_layer.cc#o...
cc/layers/content_layer.cc:135: client_->DidChangeLayerCanUseLCDText();
On 2014/11/12 18:51:05, enne wrote:
> I don't think this can go.  The reason this was implemented in
> WebContentLayerImpl and not inside of this function is because Blink wanted
> invalidations and ui::Layer did not.  ui::Layer never wants invalidations when
> lcd changes.

Ah okay, makes sense now that I saw the ui::Layer differences. Thanks.