Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984943002:
Implement support for rendering color emoji on Windows (Closed)
|
Created:
4 years, 7 months ago by Ilya Kulshin Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionImplement support for rendering color emoji on Windows
This change adds support to the DirectWrite scaler context
for detection of color fonts. If it detects a color font and
the glyph is a color glyph, it will use DirectWrite's
TranslateColorGlyphRun API and generate an image of the
glyph that can then be rendered.
Chromium tests: https://codereview.chromium.org/2003853002
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1984943002
Committed: https://skia.googlesource.com/skia/+/c4b091543b4082fd1a2f356ef3b478073c5f9418
Patch Set 1 : #
Total comments: 25
Patch Set 2 : Temp patchset for changetracking #Patch Set 3 : Switch to using paths to render color glyphs #Patch Set 4 : Add check to respect aliased rendering mode #
Total comments: 4
Patch Set 5 : Switched to using SkDraw to save an alloc #
Total comments: 10
Patch Set 6 : Add skia tests and other review feedback #
Total comments: 2
Patch Set 7 : Support font fallback in test app #
Total comments: 6
Patch Set 8 : Fix linebreaks and don't pass locale to matchFamilyStyleCharacter #
Total comments: 2
Patch Set 9 : rebase #
Messages
Total messages: 52 (20 generated)
Description was changed from ========== Color emoji impl BUG=skia: ========== to ========== Color emoji impl BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Color emoji impl BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kulshin@chromium.org changed reviewers: + bungeman@google.com
ptal
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:823: colorGlyph->runColor.r * 255, is runColor premultiplied?
https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:823: colorGlyph->runColor.r * 255, On 2016/05/18 00:42:53, reed1 wrote: > is runColor premultiplied? I'm not actually sure. It's of type D3DCOLORVALUE. The phrasing of the docs makes me think it isn't, but I haven't found anything definitive. In either case, while testing I've only seen alpha values of 0 and 1.
If you have ever seen alpha == 0 but red/green/blue not 0, then it is unpremultiplied.
https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:214: SkTScopedComPtr<IDWriteFactory> factory(SkSafeRefComPtr(sk_get_dwrite_factory())); You cannot create a factory here, you must use the one provided by fTypeface->fFactory. You'll then call QueryInterface on that. In general, you cannot use sk_get_dwrite_factory unless you're just creating the top level object; the user may have provided their own factory (and Chromium always does). https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:502: 0, 0, &run, nullptr, DWRITE_MEASURING_MODE_NATURAL, nullptr, 0, colorGlyph); Need to pass in fXform and fMeasuringMode. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:508: return false; The return value handling here should be more like if (DWRITE_E_NOCOLOR == hr) { return false; } HRBM(hr, "Failed to get color glyph run.") return true; it's unfortunate that in this case they used the return value to indicate 'negative success' instead of an actual error. Most other calls in the DirectWrite API have an out BOOL parameter to indicate this instead. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:773: HRESULT hr = S_OK; Remove this. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:794: if (!SUCCEEDED(colorLayers->GetCurrentRun(&colorGlyph))) { Use the HRNM macro for proper logging and for consistency. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:802: hr = fTypeface->fFactory->CreateGlyphRunAnalysis(&colorGlyph->glyphRun, The result is never checked. Use the HRNM macro like drawDWMask does. In general it's an anti-pattern to have a local 'hr' variable. Usually if the result isn't immediately checked then it's ignored, like here. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:804: nullptr, If you don't pass the fXForm, how does this work for anything other than identity matrix? https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:806: DWRITE_MEASURING_MODE_NATURAL, fRenderingMode and fMeasuringMode https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:815: hr = glyphRunAnalysis->CreateAlphaTexture(DWRITE_TEXTURE_ALIASED_1x1, HRNM, fRenderingMode https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:822: SkColor color = SkColorSetARGB(colorGlyph->runColor.a * 255, Note that if all components of runColor are 0 or (equivalently) paletteIndex is 0xFFFF then the 'current brush' is used. fRec.getLuminanceColor() is kinda sorta what is wanted here, but not really, it will often be the wrong value because it wan't designed for this. Fixing this is quite difficult and one of the reasons this hasn't already been done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:827: if (fBits[pixel] != 0) { This seems like a really bad blend, this should be SrcOver. It appears that's why you're hard coding ALIASED? That seems like a bad idea.
https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:214: SkTScopedComPtr<IDWriteFactory> factory(SkSafeRefComPtr(sk_get_dwrite_factory())); On 2016/05/18 14:50:55, bungeman-skia wrote: > You cannot create a factory here, you must use the one provided by > fTypeface->fFactory. You'll then call QueryInterface on that. In general, you > cannot use sk_get_dwrite_factory unless you're just creating the top level > object; the user may have provided their own factory (and Chromium always does). Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:502: 0, 0, &run, nullptr, DWRITE_MEASURING_MODE_NATURAL, nullptr, 0, colorGlyph); On 2016/05/18 14:50:55, bungeman-skia wrote: > Need to pass in fXform and fMeasuringMode. Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:508: return false; On 2016/05/18 14:50:55, bungeman-skia wrote: > The return value handling here should be more like > > if (DWRITE_E_NOCOLOR == hr) { > return false; > } > HRBM(hr, "Failed to get color glyph run.") > return true; > > it's unfortunate that in this case they used the return value to indicate > 'negative success' instead of an actual error. Most other calls in the > DirectWrite API have an out BOOL parameter to indicate this instead. Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:773: HRESULT hr = S_OK; On 2016/05/18 14:50:55, bungeman-skia wrote: > Remove this. Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:794: if (!SUCCEEDED(colorLayers->GetCurrentRun(&colorGlyph))) { On 2016/05/18 14:50:55, bungeman-skia wrote: > Use the HRNM macro for proper logging and for consistency. Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:802: hr = fTypeface->fFactory->CreateGlyphRunAnalysis(&colorGlyph->glyphRun, On 2016/05/18 14:50:55, bungeman-skia wrote: > The result is never checked. Use the HRNM macro like drawDWMask does. In general > it's an anti-pattern to have a local 'hr' variable. Usually if the result isn't > immediately checked then it's ignored, like here. Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:804: nullptr, On 2016/05/18 14:50:55, bungeman-skia wrote: > If you don't pass the fXForm, how does this work for anything other than > identity matrix? Added fXfrom, but I'm not sure when it will be something other than the identity matrix. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:806: DWRITE_MEASURING_MODE_NATURAL, On 2016/05/18 14:50:55, bungeman-skia wrote: > fRenderingMode and fMeasuringMode Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:815: hr = glyphRunAnalysis->CreateAlphaTexture(DWRITE_TEXTURE_ALIASED_1x1, On 2016/05/18 14:50:55, bungeman-skia wrote: > HRNM, fRenderingMode Done. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:827: if (fBits[pixel] != 0) { On 2016/05/18 14:50:54, bungeman-skia wrote: > This seems like a really bad blend, this should be SrcOver. It appears that's > why you're hard coding ALIASED? That seems like a bad idea. Originally I thought antialiasing/subpixel rendering wouldn't work with color glyphs, but now I think it could. Although I'm not sure exactly how the different layers need to be combined when one or both are partially transparent. I need to think a bit more about this.
Did you mean to upload a new patch set? I only see patch set 1 here. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:804: nullptr, On 2016/05/18 22:43:38, Ilya Kulshin wrote: > On 2016/05/18 14:50:55, bungeman-skia wrote: > > If you don't pass the fXForm, how does this work for anything other than > > identity matrix? > > Added fXfrom, but I'm not sure when it will be something other than the identity > matrix. Any time there is skew/rotation.
I haven't uploaded a new patchset yet. Still working on getting AA/subpixel blending right.
ptal. I changed the code to use paths which are then drawn into a canvas to generate the image. This makes it much easier to get antialiasing working, and should have better support for partially transparent glyphs in general. https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/60001/src/ports/SkScalerConte... src/ports/SkScalerContext_win_dw.cpp:822: SkColor color = SkColorSetARGB(colorGlyph->runColor.a * 255, On 2016/05/18 14:50:55, bungeman-skia wrote: > Note that if all components of runColor are 0 or (equivalently) paletteIndex is > 0xFFFF then the 'current brush' is used. fRec.getLuminanceColor() is kinda sorta > what is wanted here, but not really, it will often be the wrong value because it > wan't designed for this. Fixing this is quite difficult and one of the reasons > this hasn't already been done. Changed to fRec.getLuminanceColor() and added a comment and an assert. In practice, I haven't found a glyph that uses the current brush, and verified that no such glyphs exist in Microsoft's color emoji font. Perhaps this case was added for handling runs of mixed color and non-color glyphs?
seems fine; deferring to bunge https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:780: SkAutoTUnref<SkCanvas> canvas( If this routine shows up in a profile, two ways to speed it up: 1. put the SkCanvas on the stack (avoid the new call) 2. use SkDraw (on the stack) instead of creating a canvas at all. https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:839: canvas->flush(); not needed, since the canvas goes out of scope before we need to look at the pixels.
https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:780: SkAutoTUnref<SkCanvas> canvas( On 2016/05/20 12:27:32, reed1 wrote: > If this routine shows up in a profile, two ways to speed it up: > > 1. put the SkCanvas on the stack (avoid the new call) > 2. use SkDraw (on the stack) instead of creating a canvas at all. Haven't had a chance to profile this, but switched to SkDraw anyway. https://codereview.chromium.org/1984943002/diff/120001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:839: canvas->flush(); On 2016/05/20 12:27:32, reed1 wrote: > not needed, since the canvas goes out of scope before we need to look at the > pixels. Acknowledged.
thanks
Description was changed from ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. Chromium tests: https://codereview.chromium.org/2003853002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
This should also be tested in Skia. The diff you need looks something like
diff --git a/tools/sk_tool_utils.cpp b/tools/sk_tool_utils.cpp
index 7343ce4..ba34965 100644
--- a/tools/sk_tool_utils.cpp
+++ b/tools/sk_tool_utils.cpp
@@ -72,6 +72,9 @@ const char* platform_os_emoji() {
if (!strncmp(osName, "Mac", 3)) {
return "SBIX";
}
+ if (!strncmp(osName, "Win", 3)) {
+ return "COLR";
+ }
return "";
}
@@ -82,6 +85,9 @@ sk_sp<SkTypeface> emoji_typeface() {
if (!strcmp(sk_tool_utils::platform_os_emoji(), "SBIX")) {
return SkTypeface::MakeFromName("Apple Color Emoji",
SkTypeface::kNormal);
}
+ if (!strcmp(sk_tool_utils::platform_os_emoji(), "COLR")) {
+ return SkTypeface::MakeFromName("Segoe UI Emoji",
SkTypeface::kNormal);
+ }
return nullptr;
}
@@ -89,7 +95,9 @@ const char* emoji_sample_text() {
if (!strcmp(sk_tool_utils::platform_os_emoji(), "CBDT")) {
return "Hamburgefons";
}
- if (!strcmp(sk_tool_utils::platform_os_emoji(), "SBIX")) {
+ if (!strcmp(sk_tool_utils::platform_os_emoji(), "SBIX") ||
+ !strcmp(sk_tool_utils::platform_os_emoji(), "COLR"))
+ {
Then run SampleApp --slide GM:coloremojiCOLR --key os Win
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
File src/ports/SkScalerContext_win_dw.cpp (right):
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
src/ports/SkScalerContext_win_dw.cpp:505: return false;
nit: four space indent
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
src/ports/SkScalerContext_win_dw.cpp:782: matrix.postTranslate(-glyph.fLeft,
-glyph.fTop);
For some reason the bots don't seem to be running with the right warnings. These
conversions will need to look like -SkIntToScalar(glyph.fLeft) to avoid
conversion warnings.
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
src/ports/SkScalerContext_win_dw.cpp:803: color =
SkColorSetARGB(colorGlyph->runColor.a * 255,
Similarly these give warnings. I think these need to be like
SkFloatToIntRound(colorGlyph->runColor.a * 255)
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
src/ports/SkScalerContext_win_dw.cpp:813: // so I'm not sure it's even possible
(it might be reserved for a run with a mix of
Remove this 'possible' speculation. The specification explicitly states that
it's allowed.
https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont...
src/ports/SkScalerContext_win_dw.cpp:827: // If only DirectWrite had a
"GetGlyphrunOutlineForThisGlyphRun" API...
nit: it would be named GetGlyphRunOutlineForGlyphRun
Patchset #6 (id:160001) has been deleted
ptal. https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... File src/ports/SkScalerContext_win_dw.cpp (right): https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:505: return false; On 2016/05/23 22:03:41, bungeman-skia wrote: > nit: four space indent Done. https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:782: matrix.postTranslate(-glyph.fLeft, -glyph.fTop); On 2016/05/23 22:03:40, bungeman-skia wrote: > For some reason the bots don't seem to be running with the right warnings. These > conversions will need to look like -SkIntToScalar(glyph.fLeft) to avoid > conversion warnings. Done. https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:803: color = SkColorSetARGB(colorGlyph->runColor.a * 255, On 2016/05/23 22:03:41, bungeman-skia wrote: > Similarly these give warnings. I think these need to be like > > SkFloatToIntRound(colorGlyph->runColor.a * 255) Done. https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:813: // so I'm not sure it's even possible (it might be reserved for a run with a mix of On 2016/05/23 22:03:41, bungeman-skia wrote: > Remove this 'possible' speculation. The specification explicitly states that > it's allowed. Done. https://codereview.chromium.org/1984943002/diff/140001/src/ports/SkScalerCont... src/ports/SkScalerContext_win_dw.cpp:827: // If only DirectWrite had a "GetGlyphrunOutlineForThisGlyphRun" API... On 2016/05/23 22:03:41, bungeman-skia wrote: > nit: it would be named GetGlyphRunOutlineForGlyphRun Removed the comment altogether, since it wasn't really serving any purpose.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984943002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984943002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
Description was changed from ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. Chromium tests: https://codereview.chromium.org/2003853002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. Chromium tests: https://codereview.chromium.org/2003853002 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1984943002/diff/180001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/180001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:89: return SkTypeface::MakeFromName("Segoe UI Emoji", SkTypeface::kNormal); After running the trybots on this and taking a look at the output, it's rather unfortunate what is happening on win7 for these tests. Since they don't have Segoe UI Emoji this comes back with the default font instead of nullptr, which means that several of the tests draw empty squares on win7, since that font doesn't have these glyphs. Instead, if we make the body of this block more like sk_sp<SkFontMgr> fm(SkFontMgr::RefDefault()); sk_sp<SkTypeface> typeface(fm->matchFamilyStyle("Segoe UI Emoji", SkFontStyle())); if (typeface) { return typeface; } // Windows 7 does not have Segoe UI Emoji; Segoe UI Symbol has the (non-color) emoji. return SkTypeface::MakeFromName("Segoe UI Symbol", SkTypeface::kNormal); then the output on win7 doesn't look quite so confusingly bad. Basically this just says to get Segoe UI Emoji if it exists, if it doesn't exist try to get Segoe UI Symbol, otherwise get the default system font which will draw bad so we can see that we did something bad. On the other hand, I can just make this change after this lands.
https://codereview.chromium.org/1984943002/diff/180001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/180001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:89: return SkTypeface::MakeFromName("Segoe UI Emoji", SkTypeface::kNormal); On 2016/05/26 19:49:35, bungeman-skia wrote: > After running the trybots on this and taking a look at the output, it's rather > unfortunate what is happening on win7 for these tests. Since they don't have > Segoe UI Emoji this comes back with the default font instead of nullptr, which > means that several of the tests draw empty squares on win7, since that font > doesn't have these glyphs. Instead, if we make the body of this block more like > > sk_sp<SkFontMgr> fm(SkFontMgr::RefDefault()); > sk_sp<SkTypeface> typeface(fm->matchFamilyStyle("Segoe UI Emoji", > SkFontStyle())); > if (typeface) { > return typeface; > } > // Windows 7 does not have Segoe UI Emoji; Segoe UI Symbol has the > (non-color) emoji. > return SkTypeface::MakeFromName("Segoe UI Symbol", SkTypeface::kNormal); > > then the output on win7 doesn't look quite so confusingly bad. Basically this > just says to get Segoe UI Emoji if it exists, if it doesn't exist try to get > Segoe UI Symbol, otherwise get the default system font which will draw bad so we > can see that we did something bad. On the other hand, I can just make this > change after this lands. Done, although since this was creating the font manager anyway, I added code to use the font fallback API to possibly pick a font if Segoe UI Emoji is not available. I think with the fallback it doesn't even need to try to fallback on Segoe UI Symbol, but I'm having some trouble building Skia on my Win7 machine so I haven't tried it out yet. Hopefully I'll be able to try it out tomorrow - if it can figure out the fallback on Win7 then it won't need the explicit fallback on Segoe UI Symbol and I'll remove it.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984943002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984943002/200001
The CQ bit was unchecked by bungeman@google.com
https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:97: const char *bcp47 = "en-us"; en-US Windows may or may not care. Also, I think you can just pass in nullptr and 0, I'm not sure we actually care about the language here? In fact, technically, there is a particular language tag for emoji, but I don't think Windows understands it until Windows 10 with some update. https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:98: sk_sp<SkTypeface> fallback(fm->matchFamilyStyleCharacter( Note that matchFamilyStyleCharacter was never implemented in the GDI font manager and we need to keep that working for other reasons. So on GDI this call will always return nullptr, so it's still a good idea to still have the MakeFromName below. https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:99: colorEmojiFontName, SkFontStyle(), &bcp47, 1 /* bcp47Count */, 0x1f4b0 /* character: 💰 */)); nit: this line is a bit long (Skia uses 100 characters).
ptal https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:97: const char *bcp47 = "en-us"; On 2016/05/27 12:51:39, bungeman-skia wrote: > en-US > > Windows may or may not care. Also, I think you can just pass in nullptr and 0, > I'm not sure we actually care about the language here? In fact, technically, > there is a particular language tag for emoji, but I don't think Windows > understands it until Windows 10 with some update. Switched to nullptr/0. https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:98: sk_sp<SkTypeface> fallback(fm->matchFamilyStyleCharacter( On 2016/05/27 12:51:39, bungeman-skia wrote: > Note that matchFamilyStyleCharacter was never implemented in the GDI font > manager and we need to keep that working for other reasons. So on GDI this call > will always return nullptr, so it's still a good idea to still have the > MakeFromName below. Acknowledged. https://codereview.chromium.org/1984943002/diff/200001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:99: colorEmojiFontName, SkFontStyle(), &bcp47, 1 /* bcp47Count */, 0x1f4b0 /* character: 💰 */)); On 2016/05/27 12:51:39, bungeman-skia wrote: > nit: this line is a bit long (Skia uses 100 characters). Done.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984943002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984943002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1984943002/diff/220001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/220001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:105: return SkTypeface::MakeFromName("Segoe UI Symbol", SkTypeface::kNormal); This line conflicts with another change that just landed. SkTypeface::MakeFromName now takes SkFontStyle instead of SkTypeface::Style. This should now be return SkTypeface::MakeFromName("Segoe UI Symbol", SkFontStyle()); You'll probably want to rebase as well.
https://codereview.chromium.org/1984943002/diff/220001/tools/sk_tool_utils.cpp File tools/sk_tool_utils.cpp (right): https://codereview.chromium.org/1984943002/diff/220001/tools/sk_tool_utils.cp... tools/sk_tool_utils.cpp:105: return SkTypeface::MakeFromName("Segoe UI Symbol", SkTypeface::kNormal); On 2016/05/31 19:19:58, bungeman-skia wrote: > This line conflicts with another change that just landed. > SkTypeface::MakeFromName now takes SkFontStyle instead of SkTypeface::Style. > This should now be > > return SkTypeface::MakeFromName("Segoe UI Symbol", SkFontStyle()); > > You'll probably want to rebase as well. Done. Thanks for the headsup.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984943002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984943002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@google.com
lgtm Let's try it out. lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984943002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984943002/240001
Message was sent while issue was closed.
Description was changed from ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. Chromium tests: https://codereview.chromium.org/2003853002 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Implement support for rendering color emoji on Windows This change adds support to the DirectWrite scaler context for detection of color fonts. If it detects a color font and the glyph is a color glyph, it will use DirectWrite's TranslateColorGlyphRun API and generate an image of the glyph that can then be rendered. Chromium tests: https://codereview.chromium.org/2003853002 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c4b091543b4082fd1a2f356ef3b478073c5f9418 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://skia.googlesource.com/skia/+/c4b091543b4082fd1a2f356ef3b478073c5f9418
Message was sent while issue was closed.
Great, well done, kulshin@! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
