Closed
Bug 597147
Opened 14 years ago
Closed 14 years ago
switch from pango_itemize to gfxScriptItemizer
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: karlt, Assigned: karlt)
References
Details
Attachments
(19 files, 3 obsolete files)
1.04 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
21.60 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
5.39 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
12.74 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
19.45 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
12.34 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
8.04 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
8.63 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
5.23 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
9.02 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
34.71 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
17.08 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
2.49 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
12.14 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
7.14 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
To use harfbuzz shapers (Bug 569770), it will make sense to create gfxFonts directly rather than needing to create PangoFonts. This means using gfxScriptItemizer instead of pango_itemize, which is preferable anyway because pango_itemize unnecessarily performs the bidi resolution that has already been done. Pango shapers still need to be used for scripts that are not supported by HarfBuzz, and so implementation requires refactoring much of gfxPangoFonts so that a PangoFont can be constructed from a gfxFont instead of the current situation where a gfxFont is always constructed from a PangoFont. Without pango_itemize, we'll need our own code to select the appropriate Pango shaper (when using Pango).
Assignee | ||
Comment 1•14 years ago
|
||
some initial clean-up
Assignee | ||
Comment 2•14 years ago
|
||
gfxPangoFontGroup::CreateGlyphRunsFast hasn't needed a PangoFcFont for some time.
Attachment #475970 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #475963 -
Flags: review?(jfkthame)
Assignee | ||
Updated•14 years ago
|
Attachment #475970 -
Flags: review? → review?(jfkthame)
Assignee | ||
Comment 3•14 years ago
|
||
By the end of this patch series gfxFcFontSet won't know about Pango at all.
Attachment #475984 -
Flags: review?(jfkthame)
Assignee | ||
Comment 4•14 years ago
|
||
nextCh is unused and script is useful for font-selection. gfxPangoFontGroup::FindFontForChar() is based on a merge of gfxFontGroup::FindFontForChar() with the existing behavior from gfxPangoFontset and itemize_state_process_run() in pango-context.c. Font selection is still mostly done in gfxFcFontSet. I expected this to be a little slower than Pango's itemize_state_process_run() because that caches the font found for each character, but that's not showing up in talos results. gfxFcFontSet does compress the list of system fonts into a list of useful fonts, so only a subset of fonts need to be checked for fallback. Results with the full patch series at http://hg.mozilla.org/try/rev/a82885bbf4ad http://services.forerunnerdesigns.com/compare-talos/index.html?oldRevs=a4c37e52e753&newRev=91fe44ca28e8&tests=a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs&old=5362622&new=5375843
Attachment #476000 -
Flags: review?(jfkthame)
Assignee | ||
Comment 5•14 years ago
|
||
If only we didn't have UTF16...
Attachment #476003 -
Flags: review?(jfkthame)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #476013 -
Flags: review?(jfkthame)
Assignee | ||
Comment 7•14 years ago
|
||
There are still some other signed/unsigned comparisons in this file but they are harder to avoid without adjusting the algorithm.
Assignee | ||
Updated•14 years ago
|
Attachment #476017 -
Flags: review?(jfkthame)
Assignee | ||
Comment 8•14 years ago
|
||
If HB wants the language for shaping, it should be also be useful to other shapers, at least Pango.
Attachment #476020 -
Flags: review?(jfkthame)
Assignee | ||
Comment 10•14 years ago
|
||
Applies on top of bug 597212.
Attachment #476057 -
Flags: review?(jfkthame)
Assignee | ||
Comment 11•14 years ago
|
||
Attachment #476068 -
Flags: review?(jfkthame)
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #476069 -
Flags: review?(jfkthame)
Assignee | ||
Comment 13•14 years ago
|
||
This is the mechanics of creating a PangoFont from a gfxFont, but it's not so useful yet because the gfxFont can't keep a reference to the PangoFont without creating a cycle.
Attachment #476073 -
Flags: review?(jfkthame)
Assignee | ||
Comment 14•14 years ago
|
||
Now the gfxFont can keep a reference to the PangoFont. (We can probably simplify this a little after we remove pango_itemize, but we probably still want to keep the toggle ref.)
Attachment #476077 -
Flags: review?(jfkthame)
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #476080 -
Flags: review?(jfkthame)
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #476086 -
Flags: review?(jfkthame)
Assignee | ||
Comment 17•14 years ago
|
||
This switches to using gfxFontGroup::MakeTextRun and so enables the new font-selection and shaping code added in former patches. gfxPangoFontGroup::MakeTextRun are removed as well as the code they use, including use of pango_itemize. There may be some more tidying up that can be done, but this is enough for this patch.
Attachment #476098 -
Flags: review?(jfkthame)
Comment 18•14 years ago
|
||
Karl, could you possibly publish your patches for this stuff as a mercurial queue? I think that'd make it a lot easier to review the whole collection, and track any updates along the way. Thanks!
Assignee | ||
Comment 19•14 years ago
|
||
https://hg.mozilla.org/users/ktomlinson_mozilla.com/bug597147/
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #479279 -
Flags: review?(jfkthame)
Assignee | ||
Comment 21•14 years ago
|
||
Part 18 switches to using the same PangoFontMap as GTK. A skeleton gfxPangoFontMap remains, but does not create any fonts, and so now gfxPangoFcFonts always have gfxFcFonts.
Assignee | ||
Comment 22•14 years ago
|
||
Now, that gfxPangoFcFonts are always created from gfxFcFonts, the dual ownership can be managed entirely by the gfxFcFonts.
Attachment #479280 -
Flags: review?(jfkthame)
Updated•14 years ago
|
Attachment #475963 -
Flags: review?(jfkthame) → review+
Updated•14 years ago
|
Attachment #475970 -
Flags: review?(jfkthame) → review+
Updated•14 years ago
|
Attachment #475984 -
Flags: review?(jfkthame) → review+
Comment 23•14 years ago
|
||
Comment on attachment 476000 [details] [diff] [review] part 4b: pass run script to FindFontForChar and add gfxPangoFontGroup implementation > already_AddRefed<gfxFont> >-gfxFontGroup::FindFontForChar(PRUint32 aCh, PRUint32 aPrevCh, PRUint32 aNextCh, gfxFont *aPrevMatchedFont) >+gfxFontGroup::FindFontForChar(PRUint32 aCh, PRUint32 aPrevCh, >+ PRInt32 aRunScript, gfxFont *aPrevMatchedFont) Yes, script ought to be one of the inputs we use here. (This code actually needs a more substantial re-write, but that's another matter - not for this bug.) >+ if ((category == HB_CATEGORY_CONTROL || >+ category == HB_CATEGORY_FORMAT || >+ gfxFontUtils::IsVarSelector(aCh))) >+ return nsRefPtr<gfxFont>(aPrevMatchedFont).forget(); Nit time! I know the existing code isn't very consistent on this, but IIRC the style guide calls for braces around the body of the if (...) { .... }, even in the case where it's a single statement. So as we write new code, we should try to follow that. >+ if (aCh == ' ' || >+ (gfxFontUtils::IsJoinCauser(aPrevCh) && >+ static_cast<gfxFcFont*>(aPrevMatchedFont)->GetGlyph(aCh))) >+ return nsRefPtr<gfxFont>(aPrevMatchedFont).forget(); Ditto. (And there are several more occurrences in the following code.) >+ // Pango, GLib, and HarfBuzz all happen to use the same script codes. >+ const PangoScript script = static_cast<PangoScript>(aRunScript); AFAIK, this is true, but it makes me a little uncomfortable to assume it! But I suppose there's no real risk that one or the other lib would decide to rearrange them, as they're public constants in the APIs.
Attachment #476000 -
Flags: review?(jfkthame) → review+
Comment 24•14 years ago
|
||
Comment on attachment 476003 [details] [diff] [review] part 5: move glyph run routines from FontGroup to Font >+nsresult >+gfxFcFont::InitGlyphRunFast(gfxTextRun *aTextRun, .... >+ if (NS_IS_HIGH_SURROGATE(ch) && >+ next < aLength && NS_IS_LOW_SURROGATE(aString[next])) { >+ ch = SURROGATE_TO_UCS4(ch, aString[next]); >+ ++next; >+ } If I'm following the code properly, this is redundant; the "fast" path can only be reached for runs that were stored as 8-bit text, in which case there won't ever be any surrogates in the UTF16 string this function sees. >+ GList *items = pango_itemize_with_base_dir(context, dir, utf8.get(), 0, utf8.Length(), nsnull, nsnull); While you're here, it'd be nice to wrap the over-long line. r=me, with the redundant code trimmed.
Attachment #476003 -
Flags: review?(jfkthame) → review+
Comment 25•14 years ago
|
||
Comment on attachment 476013 [details] [diff] [review] part 6: move Pango shaping out of FontGroup >+ pango_shape(text, len, aAnalysis, glyphString); >+ SetupClusterBoundaries(aTextRun, text, len, utf16Offset, aAnalysis); I hope we can ultimately get rid of the custom Pango version of SetupClusterBoundaries altogether, and rely on gfxPlatform::SetupClusterBoundaries() across all back-ends. That ought to eliminate some code and provide consistent behavior. (Not saying it should happen in this patch, as this is just a step along the path of restructuring the existing code, but I wanted to mention it while it's in my mind.)
Attachment #476013 -
Flags: review?(jfkthame) → review+
Comment 26•14 years ago
|
||
Comment on attachment 476017 [details] [diff] [review] avoid some signed/unsigned comparisons I was going to fix these in bug 602044, but it's fine to keep this in your patch series so as not to disturb following patches that touch the same file. I'll update the patch in bug 602044 as needed afterwards.
Attachment #476017 -
Flags: review?(jfkthame) → review+
Comment 27•14 years ago
|
||
Comment on attachment 476020 [details] [diff] [review] part 8: pass language to font in shaping (for use by HB and Pango) Hmmm... could we reconsider this? I'm not keen on adding the language parameter to gfxFont::InitTextRun(), as that means we're "polluting" the generic shaper API with this harfbuzz-specific opaque type. The CoreText/Uniscribe/DWrite shapers shouldn't even have to know it exists. (I know there's already some leakage of harfbuzz types - especially the blob - into more generic code, but I'd like to keep it as contained as reasonably possible.) This change would also mean that gfxFontGroup::InitTextRun is always doing the work of handling language-override and lang, even when the shaper doesn't care. If you're concerned about duplicating the language-attribute code in the harfbuzz and pango shapers, maybe just factor it out into a utility function such as gfxFont::GetEffectiveHBLanguage(), and let the shapers that care call this when they want it?
Attachment #476020 -
Flags: review?(jfkthame) → review-
Comment 28•14 years ago
|
||
Oh, I also meant to mention - I did a build with the full set of patches, and although it seemed to work well, I noticed that there seemed to be a problem with language handling; the example at http://people.mozilla.com/~jkew/feature-samples/udhr-turkish.html did not show the proper Turkish behavior. I didn't try to figure out the reason yet, but that's something we'll need to look into.
Updated•14 years ago
|
Attachment #476034 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 29•14 years ago
|
||
Comment on attachment 476000 [details] [diff] [review] part 4b: pass run script to FindFontForChar and add gfxPangoFontGroup implementation (In reply to comment #23) > >+ if ((category == HB_CATEGORY_CONTROL || > >+ category == HB_CATEGORY_FORMAT || > >+ gfxFontUtils::IsVarSelector(aCh))) > >+ return nsRefPtr<gfxFont>(aPrevMatchedFont).forget(); > > Nit time! I know the existing code isn't very consistent on this, but IIRC the > style guide calls for braces around the body of the if (...) { .... }, even in > the case where it's a single statement. So as we write new code, we should try > to follow that. Much of the code around here is written without braces for if statements with a single jump statement. With a jump statement, no subsequent statements are executed so it's very hard to make mistakes about which statements are in the if block. I find it quite nice to have jump statement treated differently to make them stand out, but this was before the style guide was updated and the style guide doesn't mention this, so I've added braces in my queue.
Attachment #476000 -
Attachment description: part 4: pass run script to FindFontForChar and add gfxPangoFontGroup implementation → part 4b: pass run script to FindFontForChar and add gfxPangoFontGroup implementation
Assignee | ||
Comment 30•14 years ago
|
||
The workaround for bug 590114 messes up the pango_language_includes_script()/pango_script_get_sample_language() language-for-script logic for fontconfig language-based font-selection. fontconfig should probably have script-based font selection, but, even if so, we would not want common-script-only runs switching from native to latin fonts. The workaround seems to be for an OpenType issue and so belongs in the shaper rather than in general font-selection and shaping code.
Attachment #483907 -
Flags: review?(jfkthame)
Assignee | ||
Comment 31•14 years ago
|
||
(In reply to comment #24) > >+nsresult > >+gfxFcFont::InitGlyphRunFast(gfxTextRun *aTextRun, > .... > >+ if (NS_IS_HIGH_SURROGATE(ch) && > >+ next < aLength && NS_IS_LOW_SURROGATE(aString[next])) { > >+ ch = SURROGATE_TO_UCS4(ch, aString[next]); > >+ ++next; > >+ } > > If I'm following the code properly, this is redundant; the "fast" path can only > be reached for runs that were stored as 8-bit text, in which case there won't > ever be any surrogates in the UTF16 string this function sees. Removed the surrogate handling in the queue. https://hg.mozilla.org/users/ktomlinson_mozilla.com/bug597147/rev/e8f0d4bf446c
Assignee | ||
Comment 32•14 years ago
|
||
Comment on attachment 476020 [details] [diff] [review] part 8: pass language to font in shaping (for use by HB and Pango) (In reply to comment #27) > Hmmm... could we reconsider this? Yes. Part 8 won't be needed and I'll update parts 15 and 16 to do this better.
Attachment #476020 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #476080 -
Flags: review?(jfkthame)
Assignee | ||
Updated•14 years ago
|
Attachment #476086 -
Flags: review?(jfkthame)
Comment 33•14 years ago
|
||
Comment on attachment 483907 [details] [diff] [review] part 4a: move latin-for-common script hack from font group to shaper OK, let's move this to the shaper. As the comment implies, it's more of a band-aid than a thoroughly-designed feature, anyhow!
Attachment #483907 -
Flags: review?(jfkthame) → review+
Comment 34•14 years ago
|
||
(In reply to comment #3) > Created attachment 475984 [details] [diff] [review] > part 3: add gfxFcFont* gfxFcFontSet::GetFontAt(i) and use it for size-adjust > measuring font r?jfkthame > > By the end of this patch series gfxFcFontSet won't know about Pango at all. BTW, how about splitting the gfxFcFont* classes out from gfxPangoFonts and putting in their own file, gfxFcFonts.[cpp,h]? Ultimately, I hope the harfbuzz backend will reach the point where we'll be able to completely migrate away from pango, but we'll still need to interact with fontconfig. That split doesn't have to be done in this bug, but I'd be happy to see it happen if you care to do it while you're here. (And then we could drop gfxPangoFonts.cpp from the build entirely in --disable-pango configurations.)
Assignee | ||
Comment 35•14 years ago
|
||
(In reply to comment #34) > > By the end of this patch series gfxFcFontSet won't know about Pango at all. > > BTW, how about splitting the gfxFcFont* classes out from gfxPangoFonts and > putting in their own file, gfxFcFonts.[cpp,h]? Ultimately, I hope the harfbuzz > backend will reach the point where we'll be able to completely migrate away > from pango, but we'll still need to interact with fontconfig. While we are still using Pango, gfxFcFont (not gfxFcFontSet) manages creating PangoFonts, so I think it still belongs in gfxPangoFonts.cpp at this stage (assuming no other backend wants this kind of fontconfig support). > That split doesn't have to be done in this bug, but I'd be happy to see it > happen if you care to do it while you're here. (And then we could drop > gfxPangoFonts.cpp from the build entirely in --disable-pango configurations.) --disable-pango configurations don't use gfxPangoFonts.cpp, only gfxFT2Fonts.cpp. When Pango is no longer needed, I'm thinking gfxPangoFonts.cpp will become gfxFcFonts.cpp.
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #27) > If you're concerned about duplicating the language-attribute code in the > harfbuzz and pango shapers, maybe just factor it out into a utility function > such as gfxFont::GetEffectiveHBLanguage(), and let the shapers that care call > this when they want it? At the time that I wrote part 8, I didn't realize we had access to the font group in gfxFont::InitTextRun. Using the font group here, the Pango shaping behavior can remain consistent with what pango_itemize was doing. That is a little different to what gfxHarfBuzzShaper is doing and GetPangoLanguage is not available to gfxHarfBuzzShaper, so no utility function is used.
Attachment #476080 -
Attachment is obsolete: true
Attachment #484219 -
Flags: review?(jfkthame)
Assignee | ||
Comment 37•14 years ago
|
||
Attachment #476086 -
Attachment is obsolete: true
Attachment #484221 -
Flags: review?(jfkthame)
Assignee | ||
Comment 38•14 years ago
|
||
(In reply to comment #28) > I noticed that there seemed to be a problem > with language handling; the example at > http://people.mozilla.com/~jkew/feature-samples/udhr-turkish.html did not show > the proper Turkish behavior. Thanks. That is addressed in bug 569770 comment 23.
Updated•14 years ago
|
Attachment #476057 -
Flags: review?(jfkthame) → review+
Comment 39•14 years ago
|
||
Comment on attachment 476068 [details] [diff] [review] part 11: Use C++ instance methods in gfxPangoFcFont Yes, this seems better style, thanks!
Attachment #476068 -
Flags: review?(jfkthame) → review+
Comment 40•14 years ago
|
||
Comment on attachment 476069 [details] [diff] [review] part 12: un-inline some of gfxPangoFcFont Likewise. :)
Attachment #476069 -
Flags: review?(jfkthame) → review+
Updated•14 years ago
|
Attachment #476073 -
Flags: review?(jfkthame) → review+
Comment 41•14 years ago
|
||
Comment on attachment 476077 [details] [diff] [review] part 14: Let gfxFcFont and gfxPangoFcFont share ownership of each other I wasn't familiar with glib's toggle references until reading through this; seems to be a good solution. One note from the "nits 'R' us" department: it'd be nice to have a blank line between methods here: >+ } >+ return font.forget(); >+} >+/* static */ >+already_AddRefed<gfxFcFont> >+gfxFcFont::GetOrMakeFont(FcPattern *aRenderPattern, gfxPangoFcFont *aPangoFont) >+{
Attachment #476077 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 42•14 years ago
|
||
Requesting blocking2.0 because it is the reasonable way to set up for bug 569770.
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 43•14 years ago
|
||
Comment on attachment 484219 [details] [diff] [review] part 15: set up PangoAnalysis for Pango shaping v2 Seems fine in general. I noticed a couple of if (...) return PR_FALSE; instances, which I suppose ought to be braced for conformity. More significantly: >+PRBool >+gfxFcFont::InitGlyphRunWithPango(gfxTextRun *aTextRun, >+ const PRUnichar *aString, >+ PRUint32 aRunStart, PRUint32 aRunLength, >+ PangoScript aScript) >+{ >+ NS_ConvertUTF16toUTF8 utf8(aString + aRunStart, aRunLength); What is the OOM behavior of NS_ConvertUTF16toUTF8? I'm not entirely confident of the answer, but it looks like it could fail silently at http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsReadableUtils.cpp#209 with the result that the string remains zero-length. That may be safe (as utf8.length() is passed to InitGlyphRunWithPangoAnalysis later, so it will simply see a zero-length string), but we won't get the required glyphs - not even unshaped. It would seem better to immediately return PR_FALSE here if the conversion fails, I think, and handle it as a shaping failure. r=me with this addressed (or confirm that it really isn't needed).
Attachment #484219 -
Flags: review?(jfkthame) → review+
Comment 44•14 years ago
|
||
Comment on attachment 484221 [details] [diff] [review] part 16: Add gfxFcFont::InitTextRun to shape with Pango v2 >+ if (useFastPath && >+ NS_SUCCEEDED(InitGlyphRunFast(aTextRun, aString, >+ aRunStart, aRunLength))) >+ return PR_TRUE; Braces. BTW, do you know what perf difference the "fast path" gives? Just curious how much it's gaining us.
Attachment #484221 -
Flags: review?(jfkthame) → review+
Comment 45•14 years ago
|
||
Comment on attachment 476098 [details] [diff] [review] part 17: switch from pango_itemize to gfxScriptItemizer Nearly 600 lines of code getting deleted here - great! :)
Attachment #476098 -
Flags: review?(jfkthame) → review+
Comment 46•14 years ago
|
||
Comment on attachment 479279 [details] [diff] [review] part 18: use GDK's default PangoFontMap when suitable >+ // Future proofing: We need a PangoFcFontMap for gfxPangoFcFont. >+ // pango_cairo_font_map_get_default() is expected to return a >+ // PangoFcFontMap on Linux systems, but, just in case this ever >+ // changes, we provide our own basic implementation. I hope we'll see harfbuzz develop to the point where we can eliminate our pango path altogether before that "future" ever arrives! But in the meantime this looks correct, yes.
Attachment #479279 -
Flags: review?(jfkthame) → review+
Comment 47•14 years ago
|
||
Comment on attachment 479280 [details] [diff] [review] part 19: simplify gfxFcFont/gfxPangoFcFont ownership a little Simpler is good, thanks! (I still have a hard time keeping track of all this, but it appears to be ok; and I ran a build with added instrumentation to check that things were being created/destroyed as expected, and it seemed to behave properly.)
Attachment #479280 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 48•14 years ago
|
||
(In reply to comment #44) > BTW, do you know what perf difference the "fast path" gives? Just curious how > much it's gaining us. Back in bug 495455 comment 15, disabling the fast path increased Tp from 224 to 230, a 2-3% increase. At that stage the "slow" path did pango_itemize as well as pango_shape, so some of that time may have been due to pango_itemize, but I would guess that reading the OpenType tables would have been significant. With the patches in this bug, the fast path doesn't save us a pango_itemize but it does save us from creating a PangoFont. Once we are using gfxHarfBuzzShaper (bug 569770), removing the fast path may make more sense. It would only be making non-SFNT shaping faster. I had in mind keeping it (moving it to gfxFT2FontBase) for use in gfxFT2Fonts once they used HarfBuzz. However, with GetGlyph provided by the font, gfxHarfBuzzShaper would work even for non-SFNT fonts.
Comment 49•14 years ago
|
||
(In reply to comment #48) > Back in bug 495455 comment 15, disabling the fast path increased Tp from 224 to > 230, a 2-3% increase. ..... Thanks for the details. Yes, I wasn't meaning that we should remove it right now, but it's something to keep in mind as we go forward - it'd be nice if we can eventually reach the point where it's no longer needed, and all text goes through the same "universal" path. But of course we don't want to regress performance for simple pages, so that depends on making sure the harfbuzz path is fast enough.
Assignee | ||
Comment 50•14 years ago
|
||
(In reply to comment #43) > >+PRBool > >+gfxFcFont::InitGlyphRunWithPango(gfxTextRun *aTextRun, > >+ const PRUnichar *aString, > >+ PRUint32 aRunStart, PRUint32 aRunLength, > >+ PangoScript aScript) > >+{ > >+ NS_ConvertUTF16toUTF8 utf8(aString + aRunStart, aRunLength); > > What is the OOM behavior of NS_ConvertUTF16toUTF8? I'm not entirely confident > of the answer, but it looks like it could fail silently at > http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsReadableUtils.cpp#209 > with the result that the string remains zero-length. Yes, I think you are right. nsStringBuffer uses realloc and malloc, and I don't know that we can assume these infallible. > That may be safe (as utf8.length() is passed to > InitGlyphRunWithPangoAnalysis later, so it will simply see a zero-length > string), but we won't get the required glyphs - not even unshaped. Yes, and gfxTextRun::Create has initialized the CompressedGlyphs to missing glyphs with zero glyph count and so no glyphs will be drawn. > It would seem better to immediately return PR_FALSE here if the > conversion fails, I think, and handle it as a shaping failure. Returning PR_FALSE would attempt to create proper missing glyphs (for hex boxes). Perhaps that would be nice, but it would need to at least allocate a DetailedGlyph for each character. That allocation is performed with infallible new, so the program would most likely abort. The zero-glyph-count missing CompressedGlyphs actually seems the best way to limp through here, so I'd prefer not to add code to detect OOM.
Assignee | ||
Comment 51•14 years ago
|
||
Pushed parts 1 to 12 (with no part 8): http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d1649deec025&tochange=85b93f3ea9d1
Assignee | ||
Comment 52•14 years ago
|
||
and remaining parts 13 to 19 (with braces added) http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8811f3dce530&tochange=0da762e2fcdb
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in
before you can comment on or make changes to this bug.
Description
•