Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162023002:
Correct advances for 'monospace' fonts in PDF. (Closed)
|
Created:
4 years, 5 months ago by bungeman-skia Modified:
4 years, 5 months ago Reviewers:
hal.canary CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionCorrect advances for 'monospace' fonts in PDF.
FT_IS_FIXED_WIDTH, kCTFontMonoSpaceTrait, and TMPF_FIXED_PITCH
are style bits, they do not imply that all advances are the same.
BUG=skia:5537
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002
Committed: https://skia.googlesource.com/skia/+/f1491693527a70919de5d624a049cae38384474e
Patch Set 1 #Patch Set 2 : CG and GDI as well. #
Total comments: 2
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH is a style bit, it does not imply that all advances are the same. BUG=skia:5537 ========== to ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH is a style bit, it does not imply that all advances are the same. BUG=skia:5537 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002 ==========
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/v2/patch-status/codereview.chromium.or...
bungeman@google.com changed reviewers: + halcanary@google.com
I don't know if this fixed the linked bug, but it might and it removes a bug anyway. This does lose the 'kDefault' optimization (I'm not sure why it was only applied to stylistically 'fixed width' fonts only). The current design makes it difficult to create the 'kDefault' bit correctly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dml@, does this patch fix this bug? I'm working on a GM to test this:
/*
* Copyright 2016 Google Inc.
*
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
#include "gm.h"
// https://bugs.skia.org/5337
DEF_SIMPLE_GM(skbug_5337, canvas, 128, 64) {
const char* const kMonospaceFonts[] = {
"Monaco", "Consolas", "Lucida Console", "Courier",
};
sk_sp<SkTypeface> tf;
for (auto fontFamily : kMonospaceFonts) {
tf = SkTypeface::MakeFromName(fontFamily, SkFontStyle());
if (tf) { break; }
}
if (!tf) { return; }
SkPaint paint;
paint.setTypeface(std::move(tf));
paint.setTextEncoding(SkPaint::kUTF8_TextEncoding);
paint.setTextSize(12);
const char text[] = "ABCDE FGHIGJ";
canvas->drawText(text, strlen(text), 8, 32, paint);
}
I get the same bug on my macbook. bungeman@, can you look at SkFontHost_mac.cpp too?
Now with CG and GDI. Turns out I wrote the DirectWrite one 'correctly' already, in that it checks that there is only one horizontal metric (which is almost never true).
On 2016/07/19 16:04:52, bungeman-skia wrote: > Now with CG and GDI. Turns out I wrote the DirectWrite one 'correctly' already, > in that it checks that there is only one horizontal metric (which is almost > never true). Let's add that GM I wrote (or something like it).
On 2016/07/19 17:11:07, Hal Canary wrote: > On 2016/07/19 16:04:52, bungeman-skia wrote: > > Now with CG and GDI. Turns out I wrote the DirectWrite one 'correctly' > already, > > in that it checks that there is only one horizontal metric (which is almost > > never true). > > Let's add that GM I wrote (or something like it). Hmmm... typefacestylesUbuntu with the Pdfium rasterizer already shows this issue in gold.
On 2016/07/19 17:28:39, bungeman-skia wrote: > On 2016/07/19 17:11:07, Hal Canary wrote: > > On 2016/07/19 16:04:52, bungeman-skia wrote: > > > Now with CG and GDI. Turns out I wrote the DirectWrite one 'correctly' > > already, > > > in that it checks that there is only one horizontal metric (which is almost > > > never true). > > > > Let's add that GM I wrote (or something like it). > > Hmmm... typefacestylesUbuntu with the Pdfium rasterizer already shows this issue > in gold. SGTM. I'll look a the code when I get a chance.
On 2016/07/19 15:48:38, Hal Canary wrote: > dml@, does this patch fix this bug? I'm working on a GM to test this: Sorry about the delay, was OOO. Yep, that fixes it. Thanks :)
Description was changed from ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH is a style bit, it does not imply that all advances are the same. BUG=skia:5537 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002 ========== to ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH, kCTFontMonoSpaceTrait, and TMPF_FIXED_PITCH are style bits, they do not imply that all advances are the same. BUG=skia:5537 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002 ==========
The CQ bit was checked by halcanary@google.com
lgtm LGTM. https://codereview.chromium.org/2162023002/diff/20001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2162023002/diff/20001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:599: info->fType == SkAdvancedTypefaceMetrics::kType1_Font) style nit: `{` on previous line https://codereview.chromium.org/2162023002/diff/20001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:614: face->num_charmaps) same
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH, kCTFontMonoSpaceTrait, and TMPF_FIXED_PITCH are style bits, they do not imply that all advances are the same. BUG=skia:5537 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002 ========== to ========== Correct advances for 'monospace' fonts in PDF. FT_IS_FIXED_WIDTH, kCTFontMonoSpaceTrait, and TMPF_FIXED_PITCH are style bits, they do not imply that all advances are the same. BUG=skia:5537 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2162023002 Committed: https://skia.googlesource.com/skia/+/f1491693527a70919de5d624a049cae38384474e ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/f1491693527a70919de5d624a049cae38384474e |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
