|
|
DescriptionColored Emoji not drawn in Chrome if font style is set as Bold.
When Bold font is asked, then Skia try to generate image/bitmap from
path (if Bold font is not present) which is not correct case for Colored Emoji
bitmap font.
BUG=397069
Patch Set 1 #Patch Set 2 : Fixing typo error #
Total comments: 4
Patch Set 3 : Comment fixes #
Messages
Total messages: 29 (2 generated)
Pls review.
Looks right to me, though I'd default to enabled and only disable when needed. Skia guys have to decide though.
@Behdad: Thank u for added Skia members. "fGenerateImageFromPath" is always true when we request for Bold style of some font which does not has Bold font present in system. Initially was thinking store some information in SkTypeface when Font is loaded and make "fGenerateImageFromPath" as false at creation, but as single Font can have both Bitmap and Scalable glyphs so turning ON/OFF "fGenerateImageFromPath" seems better approach.
Pls review
@reed/bungeman1: Pls review patch
On 2014/07/29 13:14:22, h.joshi wrote: > @reed/bungeman1: Pls review patch This patch cannot be used as it introduces a large number of bugs. Also, while Skia acts this way by default, defining SK_USE_FREETYPE_EMBOLDEN in SkUserConfig.h works around this. I believe that most of our users set this, but I would need to check again to make sure that Clankium and Android are still doing so. Skia does need some work on color and bitmap support in general, which will be done after fixing typeface selection.
@bungman1: SK_USE_FREETYPE_EMBOLDEN is already defined in "src/skia/config/SkUserConfig.h" file. Can you pls suggest what kind of bugs this patch will introduce. This will help to make patch better.
On 2014/08/05 05:22:55, h.joshi wrote: > @bungman1: SK_USE_FREETYPE_EMBOLDEN is already defined in > "src/skia/config/SkUserConfig.h" > file. Can you pls suggest what kind of bugs this patch will introduce. This will > help to > make patch better. The actual issue here is the call to getTextPath in Source/platform/fonts/skia/SimpleFontDataSkia.cpp . For fonts without a path, this will give back the empty path which will have empty bounds so Blink won't draw the glyph. This getting of the path is a workaround for something... vertical font metrics maybe? I don't remember what at the moment. I think this worked until recently since GDI would always give some path (from some fallback font, though this probably gives the wrong, but non-zero, bounds), DirectWrite always has a path, and CoreText always has a path. I think at some point the idea was that if there was no path to return a path the size of the bounds. Related: Issue 25045010, Issue 111043003 .
@bungeman1: Thank u for sharing this information, will debug more on the shared points. I have a doubt, why above shared bound comes empty when we have bold style associated with Colored Emoji. Will debug to get more information. Will also check if we can check for Bitmap font property in Webkit using any Skia exposed API.
On 2014/08/06 16:25:48, h.joshi wrote: > @bungeman1: Thank u for sharing this information, will debug > more on the shared points. I have a doubt, why above shared bound > comes empty when we have bold style associated with Colored Emoji. > Will debug to get more information. Will also check if we can check > for Bitmap font property in Webkit using any Skia exposed API. Hmmm... you're right... we probably have clipping issues with fake bold bitmaps. Also, I'm not sure why this issue doesn't seem to affect non fake bold. Locally, I've set up the coloremoji GM. With SK_USE_FREETYPE_EMBOLDEN defined fake bold on a color bitmap font simply doesn't bold anything (as expected, we just get the regular bitmap) and without SK_USE_FREETYPE_EMBOLDEN we don't draw anything. Now I'm not so sure what's getting in the way.
@bungeman1: Going through code, to fix this issue we can either revert back to "eae" patch by adding following : if (bounds.isEmpty()) paint.measureText(&glyph, 2, &bounds); To getSkiaBoundsForGlyph in SimpleFontDataSkia.cpp I am having doubt that Skia is doing correct when Bitmap font is encountered, as with bitmap font we cannot associate path, so path should not be checked with bitmap font. Do we have any other thing which I should check related to this bug in Skia Pls Suggest.
Per Ben's August 4th comment on https://code.google.com/p/chromium/issues/detail?id=397069, this bug should no longer occur with a tip-of-tree Chrome on Android?
My code base is little bit old and issue is reproducible in that, will update code tomorrow and check. But I have one doubt, if SK_USE_FREETYPE_EMBOLDEN is undefined then Skia is trying to find outline for glyph in Bitmap font glyphs which is not correct.
Getting some build error in sandbox linux, trying to find fix for the same. Once able to generate Chrome APK, will test and update result here.
Some issue in my repo due to which issue was faced. I checked latest code and issue is still reproducible, I also checked FreeType 2.5.3 code and in that also in case of Emoji we return from FT_Bitmap_Embolden, code is following : case FT_PIXEL_MODE_BGRA: /* We don't embolden color glyphs. */ return FT_Err_Ok; I feel above patch is also doing the same, we should not check Outline case in case of Emoji and do normal path following. Pls Suggest
Check in todays build as well and issue is reproducible. Using SK_USE_FREETYPE_EMBOLDEN will call FreeType Embolden code for all cases. and Skia embolden code will not be used, then we can remove embolden code from Skia. Pls suggest.
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/411313002/diff/20001/src/core/SkScalerContext.h File src/core/SkScalerContext.h (right): https://codereview.chromium.org/411313002/diff/20001/src/core/SkScalerContext... src/core/SkScalerContext.h:272: void updateImageFromPath(bool value) { fGenerateImageFromPath = value; } can we combine these two, and only have one method for changing this attribute? e.g. setGenerateImageFromPath(bool) ? https://codereview.chromium.org/411313002/diff/20001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/411313002/diff/20001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1025: updateImageFromPath(true); nit: prefix method calls with this->
@reed: Thank you for reviewing and sharing comments. Suggested changes are done, Please review. https://codereview.chromium.org/411313002/diff/20001/src/core/SkScalerContext.h File src/core/SkScalerContext.h (right): https://codereview.chromium.org/411313002/diff/20001/src/core/SkScalerContext... src/core/SkScalerContext.h:272: void updateImageFromPath(bool value) { fGenerateImageFromPath = value; } On 2014/09/05 13:29:42, reed1 wrote: > can we combine these two, and only have one method for changing this attribute? > > e.g. setGenerateImageFromPath(bool) ? Done. https://codereview.chromium.org/411313002/diff/20001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/411313002/diff/20001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1025: updateImageFromPath(true); On 2014/09/05 13:29:42, reed1 wrote: > nit: prefix method calls with this-> Done.
@reed1: Made suggested changes Pls review
@reed1: pls review this patch.
ben is the skia-font expert, so deferring to him
@ben: Please review this patch.
@ben: Pls review this patch, it's more than a month old now.
@ben: Pls review this patch, its been pending for 2 months now.
Pls suggest for this patch, its pending for more than 2 month now.
Issue is still reproducible in latest Chrome 41 developer build. Pls review this patch, if some changes are required will do.
h.joshi@samsung.com changed reviewers: + jshin@chromium.org
Issue is still reproducible and can also be seen in Google search also (google.co.kr) when rendered in complete CJK. Issue is also seen in other Korean and Chinese sites as well, as most of the sites use "bold" for search box. Pls review and share comment. In case we do not want to fix this issue now, pls suggest will close this patch. |