[Night Light] Backend ui::Layer work

[Night Light] Backend ui::Layer work

Add support to change the layer's color temperature by toning down the
blue and green colors scales.

BUG=705816
TEST=compositor_unittests --gtest_filter=LayerAnimatorTest.Temperature

Review-Url: https://codereview.chromium.org/2854653003
Cr-Commit-Position: refs/heads/master@{#468873}
Committed: https://chromium.googlesource.com/chromium/src/+/90fd23c70a8d62bf83332ea62449484cdedee5af

[afakhry, 2017-05-01 21:51:41]

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

[afakhry, 2017-05-01 21:51:42]

danakj, could you please take a look? Thank you!

[danakj, 2017-05-01 22:02:36]

If you do this with a filter, it will cause a lot more power usage when this
feature is on. This is very different from the high contrast/inverted color
stuff begin done by folks in blink which is changing how it is painted. Why the
divergence?

[afakhry, 2017-05-02 02:04:10]

On 2017/05/01 22:02:36, danakj wrote:
> If you do this with a filter, it will cause a lot more power usage when this
> feature is on. This is very different from the high contrast/inverted color
> stuff begin done by folks in blink which is changing how it is painted. Why
the
> divergence?

I'm not aware of how the folks in blink implemented it. I'm only aware of how
high contrast is implemented here in the ui::Layer which is also using a filter:
https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?q=%22filters.Appe...
I thought that was what you pointed me to when I asked you earlier, it's not?

I also spoke last week with reveman@ and he told me that this implementation is
correct, but we'd probably need to optimize
cc::GLRenderer::UpdateRPDQWithSkiaFilters():
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc to make it more
efficient.

Please point me to the right direction. Thanks!

[danakj, 2017-05-02 14:52:18]

On Mon, May 1, 2017 at 10:04 PM, <afakhry@chromium.org> wrote:

> On 2017/05/01 22:02:36, danakj wrote:
> > If you do this with a filter, it will cause a lot more power usage when
> this
> > feature is on. This is very different from the high contrast/inverted
> color
> > stuff begin done by folks in blink which is changing how it is painted.
> Why
> the
> > divergence?
>
> I'm not aware of how the folks in blink implemented it. I'm only aware of
> how
> high contrast is implemented here in the ui::Layer which is also using a
> filter:
> https://cs.chromium.org/chromium/src/ui/compositor/
> layer.cc?q=%22filters.Append(cc::FilterOperation::
> CreateInvertFilter(1.0)%22+package:%5Echromium$&l=462
> I thought that was what you pointed me to when I asked you earlier, it's
> not?
>
> I also spoke last week with reveman@ and he told me that this
> implementation is
> correct, but we'd probably need to optimize
> cc::GLRenderer::UpdateRPDQWithSkiaFilters():
> https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc to make it
> more
> efficient.
>
> Please point me to the right direction. Thanks!
>

https://docs.google.com/document/d/1jetx4a0WpdqW8866SI1BMAOyDL3UL7tjQggNtHz7a...
is the public doc for high contrast mode. They are using a color filter
during painting (not during compositing). This means the filter affects how
we raster, and we have the same performance with or without it. I don't
know what optimization can be done to make this approach comparable. Using
a compositing filter requires the compositor to draw everything through an
intermediate buffer which doubles our memory bandwidth (which is highly
influential on power consumption).


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

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

[dmazzoni, 2017-05-02 21:07:29]

On 2017/05/02 14:52:18, danakj wrote:
> On Mon, May 1, 2017 at 10:04 PM, <mailto:afakhry@chromium.org> wrote:
> 
> > On 2017/05/01 22:02:36, danakj wrote:
> > > If you do this with a filter, it will cause a lot more power usage when
> > this
> > > feature is on. This is very different from the high contrast/inverted
> > color
> > > stuff begin done by folks in blink which is changing how it is painted.
> > Why
> > the
> > > divergence?
> >
> > I'm not aware of how the folks in blink implemented it. I'm only aware of
> > how
> > high contrast is implemented here in the ui::Layer which is also using a
> > filter:
> > https://cs.chromium.org/chromium/src/ui/compositor/
> > layer.cc?q=%22filters.Append(cc::FilterOperation::
> > CreateInvertFilter(1.0)%22+package:%5Echromium$&l=462
> > I thought that was what you pointed me to when I asked you earlier, it's
> > not?
> >
> > I also spoke last week with reveman@ and he told me that this
> > implementation is
> > correct, but we'd probably need to optimize
> > cc::GLRenderer::UpdateRPDQWithSkiaFilters():
> > https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc to make it
> > more
> > efficient.
> >
> > Please point me to the right direction. Thanks!
> >
> 
>
https://docs.google.com/document/d/1jetx4a0WpdqW8866SI1BMAOyDL3UL7tjQggNtHz7a...
> is the public doc for high contrast mode. They are using a color filter
> during painting (not during compositing). This means the filter affects how
> we raster, and we have the same performance with or without it. I don't
> know what optimization can be done to make this approach comparable. Using
> a compositing filter requires the compositor to draw everything through an
> intermediate buffer which doubles our memory bandwidth (which is highly
> influential on power consumption).

Hi - I just chatted with Ahmed.

So unfortunately the "new" high contrast mode is still in progress. I'm not
working on it full-time and I've still got a ways to go before it's ready.
Even if it was ready today, though, it only affects web contents. It's not clear
where we'd add code to apply a color filter to the "rest" of the screen.

Note that the Chrome OS full-screen high contrast mode (inverted colors only)
does apply a filter to the whole screen now. So we're already paying that cost
for the users who have that mode on. (According to UMA it's about 50k
Chromebook users daily.)

I wonder if the other approach we considered for high-contrast mode might
make sense here - Idea 1 from the doc, where we wrap SkCanvas in
cc/raster. Would it possible that would allow us to apply a filter to the
whole Chrome OS screen?

That may be a compromise, in that it'd be a lot more efficient for most
drawing primitives - but images and gradients would still need to be
filtered.

Note that there are some other places where we may want to apply a filter
to the whole screen, such as colorblindness filters.

It may make sense to try to find one unified approach for all of those
whole-screen filters. High contrast mode v2 is fundamentally different
because it proposes not filtering everything, and the best place to do
that is in Blink.

Let me know what you think about cc/raster for this whole-screen filter.

But otherwise I wonder if we should consider just landing this filter
anyway and then optimizing it based on usage and user demand.
I think for many users, having the feature at all may be worth the
added cost.

Finally, one last argument is that right now users are installing one of
several Chrome extensions that apply CSS filters to web pages to
achieve a similar effect.

I'm pretty sure the cost of applying one compositor filter to the whole
screen would be less than the cost of one of those extensions,
because you're not only going through the overhead of CSS filters,
but also paying the memory overhead of the extension process.

So I guess I'm saying that if there's no straightforward more
optimal solution, filtering the whole screen might actually be an
improvement for a lot of users. Then if usage is high, that's good
motivation to come back and optimize it.

[danakj, 2017-05-02 21:25:38]

Thanks for having a look dmazzoni. You make some good points, as well the UI is
built from many different layer types some of which do not go through the
cc/raster code at all. As such if this has to apply to the UI too I'm not sure
what to suggest beyond this exactly either. LGTM

[dmazzoni, 2017-05-02 21:32:05]

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

[dmazzoni, 2017-05-02 21:32:06]

OK cool.

I think if usage is significant we should definitely
look into potential optimizations for all of these
whole-screen filters.

[afakhry, 2017-05-02 23:41:29]

Thank you very much dmazzoni and danakj! I appreciate your guidance on this.

As part of this feature implementation, I plan to add UMA stats to track the
usage and hopefully build a case to prioritize optimizing the color filters
implementation.

[afakhry, 2017-05-03 00:06:34]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:06:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 02:12:03]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493769994669140, "parent_rev":
"052227ed388adccb0d14828db147f18a31ded2f2", "commit_rev":
"90fd23c70a8d62bf83332ea62449484cdedee5af"}

[commit-bot: I haz the power, 2017-05-03 02:12:16]

Description was changed from

==========
[Night Light] Backend ui::Layer work

Add support to change the layer's color temperature by toning down the
blue and green colors scales.

BUG=705816
TEST=compositor_unittests --gtest_filter=LayerAnimatorTest.Temperature
==========

to

==========
[Night Light] Backend ui::Layer work

Add support to change the layer's color temperature by toning down the
blue and green colors scales.

BUG=705816
TEST=compositor_unittests --gtest_filter=LayerAnimatorTest.Temperature

Review-Url: https://codereview.chromium.org/2854653003
Cr-Commit-Position: refs/heads/master@{#468873}
Committed:
https://chromium.googlesource.com/chromium/src/+/90fd23c70a8d62bf83332ea62449...
==========

[commit-bot: I haz the power, 2017-05-03 02:12:17]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/90fd23c70a8d62bf83332ea62449...