Currently gfxPangoFontGroup font selection uses pango_itemize and that uses a PangoFcFontMap to find the faces most closely matching the CSS properties. Making the user font faces available to the pango-provided PangoFcFontMap would require adding the faces to the current global FcConfig. Switching between multiple FcConfig's to avoid fonts being used outside the document (or element) would be clumsy and likely duplicate much data. Although it would be possible to filter out foreign user font faces between the PangoFcFontMap and pango_itemize within gfxPangoFontGroup, but keeping the faces other users of fontconfig would require hacks to replace symbols in system libraries. A better approach would be to refactor the gfxPangoFontGroup font selection. Implementing a new PangoFcFontMap would be sufficient to enable user font face selection, but it may be better to replace the pango_itemize functionality currently used. Using Pango shapers requires being able to construct a PangoFont from a face. The best way to do this is to implement a PangoFcFont using a cairo_scaled_font_t constructed from the face.
We need this for downloadable fonts, but completion by August 19 is improbable.
I considered a hacky shortcut to add user fonts: add them to the global FcConfig before each call to a Pango function that might use fontconfig, and remove them again each time. It would be necessary to ensure that the caches in the PangoFcFontMap didn't provide the user fonts to other callers, possibly through vigorous use of pango_fc_font_map_cache_clear or through using a bogus family in the pango_font_description as a namespace. The only way to defeat PangoFcFontMap's coverage_hash would be to use a unique filename for each user font (or use a separate PangoFcFontMap). The need to defeat the PangoFcFontMap's caches suggests that we would be using it in a way for which it was not intended, and that we should be implementing our own font matching.
(In reply to comment #0) > Using Pango shapers requires being able to construct a PangoFont from a face. > The best way to do this is to implement a PangoFcFont using a > cairo_scaled_font_t constructed from the face. That has been done in bug 385263.
Created attachment 341049 [details] [diff] [review] font selection in Mozilla WIP 0.1.2 This is a basic implementation of a Mozilla-specific PangoFcFontMap, that makes it possible for us to do our own font selection. It doesn't make any attempt cache information to optimize speed. With this patch, memory use (Tp RSS) drops from 108 to 82 MB (1:0.76), but the cost is an increase in page load time (Tp) from 440 to 638 ms (0.69:1). This shows the potential for memory savings by choosing what to cache carefully. Roughly 7 MB of the 26 MB memory savings come from removal of the gfxPangoFontCache (bug 428425 comment 5).
Nice, Karl! Looking forward to see if we can find a happy medium here.
Created attachment 342841 [details] [diff] [review] font selection in Mozilla's PangoFcFontMap 0.4 This is a proof of concept patch. It still needs a bit of tidying up. Results from Talos on the try server: ref w/patch change Ts (ms) 1435 1463 +2% Tp (ms) 446 441 -1% Tp_RSS (MB) 110 80 -27% Reference log: w/patch:.) There is more happening in the Tp test than is indicated in the overall result here. The Tp test runs through 400 pages then re-runs through the same set of 400 pages another 9 times. The reported result is an average of some sort over all 10 runs of 400 pages (possibly with some values discarded or other magic thrown in). Without the patch, the mean page load time for the first run is 584 ms, the mean for the second run is 444 ms, and the mean over the remaining runs is 460 ms. The increase after the second run is a bit concerning but it is what would be expected from bug 453200. With this patch, the mean for the first run is 446 ms (-24%), the mean for the second run is 442 ms (~0%), and the mean over the remaining runs is 459 ms (~0%). The time taken for zooming is most related (but not equivalent) to the time for the first run, so IMO having a fast first run is more important than the weight attributed to it in Talos Tp.
(In reply to comment #6) >.) Yes, removing gfxFontconfigUtils::mAliasTable, which is not used (and the initialization of which this patch would have activated) drops Ts w/patch by about 100 ms to 1350-1360 ms (-5 to -6% wrt the reference without the patch). > Without the patch, ... > The increase after the second run is a bit concerning but it is > what would be expected from bug 453200. No, it's not bug 453200. Fixing that reduces page load times consistently across all runs by 15 - 20 ms (Tp is -5 to -6%).
Created attachment 344063 [details] [diff] [review] font selection in Mozilla's PangoFcFontMap 1.0 This includes removal of gfxFontconfigUtils::mAliasTable for startup time improvement, as well as tidying up and making things more robust. It feels like jumping through a couple of hoops to use a PangoFcFontMap, but it seems worth it to avoid risks associated with replacing pango_itemize at this stage.
+ nsTArray<FontSetByLangEntry> mFontSets; This usually contains one element, right? You probably should make it nsAutoTArray<FontSetByLangEntry,1>. nsStringArray looks a lot worse than nsTArray<nsString>. I filed bug 461047 on that. + // mLangStrings owns the memory for keys to mBestLangSupportTable and + // mFontsByLang. The language support and the fonts for each lang are in + // separate tables because mFontsByLang is expected to be used only when + // no default fonts support the language. There would be a large number + // of fonts in entries for languages using Latin script but these do not + // need to be created because default fonts already support these + // languages. As we discussed, let's combine the tables into one and just allow the font list to be missing from some entries. Seems like gfxRefPtrWrap should go into XPCOM and be split into "refcounted object" and "custom-destructor object". How about nsRefObj/nsAutoObj? (The template parameter doesn't need to be a pointer, after all --- nsAutoObj would be useful for HDCs, for example). + class familyComparator { + class langComparator : familyComparator { FamilyComparator, LangComparator + if (existingFamilies.Contains(family, familyComparator())) { + continue; + } Doesn't this get a bit slow? Maybe we should make existingFamilies a hash table? + for (PRUint32 l; l < findLangMatch.Length(); ++l) { + for (PRUint32 l; l < findLangMatch.Length(); ++l) { Don't call variables 'l', it's hard to distinguish from '1' + if(FcFontSetAdd(fontSet, font)) { "if (" (3 occurrences) + while (!mFcFontSet) { + if (mHaveFallbackFonts) + return nsnull; + + SortFallbackFonts(); + mHaveFallbackFonts = PR_TRUE; + } Why is this a 'while' and not an 'if'? + gfxRefPtrWrap<FcPattern> mSortPattern; + gfxRefPtrWrap<FcFontSet> mFcFontSet; + gfxRefPtrWrap<FcCharSet> mCharSet; + nsTArray<FontEntry> mFonts; + int mFcFontsTrimmed; + PRPackedBool mHaveFallbackFonts; Comment these please. + if (mFcFontsTrimmed == mFcFontSet->nfont) { + // finished with mFcFontSet + mFcFontSet.Reset(); + mFcFontsTrimmed = 0; Why reset to 0? That just seems confusing. + const cairo_font_options_t *options = + gdk_screen_get_font_options(gdk_screen_get_default()); If we're not compiling with system cairo, this might be a problem.
One thing that would help is a big comment somewhere explaining what the overall strategy is.
(In reply to comment #9) > + const cairo_font_options_t *options = > + gdk_screen_get_font_options(gdk_screen_get_default()); > > If we're not compiling with system cairo, this might be a problem. Good catch. Filed bug 462798 on that. (It is not new in the patch here.)
Created attachment 346004 [details] [diff] [review] font selection in Mozilla's PangoFcFontMap 1.2 Addresses the issues in comment 9 and comment 10 (except for bug 462798).
Created attachment 346344 [details] [diff] [review] font selection in Mozilla's PangoFcFontMap 1.2.2 Makes changes to template specializations due to changes in bug 461087, and corrects some errors that showed up in testing.
What errors?
Uninitialized variables: --- a/gfx/thebes/src/gfxPangoFonts.cpp +++ b/gfx/thebes/src/gfxPangoFonts.cpp @@ -807,1 +807,1 @@ gfxFcPangoFontSet::SortPreferredFonts() - for (PRUint32 r; r < requiredLangs.Length(); ++r) { + for (PRUint32 r = 0; r < requiredLangs.Length(); ++r) { @@ -826,1 +826,1 @@ gfxFcPangoFontSet::SortPreferredFonts() - for (PRUint32 r; r < requiredLangs.Length(); ++r) { + for (PRUint32 r = 0; r < requiredLangs.Length(); ++r) { Wrong test: --- a/gfx/thebes/src/gfxFontconfigUtils.h +++ b/gfx/thebes/src/gfxFontconfigUtils.h @@ -213,1 +213,1 @@ public: - PRBool IsKeyInitialized() { return mKey.IsVoid(); } + PRBool IsKeyInitialized() { return !mKey.IsVoid(); } The aForce parameter to UpdateFontListInternal forces a check for changes now, but that doesn't guarantee invalidation of caches (unless a change is detected). --- a/gfx/thebes/src/gfxFontconfigUtils.cpp +++ b/gfx/thebes/src/gfxFontconfigUtils.cpp @@ -923,6 +923,7 @@ gfxFontconfigUtils::GetLangSupportEntry( // entry->mSupport needs to be recalculated, but this is an // indication that the set of installed fonts has changed, so // update all caches. + mLastConfig = NULL; // invalidates caches UpdateFontListInternal(PR_TRUE); return GetLangSupportEntry(aLang, aWithFonts); }
Created attachment 346567 [details] [diff] [review] avoid static object Our leak test tools don't seem to like (non-POD?) static objects (even with function scope).
Created attachment 346569 [details] [diff] [review] avoid static object v2 Putting the smaller object after the larger hash tables.
Comment on attachment 346569 [details] [diff] [review] avoid static object v2 Sorry, I should have caught that. Thanks. Get back in the landing queue.
Created attachment 346583 [details] [diff] [review] including all changes in one patch Including all changes in one patch. The only new change here is removal of the "static" keyword from the typedef (which some compilers didn't seem to mind/notice). --- a/gfx/thebes/src/gfxPangoFonts.cpp +++ b/gfx/thebes/src/gfxPangoFonts.cpp @@ -601,8 +601,6 @@ private: PRPackedBool mHaveFallbackFonts; }; - -static typedef FcBool (*FcPatternRemoveFunction)(FcPattern *p, const char *object, int id);
Landed this but had to back out due to: REFTEST TEST-PASS | | REFTEST TEST-KNOWN-FAIL | | REFTEST TEST-KNOWN-FAIL | | REFTEST TEST-PASS | | ../../objdir/dist/bin/run-mozilla.sh: line 131: 6372 Segmentation fault "$prog" ${1+"$@"} program finished with exit code 139 TinderboxPrint: reftest<br/>176/0/10 Haven't yet succeeded in reproducing.
Try it under Valgrind.
(In reply to comment #21) Thanks. Valgrind found the uninitialized variables in comment 15, but it hasn't reported a problem with the bidi test, with debug and optimized builds.
Created attachment 346830 [details] [diff] [review] what eventually landed Thanks, Vlad for your help. Two more uninitialized variables: --- a/gfx/thebes/src/gfxPangoFonts.cpp +++ b/gfx/thebes/src/gfxPangoFonts.cpp @@ -872,7 +872,7 @@ gfxFcPangoFontSet::SortPreferredFonts() if (truncateMarker != NULL && fontSet) { nsAutoRef<FcFontSet> truncatedSet(FcFontSetCreate()); - for (int f; f < fontSet->nfont; ++f) { + for (int f = 0; f < fontSet->nfont; ++f) { FcPattern *font = fontSet->fonts[f]; if (font == truncateMarker) break; @@ -1127,7 +1127,7 @@ gfx_pango_fontset_get_font(PangoFontset { gfxPangoFontset *self = GFX_PANGO_FONTSET(fontset); - PangoFont *result; + PangoFont *result = NULL; FcPattern *baseFontPattern = NULL; if (self->mBaseFont) {
Created attachment 352261 [details] 1.9.1 memory use (Wanted somewhere to host this image, as I haven't worked out how to prevent blogger from scaling images for me.)Created attachment 352261 [details] 1.9.1 memory use (Wanted somewhere to host this image, as I haven't worked out how to prevent blogger from scaling images for me.)