Bug 98339 - [chromium] Spelling and grammar markers are pixelated in hidpi.
Summary: [chromium] Spelling and grammar markers are pixelated in hidpi.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Varun Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-03 21:40 PDT by Varun Jain
Modified: 2012-10-16 17:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (249.19 KB, patch)
2012-10-03 21:43 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (249.57 KB, patch)
2012-10-04 11:16 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (249.46 KB, patch)
2012-10-10 10:44 PDT, Varun Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Varun Jain 2012-10-03 21:40:25 PDT
[chromium] Spelling and grammar markers are pixelated in hidpi.
Comment 1 Varun Jain 2012-10-03 21:43:19 PDT
Created attachment 167029 [details]
Patch
Comment 2 Hironori Bono 2012-10-03 22:29:49 PDT
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?
Comment 3 Hironori Bono 2012-10-03 22:32:11 PDT
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?
Comment 4 Build Bot 2012-10-04 10:43:26 PDT
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
Comment 5 Varun Jain 2012-10-04 11:16:12 PDT
Created attachment 167138 [details]
Patch
Comment 6 Varun Jain 2012-10-04 11:16:22 PDT
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
Comment 7 Hironori Bono 2012-10-05 01:55:20 PDT
Greetings,

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

Regards,

Hironori Bono
Comment 8 Mike Reed 2012-10-05 11:01:14 PDT
I don't see anything wrong, though I have some comments/questions.
Comment 9 Mike Reed 2012-10-05 12:29:43 PDT
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?
Comment 10 Varun Jain 2012-10-10 10:44:55 PDT
Created attachment 168029 [details]
Patch
Comment 11 Varun Jain 2012-10-10 10:49:45 PDT
(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.
Comment 12 Mike Reed 2012-10-10 11:01:53 PDT
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.
Comment 13 Stephen White 2012-10-10 11:36:46 PDT
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.
Comment 14 Mike Reed 2012-10-10 11:46:21 PDT
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.
Comment 15 Stephen White 2012-10-10 11:49:33 PDT
(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
Comment 16 WebKit Review Bot 2012-10-10 12:15:21 PDT
Comment on attachment 168029 [details]
Patch

Clearing flags on attachment: 168029

Committed r130940: <http://trac.webkit.org/changeset/130940>
Comment 17 WebKit Review Bot 2012-10-10 12:15:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Julien Chaffraix 2012-10-10 13:03:06 PDT
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
Comment 19 Varun Jain 2012-10-10 13:50:37 PDT
(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 :(
Comment 20 Adam Barth 2012-10-10 14:28:16 PDT
Fixed another borkage with this patch in http://trac.webkit.org/changeset/130961
Comment 21 Nico Weber 2012-10-16 10:42:28 PDT
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.
Comment 22 Varun Jain 2012-10-16 11:29:58 PDT
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).
Comment 23 Nico Weber 2012-10-16 17:55:06 PDT
> 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?