Bug 98339 – [chromium] Spelling and grammar markers are pixelated in hidpi.

• Component: New Bugs
• Status: RESOLVED FIXED

[Varun Jain]

[chromium] Spelling and grammar markers are pixelated in hidpi.

[Varun Jain]

Created attachment 167029 [details]
Patch

[Hironori Bono]

Comment on attachment 167029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167029&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:122
> +        SkPreMultiplyARGB(0xFF, 0xFF, 0x00, 0x00), // Opaque red.

Even though I'm not totally sure, this code may add a static initializer. Is it possible to use a pre-calculated constant "0xFF << SK_A32_SHIFT | 0xFF << SK_R32_SHIFT" instead of calling SkPreMultiplyARGB for all SkPreMultiplyARGB calls in this function?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:718
> +        if (deviceScaleFactor == 2)

nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:751
> +    if (deviceScaleFactor == 2) {

nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:753
> +        platformContext()->canvas()->scale(1 / deviceScaleFactor, 1 / deviceScaleFactor);

nit: '1 / deviceScaleFactor' -> Sk_ScalarHalf for the case when SkScalar is SkFixed?

[Hironori Bono]

Greetings,

For what it's worth, I'm not a WebKit reviewer and it is OK to ignore my comments. (These comments are just out of curiosity.)

Regards,

Hironori Bono

(In reply to comment #2)
> (From update of attachment 167029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167029&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:122
> > +        SkPreMultiplyARGB(0xFF, 0xFF, 0x00, 0x00), // Opaque red.
> 
> Even though I'm not totally sure, this code may add a static initializer. Is it possible to use a pre-calculated constant "0xFF << SK_A32_SHIFT | 0xFF << SK_R32_SHIFT" instead of calling SkPreMultiplyARGB for all SkPreMultiplyARGB calls in this function?
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:718
> > +        if (deviceScaleFactor == 2)
> 
> nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:751
> > +    if (deviceScaleFactor == 2) {
> 
> nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:753
> > +        platformContext()->canvas()->scale(1 / deviceScaleFactor, 1 / deviceScaleFactor);
> 
> nit: '1 / deviceScaleFactor' -> Sk_ScalarHalf for the case when SkScalar is SkFixed?

[Build Bot]

Comment on attachment 167029 [details]
Patch

Attachment 167029 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14172287

New failing tests:
http/tests/workers/terminate-during-sync-operation.html

[Varun Jain]

Created attachment 167138 [details]
Patch

[Varun Jain]

Comment on attachment 167029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167029&action=review

>>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:122
>>> +        SkPreMultiplyARGB(0xFF, 0xFF, 0x00, 0x00), // Opaque red.
>> 
>> Even though I'm not totally sure, this code may add a static initializer. Is it possible to use a pre-calculated constant "0xFF << SK_A32_SHIFT | 0xFF << SK_R32_SHIFT" instead of calling SkPreMultiplyARGB for all SkPreMultiplyARGB calls in this function?
> 
> 

Done

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:718
>> +        if (deviceScaleFactor == 2)
> 
> nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?

Done

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:751
>> +    if (deviceScaleFactor == 2) {
> 
> nit: 2 -> SkIntToScalar(2) for the case when SkScalar is SkFixed?

Done

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:753
>> +        platformContext()->canvas()->scale(1 / deviceScaleFactor, 1 / deviceScaleFactor);
> 
> nit: '1 / deviceScaleFactor' -> Sk_ScalarHalf for the case when SkScalar is SkFixed?

Done

[Hironori Bono]

Greetings,

Even though your second patch looks good to me, I recommend to ask abarth or jamesr for their approvals.

Regards,

Hironori Bono

[Mike Reed]

I don't see anything wrong, though I have some comments/questions.

[Mike Reed]

Comment on attachment 167138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167138&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:119
> +void draw2xMarker(SkBitmap* bitmap, int index)

Wow, that's a big function to make a bitmap. Do we not have a way to store this as a PNG somewhere, so the specific colors don't need to be baked into a webkit-file?

If we have to do it this way, perhaps this could be done more compactly (not sure)...

1. might write a helper macro to build-up a PMColor, rather than use the SHIFT values over and over. e.g. MakePM(a, r, g, b) ...
2. would it work to prebuild each line-of-8, and just memcpy them into the bitmap?

e.g.

static const SkPMColor line0[] = { lineColor, antiColor1, antiColor2, antiColor2, lineColor, ... };
static const SkPMColor line1[] = { ... };

int remaining = bitmap->width();
for (x = 0; x < bitmap->width(); x += 8) {
    int count = min(remaining, 8);
    memcpy(addr, line, count * sizeof(SkPMColor);
    remaining -= 8;
    ...
}

Just a thought, not critical (unless it made the code a lot more readable).

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:679
> +    matrix.reset();

You can collaps the reset() and postTranslate...

SkMatrix matrix;
matrix.setTranslate(originX, originY);
shader->setLocalMatrix(matrix);

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:687
> +    paint.setShader(shader);

You can combine the setShader and unref...

paint.setShader(shader)->unref();

All of the setters in skia that take a ref-counted object return their argument, exactly to make this sort of pattern easy.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:721
> +        if (deviceScaleFactor == skScalarTwo)

is deviceScalerFactory always either 1 or 2, guaranteed? If so, can we store it as an int instead of a float? I ask because this sort of == test between two floats has the appearance of being numerically fragile... what if scale is 1.99 or 2.01?

[Varun Jain]

Created attachment 168029 [details]
Patch

[Varun Jain]

(In reply to comment #9)
> (From update of attachment 167138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167138&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:119
> > +void draw2xMarker(SkBitmap* bitmap, int index)
> 
> Wow, that's a big function to make a bitmap. Do we not have a way to store this as a PNG somewhere, so the specific colors don't need to be baked into a webkit-file?
> 
> If we have to do it this way, perhaps this could be done more compactly (not sure)...
> 
> 1. might write a helper macro to build-up a PMColor, rather than use the SHIFT values over and over. e.g. MakePM(a, r, g, b) ...


There is already SkPreMultipleARGB(). However, hbono mentioned that it might introduce a static initializer. I am not sure it does though. I have changed it back to use the SkPremultipleARGB. Let me know if its ok.

> 2. would it work to prebuild each line-of-8, and just memcpy them into the bitmap?
> 
> e.g.
> 
> static const SkPMColor line0[] = { lineColor, antiColor1, antiColor2, antiColor2, lineColor, ... };
> static const SkPMColor line1[] = { ... };
> 
> int remaining = bitmap->width();
> for (x = 0; x < bitmap->width(); x += 8) {
>     int count = min(remaining, 8);
>     memcpy(addr, line, count * sizeof(SkPMColor);
>     remaining -= 8;
>     ...
> }
> 
> Just a thought, not critical (unless it made the code a lot more readable).


Done. It does look shorter. More readable? I am not sure :).. certainly not any less readable.


> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:679
> > +    matrix.reset();
> 
> You can collaps the reset() and postTranslate...

Done.

> 
> SkMatrix matrix;
> matrix.setTranslate(originX, originY);
> shader->setLocalMatrix(matrix);
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:687
> > +    paint.setShader(shader);
> 
> You can combine the setShader and unref...
> 
> paint.setShader(shader)->unref();
> 
> All of the setters in skia that take a ref-counted object return their argument, exactly to make this sort of pattern easy.


Done.

> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:721
> > +        if (deviceScaleFactor == skScalarTwo)
> 
> is deviceScalerFactory always either 1 or 2, guaranteed? If so, can we store it as an int instead of a float? I ask because this sort of == test between two floats has the appearance of being numerically fragile... what if scale is 1.99 or 2.01?

Done.

[Mike Reed]

Thank you. I do find that more readable/understandable.

If we do get a static initializer, try SkPremultiplyARGBInline (in SkColorPriv.h) or we can write something trivially that will work.

[Stephen White]

Comment on attachment 168029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:128
> +        SkPreMultiplyARGB(0xB0, 0xFF, 0x00, 0x00), // Semitransparent red
> +        SkPreMultiplyARGB(0xB0, 0xC0, 0xC0, 0xC0), // Semitransparent gray

To avoid static initializers, another option would be for you to premultply the colours yourself, and bake that into the hex.  E.g., 

static const SkPMColor antiColors1[2] = { 0xB0B00000, 0xB0848484 };  // if I've done my math right

It would be a lot more concise too.

[Mike Reed]

Can't bake in hex, as the byte-order for SkPMColor varies depending on the build.

If the function is not getting inlined, we can also call SkPackARGB32, which will handle the byte order for an already-premultiplied color.

[Stephen White]

(In reply to comment #13)
> (From update of attachment 168029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:128
> > +        SkPreMultiplyARGB(0xB0, 0xFF, 0x00, 0x00), // Semitransparent red
> > +        SkPreMultiplyARGB(0xB0, 0xC0, 0xC0, 0xC0), // Semitransparent gray
> 
> To avoid static initializers, another option would be for you to premultply the colours yourself, and bake that into the hex.  E.g., 
> 
> static const SkPMColor antiColors1[2] = { 0xB0B00000, 0xB0848484 };  // if I've done my math right
> 
> It would be a lot more concise too.


> Can't bake in hex, as the byte-order for SkPMColor varies depending on the build.
> 
> If the function is not getting inlined, we can also call SkPackARGB32, which will handle the byte order for an already-premultiplied color.

True.  Varun points out that these are actually function-level statics, so they should be ok anyway (modulo threading issues).  r=me

[WebKit Review Bot]

Comment on attachment 168029 [details]
Patch

Clearing flags on attachment: 168029

Committed r130940: <http://trac.webkit.org/changeset/130940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Comment on attachment 168029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:649
> +    SkMatrix matrix;
> +    SkMatrix matrix;

Defining the same variable twice is an error under clang, I don't know why the EWS were fine with that but the build bots were not. Fixed the build in http://trac.webkit.org/changeset/130944

[Varun Jain]

(In reply to comment #18)
> (From update of attachment 168029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:649
> > +    SkMatrix matrix;
> > +    SkMatrix matrix;
> 
> Defining the same variable twice is an error under clang, I don't know why the EWS were fine with that but the build bots were not. Fixed the build in http://trac.webkit.org/changeset/130944

thanks for the fix! dont know how i missed that :(

[Adam Barth]

Fixed another borkage with this patch in http://trac.webkit.org/changeset/130961

[Nico Weber]

Comment on attachment 168029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review

> Source/WebKit/chromium/src/PageWidgetDelegate.cpp:93
> +    gc.platformContext()->setDeviceScaleFactor(page->deviceScaleFactor());

Should this be done in GraphicsContext::platformApplyDeviceScaleFactor() instead? That's where the mac port does it.

[Varun Jain]

Comment on attachment 168029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168029&action=review

>> Source/WebKit/chromium/src/PageWidgetDelegate.cpp:93
>> +    gc.platformContext()->setDeviceScaleFactor(page->deviceScaleFactor());
> 
> Should this be done in GraphicsContext::platformApplyDeviceScaleFactor() instead? That's where the mac port does it.

That was in fact my first attempt at doing this. However, gc.applyDeviceScaleFactor() is not called on every path. For accelerated composition mode, the device scale is applied directly to the canvas (in CanvasLayerTextureUpdater).

[Nico Weber]

> That was in fact my first attempt at doing this. However, gc.applyDeviceScaleFactor() is not called on every path. For accelerated composition mode, the device scale is applied directly to the canvas (in CanvasLayerTextureUpdater).

Are there good reasons for this? The mac port does the corresponding work in applyDeviceScaleFactor(), so I suppose they call it on all code paths. Can't we just do that too?