Scale the ALPHA_THRESHOLD filter's region based on the device scale factor.

Scale the ALPHA_THRESHOLD filter's region based on the device scale factor.

This cl passes the current DSF (device scale factor) into BuildImageFilter
so that it can be used by the AlphaThreshold filter to scale the region
based on the current pixel density.

This requires https://codereview.chromium.org/340613005/ to land first.

BUG=376532

[jamesr, 2014-06-05 20:06:26]

shouldn't this be using the scale into the surface being filtered, not the
device scale factor? the two could be very different

[danakj, 2014-06-05 21:56:23]

Yes. Please do this the way the zoom filter does, using the surface size.
On Jun 5, 2014 4:06 PM, <jamesr@chromium.org> wrote:

> shouldn't this be using the scale into the surface being filtered, not the
> device scale factor? the two could be very different
>
> https://codereview.chromium.org/317663005/
>

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

[chromium-reviews, 2014-06-05 21:57:11]

The surface size doesn't communicate any useful information about what DSF
the content was produced with - to invert the DSF you have to use the
actual device scale from the renderer.


On Thu, Jun 5, 2014 at 2:56 PM, Dana Jansens <danakj@chromium.org> wrote:

> Yes. Please do this the way the zoom filter does, using the surface size.
> On Jun 5, 2014 4:06 PM, <jamesr@chromium.org> wrote:
>
>> shouldn't this be using the scale into the surface being filtered, not the
>> device scale factor? the two could be very different
>>
>> https://codereview.chromium.org/317663005/
>>
>

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

[garykac, 2014-06-05 23:04:39]

On 2014/06/05 21:57:11, chromium-reviews_chromium.org wrote:
> The surface size doesn't communicate any useful information about what DSF
> the content was produced with - to invert the DSF you have to use the
> actual device scale from the renderer.

I don't see a scaling factor in GLRenderer, DirectRenderer or Renderer.

But DirectRenderer::DrawFrame is passed a device_scale_factor that is then used
to reshape the output_surface_. So the DSF from the OutputSurface is the same
DSF passed into DrawFrame.

Is there a better place to get the correct scaling factor?  I'm not very
familiar with this code, so any pointers would be appreciated.

[danakj, 2014-06-06 15:31:47]

On Fri, Jun 6, 2014 at 1:04 AM, <garykac@chromium.org> wrote:

> On 2014/06/05 21:57:11, chromium-reviews_chromium.org wrote:
>
>> The surface size doesn't communicate any useful information about what DSF
>> the content was produced with - to invert the DSF you have to use the
>> actual device scale from the renderer.
>
>
Hm... I guess this filter's content is coming from the renderer, even
though it's being set in the browser via ui::Layer? I spose that makes it
pretty different.


> I don't see a scaling factor in GLRenderer, DirectRenderer or Renderer.
>
> But DirectRenderer::DrawFrame is passed a device_scale_factor that is then
> used
> to reshape the output_surface_. So the DSF from the OutputSurface is the
> same
> DSF passed into DrawFrame.
>
> Is there a better place to get the correct scaling factor?  I'm not very
> familiar with this code, so any pointers would be appreciated.
>

Sounds like you want to scale it by the DSF from the last swapped frame
from the renderer. Using the browser's DSF won't be the right answer
because it doesn't always match what the renderer has sent, so you want to
do this in ui::Layer or earlier, where you could know the renderer's DSF
and apply it to the FilterOperation.


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

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

[garykac, 2014-06-06 22:50:37]

PTAL. I still need to add unittests, but wanted to run this past you to verify
the approach.

[Wez, 2014-06-08 00:55:54]

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:939: // a path, scale the path and then convert back to a
SkRegion.
Should we add an SkRegion scaler? Or pass SkPaths down to here so we avoid too
much conversion back-and-forth?

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:948: alpha_shape_->setPath(path, clip);
This won't work for |raw_alpha_shape_|s that are non-contiguous (e.g. if the
region consists of two non-adjacent rects) - IIRC setPath doesn't currently cope
with that case.

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:377: void RecomputeAlphaShape();
Add an explanatory comment?

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:444: scoped_ptr<SkRegion> raw_alpha_shape_;
nit: Would |dips_alpha_shape_| and |device_alpha_shape_| here be clearer;
otherwise you have end up having to explain what the "raw" shape is, as in your
comment. :)

[garykac, 2014-06-09 21:30:00]

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:939: // a path, scale the path and then convert back to a
SkRegion.
On 2014/06/08 00:55:53, Wez wrote:
> Should we add an SkRegion scaler?

My first approach was to add a SkRegion scaler. See crrev.com/315083004 for the
discussion of why Skia didn't want that.

> Or pass SkPaths down to here so we avoid too
> much conversion back-and-forth?

We only convert when the Shape is set or the DSF changes, so we don't have lots
of back-and-forth conversions.

Note that if we expand the SetShape API to accept paths, then we'll need to
re-think this.

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:948: alpha_shape_->setPath(path, clip);
On 2014/06/08 00:55:54, Wez wrote:
> This won't work for |raw_alpha_shape_|s that are non-contiguous (e.g. if the
> region consists of two non-adjacent rects) - IIRC setPath doesn't currently
cope
> with that case.

It worked when I tried it (with 3 non-overlapping rects).

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:377: void RecomputeAlphaShape();
On 2014/06/08 00:55:54, Wez wrote:
> Add an explanatory comment?

Done.

https://codereview.chromium.org/317663005/diff/80001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:444: scoped_ptr<SkRegion> raw_alpha_shape_;
On 2014/06/08 00:55:54, Wez wrote:
> nit: Would |dips_alpha_shape_| and |device_alpha_shape_| here be clearer;
> otherwise you have end up having to explain what the "raw" shape is, as in
your
> comment. :)

I updated raw_ to dips_ and updated the comment.

[danakj, 2014-06-09 22:21:40]

https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:939: // a path, scale the path and then convert back to a
SkRegion.
:/ Storing 2 regions now, regions are costly, so I'm not excited about that.
Would it work to just add a scale to the FilterOperation, and make use of that
scale when applying the region to the alpha channel?

[garykac, 2014-06-16 23:31:28]

Sorry for the delay responding - I was blocked on testing this for part of last
week.

On 2014/06/09 22:21:40, danakj wrote:
> https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc#...
> ui/compositor/layer.cc:939: // a path, scale the path and then convert back to
a
> SkRegion.
> :/ Storing 2 regions now, regions are costly, so I'm not excited about that.
> Would it work to just add a scale to the FilterOperation, and make use of that
> scale when applying the region to the alpha channel?

I've changed it back so that we only have one SkRegion and I pass the scale
factor into the filter.

BTW, is there a better name than |device_scale_factor| that I could use here? 
There are apparently 2 numbers floating in the codebase around with the same
name that are not quite the same thing:
 * The (wrong) device_scale_factor_ that I tried to get from OutputSurface
 * The device_scale_factor_ in the Layer
Both of them seemed to work for my purpose, and I don't know under what
scenarios they would be different.

It would be nice to call out that difference in the name, e.g., something like
|surface_scale_factor|, |layer_scale_factor|, ...  I don't know the subtleties
of the difference, so any naming suggestions would be appreciated.

[Wez, 2014-06-17 01:57:48]

https://codereview.chromium.org/317663005/diff/130008/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/317663005/diff/130008/ui/compositor/layer.h#n...
ui/compositor/layer.h:456: // in device independent units, so it must be scaled
by the
nit: device independent units -> DIPs?

[danakj, 2014-06-17 21:35:28]

The layer's DSF is the device scale which the layer will be drawing with. The
OutputSurface DSF.. I didn't even remember that it's there, and it might not be
used for ChromeOS, I'm not sure (that class owns a fair bit of technical debt
that we are trying to eventually clean up). Anyhow, it would be set later and if
things changed in the frame, making use of it would be wrong until the next
frame was submitted. There's basically a big pipeline thru the compositor and
the OutputSurface is at the very end. Conceptually the DSF can be in many places
but they may have a different view of it at different times.

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
cc/output/filter_operation.cc:110: amount_(inner_threshold),
how about putting the scale factor into amount_ and move inner_threshold to
another variable instead, as this makes it match with outer_threshold.

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
cc/output/filter_operation.h:157: float device_scale_factor,
The fact that you're setting this to the DSF doesn't seem relevant to the
FilterOperation, it's just a scale that will be applied to the alpha filter.
Let's just call it a general "scale factor" in this code.

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
cc/output/render_surface_filters.cc:280:
skia::AdoptRef(SkAlphaThresholdFilter::Create(
Can we just add the scale to this SkAlphaThresholdFilter? That would be a lot
cheaper, to just multiply values inside the filter instead of doing this
Region->Path->Region thing.

[danakj, 2014-06-17 21:42:02]

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
cc/output/render_surface_filters.cc:281: scaled_shape, op.amount(),
op.outer_threshold()));
If you put a transform on the ui::Layer with a scale in it, will you get the
right result just using the DSF? Can you test that case?

[garykac, 2014-07-10 23:02:24]

https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/317663005/diff/100001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:939: // a path, scale the path and then convert back to a
SkRegion.
On 2014/06/09 22:21:39, danakj wrote:
> :/ Storing 2 regions now, regions are costly, so I'm not excited about that.
> Would it work to just add a scale to the FilterOperation, and make use of that
> scale when applying the region to the alpha channel?

Acknowledged.

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
cc/output/filter_operation.cc:110: amount_(inner_threshold),
On 2014/06/17 21:35:28, danakj wrote:
> how about putting the scale factor into amount_ and move inner_threshold to
> another variable instead, as this makes it match with outer_threshold. 

No longer necessary.

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/filter_operat...
cc/output/filter_operation.h:157: float device_scale_factor,
On 2014/06/17 21:35:28, danakj wrote:
> The fact that you're setting this to the DSF doesn't seem relevant to the
> FilterOperation, it's just a scale that will be applied to the alpha filter.
> Let's just call it a general "scale factor" in this code.

Acknowledged.

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
cc/output/render_surface_filters.cc:280:
skia::AdoptRef(SkAlphaThresholdFilter::Create(
On 2014/06/17 21:35:28, danakj wrote:
> Can we just add the scale to this SkAlphaThresholdFilter? That would be a lot
> cheaper, to just multiply values inside the filter instead of doing this
> Region->Path->Region thing.

Done.

https://codereview.chromium.org/317663005/diff/130008/cc/output/render_surfac...
cc/output/render_surface_filters.cc:281: scaled_shape, op.amount(),
op.outer_threshold()));
On 2014/06/17 21:42:01, danakj wrote:
> If you put a transform on the ui::Layer with a scale in it, will you get the
> right result just using the DSF? Can you test that case?

I tested this and we need to pass the layer scale as well as use the DSF. Code
has been updated to handle this.

https://codereview.chromium.org/317663005/diff/130008/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/317663005/diff/130008/ui/compositor/layer.h#n...
ui/compositor/layer.h:456: // in device independent units, so it must be scaled
by the
On 2014/06/17 01:57:48, Wez wrote:
> nit: device independent units -> DIPs?

Done.

[danakj, 2014-07-11 16:08:22]

Thanks! This looks good. Can you add a test to
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
that demonstrates this scale is being used correctly?

https://codereview.chromium.org/317663005/diff/210001/cc/layers/render_surfac...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/317663005/diff/210001/cc/layers/render_surfac...
cc/layers/render_surface_impl.cc:245: owning_layer_scale);
let's call this "owning_layer_to_target_scale" here and everywhere else
(including comments, excluding render_surface_filters.cc and below)

https://codereview.chromium.org/317663005/diff/210001/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/317663005/diff/210001/cc/output/filter_operat...
cc/output/filter_operation.cc:252: const FilterOperation& copy_op = progress >
0.5 ? to_op : from_op;
is this needed?

https://codereview.chromium.org/317663005/diff/210001/cc/quads/render_pass_dr...
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/317663005/diff/210001/cc/quads/render_pass_dr...
cc/quads/render_pass_draw_quad.h:66: // Layer scale. This scale has already been
applied to the pixels. It is
"The scale from layer space of root layer of the render pass to the render pass
physical pixels."

https://codereview.chromium.org/317663005/diff/210001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/317663005/diff/210001/ui/compositor/layer.h#n...
ui/compositor/layer.h:456: // in DIPs, so it must be scaled by the layer's
|device_scale_factor|.
this comment seems out of date now

[danakj, 2014-07-11 20:10:09]

Stephen just pointed out to me https://codereview.chromium.org/191123002/ which
I had completely forgotten about.

Does this fix your issue for you without even needing to add the transform to
the filter operation?

[Stephen White, 2014-07-11 20:16:43]

If SkAlphaTresholdFilter applies its CTM to the region, it shouldn't be
necessary to modify SkAlphaThresholFilter. (And if it doesn't, it should -- all
SkImageFilter subclasses now scale their parameters at draw time by the CTM).

I have a patch on the Chrome side that plumbs through the DSF to the CTM:
https://codereview.chromium.org/191123002/

It's not quite finished (needs tests, and may not be 100% correct, but I think
it's a more general direction than plumbing through a special case just for
SkAlphaThresholdFilter.

Would you mind taking that on?

[garykac, 2014-07-11 20:17:56]

On 2014/07/11 20:10:09, danakj wrote:
> Stephen just pointed out to me https://codereview.chromium.org/191123002/
which
> I had completely forgotten about.
> 
> Does this fix your issue for you without even needing to add the transform to
> the filter operation?

I'll test it and find out.

[garykac, 2014-07-12 01:36:19]

On 2014/07/11 20:16:43, Stephen White wrote:
> If SkAlphaTresholdFilter applies its CTM to the region, it shouldn't be
> necessary to modify SkAlphaThresholFilter. (And if it doesn't, it should --
all
> SkImageFilter subclasses now scale their parameters at draw time by the CTM).
> 
> I have a patch on the Chrome side that plumbs through the DSF to the CTM:
> https://codereview.chromium.org/191123002/
> 
> It's not quite finished (needs tests, and may not be 100% correct, but I think
> it's a more general direction than plumbing through a special case just for
> SkAlphaThresholdFilter.
> 
> Would you mind taking that on?

That cl fixes the problem we were having. I'll work on pushing that through
instead of this AlphaThresholdFilter-specific code.

Thanks!

[garykac, 2014-07-25 18:18:59]

These issues have been addressed in crrev.com/394193003, which is replacing this
cl.

https://codereview.chromium.org/317663005/diff/210001/cc/layers/render_surfac...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/317663005/diff/210001/cc/layers/render_surfac...
cc/layers/render_surface_impl.cc:245: owning_layer_scale);
On 2014/07/11 16:08:22, danakj wrote:
> let's call this "owning_layer_to_target_scale" here and everywhere else
> (including comments, excluding render_surface_filters.cc and below)

Done.

https://codereview.chromium.org/317663005/diff/210001/cc/quads/render_pass_dr...
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/317663005/diff/210001/cc/quads/render_pass_dr...
cc/quads/render_pass_draw_quad.h:66: // Layer scale. This scale has already been
applied to the pixels. It is
On 2014/07/11 16:08:22, danakj wrote:
> "The scale from layer space of root layer of the render pass to the render
pass
> physical pixels."

Done.