Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253993002:
SkPDF: cache metrics once. (Closed)
|
Created:
4 years, 4 months ago by hal.canary Modified:
4 years, 4 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: cache metrics once.
Motivation: drawText can look up unicode mapping at draw time to see
if ActualText should be used after crrev.com/2084533004 lands.
For each SkTypeface, only call getAdvancedTypefaceMetrics() once per
document. Cache the result in the SkPDFCanon, indexed by SkFontID.
Also cache PDF FontDescriptors in the canon, also indexed by SkFontID
(Type1 fonts only).
Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map
that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(),
SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match.
SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics.
Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset()
get metrics from canon.
SkPDFFont multybite bool is now a function of type.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Release-GDI-Trybot,Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Debug-GDI-Trybot
Committed: https://skia.googlesource.com/skia/+/cee13425b5cd862189d1e5d7cf8f258bccae5f5d
Patch Set 1 #
Total comments: 14
Patch Set 2 : 2016-08-17 (Wednesday) 14:22:07 EDT #
Total comments: 4
Patch Set 3 : 2016-08-17 (Wednesday) 15:29:42 EDT #
Total comments: 8
Patch Set 4 : 2016-08-17 (Wednesday) 16:14:00 EDT #Patch Set 5 : metrics can be null 2016-08-18 (Thursday) 10:59:37 EDT #
Total comments: 4
Patch Set 6 : 2016-08-18 (Thursday) 11:37:09 EDT #
Dependent Patchsets: Messages
Total messages: 54 (37 generated)
Description was changed from ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. Amend SkTHashMap to use move semantics. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. Amend SkTHashMap to use move semantics. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 ==========
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...
halcanary@google.com changed reviewers: + bungeman@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Description was changed from ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. Amend SkTHashMap to use move semantics. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 ==========
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...
https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFCanon.cpp File src/pdf/SkPDFCanon.cpp (left): https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFCanon.cpp#oldc... src/pdf/SkPDFCanon.cpp:31: fPDFBitmapMap.reset(); Not clear why this reset is being removed but not the one above for fGraphicStateRecords. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:218: SkAdvancedTypefaceMetrics::kNotEmbeddable_FontFlag); If the '!SkToBool(...)' part will fit if formatted like return !metrics || !SkToBool(...); I think that would be preferable. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:267: if (!typeface) { Note that 'SkTypeface::UniqueID is already doing this work above. If you need to resolve anyway (so the code is going to be here anyway), might as well resolve 'typeface' to non-null in the first place and just call typeface->uniqueID() above. It may simply be out of scope for this CL anyway, but it seems strange that we get this far and are passing around nullptr SkTypeface*. Is there any reason not to resolve them to the default SkTypeface at the top of the stack and not have to keep checking? https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:288: return gid > 1 ? gid - (gid - 1) % 255 : 1; 1 - 0 % 255 == 1 gid > 0? https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:568: uint16_t emSize = (uint16_t)metrics.fEmSize; It's ugly that we cast this to uint16_t here for no real reason and then the one place it's used (in from_font_units) it's actually turned back into a float. Why the cast anyway? Isn't it already uint16_t? https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:680: SkAdvancedTypefaceMetrics::kType1_Font) { I would much prefer the brace on the next line at column 0. The body of this function is now five lines and four different indentation levels from the start of the definition, which make it difficult to see the brace. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:821: if (metrics/* && metrics->fGlyphToUnicode.count() > 0 */) { commented out?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.cpp File src/pdf/SkPDFCanon.cpp (right): https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.cpp#... src/pdf/SkPDFCanon.cpp:16: template <typename K, typename V> struct KVUnrefer { This name makes me think it's going to unref both the key and value. From the way it's used maybe name it UnrefValue. https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.h File src/pdf/SkPDFCanon.h (right): https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.h#ne... src/pdf/SkPDFCanon.h:73: nit: seems like this blank line could go away
https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFCanon.cpp File src/pdf/SkPDFCanon.cpp (left): https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFCanon.cpp#oldc... src/pdf/SkPDFCanon.cpp:31: fPDFBitmapMap.reset(); On 2016/08/17 18:56:04, bungeman-skia wrote: > Not clear why this reset is being removed but not the one above for > fGraphicStateRecords. Done. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:218: SkAdvancedTypefaceMetrics::kNotEmbeddable_FontFlag); On 2016/08/17 18:56:04, bungeman-skia wrote: > If the '!SkToBool(...)' part will fit if formatted like > > return !metrics || > !SkToBool(...); > > I think that would be preferable. Done. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:267: if (!typeface) { On 2016/08/17 18:56:04, bungeman-skia wrote: > Note that 'SkTypeface::UniqueID is already doing this work above. If you need to > resolve anyway (so the code is going to be here anyway), might as well resolve > 'typeface' to non-null in the first place and just call typeface->uniqueID() > above. > > It may simply be out of scope for this CL anyway, but it seems strange that we > get this far and are passing around nullptr SkTypeface*. Is there any reason not > to resolve them to the default SkTypeface at the top of the stack and not have > to keep checking? Why keep churning reference counts? I intend to use this function later inside SkPDFDevice. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:288: return gid > 1 ? gid - (gid - 1) % 255 : 1; On 2016/08/17 18:56:04, bungeman-skia wrote: > 1 - 0 % 255 == 1 > > gid > 0? Done. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:568: uint16_t emSize = (uint16_t)metrics.fEmSize; On 2016/08/17 18:56:04, bungeman-skia wrote: > It's ugly that we cast this to uint16_t here for no real reason and then the one > place it's used (in from_font_units) it's actually turned back into a float. > > Why the cast anyway? Isn't it already uint16_t? Done. that was some sort of cut-and-paste error. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:680: SkAdvancedTypefaceMetrics::kType1_Font) { On 2016/08/17 18:56:04, bungeman-skia wrote: > I would much prefer the brace on the next line at column 0. The body of this > function is now five lines and four different indentation levels from the start > of the definition, which make it difficult to see the brace. Acknowledged. https://codereview.chromium.org/2253993002/diff/1/src/pdf/SkPDFFont.cpp#newco... src/pdf/SkPDFFont.cpp:821: if (metrics/* && metrics->fGlyphToUnicode.count() > 0 */) { On 2016/08/17 18:56:04, bungeman-skia wrote: > commented out? Apparently we sometimes have fGlyphToUnicode.count() == 0. By commenting this out, I can verify that no PDFs change with the CL. I think I can un-comment it in a later CL. https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.cpp File src/pdf/SkPDFCanon.cpp (right): https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.cpp#... src/pdf/SkPDFCanon.cpp:16: template <typename K, typename V> struct KVUnrefer { On 2016/08/17 19:03:07, bungeman-skia wrote: > This name makes me think it's going to unref both the key and value. From the > way it's used maybe name it UnrefValue. Done. https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.h File src/pdf/SkPDFCanon.h (right): https://codereview.chromium.org/2253993002/diff/20001/src/pdf/SkPDFCanon.h#ne... src/pdf/SkPDFCanon.h:73: On 2016/08/17 19:03:07, bungeman-skia wrote: > nit: seems like this blank line could go away Done.
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...
lgtm with nits https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:274: SkTypeface::kToUnicode_PerGlyphInfo, nullptr, 0)); It's really strange formatting to break a single parameter across multiple lines but also put additional parameters after on the same line. Can this be SkTypeface::kGlyphNames_PerGlyphInfo | SkTypeface::kToUnicode_PerGlyphInfo, nullptr, 0)); which I think fits. https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:385: fFirstGlyphID = first_glyph_for_single_byte_encoding(glyphID); nit: two spaces after '=' https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:678: : SkPDFFont(std::move(typeface),SkAdvancedTypefaceMetrics::kType1_Font) { nit: space after comma also, there's a lot of formatting in general like this which makes it very difficult to visually scan for the start of the body of the method. https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:818: SkPDFFont::GetMetrics(typeface, canon); nit: any reason not to have this on one line?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:274: SkTypeface::kToUnicode_PerGlyphInfo, nullptr, 0)); On 2016/08/17 19:53:51, bungeman-skia wrote: > It's really strange formatting to break a single parameter across multiple lines > but also put additional parameters after on the same line. Can this be > > SkTypeface::kGlyphNames_PerGlyphInfo | > SkTypeface::kToUnicode_PerGlyphInfo, > nullptr, 0)); > > which I think fits. Done. https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:385: fFirstGlyphID = first_glyph_for_single_byte_encoding(glyphID); On 2016/08/17 19:53:51, bungeman-skia wrote: > nit: two spaces after '=' Done. https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:678: : SkPDFFont(std::move(typeface),SkAdvancedTypefaceMetrics::kType1_Font) { On 2016/08/17 19:53:51, bungeman-skia wrote: > nit: space after comma > > also, there's a lot of formatting in general like this which makes it very > difficult to visually scan for the start of the body of the method. Done. https://codereview.chromium.org/2253993002/diff/40001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:818: SkPDFFont::GetMetrics(typeface, canon); On 2016/08/17 19:53:51, bungeman-skia wrote: > nit: any reason not to have this on one line? Done.
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 halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2253993002/#ps60001 (title: "2016-08-17 (Wednesday) 16:14:00 EDT")
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 ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 Committed: https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94
Message was sent while issue was closed.
On 2016/08/17 21:21:45, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) as > https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94 DM's started crashing with a pretty good looking stack trace on the Google3 roll. Mind taking a look?
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2251813006/ by robertphillips@google.com. The reason for reverting is: because.
Message was sent while issue was closed.
On 2016/08/17 23:28:17, robertphillips wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2251813006/ by mailto:robertphillips@google.com. > > The reason for reverting is: because. Seems to be breaking Google3 and, on Windows, Caught exception 3221225477 EXCEPTION_ACCESS_VIOLATION, was running: Likely culprit: pdf gm fontmgr_boundsWinGDI
Message was sent while issue was closed.
Description was changed from ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 Committed: https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94 ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 Committed: https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94 ==========
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 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...
bungeman@, take a look at newest diff
https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:277: SkDebugf("SkPDF: SkTypeface:getAdvancedTypefaceMetrics() returned null."); This should be SkDEBUGF so we're not spewing this message in release. https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:421: if (!can_subset(*metrics)) { if (!metrics || !can_subset(*metrics)) { or (to match can_embed) if (!(metrics && can_subset(*metrics)) {
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:277: SkDebugf("SkPDF: SkTypeface:getAdvancedTypefaceMetrics() returned null."); On 2016/08/18 15:17:28, bungeman-skia wrote: > This should be SkDEBUGF so we're not spewing this message in release. Done. https://codereview.chromium.org/2253993002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:421: if (!can_subset(*metrics)) { On 2016/08/18 15:17:28, bungeman-skia wrote: > if (!metrics || !can_subset(*metrics)) { > > or (to match can_embed) > > if (!(metrics && can_subset(*metrics)) { Done.
Description was changed from ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 Committed: https://skia.googlesource.com/skia/+/0a61270f4ba85d10659fb63a86817b435ec04c94 ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Release-GDI-Trybot,Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Debug-GDI-Trybot ==========
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...
generally still lgtm
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 halcanary@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 ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Release-GDI-Trybot,Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Debug-GDI-Trybot ========== to ========== SkPDF: cache metrics once. Motivation: drawText can look up unicode mapping at draw time to see if ActualText should be used after crrev.com/2084533004 lands. For each SkTypeface, only call getAdvancedTypefaceMetrics() once per document. Cache the result in the SkPDFCanon, indexed by SkFontID. Also cache PDF FontDescriptors in the canon, also indexed by SkFontID (Type1 fonts only). Simplify PDF font lookup, map SkFontID+SkGlyphID into a uint64_t. Map that uint64_t to SkPDFFonts. Remove SkPDFCanon::findFont(), SkPDFCanon::addFont(), SkPDFFont::IsMatch(), and enum SkPDFFont::Match. SkPDFFont no longer holds on to ref of SkAdvancedTypefaceMetrics. Instead, SkPDFFont::GetFontResource() and SkPDFFont::getFontSubset() get metrics from canon. SkPDFFont multybite bool is now a function of type. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2253993002 CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Release-GDI-Trybot,Test-Win-MSVC-GCE-CPU-AVX2-x86_64-Debug-GDI-Trybot Committed: https://skia.googlesource.com/skia/+/cee13425b5cd862189d1e5d7cf8f258bccae5f5d ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/cee13425b5cd862189d1e5d7cf8f258bccae5f5d |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
