| Summary: | [chromium] Spelling and grammar markers are pixelated in hidpi. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Varun Jain <varunjain> | ||||||||
| Component: | New Bugs | Assignee: | Varun Jain <varunjain> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, hbono, jchaffraix, reed, senorblanco, thakis, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Varun Jain
2012-10-03 21:40:25 PDT
Created attachment 167029 [details]
Patch
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? 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 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 Created attachment 167138 [details]
Patch
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 Greetings, Even though your second patch looks good to me, I recommend to ask abarth or jamesr for their approvals. Regards, Hironori Bono I don't see anything wrong, though I have some comments/questions. 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? Created attachment 168029 [details]
Patch
(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. 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 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. 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. (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 on attachment 168029 [details] Patch Clearing flags on attachment: 168029 Committed r130940: <http://trac.webkit.org/changeset/130940> All reviewed patches have been landed. Closing bug. 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 (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 :( Fixed another borkage with this patch in http://trac.webkit.org/changeset/130961 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 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). > 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?
|