Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23684041:
improve bitmap font support (FreeType only) (Closed)
|
Created:
7 years, 3 months ago by violets Modified:
7 years ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionimprove bitmap font support (FreeType only)
This commit improves SkFontHost_FreeType's support for bitmap fonts,
adding a number of features:
- Intelligent bitmap strike selection.
- Inter-strike bitmap font scaling.
- Colour bitmap font support (FreeType 2.5.0+).
BUG=
R=reed@google.com
Committed: https://code.google.com/p/skia/source/detail?r=12607
Patch Set 1 #Patch Set 2 : added test fonts #Patch Set 3 : disable linear text for bitmap fonts #Patch Set 4 : bring metrics within one pixel of original values, fix linear text + bitmap font #Patch Set 5 : bring metrics in line with previous values #Patch Set 6 : match pre-refactor ascent/descent/leading exactly #Patch Set 7 : no really this time #Patch Set 8 : filter scaled glyph images #
Total comments: 3
Patch Set 9 : fix silly transcription error #
Total comments: 18
Patch Set 10 : reduce indentation #
Total comments: 1
Patch Set 11 : a round of type-specificity... #
Total comments: 4
Patch Set 12 : Rebase against current; build with older FreeTypes. #
Total comments: 2
Patch Set 13 : Can resize. #Patch Set 14 : #
Total comments: 5
Patch Set 15 : Correct selection logic, remove debug code. #
Total comments: 1
Messages
Total messages: 59 (0 generated)
+reed as bungeman is out of the office.
We (skia) definitely needs some test font(s) added, so we can always exercise/test this new code. Can those be attached as part of this CL?
On 2013/09/10 17:45:50, reed1 wrote: > We (skia) definitely needs some test font(s) added, so we can always > exercise/test this new code. Can those be attached as part of this CL? Sure, where does Skia store its test fonts? I don't see any TTF files in trunk...
the tools look in resources/ for other assets (e.g. images), so that would be my guess as the place to add them.
We'll want to augment the tests also to load from there, so we can access them at runtime.
On 2013/09/10 17:49:26, reed1 wrote: > We'll want to augment the tests also to load from there, so we can access them > at runtime. Added some test fonts from https://code.google.com/p/color-emoji/ I don't think these fonts can be redistributed by Skia without attribution, though - they're distributed under the Apache 2.0 license... it may be appropriate to put them in their own specific directory with whatever attributive files they were originally distributed with? I can revise if so.
On 2013/09/10 18:25:57, violets wrote: > On 2013/09/10 17:49:26, reed1 wrote: > > We'll want to augment the tests also to load from there, so we can access them > > at runtime. > > Added some test fonts from https://code.google.com/p/color-emoji/ > > I don't think these fonts can be redistributed by Skia without attribution, > though - they're distributed under the Apache 2.0 license... it may be > appropriate to put them in their own specific directory with whatever > attributive files they were originally distributed with? I can revise if so. We can relicense to whatever suits Skia.
The tree has been red all day today, so we will have to run the (skia) try-bots tomorrow. I see what looks like the results of chrome-try-bots? (linux_layout_rel)...?
On 2013/09/11 19:05:56, reed1 wrote: > The tree has been red all day today, so we will have to run the (skia) try-bots > tomorrow. > > I see what looks like the results of chrome-try-bots? (linux_layout_rel)...? Oh, that doesn't sound good... a lot of image diffs. I can investigate...
I ran patch set 2 against the layout test bots. This can be done by applying the change to your local chromium checkout and sending the tryjob something like ~/chromium/src$ ~/depot_tools/trychange.py --email=yourname@chromium.org --name=skia_freetype --rietveld_url=codereview.chromium.org --issue=23684041 --patchset=7001 --file=third_party/skia/src/ports/SkFontHost_FreeType.cpp --file=third_party/skia/src/ports/SkFontHost_FreeType_common.cpp --bot=linux_layout Note that the patchset must be specified or the files will be ignored and the patch of the latest issue will be used instead. The patchset number can be found in the anchor of the link on the patch set title (and in the name of the 'raw' patch downlad). There are currently more than 5000 failures. Doing some spot checking it appears that this is mostly drawing the same thing, but the font metrics for the line height / ascent appear to be larger. As a result, all text is moving down (and further apart). This may or may not be correct. If it isn't correct it should be fixed, if it is correct then we'll need to put this change (or enough of it) behind a compile flag so that we can rebaseline (see https://sites.google.com/site/skiadocs/developer-documentation/managing-chrom... ).
On 2013/09/11 20:10:55, bungeman1 wrote: > I ran patch set 2 against the layout test bots. This can be done by applying the > change to your local chromium checkout and sending the tryjob something like > > ~/chromium/src$ ~/depot_tools/trychange.py mailto:--email=yourname@chromium.org > --name=skia_freetype --rietveld_url=codereview.chromium.org --issue=23684041 > --patchset=7001 --file=third_party/skia/src/ports/SkFontHost_FreeType.cpp > --file=third_party/skia/src/ports/SkFontHost_FreeType_common.cpp > --bot=linux_layout > > Note that the patchset must be specified or the files will be ignored and the > patch of the latest issue will be used instead. The patchset number can be found > in the anchor of the link on the patch set title (and in the name of the 'raw' > patch downlad). > > There are currently more than 5000 failures. Doing some spot checking it appears > that this is mostly drawing the same thing, but the font metrics for the line > height / ascent appear to be larger. As a result, all text is moving down (and > further apart). This may or may not be correct. If it isn't correct it should be > fixed, if it is correct then we'll need to put this change (or enough of it) > behind a compile flag so that we can rebaseline (see > https://sites.google.com/site/skiadocs/developer-documentation/managing-chrom... > ). Awesome, thank you so much. I'll grab chromium, run some tests locally, and see whether I need to adjust.
On 2013/09/11 20:14:28, violets wrote: > On 2013/09/11 20:10:55, bungeman1 wrote: > > I ran patch set 2 against the layout test bots. > > Awesome, thank you so much. I'll grab chromium, run some tests locally, and see > whether I need to adjust. Just wanted to point out that the layout test results from the above runs are available to download at https://storage.googleapis.com/chromium-layout-test-archives/linux_layout/959... . This zip is currently somewhat awkward to use as it only contains the actual results and no simple means of determining the diff or comparing against the expected (I'm currently looking into that).
FreeType only reports linear text advances for outline fonts, so it was necessary to pull advance data from a different source when rendering bitmap fonts with linear text enabled. I'm still going through the metrics code with a fine-toothed comb, so this new patchset isn't really ready for review yet - I may have another revision coming first.
OK, this is ready for review now. I've meticulously compared metrics against previous values and they're acceptable as far as I'm cared. More importantly, this fixes patchset #3's issue with linear text metrics when using a bitmap font. I'm very happy with the results. =)
We should land the fonts and (new) tests that actually exercise them before we land the code changes. What (if anything) is needed regarding license terms before we can land the fonts?
With patch set 5 the metrics are now somewhat smaller than they used to be. Once again, this may (or may not) be correct.
On 2013/09/13 16:17:32, bungeman1 wrote: > With patch set 5 the metrics are now somewhat smaller than they used to be. Once > again, this may (or may not) be correct. Which specific metrics? Patch set 5 reverts a couple of discrepancies that had crept in through the refactoring that I had done in the first patchset: The xMin and xMax fields were previously unscaled and my refactoring had switched them to scaled values. In the interest of not breaking compatibility I reverted that. Likewise, my refactored version was pulling the "leading" value out of the OS/2 table, which changed it significantly from the previously-computed version. I reverted that, as well. Otherwise, the metrics have turned out to be within subpixel range of the previous values in all of the tests that I've run...
On 2013/09/13 16:38:49, violets wrote: > On 2013/09/13 16:17:32, bungeman1 wrote: > > With patch set 5 the metrics are now somewhat smaller than they used to be. > Once > > again, this may (or may not) be correct. > > Which specific metrics? Not sure, but we still have 5000 failures on the Chromium bots with this change (at patch set 5). Now that I have a way to compare them easier (see https://codereview.chromium.org/23889025/ ) it's fairly easy to see that the line spacing is now smaller (again, with PS5). The ascent is probably the same. As an example, take a look at fast/css/margin-bottom-form-element-strict.html . The image here is just text with a green background. In the expected image the green background descends one more pixel than in the actual. Probably either the descent or leading is different somehow. It could even be that they were being rounded before and now they aren't? In any event, judging by the way Blink uses this information, it seems likely that ascent + descent + leading is different now. It may even be more correct. If it is (for example, show that our output now matches other users of FreeType better) and we want to make this change, then we'll need to put this change behind a compile flag so that the rebaseline can even be done.
On 2013/09/13 16:53:29, bungeman1 wrote: > On 2013/09/13 16:38:49, violets wrote: > > On 2013/09/13 16:17:32, bungeman1 wrote: > > > With patch set 5 the metrics are now somewhat smaller than they used to be. > > Once > > > again, this may (or may not) be correct. > > > > Which specific metrics? > > Not sure, but we still have 5000 failures on the Chromium bots with this change > (at patch set 5). Now that I have a way to compare them easier (see > https://codereview.chromium.org/23889025/ ) it's fairly easy to see that the > line spacing is now smaller (again, with PS5). The ascent is probably the same. > As an example, take a look at fast/css/margin-bottom-form-element-strict.html . > The image here is just text with a green background. In the expected image the > green background descends one more pixel than in the actual. Probably either the > descent or leading is different somehow. It could even be that they were being > rounded before and now they aren't? In any event, judging by the way Blink uses > this information, it seems likely that ascent + descent + leading is different > now. It may even be more correct. If it is (for example, show that our output > now matches other users of FreeType better) and we want to make this change, > then we'll need to put this change behind a compile flag so that the rebaseline > can even be done. Oh, yes. What's happening here is that the metric values provided by FreeType in the FT_Face object are only applicable to outline fonts (ie FT returns 0 in all fields for bitmap fonts). In order for bitmap font metrics to be at all usable, I had to pull them out of the active FT_Size object instead, and those values carry subpixel differences from values computed from the FT_Face object. In theory I could provide *completely* different metrics generation functions for bitmap glyphs vs outline glyphs, but I felt like it made more sense for the two cases to share as much code as possible, which means subpixel deviations from previous values on a handful of the metrics fields. I'll give this another round and see whether I can bring this even closer to the original values.
OK, patch set 6 splits ascent/descent/leading metrics calculation back into separate paths for outline/bitmap fonts. As a result, outline font metrics should exactly match the values returned before support for bitmap font metrics was added.
I also simplified the fDoLinearMetrics solution while I was at it.
On 2013/09/13 20:41:24, violets wrote: > I also simplified the fDoLinearMetrics solution while I was at it. It turned out that the values coming out of the OS2 table were getting overscaled, oops! Fixed this and verified the generated metrics are identical for Roboto and TNR. Time to figure out how to run layout tests...
On 2013/09/16 20:37:21, violets wrote: > On 2013/09/13 20:41:24, violets wrote: > > I also simplified the fDoLinearMetrics solution while I was at it. > > It turned out that the values coming out of the OS2 table were getting > overscaled, oops! Fixed this and verified the generated metrics are identical > for Roboto and TNR. Time to figure out how to run layout tests... OK, I've tried running webkit tests locally *without* my patch, and they also fail identically. At this point I've verified that my patch produces identical font metric values (line height most specifically) for every font I've tested, including Times New Roman, which I believe is the font used in the layout tests. Is it possible that there is a test methodology error at work here?
Thanks violets@ for upstreaming the change and bungeman@ for examining the layout test results. What is the status of the last Patch Set?
On 2013/10/08 18:51:30, Xianzhu wrote: > Thanks violets@ for upstreaming the change and bungeman@ for examining the > layout test results. > > What is the status of the last Patch Set? The latest patchset has not yet been committed, as it fails layout tests. However, those same layout tests fail identically *without* this patchset. Additionally, the Windows and OSX tests are *also* failing, despite not even using SkFontHost_FreeType. All evidence points to the expected results being incorrect.
Just tried the latest patch locally on Linux. The result is same as what bungeman@ observed on the bot for Patch Set 2: all tests passed without the patch, and about 6000 failed with the patch. (Please ignore the red bots on the latest patch. I scheduled the try tasks with incorrect parameters so all of them failed at update or apply_issue stage.)
FYI, skia log (with #define DEBUG_METRICS) when testing fast/text/word-break.html: [10348:10348:1104/165533:1471062214793:INFO:SkFontHost_FreeType.cpp(1305)] post-scale glyph metrics: [10348:10348:1104/165533:1471062215018:INFO:SkFontHost_FreeType.cpp(801)] fAdvanceX: 11.000000 [10348:10348:1104/165533:1471062215282:INFO:SkFontHost_FreeType.cpp(802)] fAdvanceY: 0.000000 [10348:10348:1104/165533:1471062215532:INFO:SkFontHost_FreeType.cpp(803)] fWidth: 9 [10348:10348:1104/165533:1471062215745:INFO:SkFontHost_FreeType.cpp(804)] fHeight: 7 [10348:10348:1104/165533:1471062215995:INFO:SkFontHost_FreeType.cpp(805)] fTop: -8 [10348:10348:1104/165533:1471062216245:INFO:SkFontHost_FreeType.cpp(806)] fLeft: 1 [10348:10348:1104/165533:1471062216525:INFO:SkFontHost_FreeType.cpp(1305)] post-scale glyph metrics: [10348:10348:1104/165533:1471062216773:INFO:SkFontHost_FreeType.cpp(801)] fAdvanceX: 11.000000 [10348:10348:1104/165533:1471062217022:INFO:SkFontHost_FreeType.cpp(802)] fAdvanceY: 0.000000 [10348:10348:1104/165533:1471062217269:INFO:SkFontHost_FreeType.cpp(803)] fWidth: 9 [10348:10348:1104/165533:1471062217475:INFO:SkFontHost_FreeType.cpp(804)] fHeight: 7 [10348:10348:1104/165533:1471062217717:INFO:SkFontHost_FreeType.cpp(805)] fTop: -8 [10348:10348:1104/165533:1471062217964:INFO:SkFontHost_FreeType.cpp(806)] fLeft: 1 [10348:10348:1104/165533:1471062222639:INFO:SkFontHost_FreeType.cpp(1305)] post-scale glyph metrics: [10348:10348:1104/165533:1471062222886:INFO:SkFontHost_FreeType.cpp(801)] fAdvanceX: 11.000000 [10348:10348:1104/165533:1471062223120:INFO:SkFontHost_FreeType.cpp(802)] fAdvanceY: 0.000000 [10348:10348:1104/165533:1471062223351:INFO:SkFontHost_FreeType.cpp(803)] fWidth: 9 [10348:10348:1104/165533:1471062223583:INFO:SkFontHost_FreeType.cpp(804)] fHeight: 7 [10348:10348:1104/165533:1471062223812:INFO:SkFontHost_FreeType.cpp(805)] fTop: -8 [10348:10348:1104/165533:1471062224061:INFO:SkFontHost_FreeType.cpp(806)] fLeft: 1 [10348:10348:1104/165533:1471062224402:INFO:SkFontHost_FreeType.cpp(1305)] post-scale glyph metrics: [10348:10348:1104/165533:1471062224610:INFO:SkFontHost_FreeType.cpp(801)] fAdvanceX: 11.000000 [10348:10348:1104/165533:1471062224858:INFO:SkFontHost_FreeType.cpp(802)] fAdvanceY: 0.000000 [10348:10348:1104/165533:1471062225108:INFO:SkFontHost_FreeType.cpp(803)] fWidth: 9 [10348:10348:1104/165533:1471062225359:INFO:SkFontHost_FreeType.cpp(804)] fHeight: 7 [10348:10348:1104/165533:1471062225611:INFO:SkFontHost_FreeType.cpp(805)] fTop: -8 [10348:10348:1104/165533:1471062225861:INFO:SkFontHost_FreeType.cpp(806)] fLeft: 1 The test result mismatch is about the height of the lines. Many actual results are 1 pixel smaller than the expected results.
https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... src/ports/SkFontHost_FreeType.cpp:1508: leading = SkIntToScalar(face->height / upem + (face->descender - face->ascender)) / upem; I believe this line causes the layout test failures. Should it be: leading = SkIntToScalar(face->height / upem) + ascent - descent; instead?
https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... src/ports/SkFontHost_FreeType.cpp:1508: leading = SkIntToScalar(face->height / upem + (face->descender - face->ascender)) / upem; s/ \/ upem)/) \/ upem/
https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/40001/src/ports/SkFontHost_Free... src/ports/SkFontHost_FreeType.cpp:1508: leading = SkIntToScalar(face->height / upem + (face->descender - face->ascender)) / upem; On 2013/11/05 18:54:44, Xianzhu wrote: > I believe this line causes the layout test failures. Should it be: > > leading = SkIntToScalar(face->height / upem) + ascent - descent; > > instead? or should it be leading = SkIntToScalar(face->height )/upem + descent - ascent; ?
All layout tests passed with the line changed to #29/#30 version. violets@ could you upload a new patch? We do need this change to let color bitmap font work in Chrome (crbug.com/290583).
On 2013/11/05 19:23:09, Xianzhu wrote: > All layout tests passed with the line changed to #29/#30 version. violets@ could > you upload a new patch? We do need this change to let color bitmap font work in > Chrome (crbug.com/290583). Oh wow, lovely sleuthing, thank you so much! I'll make this change and do my own double-checking, as well... Thank you so much for this review! Very helpful!
On 2013/11/05 19:23:09, Xianzhu wrote: > All layout tests passed with the line changed to #29/#30 version. violets@ could > you upload a new patch? We do need this change to let color bitmap font work in > Chrome (crbug.com/290583). Which proposed change did you test? #29 or #30
On 2013/11/05 20:23:12, reed1 wrote: > Which proposed change did you test? #29 or #30 They are actually the same :) I made a mistake in #28 and #29/#30 corrected it.
On 2013/11/05 20:26:19, Xianzhu wrote: > On 2013/11/05 20:23:12, reed1 wrote: > > Which proposed change did you test? #29 or #30 > > They are actually the same :) I made a mistake in #28 and #29/#30 corrected it. Ah, gotcha.
Oh gosh, this is embarrassing. I went to test the proposed change in the Android tree and it was *already there*. It seems that working in two trees and hand-copying little tweaks back and forth as they were made ended up resulting in a simple transcription error. I'll have a fixed patch up soon! Big big thanks for catching this!
Thanks violets@ for the new patch. reed@ and bungeman@, could you review the new patch? Thanks. One of my bug needs it.
can we try the trybots again? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1162: glyph.fWidth = SkToU16(SkScalarRoundToInt(SkScalarMul(SkIntToScalar(glyph.fWidth), scale))); These 4 uses of SkScalarMul are not needed, and make it harder to read. You can just say glyph.fWidth * scale https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, Why are we passing uint8_t for the format type, instead of SkMask::Format ? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, Since we are copying into a bitmap, why not pass in a bitmap? e.g. copyFTBitmap(const FT_Bitmap& src, SkBitmap* dst); The caller seems to already have it on the stack. That would remove the need for these separate parameters (dst, dstFormat, dstRowBytes). https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:194: inline uint8_t skFormatForFTPixelMode(char pixel_mode) { 1. Shouldn't this return SkMask::Format, instead of uint8_t? 2. What type is pixel_mode in FreeType? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:210: inline SkBitmap::Config skConfigForFTPixelMode(char pixel_mode) { What is the type of pixel_mode? Is there an enum for this in FreeType? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:226: inline SkBitmap::Config skConfigForFormat(uint8_t format) { Can we declare format to be SkMask::Format ?
On 2013/11/13 20:21:45, reed1 wrote: > can we try the trybots again? > I used my trybot magic to run this (PS9) against the layout tests, which look good now. linux_layout_rel is green, and linux_layout appears to only have failed because it got caught by the aura scroll-bar transition (the text looks ok where I spot checked). It looks like the Skia trybots failed to apply the patch due to the .ttf data being missing in the patch. It doesn't look like this change actually contains any tests which actually use these files?
https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:248: { This introduces an unnecessary amount of indentation which isn't actually helping much. The '{' should go after the ':' on the previous line and the body un-indented one level.
On 2013/11/15 00:19:48, bungeman1 wrote: > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > File src/ports/SkFontHost_FreeType_common.cpp (right): > > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType_common.cpp:248: { > This introduces an unnecessary amount of indentation which isn't actually > helping much. The '{' should go after the ':' on the previous line and the body > un-indented one level. Thank you so much for the round of in-depth reviews. I'll try to get an updated patch up today!
https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1162: glyph.fWidth = SkToU16(SkScalarRoundToInt(SkScalarMul(SkIntToScalar(glyph.fWidth), scale))); On 2013/11/13 20:21:46, reed1 wrote: > These 4 uses of SkScalarMul are not needed, and make it harder to read. You can > just say > > glyph.fWidth * scale Does that work on builds where SK_SCALAR_IS_FLOAT is not defined? I don't see any operator overloads for SkFixed, and was under the impression that this was why SkScalar and the associated functions existed to begin with. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, On 2013/11/13 20:21:46, reed1 wrote: > Why are we passing uint8_t for the format type, instead of SkMask::Format ? SkGlyph::fMaskFormat, the value being passed into this function, is declared as uint8_t. C/C++ treats enums as strong types, so implicit conversion to SkMask::Format is not provided by the compiler. Any time we try to feed SkGlyph::fMaskFormat or SkScalerContext::Rec::fMaskFormat to a function that expects SkMask::Format, the compiler will throw an error if there is not an explicit static_cast<SkMask::Format>. That gets a little cluttery, so I figured it probably best to just work with the data type SkGlyph::fMaskFormat is actually declared as. If you'd rather I go the other way and do the necessary casting, I can totally do that, but I thought I'd verify first since it does get a little messy... https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, On 2013/11/13 20:21:46, reed1 wrote: > Since we are copying into a bitmap, why not pass in a bitmap? > > e.g. > > copyFTBitmap(const FT_Bitmap& src, SkBitmap* dst); > > The caller seems to already have it on the stack. That would remove the need for > these separate parameters (dst, dstFormat, dstRowBytes). One of the two calls to this function passes in glyph.fImage, which is a flat uint8_t buffer. If I alter this function to take an SkBitmap, the second invocation will require an SkBitmap that currently does not exist be constructed. That seems a little heavyweight compared to simply operating on uint8_t buffers, is that really what we want? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:194: inline uint8_t skFormatForFTPixelMode(char pixel_mode) { On 2013/11/13 20:21:46, reed1 wrote: > 1. Shouldn't this return SkMask::Format, instead of uint8_t? > 2. What type is pixel_mode in FreeType? 1. I think that this should return the same type as SkGlyph::fMaskFormat, to save us a bunch of unnecessary explicit casts. 2. pixel_mode is defined as type "char" in FreeType. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:210: inline SkBitmap::Config skConfigForFTPixelMode(char pixel_mode) { On 2013/11/13 20:21:46, reed1 wrote: > What is the type of pixel_mode? Is there an enum for this in FreeType? pixel_mode is type "char". There is an associated enum, FT_Pixel_Mode, but FreeType never declares variables of that type, so converting between the two requires explicit casts. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:226: inline SkBitmap::Config skConfigForFormat(uint8_t format) { On 2013/11/13 20:21:46, reed1 wrote: > Can we declare format to be SkMask::Format ? We can, but the uint8_t data that we pass into this function will then need an explicit cast. Do we still want to do that? https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:248: { On 2013/11/15 00:19:48, bungeman1 wrote: > This introduces an unnecessary amount of indentation which isn't actually > helping much. The '{' should go after the ':' on the previous line and the body > un-indented one level. Done.
https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, On 2013/11/19 18:38:06, violets wrote: > On 2013/11/13 20:21:46, reed1 wrote: > > Why are we passing uint8_t for the format type, instead of SkMask::Format ? > > SkGlyph::fMaskFormat, the value being passed into this function, is declared as > uint8_t. C/C++ treats enums as strong types, so implicit conversion to > SkMask::Format is not provided by the compiler. Any time we try to feed > SkGlyph::fMaskFormat or SkScalerContext::Rec::fMaskFormat to a function that > expects SkMask::Format, the compiler will throw an error if there is not an > explicit static_cast<SkMask::Format>. That gets a little cluttery, so I figured > it probably best to just work with the data type SkGlyph::fMaskFormat is > actually declared as. > > If you'd rather I go the other way and do the necessary casting, I can totally > do that, but I thought I'd verify first since it does get a little messy... I think that may be worth it, allowing us to have this function signature be much clearer, *especially* since we are dealing with two domains (FT and Skia), it only helps to be explicit whose format we are talking about. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, On 2013/11/19 18:38:06, violets wrote: > On 2013/11/13 20:21:46, reed1 wrote: > > Since we are copying into a bitmap, why not pass in a bitmap? > > > > e.g. > > > > copyFTBitmap(const FT_Bitmap& src, SkBitmap* dst); > > > > The caller seems to already have it on the stack. That would remove the need > for > > these separate parameters (dst, dstFormat, dstRowBytes). > > One of the two calls to this function passes in glyph.fImage, which is a flat > uint8_t buffer. If I alter this function to take an SkBitmap, the second > invocation will require an SkBitmap that currently does not exist be > constructed. > > That seems a little heavyweight compared to simply operating on uint8_t buffers, > is that really what we want? Ah, good point. Creating a bitmap just for that does seem overkill. However, consider taking SkMask instead, since those are free to put on the stack (unlike SkBitmap), but that is just a suggestion. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:194: inline uint8_t skFormatForFTPixelMode(char pixel_mode) { On 2013/11/19 18:38:06, violets wrote: > On 2013/11/13 20:21:46, reed1 wrote: > > 1. Shouldn't this return SkMask::Format, instead of uint8_t? > > 2. What type is pixel_mode in FreeType? > > 1. I think that this should return the same type as SkGlyph::fMaskFormat, to > save us a bunch of unnecessary explicit casts. > 2. pixel_mode is defined as type "char" in FreeType. I still vote for a named type, at least for the return type., *esp* if FT does not give us a real typename for its type. https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:226: inline SkBitmap::Config skConfigForFormat(uint8_t format) { On 2013/11/19 18:38:06, violets wrote: > On 2013/11/13 20:21:46, reed1 wrote: > > Can we declare format to be SkMask::Format ? > > We can, but the uint8_t data that we pass into this function will then need an > explicit cast. Do we still want to do that? I do. I'm also fine if we had a helper to SkGlyph... SkMask::Format format() const { return (SkMask::Format)fFormat; } https://codereview.chromium.org/23684041/diff/519001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/519001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1162: glyph.fWidth = SkToU16(SkScalarRoundToInt(SkScalarMul(SkIntToScalar(glyph.fWidth), scale))); SkScalar has not been SkFixed for over a year, and will never ever be that again, hence my suggestion to remove both SkScalarMul and SkIntToScalar in this new code, to make it simpler/more-readable.
On 2013/11/20 21:52:11, reed1 wrote: > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > File src/ports/SkFontHost_FreeType_common.cpp (right): > > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const > FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, > On 2013/11/19 18:38:06, violets wrote: > > On 2013/11/13 20:21:46, reed1 wrote: > > > Why are we passing uint8_t for the format type, instead of SkMask::Format ? > > > > SkGlyph::fMaskFormat, the value being passed into this function, is declared > as > > uint8_t. C/C++ treats enums as strong types, so implicit conversion to > > SkMask::Format is not provided by the compiler. Any time we try to feed > > SkGlyph::fMaskFormat or SkScalerContext::Rec::fMaskFormat to a function that > > expects SkMask::Format, the compiler will throw an error if there is not an > > explicit static_cast<SkMask::Format>. That gets a little cluttery, so I > figured > > it probably best to just work with the data type SkGlyph::fMaskFormat is > > actually declared as. > > > > If you'd rather I go the other way and do the necessary casting, I can totally > > do that, but I thought I'd verify first since it does get a little messy... > > I think that may be worth it, allowing us to have this function signature be > much clearer, *especially* since we are dealing with two domains (FT and Skia), > it only helps to be explicit whose format we are talking about. > > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType_common.cpp:126: static void copyFTBitmap(const > FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat, > On 2013/11/19 18:38:06, violets wrote: > > On 2013/11/13 20:21:46, reed1 wrote: > > > Since we are copying into a bitmap, why not pass in a bitmap? > > > > > > e.g. > > > > > > copyFTBitmap(const FT_Bitmap& src, SkBitmap* dst); > > > > > > The caller seems to already have it on the stack. That would remove the need > > for > > > these separate parameters (dst, dstFormat, dstRowBytes). > > > > One of the two calls to this function passes in glyph.fImage, which is a flat > > uint8_t buffer. If I alter this function to take an SkBitmap, the second > > invocation will require an SkBitmap that currently does not exist be > > constructed. > > > > That seems a little heavyweight compared to simply operating on uint8_t > buffers, > > is that really what we want? > > Ah, good point. Creating a bitmap just for that does seem overkill. However, > consider taking SkMask instead, since those are free to put on the stack (unlike > SkBitmap), but that is just a suggestion. > > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType_common.cpp:194: inline uint8_t > skFormatForFTPixelMode(char pixel_mode) { > On 2013/11/19 18:38:06, violets wrote: > > On 2013/11/13 20:21:46, reed1 wrote: > > > 1. Shouldn't this return SkMask::Format, instead of uint8_t? > > > 2. What type is pixel_mode in FreeType? > > > > 1. I think that this should return the same type as SkGlyph::fMaskFormat, to > > save us a bunch of unnecessary explicit casts. > > 2. pixel_mode is defined as type "char" in FreeType. > > I still vote for a named type, at least for the return type., *esp* if FT does > not give us a real typename for its type. > > https://codereview.chromium.org/23684041/diff/359001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType_common.cpp:226: inline SkBitmap::Config > skConfigForFormat(uint8_t format) { > On 2013/11/19 18:38:06, violets wrote: > > On 2013/11/13 20:21:46, reed1 wrote: > > > Can we declare format to be SkMask::Format ? > > > > We can, but the uint8_t data that we pass into this function will then need an > > explicit cast. Do we still want to do that? > > I do. > > I'm also fine if we had a helper to SkGlyph... > > SkMask::Format format() const { return (SkMask::Format)fFormat; } > > https://codereview.chromium.org/23684041/diff/519001/src/ports/SkFontHost_Fre... > File src/ports/SkFontHost_FreeType.cpp (right): > > https://codereview.chromium.org/23684041/diff/519001/src/ports/SkFontHost_Fre... > src/ports/SkFontHost_FreeType.cpp:1162: glyph.fWidth = > SkToU16(SkScalarRoundToInt(SkScalarMul(SkIntToScalar(glyph.fWidth), scale))); > SkScalar has not been SkFixed for over a year, and will never ever be that > again, hence my suggestion to remove both SkScalarMul and SkIntToScalar in this > new code, to make it simpler/more-readable. OK! I'll get up on these, just wanted to check my own understanding before I did! Thank you!
Mostly looking good, but you'll need to rebase since kerning support was added (which is why the Skia bots failed here). https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:66: //#define DEBUG_METRICS Note to myself: remove most, if not all, of this after a week of baking. https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1234: #ifdef FT_LOAD_COLOR Maybe these are equivalent (in that they were added at the same time), but shouldn't this be #ifdef FT_PIXEL_MODE_BGRA, since that's what it's protecting? Mostly note to self: In general it doesn't look like this CL actually uses any new API at runtime, instead it just sets an extra flag which will be ignored on older versions of FreeType and tests for a new flag which will not be produced by older versions of FreeType. This CL is really just interested in the compile time constants FT_PIXEL_MODE_BGRA and FT_LOAD_COLOR. Should we consider setting these to their now known values if they aren't defined for 'forward compatibility'? This isn't an issue for Android since the practice there is to build with the FreeType headers which will match the runtime, but Chromium will (often, and for some time) build against an older FreeType without these constants. As things stand, Chromium will never request color fonts. This sort of update will need to be made after this change lands. https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:168: #ifdef FT_LOAD_COLOR Same here (and three more instances below), should this be FT_PIXEL_MODE_BGRA?
https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/589001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1234: #ifdef FT_LOAD_COLOR On 2013/11/22 18:09:14, bungeman1 wrote: > Maybe these are equivalent (in that they were added at the same time), but > shouldn't this be #ifdef FT_PIXEL_MODE_BGRA, since that's what it's protecting? > > Mostly note to self: In general it doesn't look like this CL actually uses any > new API at runtime, instead it just sets an extra flag which will be ignored on > older versions of FreeType and tests for a new flag which will not be produced > by older versions of FreeType. This CL is really just interested in the compile > time constants FT_PIXEL_MODE_BGRA and FT_LOAD_COLOR. Should we consider setting > these to their now known values if they aren't defined for 'forward > compatibility'? This isn't an issue for Android since the practice there is to > build with the FreeType headers which will match the runtime, but Chromium will > (often, and for some time) build against an older FreeType without these > constants. As things stand, Chromium will never request color fonts. This sort > of update will need to be made after this change lands. Ack, now I see the issue, FT_LOAD_COLOR is a macro, but FT_PIXEL_MODE_BGRA is an enum member.
Some changes have been made so that the GPU side can eventually render the color fonts correctly so long as the scaler context doesn't say it's A8 but produce RGBA glyphs. There is still a bit more work and clarification needed before this can go in. Also the A1 blitter is now completely removed since it was never actually supported or implemented. https://codereview.chromium.org/23684041/diff/679001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/679001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1000: loadFlags |= FT_LOAD_COLOR; We should actually check FT_HAS_COLOR(face) and set the fMaskFormat appropriately here so that this isn't lying. Of course, FT_HAS_COLOR is only in 2.5.1 (not 2.5.0). https://codereview.chromium.org/23684041/diff/679001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/23684041/diff/679001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType_common.cpp:329: SkBitmap unscaledBitmap; This scaling code is incorrect for two reasons. First, many of the config/mask formats are incorrect, this is fixable. Second, there is no A1 blitter at all, so MONO bitmap fonts that get in here will just fail. I'm not sure how to fix this yet.
At PS13, the bitmap scaling should work acceptably in all practical cases. However, there are still at least two things still required. 1. Determination if the font is a color font in onFilterRec (FT_HAS_COLOR). This is needed for this to work with the GPU, which will not expect color bitmaps unless the ScalerContext's mask format is color bitmap. This will also greatly reduce the cache as we can zero out a number of other fields in the rec. 2. The metrics and scaling are incorrect when rotated or stretched. The bitmap scaling is done based on the width, but it needs to be based instead on the 2x2 (as well as the metrics). Also, in this new code non-bitmap metrics appear to be a bit jumpy when rotating.
On 2013/12/04 21:39:58, bungeman1 wrote: > At PS13, the bitmap scaling should work acceptably in all practical cases. > However, there are still at least two things still required. > > 1. Determination if the font is a color font in onFilterRec (FT_HAS_COLOR). This > is needed for this to work with the GPU, which will not expect color bitmaps > unless the ScalerContext's mask format is color bitmap. This will also greatly > reduce the cache as we can zero out a number of other fields in the rec. > > 2. The metrics and scaling are incorrect when rotated or stretched. The bitmap > scaling is done based on the width, but it needs to be based instead on the 2x2 > (as well as the metrics). Also, in this new code non-bitmap metrics appear to be > a bit jumpy when rotating. Lovely, thank you for the reviews here. I'll produce another round of patch as soon as the opportunity presents itself. I'll also rebase again when I do so. =)
It turns out the advances being incorrect in PS13 is actually an issue with my newer FreeType and not an issue with this patch (running the without this patch with the newer FreeType also exhibits the issue). I will investigate.
I think we've decided that this is at least ok to land at PS14. The metrics being wrong with rotated text turns out to be a pre-existing condition which I will take a look into later. There are issues with the gpu at the moment, but we've decided to take a look into the gpu code being more flexible about glyph mask formats being mixed in a scaler context. Since I've done quite a bit to this myself, I'd be interested in someone else providing some review as well at this point. https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:914: // now compute our scale factors It turns out this block of code is responsible for the odd behavior with rotated text. This is (mostly) masked by most distros patching out FreeType b0962ac34.
Do we have good gms etc. to exercise the new feature/code? https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontConfigIn... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontConfigIn... src/ports/SkFontConfigInterface_direct.cpp:333: // FcBool is_scalable; Why is this section commented out? Can we delete it? https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1398: if (false) { Can we move this to the upem check directly?
The already existing coloremoji gm does cover the color font part of this. In the future we will also want an A1 style bitmap font to check in, since I didn't have any locally installed on my Ubuntu distro (there are some available in the repo, but we should get our own). https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontConfigIn... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontConfigIn... src/ports/SkFontConfigInterface_direct.cpp:333: // FcBool is_scalable; On 2013/12/09 16:58:42, reed1 wrote: > Why is this section commented out? Can we delete it? This change is required so that bitmap fonts are not rejected outright on Linux. With this code still in place, users of this path (which is currently Skia tools and Chromium) will never see a bitmap font. This is currently like this due to the uncertainty of simply enabling these in Chrome. Seeing that the layout tests pass with this change, it seems like we might be ok to remove this. https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/23684041/diff/719001/src/ports/SkFontHost_Fre... src/ports/SkFontHost_FreeType.cpp:1398: if (false) { On 2013/12/09 16:58:42, reed1 wrote: > Can we move this to the upem check directly? Not sure if this comment is in the right place? This is debug only code for the moment (currently ifdef'ed out) which is just enough to simulate the old code path for checking that the onld and new metrics are compatible. This will be removed shortly after landing.
Addressed most of reed's comments and some of my own. We still need a test with an A1 font (which I have been testing locally). https://codereview.chromium.org/23684041/diff/739001/gm/coloremoji.cpp File gm/coloremoji.cpp (left): https://codereview.chromium.org/23684041/diff/739001/gm/coloremoji.cpp#oldcode53 gm/coloremoji.cpp:53: paint.setEmbeddedBitmapText(true); The 'embedded bitmap' flag was intended to be used to control whether embedded bitmaps in scalable fonts should be used. This could be thought of as a legibility vs artistic trade-off. This flag really shouldn't matter to bitmap only fonts, as there is no such trade-off anymore. The code in the ScalerContext has been updated to reflect this in PS15.
lgtm
Message was sent while issue was closed.
Committed patchset #15 manually as r12607.
Message was sent while issue was closed.
awesome!
Message was sent while issue was closed.
Oh, lovely! Big thanks to everyone involved, I'm really excited that this finally got upstreamed!
Message was sent while issue was closed.
On 2013/12/10 18:49:48, violets wrote: > Oh, lovely! Big thanks to everyone involved, I'm really excited that this > finally got upstreamed! Thanks all. Perhaps this will make it to the next Android release now! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
