switch from pango_itemize to gfxScriptItemizer

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla2.0b8
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(19 attachments, 3 obsolete attachments)

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
(Assignee)

Description

8 years ago
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

8 years ago
Created attachment 475963 [details] [diff] [review]
remove unused WhichFontSupportsChar

some initial clean-up
(Assignee)

Comment 2

8 years ago
Created attachment 475970 [details] [diff] [review]
part 2: rename gfxFcPangoFontSet to gfxFcFontSet, add GetBaseFont to re turn a gfxFcFont

gfxPangoFontGroup::CreateGlyphRunsFast hasn't needed a PangoFcFont for some time.
Attachment #475970 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #475963 - Flags: review?(jfkthame)
(Assignee)

Updated

8 years ago
Attachment #475970 - Flags: review? → review?(jfkthame)
(Assignee)

Comment 3

8 years ago
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.
Attachment #475984 - Flags: review?(jfkthame)
(Assignee)

Comment 4

8 years ago
Created attachment 476000 [details] [diff] [review]
part 4b: pass run script to FindFontForChar and add gfxPangoFontGroup implementation

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

8 years ago
Created attachment 476003 [details] [diff] [review]
part 5: move glyph run routines from FontGroup to Font

If only we didn't have UTF16...
Attachment #476003 - Flags: review?(jfkthame)
(Assignee)

Comment 6

8 years ago
Created attachment 476013 [details] [diff] [review]
part 6: move Pango shaping out of FontGroup
Attachment #476013 - Flags: review?(jfkthame)
(Assignee)

Comment 7

8 years ago
Created attachment 476017 [details] [diff] [review]
avoid some signed/unsigned comparisons

There are still some other signed/unsigned comparisons in this file but they are harder to avoid without adjusting the algorithm.
(Assignee)

Updated

8 years ago
Attachment #476017 - Flags: review?(jfkthame)
(Assignee)

Comment 8

8 years ago
Created attachment 476020 [details] [diff] [review]
part 8: pass language to font in shaping (for use by HB and Pango)

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 9

8 years ago
Created attachment 476034 [details] [diff] [review]
part 9: make language GetSampleLangForGroup parameter an atom

cleanup
Attachment #476034 - Flags: review?(jfkthame)
(Assignee)

Updated

8 years ago
Depends on: 597212
(Assignee)

Comment 10

8 years ago
Created attachment 476057 [details] [diff] [review]
part 10: make font group language a real language for font shaping

Applies on top of bug 597212.
Attachment #476057 - Flags: review?(jfkthame)
(Assignee)

Comment 11

8 years ago
Created attachment 476068 [details] [diff] [review]
part 11: Use C++ instance methods in gfxPangoFcFont
Attachment #476068 - Flags: review?(jfkthame)
(Assignee)

Comment 12

8 years ago
Created attachment 476069 [details] [diff] [review]
part 12: un-inline some of gfxPangoFcFont
Attachment #476069 - Flags: review?(jfkthame)
(Assignee)

Comment 13

8 years ago
Created attachment 476073 [details] [diff] [review]
part 13: Add a factory method to create a gfxPangoFcFont from gfxFcFont

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

8 years ago
Created attachment 476077 [details] [diff] [review]
part 14: Let gfxFcFont and gfxPangoFcFont share ownership of each other

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

8 years ago
Created attachment 476080 [details] [diff] [review]
part 15: set up PangoAnalysis for Pango shaping
Attachment #476080 - Flags: review?(jfkthame)
(Assignee)

Comment 16

8 years ago
Created attachment 476086 [details] [diff] [review]
part 16: Add gfxFcFont::InitTextRun to shape with Pango
Attachment #476086 - Flags: review?(jfkthame)
(Assignee)

Comment 17

8 years ago
Created attachment 476098 [details] [diff] [review]
part 17: switch from pango_itemize to gfxScriptItemizer

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)
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 20

8 years ago
Created attachment 479279 [details] [diff] [review]
part 18: use GDK's default PangoFontMap when suitable
Attachment #479279 - Flags: review?(jfkthame)
(Assignee)

Comment 21

8 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

8 years ago
Created attachment 479280 [details] [diff] [review]
part 19: simplify gfxFcFont/gfxPangoFcFont ownership a little

Now, that gfxPangoFcFonts are always created from gfxFcFonts, the dual
ownership can be managed entirely by the gfxFcFonts.
Attachment #479280 - Flags: review?(jfkthame)
Attachment #475963 - Flags: review?(jfkthame) → review+
Attachment #475970 - Flags: review?(jfkthame) → review+
Attachment #475984 - Flags: review?(jfkthame) → review+
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 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 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 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 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-
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.
Attachment #476034 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 29

8 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

8 years ago
Created attachment 483907 [details] [diff] [review]
part 4a: move latin-for-common script hack from font group to shaper

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

8 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

8 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

8 years ago
Attachment #476080 - Flags: review?(jfkthame)
(Assignee)

Updated

8 years ago
Attachment #476086 - Flags: review?(jfkthame)
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+
(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

8 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

8 years ago
Created attachment 484219 [details] [diff] [review]
part 15: set up PangoAnalysis for Pango shaping v2

(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

8 years ago
Created attachment 484221 [details] [diff] [review]
part 16: Add gfxFcFont::InitTextRun to shape with Pango v2
Attachment #476086 - Attachment is obsolete: true
Attachment #484221 - Flags: review?(jfkthame)
(Assignee)

Comment 38

8 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.
Attachment #476057 - Flags: review?(jfkthame) → review+
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 on attachment 476069 [details] [diff] [review]
part 12: un-inline some of gfxPangoFcFont

Likewise. :)
Attachment #476069 - Flags: review?(jfkthame) → review+
Attachment #476073 - Flags: review?(jfkthame) → review+
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

8 years ago
Requesting blocking2.0 because it is the reasonable way to set up for bug 569770.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
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 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 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 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 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

8 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.
(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

8 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 52

8 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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Depends on: 611254
(Assignee)

Updated

8 years ago
Depends on: 614476
(Assignee)

Updated

8 years ago
Blocks: 427672
You need to log in before you can comment on or make changes to this bug.