Remove GraphicsContext::applyDeviceScaleFactor()

Remove GraphicsContext::applyDeviceScaleFactor()

Unnecessary GC::scale() alias.

R=schenney@chromium.org,senorblanco@chromium.org
BUG=424655
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185388

[f(malita), 2014-11-14 03:24:57]

https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
File Source/web/PageWidgetDelegate.cpp (left):

https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
Source/web/PageWidgetDelegate.cpp:71:
gc.setDeviceScaleFactor(page.deviceScaleFactor());
This looks weird: the device scale factor is supposed to convey a supplemental
scale not reflected in the CTM; scaling AND setting a DSF doesn't smell right.

Just a thought, probably better to not stir it up :)

[Stephen Chennney, 2014-11-14 15:26:22]

I'm curious what will happen if you set the device scale in the popup menu code,
as I think it should be.

https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.h:181: // Speicy the device scale
factor which may change the way document markers
Nit: Could you fix this the typo. Speicy -> Specify.

https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
File Source/web/PageWidgetDelegate.cpp (left):

https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
Source/web/PageWidgetDelegate.cpp:71:
gc.setDeviceScaleFactor(page.deviceScaleFactor());
On 2014/11/14 03:24:56, Florin Malita wrote:
> This looks weird: the device scale factor is supposed to convey a supplemental
> scale not reflected in the CTM; scaling AND setting a DSF doesn't smell right.
> 
> Just a thought, probably better to not stir it up :)

It's does what it was designed to do, but that may not be right. The code needs
to both apply the factor, and tell GraphicsContext what that factor was.
GraphicsContext then uses the deviceScaleFactior in two other drawing ops.

The interesting question is: why use that scale factor in other ops if it is
already baked into the transform.

I think the GraphicsContext::drawPicture usage is just plain wrong (because you
still can't determine if it will be rasterized at 1-1 size, and even if you
ignore that, you should be dividing by the scale, not multiplying).

The marker drawing code that uses it is correct in that it makes use of the
device scale to generate higher res markers, then it unapplies the
deviceScaleFactor before drawing. Effectively it is trying to draw something the
same visual size but with higher resolution if the device scale factor is
higher. This code will break if we ever have high device scale factors.

https://codereview.chromium.org/725043002/diff/1/Source/web/WebPopupMenuImpl.cpp
File Source/web/WebPopupMenuImpl.cpp (right):

https://codereview.chromium.org/725043002/diff/1/Source/web/WebPopupMenuImpl....
Source/web/WebPopupMenuImpl.cpp:232: context.scale(scaleFactor, scaleFactor);
This really needs to set it also, to be sure we also paint document markers
right in popups.

[Stephen White, 2014-11-14 16:04:40]

On 2014/11/14 15:26:22, Stephen Chenney wrote:
> I'm curious what will happen if you set the device scale in the popup menu
code,
> as I think it should be.
> 
>
https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
> File Source/platform/graphics/GraphicsContext.h (right):
> 
>
https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
> Source/platform/graphics/GraphicsContext.h:181: // Speicy the device scale
> factor which may change the way document markers
> Nit: Could you fix this the typo. Speicy -> Specify.
> 
>
https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
> File Source/web/PageWidgetDelegate.cpp (left):
> 
>
https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
> Source/web/PageWidgetDelegate.cpp:71:
> gc.setDeviceScaleFactor(page.deviceScaleFactor());
> On 2014/11/14 03:24:56, Florin Malita wrote:
> > This looks weird: the device scale factor is supposed to convey a
supplemental
> > scale not reflected in the CTM; scaling AND setting a DSF doesn't smell
right.
> > 
> > Just a thought, probably better to not stir it up :)
> 
> It's does what it was designed to do, but that may not be right. The code
needs
> to both apply the factor, and tell GraphicsContext what that factor was.
> GraphicsContext then uses the deviceScaleFactior in two other drawing ops.
> 
> The interesting question is: why use that scale factor in other ops if it is
> already baked into the transform.

This is a very good question. As a reference, the filters code in Blink now 
has no special-case code for HiDPI, and Skia just uses the scale in the
transform 
at draw time. It would be nice to get everything to that model, if possible.

> I think the GraphicsContext::drawPicture usage is just plain wrong (because
you
> still can't determine if it will be rasterized at 1-1 size, and even if you
> ignore that, you should be dividing by the scale, not multiplying).
> 
> The marker drawing code that uses it is correct in that it makes use of the
> device scale to generate higher res markers, then it unapplies the
> deviceScaleFactor before drawing. Effectively it is trying to draw something
the
> same visual size but with higher resolution if the device scale factor is
> higher. This code will break if we ever have high device scale factors.

I think it also may break for the dual-monitor use case where one is HiDPI and 
one not. I'm pretty sure we'll have to divorce Blink of all high-DPI knowledge 
to get that case right, but Dana knows this case better than me.

>
https://codereview.chromium.org/725043002/diff/1/Source/web/WebPopupMenuImpl.cpp
> File Source/web/WebPopupMenuImpl.cpp (right):
> 
>
https://codereview.chromium.org/725043002/diff/1/Source/web/WebPopupMenuImpl....
> Source/web/WebPopupMenuImpl.cpp:232: context.scale(scaleFactor, scaleFactor);
> This really needs to set it also, to be sure we also paint document markers
> right in popups.

[danakj, 2014-11-14 16:09:36]

On 2014/11/14 16:04:40, Stephen White wrote:
> On 2014/11/14 15:26:22, Stephen Chenney wrote:
> > I'm curious what will happen if you set the device scale in the popup menu
> code,
> > as I think it should be.
> > 
> >
>
https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
> > File Source/platform/graphics/GraphicsContext.h (right):
> > 
> >
>
https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
> > Source/platform/graphics/GraphicsContext.h:181: // Speicy the device scale
> > factor which may change the way document markers
> > Nit: Could you fix this the typo. Speicy -> Specify.
> > 
> >
>
https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
> > File Source/web/PageWidgetDelegate.cpp (left):
> > 
> >
>
https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
> > Source/web/PageWidgetDelegate.cpp:71:
> > gc.setDeviceScaleFactor(page.deviceScaleFactor());
> > On 2014/11/14 03:24:56, Florin Malita wrote:
> > > This looks weird: the device scale factor is supposed to convey a
> supplemental
> > > scale not reflected in the CTM; scaling AND setting a DSF doesn't smell
> right.
> > > 
> > > Just a thought, probably better to not stir it up :)
> > 
> > It's does what it was designed to do, but that may not be right. The code
> needs
> > to both apply the factor, and tell GraphicsContext what that factor was.
> > GraphicsContext then uses the deviceScaleFactior in two other drawing ops.
> > 
> > The interesting question is: why use that scale factor in other ops if it is
> > already baked into the transform.
> 
> This is a very good question. As a reference, the filters code in Blink now 
> has no special-case code for HiDPI, and Skia just uses the scale in the
> transform 
> at draw time. It would be nice to get everything to that model, if possible.
> 
> > I think the GraphicsContext::drawPicture usage is just plain wrong (because
> you
> > still can't determine if it will be rasterized at 1-1 size, and even if you
> > ignore that, you should be dividing by the scale, not multiplying).
> > 
> > The marker drawing code that uses it is correct in that it makes use of the
> > device scale to generate higher res markers, then it unapplies the
> > deviceScaleFactor before drawing. Effectively it is trying to draw something
> the
> > same visual size but with higher resolution if the device scale factor is
> > higher. This code will break if we ever have high device scale factors.
> 
> I think it also may break for the dual-monitor use case where one is HiDPI and

> one not. I'm pretty sure we'll have to divorce Blink of all high-DPI knowledge

> to get that case right, but Dana knows this case better than me.

I think you don't ever want to bake a device scale factor into a transform. CC
will already scale everything by the DSF, so if you scale also, it'll double
scale.

However, you might want to use the device scale factor to snap things onto
physical pixels (think 1.5 device scale) or other similar purposes, with
subpixel positioning. That's a valid use of the DSF, but that's not scaling
anything by it, just positioning things aware of it.

[f(malita), 2014-11-14 16:38:17]

On 2014/11/14 16:09:36, danakj wrote:
> On 2014/11/14 16:04:40, Stephen White wrote:
> > On 2014/11/14 15:26:22, Stephen Chenney wrote:
>
https://codereview.chromium.org/725043002/diff/1/Source/web/PageWidgetDelegat...
> > > Source/web/PageWidgetDelegate.cpp:71:
> > > gc.setDeviceScaleFactor(page.deviceScaleFactor());
> > > On 2014/11/14 03:24:56, Florin Malita wrote:
> > > > This looks weird: the device scale factor is supposed to convey a
> > supplemental
> > > > scale not reflected in the CTM; scaling AND setting a DSF doesn't smell
> > right.
> > > > 
> > > > Just a thought, probably better to not stir it up :)
> > > 
> > > It's does what it was designed to do, but that may not be right. The code
> > needs
> > > to both apply the factor, and tell GraphicsContext what that factor was.
> > > GraphicsContext then uses the deviceScaleFactior in two other drawing ops.
> > > 
> > > The interesting question is: why use that scale factor in other ops if it
is
> > > already baked into the transform.
> > 
> > This is a very good question. As a reference, the filters code in Blink now 
> > has no special-case code for HiDPI, and Skia just uses the scale in the
> > transform 
> > at draw time. It would be nice to get everything to that model, if possible.
> > 
> > > I think the GraphicsContext::drawPicture usage is just plain wrong
(because
> > you
> > > still can't determine if it will be rasterized at 1-1 size, and even if
you
> > > ignore that, you should be dividing by the scale, not multiplying).
> > > 
> > > The marker drawing code that uses it is correct in that it makes use of
the
> > > device scale to generate higher res markers, then it unapplies the
> > > deviceScaleFactor before drawing. Effectively it is trying to draw
something
> > the
> > > same visual size but with higher resolution if the device scale factor is
> > > higher. This code will break if we ever have high device scale factors.
> > 
> > I think it also may break for the dual-monitor use case where one is HiDPI
and
> 
> > one not. I'm pretty sure we'll have to divorce Blink of all high-DPI
knowledge
> 
> > to get that case right, but Dana knows this case better than me.
> 
> I think you don't ever want to bake a device scale factor into a transform. CC
> will already scale everything by the DSF, so if you scale also, it'll double
> scale.

That is my understanding also, and the reason I thought the code smells funny.


> However, you might want to use the device scale factor to snap things onto
> physical pixels (think 1.5 device scale) or other similar purposes, with
> subpixel positioning. That's a valid use of the DSF, but that's not scaling
> anything by it, just positioning things aware of it.

Sizing intermediate bitmaps to match the device resolution falls into this
category I think (drawLineForDocumentMarker's intent AFAICT), but we're working
pretty hard to get rid of record-time rasterization, so this should be
discouraged.


On 2014/11/14 15:26:22, Stephen Chenney wrote:
> I'm curious what will happen if you set the device scale in the popup menu
code,
> as I think it should be.

I would prefer to not make functional changes in this CL, since it's not clear
whether that usage pattern is correct (seems like we should set DSF and remove
the scale). I'll open an issue + follow-up.

[f(malita), 2014-11-14 16:41:57]

https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/725043002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.h:181: // Speicy the device scale
factor which may change the way document markers
On 2014/11/14 15:26:22, Stephen Chenney wrote:
> Nit: Could you fix this the typo. Speicy -> Specify.

Done.

[Stephen Chennney, 2014-11-14 17:03:18]

LGTM.

So if we shouldn't be scaling, and we are, why isn't everything visually twice
as big on a HDPI display? Something in our understanding and the actuality is
not matching.

I think the drawPicture code should just be changed in a follow up. Check with
junov that he didn't have a very specific reason for using it (i.e. he was
trying to optimize only).

Do you have any ideas on how else to draw the marker code, avoiding the bitmaps?
Lots of tiny rects? Points? Smaller bitmap + pattern? Gradient? We have freedom
in how we draw it ASAIK; there's no standard defining the appearance.

[Stephen Chennney, 2014-11-14 17:03:56]

On 2014/11/14 17:03:18, Stephen Chenney wrote:
> LGTM.
> 
> So if we shouldn't be scaling, and we are, why isn't everything visually twice
> as big on a HDPI display? Something in our understanding and the actuality is
> not matching.
> 
> I think the drawPicture code should just be changed in a follow up. Check with
> junov that he didn't have a very specific reason for using it (i.e. he was
> trying to optimize only).
> 
> Do you have any ideas on how else to draw the marker code, avoiding the
bitmaps?
> Lots of tiny rects? Points? Smaller bitmap + pattern? Gradient? We have
freedom
> in how we draw it ASAIK; there's no standard defining the appearance.

ASAIK -> AFAIK

[f(malita), 2014-11-14 18:07:17]

On 2014/11/14 17:03:18, Stephen Chenney wrote:
> So if we shouldn't be scaling, and we are, why isn't everything visually twice
> as big on a HDPI display? Something in our understanding and the actuality is
> not matching.

Hence my hesitation to touch it :)

I'm guessing it may be compensated in some way or the results are just wrong on
hidpi.


> I think the drawPicture code should just be changed in a follow up. Check with
> junov that he didn't have a very specific reason for using it (i.e. he was
> trying to optimize only).

I recall looking at it some time ago and concluding that it is one of the few
DSF users which is justified (the canvas2d spec required rasterization at a
particular resolution and it cannot behave like a vector graphics backend => we
need to figure out what that resolution is and how to scale it into device space
using an image filter), but the details are blurry.


> Do you have any ideas on how else to draw the marker code, avoiding the
bitmaps?
> Lots of tiny rects? Points? Smaller bitmap + pattern? Gradient? We have
freedom
> in how we draw it ASAIK; there's no standard defining the appearance.

Off the top of my head I'd say picture + picture shader, but how to convert the
content from a collection of colored pixels to a device independent shape is
unclear. To size the rects correctly we need full CTM/DSF knowledge so we're
back to square one, so we should probably try to convert to an actual shape
instead. I'm guessing all it draws is the squiggly line under misspelled words?

[f(malita), 2014-11-14 18:08:31]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-11-14 18:08:47]

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

[commit-bot: I haz the power, 2014-11-14 18:12:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-14 18:12:47]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[f(malita), 2014-11-14 18:18:55]

fmalita@chromium.org changed reviewers:
+ ojan@chromium.org

[f(malita), 2014-11-14 18:19:07]

On 2014/11/14 18:12:47, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

+ojan for web/

[ojan, 2014-11-14 19:32:26]

lgtm

[f(malita), 2014-11-14 19:33:41]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-11-14 19:34:03]

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

[commit-bot: I haz the power, 2014-11-14 19:40:03]

Committed patchset #2 (id:20001) as 185388