121540 - (atsui) Use ATSUI for text rendering on Mac OS X

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect, P3)

Description

[Simon Fraser [no longer active]]

ATSUI is supposedly almost as fast, if not faster, than QuickDraw when rendering
text now on Mac OS X. It also looks much prettier; we should switch to it.

Comment 1

[Simon Fraser [no longer active]]

I hacked up ATSUI text rendering for grins. Screenshots coming.

Comment 2

[Simon Fraser [no longer active]]

Attachment: QuickDraw text rendering

Comment 3

[Simon Fraser [no longer active]]

Attachment: ATSUI (with Core Graphics) text rendering

Comment 4

[Simon Fraser [no longer active]]

The trick to getting the pretty core graphics-style rendering with ATSUI, from a
Carbon app, is to assign a CGContextRef to the ATSUTextLayout object:

  CGContextRef    cgContext;
  CreateCGContextForPort(curPort, &cgContext);

...

  ByteCount             iSize = sizeof(CGContextRef);
  ATSUAttributeTag      iTag = kATSUCGContextTag;
  ATSUAttributeValuePtr iValuePtr = &cgContext;
  ATSUSetLayoutControls(theLayout, 1, &iTag, &iSize, &iValuePtr);

Only then does it draw nicely.

Comment 5

[Pierre Saslawsky]

Before it gets lost, here is Simon's response to my question...
----
> Should we open a bug to clear NS_RENDERING_HINT_FAST_8BIT_TEXT on 10.1? When
> that flag is set, we use QuickDraw instead of ATSUI.  It could be a 8%
> performance win.
> http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsRenderingContextMac.cpp#555

Unfortunately, turning on ATSUI font rendering is not easy.

Currently, ATSUI is used only as a fallback drawing mechanism, and, as such,
only gets called in a codepath that draws one character at a time. There
is an awful lot of logic relating to finding special fonts that can render
certain unicode characters that would need review.

A second problem is that layout draws this bizarre mixture of 1- and 2-byte
text, and the ATSUI font rendering is only hooked up for 2-byte rendering.
1-byte rendering is still called even when NS_RENDERING_HINT_FAST_8BIT_TEXT
is turned off.

Another thing that is required to get pretty CG-style font rendering is
a call to CreateCGContextForPort() somewhere (once per port), and then
a storage of the resulting CGContextRef such that it can be passed to 
an ATSUSetLayoutControls call.

I managed to hack some of this up to get layout to us ATSUI here and there.
It's pretty, but very broken.
----

Comment 6

[Pierre Saslawsky]

I guess that in some instances it's ok to call 1-byte rendering when 
FAST_8BIT_TEXT is turned off: we cannot blame Layout to call DrawString(char*) 
when it receives itself a |char*| as parameter.  The only thing the flag says is 
"Would you please convert your unicode strings to char* and call 1-byte rendering 
whenever possible?" but it's not a guarantee that 1-byte rendering will never be 
called when the flag is off. So...

1) Marc should make sure that we never convert unicode strings to char* when the 
flag is turned off, and that's about it.  The code looks fine to me, 
PaintAsciiText() is called only if the flag is off and I don't think we use 
DrawString(char*) anywhere else.

2) On the Mac side, we should change nsRenderingContextMac::DrawString(char*) and 
revamp the unicode rendering toolkit to always use ATSUI on 10.1.  We might be 
able to use the previous version of the toolkit which, IIRC, was doing a much 
more extensive use of ATSUI.

Marc, please reassign to Franck when you are done.  It should be just a quick 
verification on your side.

Comment 7

[Mike Pinkerton (not reading bugmail)]

I'm confused. The data we got showed that ATSUI with _quickdraw_ rendering was
fastest, with ATSUI with quartz rendering at the bottom. Why all the sudden
effort to toss in quartz (besides the aesthetics and it's what apple wants)?

Isn't this the same as the work that's already going on in bug 74245? cc'ing
ajfeldman@brown since he's been doing a lot of hacking here already.

Comment 8

[Ari]

This is the exact same issue as bug 74245. In the work on that bug, the original
idea was to use quartz functions directly ( e.g. CGContextShowTextAtPoint()
which takes a char *) to draw 1-byte text. That approach doesn't really work
because the quartz fuctions are very limited and it is hard to reliably set the
font and get the bounds of the text. Also, the Apple people on the Carbon-Dev
list have repeatedly recommended againbst using quartz fuctions directly.

The only other way to render text with quartz is to use ATSUI with the code in
comment 4 and attachment 62926 [details] [diff] [review] or to use functions like TXNDrawCFStringTextBox()
which use ATSUI indirectly.

I very much doubt that using ATSUI with quartz will be faster than the way we
currently draw text. In fact, I'm almost sure it will be slower because of all
of the anti-aliasing. But, I think our goal should be to use quartz for text in
order to make moz look as good as the rest of the OS especially since we use
Gecko to render the UI, not just web page content.

As was stated earlier, if we use ATSUI for all text drawing (I think we should),
we would need to convert all 1-byte text to unicode before rendering it. Also,
we would need to sack massive quantities of speciallized code and fallbacks in
nsUnicodeRenderingToolkit that is designed to convert unicode text to 1-byte
text before drawing it.

Comment 9

[Simon Fraser [no longer active]]

OK. I was misreading the "ATSUI is faster" data. Since ATSUI/QuickDraw looks 
exactly the same as QuickDraw text, I don't see any reason to start using it 
other than the ease of turning on CG rendering on OS X (which, apparently, is 
much slower), and maybe some minor performance benefit (since we can drop a lot 
of the ConvertFromUnicodeToText() calls when drawing Unicode, but will have to 
add some conversion to Unicode when drawing 1-byte text).

Should we just close this as a dup of bug 74245? Or should we continue to move 
towards ATSUI for OS 9, so that we can enable CG rendering on X (which is covered 
by that bug)?

Comment 10

[Simon Fraser [no longer active]]

Attachment: Hacking in gfx to use ATSUI. Just attaching this to not lose the code. Don't apply this, it's a big mess.

Comment 11

[Kevin McCluskey (gone)]

Reassigning to Don.

Comment 12

[Simon Fraser [no longer active]]

Attachment: Better patch which uses ATSUI everywhere, and works

Comment 13

[Steve Dagley]

Cool!, but did you mean to say 'sorta' works?  http://www.mozilla.org/start/
seems to have a weird layout bug with this patch - the 2nd sentence on the Fix
Bugs para is repeated as the 3rd sentence (and the expected 3rd sentence doesn't
draw).  I tried a few other pages and didn't notice any erroneously replicated
lines but I do see a lot of text collision/overwrite where it look like the
measurement is slightly off.

And a note for folks that want to play with this patch - you'll need to add the
InterfacesXStubs library to the Carbon targets of the _gfxComponent.mcp project file

Comment 14

[Steve Dagley]

After viewing a lot more pages with this patch it looks really good but we
actually are getting lots of text erroneously repeated.  Popup menus are
especially good for showing this.  And there seems to be some clipping issues as
the URL in the location field was writing across the window when running the
page load tests. 

Speaking of the page load tests, on a debug Fizzilla build we're taking about an
18% performance hit which is about in line with what Apple mentioned, right?

Comment 15

[David Hyatt]

I tried this out on Chimera, and yay, it's fast!  I don't even notice a 
perceptible performance degradation in scrolling or page load in my opt 
build (I assume there is one, but it can't be too bad).

However, there are still a lot of glitches with formatting and spacing, and 
like sdagley I saw doubled text on several pages.  

This is a very promising start though!  Woo hoo!

Comment 16

[Simon Fraser [no longer active]]

I saw the repeated lines as well; I'll debug it. Pageload times:
Before the patch: 750ms
With the patch:  1015ms

Comment 17

[Simon Fraser [no longer active]]

Taking.

Comment 18

[Simon Fraser [no longer active]]

Attachment: Better patch

Comment 19

[Ari]

nsATSUIUtils::gTxLayoutCache->Set(aFontNum, aSize, aBold, aItalic, aColor, 
txLayout);

Would it be more efficient to cache ATSUStyles instead of ATSUTextLayouts? As
far as I understand ATSUI, ATSUTextLayouts contain both text and style
information. Whereas it is unlikely that repeated calls to DrawString() will be
made with the same exact text, it is likely that DrawString will be called
multiple times with the same style but different text.

See http://developer.apple.com/qa/qa2001/qa1027.html

Comment 20

[David Hyatt]

Measurements:

(1) Full blown ATSUI Quartz - 1900
(2) Full blown ATSUI Quickdraw - 1800
(3) Hacked ATSUI Quartz that uses old QD to measure widths but still 
uses ATSUI to draw - 1300
(4) No ATSUI just QD - 1100

This data clearly demonstrates that measurement of text, specifically 
measurement of ASCII text, is the real killer.  

I have an optimization that I made on Win32 that I think can be applied 
here (and that will allow us to stay at about 1300).  

The idea is simple: use ATSUI to measure individual ASCII characters, 
caching the widths of the chars for a given ATSUI style.  Basically in the 
textlayout cache also cache a 256 byte array of ASCII character widths for 
that style.  Always measure ASCII text character by character yourself 
(avoiding calls to ATSUI's MeasureText routine unless you fault).

Comment 21

[Ari]

Might the fact that we are dealing entirely in unicode adversely affect that
kind of optimization. In order to cache the widths of ascii characters, wouldn't
we need to convert to ascii from unicode fisrt? Also, wouldn't this optimization
have no effect when rendering non-roman text?

Comment 22

[Dave Walker]

I applied sfraser's latest patch (714080 and did an opt build of FizillaMach
with a clean tree, but got reversed/upside down (but very nicely antialiased, I
might add) text.

Comment 23

[David Hyatt]

ajfeldman, the idea is that you'll get a GetWidth call with an ASCII string.  
You loop over the chars of the string and look them up in a cached array for 
that particular text style.  If the value is present in the array you add it 
to the total width.  If the value is not present, you convert the single 
character to unicode, call MeasureText on it, and cache the resulting width in 
your array so that for that character and style you never use ATSUI again.

The basic problem here is that ATSUI is slow, and a principal optimization can 
be applied to ASCII (and has been in other gfx impls) to yield a significant 
speedup.  Keep in mind that even the full-blown sluggish path is no slower 
than IE5.1 on the Mac. 

Comment 24

[David Hyatt]

The current implementation already inflates the whole string to unicode 
anyway, so this will actually do better for the ASCII text route.

Comment 25

[David Hyatt]

(By current implementation, I mean the latest patch in the bug.)

Comment 26

[Mike Pinkerton (not reading bugmail)]

i guess this approach assumes that quartz/atsui never kerns adjacent letters?

Comment 27

[Ari]

Good point. The optimization would probably preclude doing bug 105800.

Comment 28

[Simon Fraser [no longer active]]

Re: caching of ATSUStyles. Yes, we should cache these, and not the text layout 
objects. This is just the way that the i18n code was written.
Re: upside-down text. Yes, the last patch erroneously inverts all the text (since 
it flips the CG coordinate space). I'll get another patch up soonish.

Comment 29

[Greg K.]

Am I wrong in thinking that this sort of caching would help ATSUI systemwide?

Comment 30

[Simon Fraser [no longer active]]

ATSUI already does a lot of caching internally.

Comment 31

[David Hyatt]

Simon, I stumbled across this tech note.  I don't know if it's helpful or not,
but here you go.

http://developer.apple.com/technotes/tn/tn2033.html

Comment 32

[Bill McGonigle (not currently reading bugmail; please contact directly)]

[hmmm, comment didn't show last time I hit commit - trying again]

Perhaps there should be a user preference, Type Quality: [LOW,MEDIUM,HIGH].
Low would be QuickDraw, like today, for people who have to have the fastest
render or don't like anti-aliased text.
Medium would be the proposed cacheing scheme
High would sent whole words (or more?) to ATSUI, for full-quality rendering.

High would give you font beauty items like connected cursive, for Mozilla's
cursive typeface, pretty ligatures, and maybe even some of the interpretation
stuff (-- to m-dash, etc.)

Besides the beauty aspect, though, there may be a real need for the High method
for some non-english languages.  In some languages (Arabic, Hindi, some
Scandanavian?) ligatures are non-optional and semantic, such that 'A' + 'B' +
'C' != 'ABC', which is the assumption underlying the cacheing scheme.  I'm sure
there are some work-arounds for these meanings using current browser-tech, but
I'm sure that third of the world would appreciate seeing their language drawn
properly.

Thanks for the work on this one.  This is the best kind of eye candy - that with
a functional improvement, and lord knows plenty of users are drawn to eye candy
like a moth.

Just curious, what are the units in comment #20?  On this 2-year old G3, I can't
see the text render with QuickDraw.  If it took 30% longer than that few would
complain.  If I'm on the modem, the pipelined speed hit would be immeasurable.

Comment 33

[Greg K.]

See also Bug 112436 on the topic of ATSUI.

Comment 34

[Simon Fraser [no longer active]]

Pull back to 1.0

Comment 35

[Simon Fraser [no longer active]]

Attachment: New GFX patch, includes nsIRenderingContext API changes

This is another, not-quite-so hacky patch for ATSUI text rendering. It does
caching of ASCII character widths, so is reasonably fast.

This patch does not contain some gfx build changes for Macho. You'll need to
remove nsATSUIUtils.cpp and nsUnicodeRenderingToolkit.cpp from the makefile,
add nsATSUStyleCache.cpp and nsUnicodeTextRendering.cpp, and #define
UNICODE_FONT_RENDERING for all of mac gfx. You'll also need the layout patch
that's coming up next.

Comment 36

[Simon Fraser [no longer active]]

Attachment: Layout changes to sync with nsIRenderingContext.h changes

The nsIRenderingContext API changes are not strictly necessary any more, but I
don't have a patch without them to hand. So, for now, you'll need this patch.

Note that with this and the last patch, some things are still broken:
* Clipping -- ATSUI text is not clipped correctly
* i18n -- no font fallback stuff is done yet.

Comment 37

[Simon Fraser [no longer active]]

Note that for Macho, you'll need to add defined(XP_MACOSX) to the code in
nsTextFrame.cpp

Comment 38

[Simon Fraser [no longer active]]

Attachment: gfx patch;

Comment 39

[Simon Fraser [no longer active]]

Attachment: layout patch

This and the previous patch are a usable implementation of ATSUI (in macho
builds). The implementation now allows switching text renderers on the fly. For
ATSUI, I added some very basic font fallback logic, and clipping now works.

If you apply this patch to the tip, you'll probably get conflicts, since ftang
touched some of the font code.

Also, in a CFM build, I'm seeing ugly darkened text sometimes, which goes away
if I fix the clipping stuff. I'm also seeing some mistplaced caret issues in a
Macho build.

Comment 40

[Carlo]

Is there a build somewhere (with ATSUI turned on) we could test ?

Comment 41

[Simon Fraser [no longer active]]

Off to 1.1

Comment 42

[Simon Fraser [no longer active]]

Attachment: gfx patch with new files

Comment 43

[Marko Karppinen]

I hate the idea of the three-level type quality preference. In fact, I don't think 
that there should be a preference at all -- we have enough.

Unfortunately I'm afraid that the caching efforts are futile. Seems like most 
people here want ATSUI just for the non-grid-fitting antialiasing method, 
but IMHO the goal should be typographic accuracy, not just smooth 
letterforms.

Getting there implies, at the very least, caching character pair widths 
instead of the widths of single characters. Implementing this for [A-Za-z0-
9][A-Za-z0-9] means a cache of 3844 entries. It also means that you need 
a way to reduce accented characters to the base letter when fetching the 
width from the cache. And two- or three-letter ligatures will still be out of 
the picture.

I have my doubts on whether this can be done in Mozilla more effectively 
than in ATSUI itself, but if someone comes up with a fast solution, I'd 
prefer that they submitted the patch to Apple instead of Mozilla.

Mac OS X is immature. This is why I don't think that developers should be 
spending much effort writing these kinds of hacks to speed up its APIs. If 
the ATSUI implementation is demonstrably suboptimal, it is likely to get 
better in a future release.

Finally, I'd like to finish this rant by saying that I've yet to hear a crumble 
about slow rendering from the OmniWeb fan club.

Comment 44

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Does OmniWeb use ATSUI for it's text rendering?  I'm not saying it doesn't but
OmniWeb is NeXT-derived and ATSUI is OS9-derived. If it does, I agree it's
plenty fast.

But if ATSUI is killing Mozilla's speed, telling users to wait for a future OS
to get decent rendering speed isn't going to fly.  With a preference, users
could choose to take the speed hit if they wanted to.  Slow can't be the default.

Comment 45

[Greg K.]

The way to offer options is at the OS level, not application. There are already
utilities available to disable Aqua features such as antialiasing.

Comment 46

[Paul Libbrecht]

This bug, or the usage of Quartz font rendering (isn't this kind of similar?),
looks to me like the perfect thing that would solve the famous bug that prevents
MathML to run on MacOS(X):

http://bugzilla.mozilla.org/show_bug.cgi?id=74821
(Implement GetBoundingMetrics() on the Mac).

And, as I understand this, it would also ease up:Bug 107146 - ucvmath not
working with Fizzilla (http://bugzilla.mozilla.org/show_bug.cgi?id=107146).

Please help, Macs are the only platform left in Mozilla that doesn't have a
working MathML as far as I know !! (although everyone believes it has it as is
in the main branch)
Should I make a blocker ?
I am not expert enough to judge that, indeed, GetBoundingMetrics() would be
solved by ATSUI.

Comment 47

[Simon Fraser [no longer active]]

Attachment: Updated gfx patch

Comment 48

[Simon Fraser [no longer active]]

Attachment: Patch to nsTextFrame.cpp

Comment 49

[Steve K]

Simon:  FYI:

I'm trying to build your latest patches under OSX (mach-o), and I've found that
it gives many "jump crosses initializer" warnings (see
http://www.ucmb.ulb.ac.be/documents/libg++_2.7.1/g++FAQ_26.html).  I think
they're all because the jumps to "bail" skip over intializers for variables used
locally.

I was able to make the errors disappear by the prudent use of curly braces (in
two cases, I just moved the intializer up a couple of lines before the jump).

It seems, of course, that these cases are all benign, but I'm not sure if you
can make g++ ignore them, or what mozilla standards require, etc.  Just a
heads-up.  I'll probably post again if I get the build working, with my experiences.

Comment 50

[Henri Sivonen (:hsivonen)]

> Why all the sudden effort to toss in quartz (besides the aesthetics 
> and it's what apple wants)?

Because aesthetics is important. (It's one of the reasons why we use Macs,
right?) According to the OmniWeb hype, OmniWeb renders Web pages more
beautifully than anything else--and in reality, that perception is all about
font rendering, since OmniWeb lacks the capability to do CSS box layout right.

> Does OmniWeb use ATSUI for it's text rendering?

I believe OmniWeb uses the the same back-end functionality via NSText.

Comment 51

[Steve K]

Based on the utter coolness of this bug,  I made a mach-o opt build of this
available to some eager n.p.m.macosx newsgroup readers.  The build was made with
patches from attachments 76677, 76678, and changes I mentioned in comment #49.  

This build is available from http://homepage.mac.com/stevekstevek/ for people
interested.

I'll attach a diff between head+those patches, and what I built with.  It isn't
pretty, as I'm probably defining things in the wrong Makefiles, and the
butchering I did to nsUnicodeTextRenderer.cpp was just done to shut gcc up so it
would ignore the fact that simple initializations of variables that were unused
in error cases were jumped over by goto bail; statements.

Comment 52

[Steve K]

Attachment: Butchering I did to make Simon's patches build under gcc

Comment 53

[Greg K.]

Attachment: Unicode testcase for this bug's patch.

This testcase demonstrates a problem with the patch for this bug. It contains
some ASCII text as well as some Cyrillic characters.

The Cyrillic characters display properly in legacy Fizzilla builds, but fail on
experimental Fizzilla and Chimera (0.2) builds containing this bug's ATSUI
patch.

Comment 54

[Chris Casciano]

Based off stevek's build I noticed that the CSS property for letter-spacing is
being ignored. See the top right corner of http://placenamehere.com/contents.html

The line "Chris Casciano's Digital Playground" should have enough space between
the characters to be as long as the line above it. This line is being styled by:

h2#caption {  font-size:11px; color:rgb(224,211,150); font-weight:normal;
text-transform:none; text-align:right; margin:-10px -2px  0 0; padding:2px 0
20px 0; letter-spacing:6px; }

A quick test case showed word spacing was not followed either. This is a pretty
rough break in CSS2 support. I will attach a test case in just a minute.

Comment 55

[Chris Casciano]

Attachment: Letter and Word Spacing Testcase

Simple example of letter-spacing and word-spacing CSS 2 properties. Both fail
in the build by stevek. See comment 51

Comment 56

[Steve K]

I think all the text layout issues that Chris pointed out are demonstrated
simply in Viewer demo #0 (Debug->Viewer Demos->#0) and etc.

Actually, the mach-o build is handy for that, because you can have a mach-o and
a CFM mozilla running on the same machine, and see the differences.

Comment 57

[Simon Fraser [no longer active]]

It is expected that this ATSUI patch will cause some i18n regressions. Please
don't file bugs on those. However, it should not regress text layout; I'll look
at the spacing issues.

Comment 58

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Are the high-numbered characters considered part of i18n?  For instance I use
delta alot, (Δ) which shows as a block on stevek's build.
 

Comment 59

[David Hyatt]

Printing seems to be busted up also.  Can anyone verify if this is just 
Chimera, or if it also happens in Mach-o?

Comment 60

[Dave Walker]

Confirming screwed-up printing in Mozilla w/ ATSUI patch.

Comment 61

[meiering]

Just a quick comment. Some of the rumor sites showed builds of OSX10.2 where
Carbon Apps which use QD for text automatically get Quartz antialiasing. I have
no idea how Moz draws text, but if true, Apple might solve this problem for you
for free.

WWDC is in early May, so they would likely announce this if it is the case.

Comment 62

[Mike Pinkerton (not reading bugmail)]

leave the rumors to the rumor sites. not one thing that's been on them in the
past 3 years has been true.

Comment 63

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Gee, that's a pretty harsh assessment of the rumors site, but it's hard to
imagine this one could be true.  It would really muck up alot of apps that
expect their pixels to be in a specific place.  Maybe just standard TextEdit
widgets, but then the app would look pretty funny.  Apple tried some experiments
along these lines early on, then decided on the "Classic Looks like Classic"
approach.

Comment 64

[Simon Fraser [no longer active]]

Attachment: Latest gfx/layout patch

This latest patch fixes printing and letter spacing (but not i18n), and is much
cleaner. It includes changes to fix the Mach-O build. It also adds a 'slow'
ATSUI rendering mode which does kerning and ligatures (and thus has selection
problems).

This is my last ATSUI patch before I go on sabbatical, so I won't be working on
this for 8 weeks.

To fix the i18n issues, we need to tie into the font fallback logic that is
used for the QuickDraw codepath. However, that code is a big mess, and it's not
easy to determine which parts of it we need for CG text rendering (since CG
rendering can make use of more code points in the fonts that have them (e.g.
Lucida Grande). For now, I've implemented some very basic ATSUI-based font
fallback logic.

Note also that with ATSUI/CG rendering, the assumptions under which the Font
prefs panel was coded (matching fonts to the languages that they can render)
also changes.

Comment 65

[Simon Fraser [no longer active]]

*** Bug 74245 has been marked as a duplicate of this bug. ***

Comment 66

[Prachi Gauriar]

After reading posts on n.p.m.macox, seeing items on rumor sites, etc, I've come
to strongly believe that OS X 10.1.5 will have Carbon API support for QuickDraw
using Core Graphics to render fonts. I'm wondering if we should shift over and
use that instead of sfraser's patches.

Yes, I know, Mac OS X 10.1.5 has not been released yet, but it is my
understanding that ADC Select and Premier developers have a developer seed
available for download. For those of you CC:ed on this bug that are Select or
Premier devs, perhaps you could check out your ADC account, and check the
release notes for the seed. If the notes do imply that QuickDraw can use Core
Graphics for text rendering, see if you can figure out how to do this for when
the 10.1.5 update comes out. I'm just a student dev, so this is out of the
question for me.

I'm sure IE will be updated soon after 10.1.5 is released, and that will
definitely put Mozilla behind; if anyone is interested in working on this,
please, by all means, check it out, especially since sfraser is on sabbatical.

Oh, and to be perfectly clear, don't break the NDA by posting patches or
information here... I'm just seeing if anyone wants to do a little background
research early so that Mozilla can be ready soon after OS X 10.1.5's release.
Again, I'm not sure that these rumors are true, but I strongly suspect that they
are.

Comment 67

[Steve Dagley]

As far as I know the ability to use Quartz to render text with QuickDraw calls
is a feature added for Jaguar, not 10.1.5.  And if you need accurately measured
text, like we do, there is a performance hit similiar to the one we take for
using ATSUI.  TANSTAAFL - read "The Moon Is A Harsh Mistress" if you don't
recognize that acronym.

Comment 68

[Mike Pinkerton (not reading bugmail)]

> And if you need accurately measured
> text, like we do, there is a performance hit similiar to the one we take for
> using ATSUI.

Yeah, but it doesn't require a full rewrite like the attached patches which
break other stuff ;)

Comment 69

[Prachi Gauriar]

Like I said, I'm only speculating that it will be in OS X 10.1.5, but if you're
a Select or Premier ADC Member, you can find out right now by checking out the
10.1.5 developer seed (if you're not taking my hint, go find out right now if
you can). If this capability is introduced in 10.1.5, then try to start working
on a way to do this now. If not, forget about it for a few months until around
when Jaguar comes out.

And Pinkerton's right... even if the performance hit is similar, it probably
won't require as much work to get it working with the rest of Mozilla.

Comment 70

[Steve Dagley]

Hrm, would a comment of "Son of a bitch!" be considered breaking an NDA? :-) 
Without commenting on any APIs that may or may not be added for 10.1.5 I'd agree
it's something to consider.  We have talked to people at Apple about which would
be the best approach and they're in favor of ATSUI.  And ATSUI does offer some
potential that QuickDraw doesn't but perhaps we can continue to use it as a
fallback.  I'll leave the decision on our end to sfraser.

Comment 71

[Prachi Gauriar]

Well, OS X 10.1.5 was released today, and "Updated Carbon applications" can use
Quartz to render text. I guess it's now time to discuss seriously taking this
route? 

Anyone have info on new/update API? I don't think Apple has posted docs yet.

Comment 72

[Steve Dagley]

To quote a beloved former NSCP manager "Top Men are on it" :-)

Comment 73

[Brent Stephens]

wow this was my favorite bug for months...are we coming to resolution??  how is 
this going to be decided?  performance issues or ease of coding?

Comment 74

[Henri Sivonen (:hsivonen)]

The current code handles non-MacRoman characters quite inefficiently with
per-character fallbacks. If Quartz anti-aliasing is done with some updated
QuickDraw APIs, both alternatives will have to spend time with the same
rasterization algorithms. My guess is that ATSUI all the way will be faster than
Quartz anti-aliasing through updated QuickDraw while still doing the old
per-character multilevel fallbacks.

I think that it makes sense to use the new feature of 10.1.5 at first, but I
think the full ATSUI way is still worth pursuing.

Comment 75

[meiering]

If I understand correctly (just an end-user here), the implementation in 10.1.5
is the faster method, but if the text needs to be measured correclty then ATSUI
is the way to go.

If the above is the case, then what are the cases where *correct* measurement is
absolutely necessary? What are the consequences if you use Apple's
implementation in those cases where *correct* measurement is *required*? If the
consequences aren't that great then I think most end-users would opt for speed.

I think if Roman characters don't need to be measured, then Apple's
implementation should be the default method and the option should be to use the
pure ATSUI method.

Perhaps there could be a pref (I know. Another pref!) where one could choose
between, non-antialiased, antialiased QD Quartz and antialiased ATSUI Quartz. 

-------
Using Moz1.0...Great work!

Comment 76

[Dave Walker]

So does anyone have an idea how they pulled this one off?

http://www.unsanity.com/haxies/silk/

I haven't tried it yet.

Comment 77

[Avi Drissman]

Easy. Unsanity already has code to globally patch out apps from WindowShadeX and 
FruitMenu. They just repurposed it to call SwapQDTextFlags as every Carbon app is 
launched.

Talking about SwapQDTextFlags, here's some documentation (use archives/archives as 
user/pass):

http://lists.apple.com/mhonarc/carbon-development/msg23117.html

But keep in mind that it's SwapQDTextFlags, not QDSwapTextFlags as is incorrectly 
stated in that message.

Comment 78

[Mike Pinkerton (not reading bugmail)]

yeah, that api discrepancy cost me an hour. doh! anyway, i have this working in
mozilla. god it looks sweet, especially the aqua skin.

not sure if we want this for chimera, though i'd imagine we do in the short
term. i'll play with it and see if i can get it working.

Comment 79

[Mike Pinkerton (not reading bugmail)]

just ran pageload tests with chimera:

trunk, QD drawing: 815
quartz drawing, QD metrics: 851
quartz drawing, CG metrics: 913

that's only a 12% slowdown. i think we should go that route for the short term.
wow. 

Comment 80

[Greg K.]

That should get filed as a seperate bug. I presume we still want to go ATSUI
long-term.

Comment 81

[Mike Pinkerton (not reading bugmail)]

new bug filed: http://bugzilla.mozilla.org/show_bug.cgi?id=149427

Comment 82

[Mike Pinkerton (not reading bugmail)]

*** Bug 150188 has been marked as a duplicate of this bug. ***

Comment 83

[Craig Grannell]

Would love to see this fixed, so it can become the first usable browser on OS X
that enables me to see Icelandic characters properly. OmniWeb is the only one
now that can handle them, but its CSS support sucks, so I currently have to use
Windows for creating Icelandic Web sites...

It was all working fine in 0.2.8 :(

Comment 84

[Greg K.]

Please, no evangelism comments. If you want this, vote for it.

Comment 85

[Henri Sivonen (:hsivonen)]

The problem with Icelandic (and Polish) chars is bug 111728.

Comment 86

[Jo Hermans]

*** Bug 160985 has been marked as a duplicate of this bug. ***

Comment 87

[J Luh]

[change mozilla1.1 keyword to mozilla1.2 since 1.1 is already out]

Comment 88

[Greg K.]

This appears to fix bug 15116 on Mac.

Comment 89

[Henri Sivonen (:hsivonen)]

Re: Various ideas about "optimizing" ASCII or MacRoman

Trying to "optimize" the speed of a-z would mean that Mozilla would continue to
be worse than OmniWeb when it comes to the display of English (which is a very
common language :-). If OS X's new text display engine was allowed to handle
English, too, we'd get high-quality kerning and ligatures. (Isn't it kind of
unfair that we only get Arabic and Thai shaping but not English shaping, even
though the OS could provide it?)

Comment 90

[Marko Karppinen]

I think the only way to reasonably match OmniWeb's text rendering with regards
to performance and output quality would be to render with NSTextView. Hyatt's
benchmarks indicate that much of the problem here is in the text measurement
calls . It seems that layout is locked in a ping-pong exchange with the ATSUI
renderer.

By a superficial glance, Mozilla's concept of TextFrames doesn't seem to be
fundamentally incompatible with NSTextView. So maybe the solution isn't to wire
the low level ATSUI APIs to nsTextFrame.cpp but to replace the TextFrame code
completely with NSTextView. It'd be the Mother of All Patches, though...

Comment 91

[Simon Fraser [no longer active]]

> By a superficial glance, Mozilla's concept of TextFrames doesn't seem to be
> fundamentally incompatible with NSTextView.

Oh it is - very incompatible. Layout is line-based, and will break the line into
separate frames for each style chnage. NSTextView is multi-style paragraph-based.

Comment 92

[Niklas Dougherty]

Found this in the syslog:

Oct 20 06:40:02 Niklas-Doughertys-Computer
/Applications/Mozilla/Mozilla.app/Contents/MacOS/Mozilla: *** Warning:
ATSUMeasureText has been deprecated.  Use ATSUGetUnjustifiedBounds instead. *** 

Comment 93

[Greg K.]

Niklas, that's covered by bug 161332.

Comment 94

[Greg K.]

Are there any adjustments that should be made in Gecko 2 to better accomodate ATSUI?

Comment 95

[Simon Fraser [no longer active]]

I filed bug 157967 about advanced typography features in Gecko. It has been futured.

Comment 96

[Snorri Gylfason]

Attachment: Shows how Mozilla renders Icelandic chars (thorn, yacute, eth) incorrectly (also shows correct rendering by Safari for comparison)

The problem with text rendering in Mozilla renders the browser unusable for
Icelandic users. Three characters (thorn, eth, yacute) are in Latin-1 charset
but not in the MacRoman charset. It seems that Mozilla assumes these charsets
are the same. 

In the attachment you can see from Icelandic newssite (the most popular
Icelandic site) you can see example of the incorrect rendering by Mozilla and
the correct rendering by Safari. 

This is not question of encoding because these characters are encoded in html
as þ and ð

Comment 97

[Frankie]

Attachment: Mozilla renders Icelandic correctly, Snorri does not

Snorri, perhaps you are using a bad font with an incomplete character set.
Perhaps you did not include Icelandic (is) in your language list. Perhaps you
removed international support from your install of OSX. Perhaps you are using a
bizarre proxy system that screws up characters above 0x7F.

But I have no problem displaying those characters in the same Mozilla build
that you are using.

And for the record, MBL does not Not NOT html encode their characters using
ð etc. They are sent raw. Here's a clipping directly from their source
code:

BandarÌkjamenn segja a Bagdad-deild
L˝veldisvararins hafi veri eytt

Comment 98

[Prachi Gauriar]

It's been a long time since this bug has been worked on.  Is the patch good?  If
not, what are the issues with it? 

Comment 99

[Jungshik Shin]

I came here from bug 205476. 

I just skimmed over comments here. Although it's mentioned once or twice, I'm
afraid I18N aspect of this bug has not been given as much attention as it deserves. 

> Besides the beauty aspect, though, there may be a real need for the 
> High method for some non-english languages.  In some languages 
> (Arabic, Hindi, some Scandanavian?)

 Modern Scandinavian languages (written in Latin) don't need complex script
rendering support. However, Brahmi-derived scripts of South and Southeast Asia
(Devanagari, Tamil and other Indic scripts, Burmeses, Thai, Lao, Khmer(sp?)),
Korean, Hebrew, and Arabic would benefit most from ATSUI.

For them, it's more of being _correct or not_ than of being prettier or not. For
instance, go to http://www.bbc.co.uk/hindi and compare the result with Safari
and Mozilla/Camino. It may not be easy for non-Devanagari users to see the
difference. A better test case is bug  
204286 attachment 122383 [details]. 
 
In case of Latin, Greek and Cylliric scripts, most Languages written in   them
(including Vietnamese and Indonesian) are covered by 'one unicode character ->
one glyph' model, but for medivial text and African languages (that use Latin
alphabets) ATSUI's support of diacritics are essential. 

Comment 100

[John Dent]

I agree with Jungshik's comments - it is certainly a matter of correctness.

Just a word of caution not to use Safari as a reference: on my install at least,
Safari suffers from similar rendering difficulties. In fact, in some ways,
Camino/Mozilla actually does a slightly better job of Hindi (not sure about
other languages). They are both incorrect.

Comment 101

[Shoshannah Forbes]

From comment #100:
>Just a word of caution not to use Safari as a reference: on my install at least,
>Safari suffers from similar rendering difficulties

Very much agreed. For example, when it comes to Hebrew with diacritics, nither
geco based browsers nor Safari display the text properly.

In fact, there isn't even one mac browser that can display Hebrew text with
diacritics properly.

Comment 102

[Jungshik Shin]

I don't have a Mac and shouldn't have made an 'extrapolation' based on Konqueror
for Linux. Anyway, that doesn't make any weaker the case for supporting complex
scripts in Mozilla/Camnio via ATSUI. 

Comment 103

[John Dent]

I've not yet applied the patch listed above to test it but, in principle, it
sounds good.

I'm concerned at the apparent lack of progress and the lack of a statement as to
why this is.

My opinion is that we should merge the patch into the codebase (perhaps with
minor modifications and updates). As it gets real world use and gets tested, we
can later decide whether or not to remove the 'optionalness' of pure ATSUI code
path.

Is this reasonable?

An ATSUI path would vastly improve my browsing experience.

What's stopping us? Resource? A design issue? Ownership? Roles & responsibilities?

I checked on the ownership lookup tool and I could only find the owner of the
X11 renderers listed against gfx/mac. Is the Mac OS/OS X platform orphaned as
far as maintainership for rendering goes?

Comment 104

[Paul Libbrecht]

My two pence about the reason this is not achieved is that of speed. Even Camino
went to Quickdraw at some point around 0.4-0.5 because it was (really only
slightly) faster.

Paul

Comment 105

[Simon Fraser [no longer active]]

I'm nominally mac gfx owner, and the author of this patch, but I don't have
cycles to work on this any more (it's no longer my real job).

Main reasons for not landing this patch:
1. Speed. It was quite a bit slower. The width caching hacks helped somewhat,
   but forced us to turn off kerning, resulting in worse appeaance. Also, gecko's
   selection code cannot deal with kerning/ligatures.

   Speed may be better in Panther.

2. Internationalization regressions. The ATSUI code path in this patch bypasses
   a large accumulation of font fallback cases in the QuickDraw code, some of
   which may still be required.

Comment 106

[John Dent]

Okay, I understand this is very much a pre alpha feature.

I'm not necessarily suggesting that we merge the ATSUI features as a straight
replacement for the existing renderers.

Would the half way house of having a user preference defaulting to QuickDraw not
be sufficient a guard? Especially given the lack of resource?

It is clear to me that ATSUI rendering offers significan benefits in terms of
quality. If we cannot make it match QuickDraw in terms of performance, then the
preference may well need to stay.

But ATSUI + preference would fix quite a few problems with Mozilla's rendering
on OS X.

Comments?

Comment 107

[Paul Libbrecht]

I would then just vote a +1 but am unable to judge the incidence on Moizlla
rendering...

Comment 108

[Bill McGonigle (not currently reading bugmail; please contact directly)]

I mentioned a 3-way preference in comment #32, since ATSUI has different levels
of rendering.

I just made up some birth announcements using an ATSUI font in TextEdit - the
decenders (y, j) sometimes go under multiple preceeding letters, a period can
fall above the serif of the last letter of a line (e.) - it's really quite
beautiful - though text selection might be a bit more challenging.

Comment 109

[Henri Sivonen (:hsivonen)]

Text performance has improved in Panther. Re: comment #20: Perhaps now the
quality benefits of using full ATSUI (measurement and everything) outweigh the
old speed benefits of QuickDraw.

Comment 110

[David Smith]

(In reply to comment #109)
> Text performance has improved in Panther. Re: comment #20: Perhaps now the
> quality benefits of using full ATSUI (measurement and everything) outweigh the
> old speed benefits of QuickDraw.

Does anyone know if QD was improved similarly in 10.3? If not then ATSUI might actually be just 
about as fast.

Comment 111

[Jungshik Shin]

(In reply to comment #105)

> 2. Internationalization regressions. The ATSUI code path in this patch bypasses
>    a large accumulation of font fallback cases in the QuickDraw code, some of
>    which may still be required.

  With ATSUI's far superior support for Unicode, I'm pretty sure that most of
old fallbacks are not necessary any more. For instance, Korean fallback is not
necessary any more as long as you have a font with the full Korean repertoire
installed which is readily available these days.

 However, the speed seems to be still an issue in Panther. See bug 120401. 

Comment 112

[Henri Sivonen (:hsivonen)]

Re: Speed

ATSUI expects the app to maintain pre-calculated layout objects (aqcuired from
ATSUI) instead of starting over each time it draws. I think layout frames in
Gecko need a pointer to platform-specific renderer data that is passed back if
the layout has not changed. A platform-specific deallocator would be called on
the platform-specific data whenever the layout actually changes so that text
runs need to be recalculated.

Comment 113

[Simon Fraser [no longer active]]

Re: comment 112: the problem is that layout draws text line by line, word by
word or even character by character, so it really doesn't play nicely with
ATSUI, which works best when you hand it paragraph-sized chunks. See bug 157967,
bug 157967.

Comment 114

[Christopher Blizzard (:blizzard)]

Yep.  Pango suffers from exactly the same "problem."  It does best with larger
chunks.

Comment 115

[Spundun]

(In reply to comment #100)
> I agree with Jungshik's comments - it is certainly a matter of correctness.
> 
> Just a word of caution not to use Safari as a reference: on my install at least,
> Safari suffers from similar rendering difficulties. In fact, in some ways,
> Camino/Mozilla actually does a slightly better job of Hindi (not sure about
> other languages). They are both incorrect.
1-> I dont know how safari has evolved since this comment, but right now
safari's Indic rendering is near perfect, and in comparison firefox's rendering
looks ugly(or atleast unprofessional), including errors related to matras.

2-> Is there a mozilla/firefox build available with this patch in it? I would
love to test font rendering with hindi and gujarati on mozilla/firefox and
report here about my findings.

3-> currently mozilla source code doesnt support Indic scripts in address bar at
all, nor is it supported anywhere in thunderbird.

4-> <evangelist note> I have to keep safari running just to use hindi and
gujarati wikipedia, becuse of the errors, firefox is very hard to use. I use
firefox for everything else. </evangelist note>

5-> my browser is :Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3)
Gecko/20040913 Firefox/0.10 and thunderbird is: version 0.8 (20040913)

6-> There needs to be simple documentation about how to build mozilla on OSX
from source.. I tried once but couldnt jump through all the hoops. Perticularly
there seems one package that I have to seperately download from some wierd site,
I dont understand that part... if an explanation about that is included in the
build notes that will be great. Also please letme know which build notes I
should be using so that I can build my own mozilla/firefox etc. and if
time/motivation/skills permit, even contribute to it.

Thanx a lot
Spundun

Comment 116

[Spundun]

oh and my system is panther

Comment 117

[John Dent]

(In reply to comment #115)
>
> 1-> I dont know how safari has evolved since this comment, but right now
> safari's Indic rendering is near perfect, and in comparison firefox's rendering
> looks ugly(or atleast unprofessional), including errors related to matras.

I noticed this. It is a recent change - when I made the comment it was correct.
I guess the Safari guys must have integrated ATSUI in there somehow.

> 2-> Is there a mozilla/firefox build available with this patch in it? I would
> love to test font rendering with hindi and gujarati on mozilla/firefox and
> report here about my findings.

I'm afriad I binned my own development version some time ago. All I have left
are the various screenshots in these bugs.

I found the build process somewhat strange but workable. I don't think I had to
jump through any major major hoops except for requiring an HFS+ volume to build
on. I recall my original aim was to build Camino but I accidently ended up with
a Mozilla binary (which didn't matter as they are the same rendering engine) so
there was obviously something a bit odd going on.

Comment 118

[Spundun]

(In reply to comment #117)
> I found the build process somewhat strange but workable. I don't think I had to
> jump through any major major hoops except for requiring an HFS+ volume to build
> on. I recall my original aim was to build Camino but I accidently ended up with
> a Mozilla binary (which didn't matter as they are the same rendering engine) so
> there was obviously something a bit odd going on.

In my previous mail I mentioned some wierdness but didnt elaborate.. your post
made me go to google it again. :) (which is a good thing)

there url http://www.mozilla.org/build/mac.html asks me to download a binary
component called shared menus or something. I dug up its homepage to be
http://www.url-manager.com/sme.html .

This component seems released as a binary distribution. So its it not an OSS? Is
it ok to use a closed source software as a dependency like this to mozilla?
Its just that I have never seen anything linking like that on any OSS project
page so I was nervous about downloading it.
At the least, I think that build project should say what this component does,
and why is it needed, (and maybe even whether its packaged inside mozilla or
not) and also provide a link to the actual home page (see above). A link straght
to some binary installer on a "how to build mozilla" page seems out of place. 

I would like to know why and what components of mozilla need this.
Thanx
Spundun

Comment 119

[Spundun]

ugh, and I just checked, and foind out that gujarati doesnt render with mozilla
at all. 
Meaning on gu.wikipedia.org page, I see question marks and only questoin marks
in place of gujarati scripts. So on mac there is zero support for gujarati (and
maybe some other indic scripts I dont know about). Hindi support is buggy. While
gujarati rendering on safari that I come across day-to-day is perfect. So is
rendering on Firefox/windows,IE/windows. It renders on Linux with pango also...
but some bugs still there.

So yes I think there are some good reasons for getting this support in mozilla.

(btw, I gotta takebake my note on TB not supporting indic editing... that was
because I was trying with gujarati.. which is just all ???... when I try
devnagari, I do get the rendered text, the rendering ofcourse is buggy).

Comment 120

[Mike Pinkerton (not reading bugmail)]

yes, it is perfectly ok to use shared menus within mozilla. their license
permits it, as does ours. it's used to provide a hook for other applications
(like external bookmark managers, for example) to add their own menus to
mozilla's menubar.

Comment 121

[Bill McGonigle (not currently reading bugmail; please contact directly)]

(In reply to comment #118)
> it ok to use a closed source software as a dependency like this to mozilla?
> Its just that I have never seen anything linking like that on any OSS project
> page so I was nervous about downloading it.

Beyond what Mike said, consider the case of the Mac and Win32 ports of Mozilla -
both are linking against massive proprietary libraries - they just happen to be
from Apple and Microsoft.  ATSUI is closed-source, for example.  At least Shared
Menus has a fully-open API.

Comment 122

[Spundun]

(In reply to comment #121)
> Beyond what Mike said, consider the case of the Mac and Win32 ports of Mozilla -
> both are linking against massive proprietary libraries - they just happen to be
> from Apple and Microsoft.  ATSUI is closed-source, for example.  At least Shared
> Menus has a fully-open API.
Point noted.. :)

now back to the original discussion. Current text render implementation of mozilla platform on Mac 
OSX has very buggy Devnagari rendering and *no support* for gujarati (and heaven knows what other) 
script rendering.

:)
Spundun
btw. I havent yet compiled and tested mozilla as mentioned above, I will post the results here if I can 
take out time to do that.

Comment 123

[Spundun]

(In reply to comment #117)
> I found the build process somewhat strange but workable. I don't think I had to
> jump through any major major hoops except for requiring an HFS+ volume to build
> on. 

I tried building Firfox on HFS and I got build errors where make seemed to get
in to infinite recursion.

I have filed a bug against that (bug #263212 ). Note that for that build I am
using a fresh trunk checkout and I havent yet applied any patches from this bug.

I can also confirm that it needs to build on HFS :) I was trying it on ext2 and
it broke when executing asdecode because it was trying to crate a resource fork.
Some discussion with people gave me more info about resource forks on MacOS...
they are more of a MacOS thing than MacOSX, and people suggested that MacOSXonly
apps shouldnt need those resource forks. Since we have teminated support for
MacOS 9 and such.. maybe we should rework these parts of the build so that it
works without the need of HFS.

Comment 124

[Spundun]

OK, Firefox built with no patch..

But when I tried to apply the patch #79914 the patch didnt apply... 

Apart from so many failed hunks, here is a list of files missing.
First of all it seems the patch needs to be applied after cding into the directory gfx.. so I did that and 
used command patch -p0 < patchfile.

macbuild/GFXComponentConfig.h and macbuild/gfxComponent.xml couldnt be found. As a matter of 
fact the macbuild directory itself does not exist.

src/nsGraphicsImpl.cpp

Apart from the above... there were rejected hunks for the following files
spundunbhatt$ find . -name "*.rej"
./public/nsIRenderingContext.h.rej
./src/mac/nsDeviceContextMac.cpp.rej
./src/mac/nsDrawingSurfaceMac.cpp.rej
./src/mac/nsFontMetricsMac.cpp.rej
./src/mac/nsGfxFactoryMac.cpp.rej
./src/mac/nsNativeThemeMac.h.rej
./src/mac/nsRenderingContextMac.cpp.rej
./src/mac/nsRenderingContextMac.h.rej
./src/mac/nsUnicodeFontMappingMac.cpp.rej
./src/mac/nsUnicodeFontMappingMac.h.rej
./src/mac/nsUnicodeMappingUtil.cpp.rej
./src/mac/nsUnicodeMappingUtil.h.rej
./src/mac/nsUnicodeRenderingToolkit.cpp.rej
./src/nsFontList.cpp.rej

(among the above, a couple are false positives but rest are actual rejects)


I dont know what happened....
Anyone else have a better idea? 
Note: I am using seperate objdir (meaning creating object files in seperate dir called mozbuild) I hope 
none of the above patches apply to a file in there... because patches should apply to source files.

Hope this helps
Spundun

Comment 125

[John Dent]

Hi Spundun,

You could try the patch I made for bug #205476. It is not as well integrated -
bit of a bodge really. But it worked at the time. Maybe it will work for you.

denty.

Comment 126

[Spundun]

(In reply to comment #125)
> You could try the patch I made for bug #205476. It is not as well integrated -
> bit of a bodge really. But it worked at the time. Maybe it will work for you.
Thanx denty! That patch applied and it works a lot better, I have posted my comments on bug #205476

Spundun

Comment 127

[Phil Ringnalda (:philor)]

*** Bug 308893 has been marked as a duplicate of this bug. ***

Comment 128

[Jo Hermans]

*** Bug 314355 has been marked as a duplicate of this bug. ***

Comment 129

[YAMASHITA Makoto]

Attachment: Try MLTE for Unicode rendering

How about MLTE? After all its TXNDrawUnicodeTextBox is sufficient for our purpose.
Of course the speed matters and apple says it's built upon ATSUI, but it looks faster to me than calling ATSUI by hand.
The patch is a try-out of MLTE: it disables script-code based Unicode conversion and uses TXNDrawUnicodeTextBox for drawing text (it leaves QD for symbolic fonts used in MathML) If this is okay, I gonna try more comprehensive adaptation.
I don't know the right way to measure text rendering performance, so could you try this out or teach me how to do it?

Comment 130

[Katsuhiro MIHARA]

YAMASHITA-san, in the plan, Carbon widgets will be deprecated. Your effort is great. But main developers pay attention to Cocoa now. We should think about how long Carbon widgets will be used.
Bug 111230
http://wiki.mozilla.org/Mac:Cocoa_Widgets

Comment 131

[YAMASHITA Makoto]

Hmm.... But widget/src/cocoa uses NSQuickDrawView in order to reuse gfx/src/mac built upon Carbon, doesn't it? So Carbon won't be abandoned until we go into "gfx/src/cocoa" which is below the horizon yet...
Moreover I even think of MLTE adaptation as a move toward Cocoa or OSX-way or anything like that, because TXNDrawUnicodeTextBox looks like an immediate Carbon counterpart of NSString's drawAtPoint:withAttributes:/drawInRect:withAttributes: .
And please see how the patch makes things simple: it makes all the fallback code paths below DrawTextSegment in nsUnicodeRenderingToolkt.cpp unnecessary, simultaneously resolving bug 205476 for example..

Comment 132

[Jungshik Shin]

(In reply to comment #130)
> YAMASHITA-san, in the plan, Carbon widgets will be deprecated. Your effort is
> great. But main developers pay attention to Cocoa now. We should think about
> how long Carbon widgets will be used.
> Bug 111230
> http://wiki.mozilla.org/Mac:Cocoa_Widgets

It would be great if we could move onto Cocoa in the near future, but I'm afraid it may not happen soon enough to hold our breath (it's not a trivial transition. see how long ago bug 111230 and this bug were filed). If this patch works for Indic scripts and other complex scripts, this should be very good news for Indian and other South and South Eastern users.

Yamashita-san, with your patch Un font works fine (thanks !), but I couldn't find, in the font selection list, 'Devanagari MT', 'Gujarati MT' and other Indic fonts installed on my Mac. It only lists 'MT' 

Comment 133

[YAMASHITA Makoto]

> I couldn't find, in the font selection list, 'Devanagari MT', 'Gujarati MT'
> and other Indic fonts installed on my Mac. It only lists 'MT'
I don't have those fonts (I'm not sure what's happening to them on your machine), but Sanskrit 2003 looks working to me.

Comment 134

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #132)
> but I couldn't
> find, in the font selection list, 'Devanagari MT', 'Gujarati MT' and other
> Indic fonts installed on my Mac. It only lists 'MT' 

I would not be surprised at all if the Indic fonts were also victims of bug 246527 (unless this patch tries to fix that bug, of course; I wouldn't know).  My impression is that large swaths of Apple's non-European and non-CJK fonts (as well as some European ones like Armenian and Greek) are not recognized by Mozilla because of that bug.  

(Besides ATSUI support for glyph reordering, I think bug 246527 is one of the biggest obstacles to better support/platform parity for international text rendering in Mac Mozilla :-))

Comment 135

[Jungshik Shin]

(In reply to comment #133)
> > I couldn't find, in the font selection list, 'Devanagari MT', 'Gujarati MT'
> > and other Indic fonts installed on my Mac. It only lists 'MT'
> I don't have those fonts (I'm not sure what's happening to them on your
> machine), but Sanskrit 2003 looks working to me.

Those fonts are not installed by default unless your primary language needs them . However, they have been available on the OS installation CD since Mac OS 10.2. I forgot exactly how I installed them, but probably you can install them if you put in the installation CD and select a 'custom' install option (or something like that). 
I'll try Sanskrit 2003. Does it have AAT tables? 

Comment 136

[Simon Fraser [no longer active]]

The move to Cairo (bug 309571) will involve a rewrite of the text rendering APIs, probably towards a mixture of Core Graphics and ATSUI.

Comment 137

[YAMASHITA Makoto]

Attachment: adapt MLTE

This patch does MLTE adaptation.
always uses MLTE for measurement and drawing the texts
When a word in the sentence has a different style (a <a href="...">link</a> is an example) the core expects gfx to measure the width of "a " or " is". MLTE trims the whitespaces at edges, so I put the dummy characters at the start and the end for text measurement.
*always* MLTE because the core sometimes calls PRUnichar-based func for measurement and char-based one for drawing of bold-faced segments, and QD and MLTE behave inconsistent for that, resulting in glyph collisions. After all, it's not so slow...

Side effects: Improved rendering quality for the right-to-left scripts (Hebrew and Arabic) , proper baseline handling for CODE2001 font and compatibility with the fonts without mac* encoded cmap tables
TODO: bounding metrics for mathml, more cleaning old codes

I've been hit with a odd cvs trouble and can't update/diff smoothly nor build Firefox (somehow Camino is OK). My development will be very slow until it's fixed, so please feel free to participate/take on this patch.

Comment 138

[Simon Fraser [no longer active]]

Much as we welcome participation here, any patches on the existing gfx code are pretty much wasted effort at this point. It would be much more productive to get involved in the Cairo effort for Mac, particularly as we need text rendering help there. Some initial info is at http://wiki.mozilla.org/Mac:Cairo

Comment 139

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #138)
> Much as we welcome participation here, any patches on the existing gfx code are
> pretty much wasted effort at this point. It would be much more productive to
> get involved in the Cairo effort for Mac, 

Without intending to sound contrary, if we're not going to see an end-user release with Cairo (i.e., from the trunk) until sometime in 2007 (http://cbeard.typepad.com/mozilla/2005/11/mozilla_product.html), and if this patch is 1) not a massive effort, 2) is performant and 3) doesn't hinder the Cairo/ATSUI effort (none of which I'm in a position to judge), does it not make sense to look at this for the 1.8.1 branch for the 2006 end-user releases?  

(It might need bug 246527 fixed, too, to be worthwhile, because if we can't read the standard Mac fonts for these scripts, the potential benefit decreases rapidly.)  It sounds like the patch has the potential to fix our completely-busted Indic support and improve RTL support and more, all places where Gecko/Mac is sorely behind Gecko/Win and Gecko/Lin....

Comment 140

[YAMASHITA Makoto]

1)(In reply to comment #139)
> patch is 1) not a massive effort, 2) is performant and 3) doesn't hinder the
> Cairo/ATSUI effort
I'd like to say
1) No: it's true this patch is big, but only because it reduces that monstrous nsUnicodeRenderingToolkit.cpp half in size.
2) Yes, although a precise perf measurement will be better
3) No: until the bug 157967 is achieved, ATSUI is over-spec for our purpose. I think MLTE's TXNDrawUnicodeTextBox or NSString's drawAtPoint:withAttributes: are more suited for what gfx and Cairo are supposed to do now.

Comment 141

[Simon Fraser [no longer active]]

(In reply to comment #140)

> 2) Yes, although a precise perf measurement will be better

Can you work with someone at Mozilla to get pageload data? Try Josh Aas. I think we should get numbers for 10.3 and 10.4, since text rendering in the OS has changed quite a bit.

Comment 142

[Greg K.]

Perhaps MLTE support should be filed as a new bug, then, and this one left for any future non-Cairo ATSUI support that may be implemented (likely none).

(In reply to comment #140)
> 3) No: until the bug 157967 is achieved, ATSUI is over-spec for our purpose. I
> think MLTE's TXNDrawUnicodeTextBox or NSString's drawAtPoint:withAttributes:
> are more suited for what gfx and Cairo are supposed to do now.

Comment 143

[YAMASHITA Makoto]

Attachment: MLTE adaptation v3

significant changes from the previous ver include:
in nsATSUIUtils.cpp: GetTextDimensions now uses ATSUI (original approach, with no tweak for whitespaces at edges) for width and FontMetrics for vertical measurement. After all MLTE looks like inadequate for text measurement, and Hiragino font turned out to be rendered best for this FontMetrics' res (I don't know why).
in nsUnicodeRenderingToolkit.cpp: "split font runs" codes in *TextSegment funcs and "adjust to visual ordering (for bidi)" are unified.

> Can you work with someone at Mozilla to get pageload data? Try Josh Aas. I
> think we should get numbers for 10.3 and 10.4, since text rendering in the OS
> has changed quite a bit.
I have no idea what to do for perf measurement. Josh? could you help me?

> Perhaps MLTE support should be filed as a new bug, then, and this one left for
> any future non-Cairo ATSUI support that may be implemented (likely none).
I feel like keep doing this here; MLTE uses ATSUI, future non-Cairo ATSUI support is less likely and we would lose those accumulated bug dependencies. Especially my patch will be an immediate solution for bugs 105800, 129205, 148361, 165881, 190352, 205476, 219426 and 233782.

Comment 144

[Hiro]

(In reply to comment #143)
> Created an attachment (id=205327) [edit]
> MLTE adaptation v3
> Especially my patch will be an immediate solution for bugs 105800, 129205,
> 148361, 165881, 190352, 205476, 219426 and 233782.

This patch was tried. 
bug240841 seems also to do FIXED.

Comment 145

[Spundun]

(In reply to comment #144)
> This patch was tried. 
It would be nice if someone could post somewhere a trunk nightly build with this patch applied. I want to try this build but unfortunately I dont have enough time right now to figureout and attend to the firefox build right now. :( I think it will help many people interested in the bug.

Spundun

Comment 146

[Hiro]

FYI.

It is test build that applies MLTE adaptation v3.
http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-v3-testbuild.mac.dmg

Comment 147

[Asaf Romano (gone)]

Using this build, text underline is positioned too near to the text (at least w/ Windows Arial).

Comment 148

[philippe (part-time)]

With that patch using the  build from comment #146, bug 240841 is indeed fixed.
However, on my various test files, I can't get any Japanese text (Kanas or Kanjis) to output in bold. On this Apple page as a sample, the headlines (links) in the left column should be bold. They are not on my side, only the Roman characters (this reminds me of some similar bugs in older versions of Opera Mac and Omniweb).
<http://www.apple.com/jp/hotnews/>

I had some hope it would help fix bug 294733, but that was maybe optimistic.

Comment 149

[Spundun]

(In reply to comment #146)
> It is test build that applies MLTE adaptation v3.
> http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-v3-testbuild.mac.dmg
Woohoo!!!...

Thanks  everyone...
This is the first time I have gotten satisfactory rendering of gujarati and hindi text on gu.wikipedia.org and hi.wikipedia.org.

I tested the editing on the same websites and editing.. (click the third tab from left on the wikipages, the one with a + sign next to it). It seems editing on en.w.o is fine but on gu.w.o its bad... doesnt matter whether you are using indic script or english on either.

Thanks for the improvements.

Comment 150

[John Dent]

I checked www.bbc.co.uk/hindi and that seems to render much much better. The fact that it is at all readable is brilliant. But it even gets the i, ii, u and uu Hindi vowels correct. Brilliant. A massive improvement.

Comment 151

[Simon Fraser [no longer active]]

Unfortunately this patch has a pretty significant impact on pageload time:

Firefox 12/11 trunk build:
Avg. Median : 1167 msec

MLTE build
Avg. Median : 1355 msec

Comment 152

[YAMASHITA Makoto]

(In reply to comment #151)
> Unfortunately this patch has a pretty significant impact on pageload time:
Maybe this is because current version redirects char* based measurement/drawing funcs to the PRUnichar* based ones in nsRenderingContextMac.cpp, as noted in comment 137. This can be optimized to do so only for boldface/italic cases.
I want to try this and see how much it works. So could you teach me how to measure the pageloads?

Comment 153

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: bookmarks toolbar overlap with MLTE build

Comment 154

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: overlap in bugzilla with MLTEv3 build

Comment 155

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: mozilla.org/projects/firefox/ with MLTEv3 build

Comment 156

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: "rough testcases" for overlap issues with MLTE v3

Thanks, crot0, for making the build available.  I used it and went through my list of font bugs and was pleased to discover the MLTEv3 patch fixed a large number of them (mostly bad font-substitution issues in Roman script languages), based on the testcases attached: Bug 208037 (HTML testcase only), Bug 212745, Bug 201817 (only testcase 1, & only no font specified section), Bug 158560, Bug 148361 (xml testcase only), Bug 313037 and http://forums.mozillazine.org/viewtopic.php?t=352122 (know there's already a bug on this, can't find it atm).

There is one big drawback to this fix that I can see so far: there are serious rendering regressions with small fonts (screenshots above, and the illustrations in this "testcase").  Hopefully it can be worked-around?  (I'm also seeing some random crashes in libgklayout.dylib which perhaps are related to this patch, too; since I don't usually run trunk Fx, it's hard to say).

Bug 246527 is still going to be an issue for our Indic and other fonts (e.g., Devanagari MT does not appear in the font list), but this patch improves the situation by making an end-run around it and somehow using the fonts anyway, which is good.

Speed felt OK, but I don't run Fx enough (and it always feels slower than Camino) to really contribute usefully here.

Overall, it seems like a vast improvement for our non-Western European users....

Comment 157

[Paul Libbrecht]

(In reply to comment #146)
> http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-v3-testbuild.mac.dmg

I had a stab at that just to make sure MathML was working or enhanced.
For example from: 
   http://www.w3.org/Math/testsuite/
It seems to work as well as earlier.
(just FYI, MathML used to be plagued by font problems until very recently so I expected it to become better with ATSUI... I've seen no major change).

Comment 158

[YAMASHITA Makoto]

Attachment: MLTE adaptation v4

The code is now using char* based funcs as much as possible in nsRenderingContextMac.cpp:
 - layout problems for English are mostly fixed.
 - the performance is now (slightly) better than the official distribution: my (opt=-02 & static) build does Avg. Median : 1463 msec against Avg. Median : 1544 msec by the official 12/13 trunk build, at my place.

> I can't get any Japanese text (Kanas or Kanjis) to output in bold.
Hiragino boldface selection is now OK, isn't it?

> Using this build, text underline is positioned too near to the text (at least
> w/ Windows Arial).

> bookmarks toolbar overlap with MLTE build
If these still happen, please provide more specific info/testcases that isolate the problem.

> expected it to become better with ATSUI... I've seen no major change
We aren't using Unicode rendering for symbolic fonts like CM fonts, Mathematica fonts and Symbol. However the rendering of CODE2001 (baseline issue) is improved.

Comment 159

[YAMASHITA Makoto]

Comment on attachment 205953 [details] [diff] [review]
MLTE adaptation v4

I've found that MathML (symbolic font rendering) is broken by this.

Comment 160

[YAMASHITA Makoto]

Attachment: MLTE adaptation v5

fixed symbolic font issue.

Comment 161

[Hiro]

Attachment: screenn shot of MLTE-v5

(In reply to comment #160)
> Created an attachment (id=206133) [edit]
> MLTE adaptation v5
This patch was tried. 
Japanese bold is light, and doesn't make a fine show. 
Cannot this correspond by the specification?
If it is so, this is not accepted. 

Page displayed in test:http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2429

Comment 162

[philippe (part-time)]

Attachment: Screenshot in comment#161 next to real page seen in Camino

Judging from the screenshot in comment 161, the bolding of Japanese characters is indeed not good.  In my screen shot, the rendering in Camino (window on the right) is the expected rendering.

( crot0, can you share your builld again ? I don't have time , nor resources to build one my self at the moment. Thanks)

Comment 163

[Hiro]

MLTE v5 test build:
http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-v5-testbuild.dmg

Comment 164

[Greg K.]

Attachment: Demonstration of persistent substitution and baseline trouble using patch v5 build (comment 163)

There are still some lingering substitution and baseline-shift problems with patch v5. (Screenshot forthcoming.)

Comment 165

[Greg K.]

Attachment: PNG screenshot of attachment 206178 from MLTEv5 test build of comment 163

Comment 166

[YAMASHITA Makoto]

(In reply to comment #165)
Hmm... I've installed the TITUS font but couldn't reproduce your problem. The SEMISOFT SIGN is rendered with TITUS and no baseline shift occurring. Does anybody else see this problem?

Comment 167

[YAMASHITA Makoto]

(In reply to comment #164)
> Created an attachment (id=206178) [edit]
> Demonstration of persistent substitution and baseline trouble using patch v5
BTW your testcase is with CYRILLIC SMALL LETTER YAT at the third char of the last line, rather than CYRILLIC SMALL LETTER SEMISOFT SIGN, isn't it? My build looks ok with both of them anyway.

Comment 168

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I see identical results as Greg does in comment 163 (substitution and baseline shift).  10.3.9 here.

I also still have the problem where a bookmarks toolbar item overlaps the chevron; I suspect this is because Fx is incorrectly calculating the space needed for the "completely Arabic script" bookmark item to begin with, and this patch has improved bits of rendering that have exacerbated the old bug.  I'll upload my bookmarks file in a second so everyone can play along at home (note the extra space on the left side of the "completely Arabic script" item--and that space's inclusion in the "hover" oval).

I'm also still seeing crashes in libgklayout; including one at startup.  I'm not finding anything in Bugzilla or Talkback about this, so I suspect it is caused by this patch.  Am I the only one seeing it?

Comment 169

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: libgklayout crashlog

Comment 170

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: My Firefox bookmarks

Comment 171

[philippe (part-time)]

1. Comment 168, I see the same problem with the chevron on the bookmarks bar in regular Firefox trunk builds, with the default theme.

2. When I first loaded this build (comment 163), I had a strange painting bug. I loaded a page, and viewed source. After closing the view-source window, the part of the page that was not covered by the view-source window wouldn't scroll anymore. Hiding the application and showing it again, and the whole chrome disappeared. Unfortunately, I can't reproduce anymore.

3. Bold doesn't work correctly for Japanese characters, as mentioned before. A simplified test case.
http://dev.l-c-n.com/Gecko-bug121540/test2-japaneseBold.php
There is no font-substitution, and no attempt, or rather a poor attempt at emulating the bold. If I force the use of a 'bold' font-family, then I get bold, ofcourse.

4. I see overlapping strings of characters in some cases. Live example:
<http://emps.l-c-n.com/articles/97/notes-on-textpattern>. Scroll down the page to the heading 'Final'. The links overlap previous characters. 
Or <http://emps.l-c-n.com/articles/94/widgets-for-firefox>, First paragraph (the abbreviation HIG).

This happens only with certain font-families AND only when I use numerical entities (& #39; or & #8212; as examples, white-space added to avoid parsing) somewhere before the offending span (code, a, abbr, ...) in the same block-level element.

A simplified testcase:
http://dev.l-c-n.com/Gecko-bug121540/test-1-overlap-entities.php
Using 'Lucida Grande' works correctly. Using 'Optima' shows the problem. I've seen a live page that uses Arial with the same problem, but I don't remember which one :-(.

5. Rendering speed and scrolling speed seem OK, although I mostly use Camino trunk builds these days. Font-display is fine otherwise. Japanese (Hiragino) feels smoother than regular trunk builds.

Comment 172

[Greg K.]

(In reply to comment #166)
> Hmm... I've installed the TITUS font but couldn't reproduce your problem. The
> SEMISOFT SIGN is rendered with TITUS and no baseline shift occurring...

I presume you're using your own build. Try the comment 163 build also, maybe it's faulty in some way.

Comment 173

[YAMASHITA Makoto]

(In reply to comment #172)
> I presume you're using your own build. Try the comment 163 build also, maybe
> it's faulty in some way.
Both the comment 163 build and my own are working ok (and my system is 10.3.9).
To be precise, it looks like an explicit "font-family:TITUS.." is not working and the font is chosen as an fallback, which may cause the problem if you have some font settings. Does that happen when you remove "Firefox" folder from "~/Library/Application Support"?

Comment 174

[Greg K.]

(In reply to comment #173)
> Both the comment 163 build and my own are working ok (and my system is 10.3.9).
> To be precise, it looks like an explicit "font-family:TITUS.." is not working
> and the font is chosen as an fallback, which may cause the problem if you have
> some font settings. Does that happen when you remove "Firefox" folder from
> "~/Library/Application Support"?

Yes, it does. (I'm on 10.4.3, by the way). I'd just ignore it for now, since it works for you. Let's puzzle out the details later.

Comment 175

[YAMASHITA Makoto]

(In reply to comment #174)
> Yes, it does. (I'm on 10.4.3, by the way). I'd just ignore it for now, since it
> works for you. Let's puzzle out the details later.
I confirmed the problem on another machine (I have no idea why it's ok at my place). From my quick experiment the characters larger than u0460 (+ u0450 and u045D) are affected. The problem does not happen when I set "TITUS" for the default Cyrillic serif font at the pref. Let's investigate this later anyway...

Comment 176

[YAMASHITA Makoto]

Attachment: MLTE adaptation v6

What's changed:
1. Japanese goes through Quickdraw rather than MLTE
Now Hiragino boldface is heavier, but this one is not without problems (these have been present before the patch):
 The overall Hiragino rendering got shaggy a bit.
 Boldface synth is sometimes ugly, e.g. the lower horizontal stroke of KATAKANA LETTER KO is too thincompared to the other parts.
The cause of this problem (for either ATSUI or QD) is that Hiragino Kaku Gothic Pro W3 and Hiragino Kaku Gothic Pro W6 are treated as separate families, forcing the rendering system to synth the boldface version.
Since there are pros and cons for ATSUI and QD, I think this is a "take a vote" issue. I personally prefer ATSUI.

2. More greedy ASCII optimisation
 resolves the problem reported in comment 171, 4.

Comment 177

[Hiro]

(In reply to comment #176)
> Created an attachment (id=206387) [edit]
> MLTE adaptation v6

test build:
http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-v6-testbuild.dmg

> 1. Japanese goes through Quickdraw rather than MLTE
> Now Hiragino boldface is heavier, but this one is not without problems (these
> have been present before the patch):
>  The overall Hiragino rendering got shaggy a bit.
>  Boldface synth is sometimes ugly, e.g. the lower horizontal stroke of KATAKANA
> LETTER KO is too thincompared to the other parts.
> The cause of this problem (for either ATSUI or QD) is that Hiragino Kaku Gothic
> Pro W3 and Hiragino Kaku Gothic Pro W6 are treated as separate families,
> forcing the rendering system to synth the boldface version.
> Since there are pros and cons for ATSUI and QD, I think this is a "take a vote"
> issue. I personally prefer ATSUI.

I am usually using Hiragino Maru Gothic by Camino or Firefox. 
It used it with Safari for the first time by this font. 
As a result, it became the same display as attachment (id=206152).

Bold might worsen by Firefox etc. in the font setting of default. 
But when the font used is changed into another, it has the possibility that the same 
display as Safari results. 
And because ATSUI smoothly displays the character, I also like this. 

Comment 178

[philippe (part-time)]

(In reply to comment #176)
> Created an attachment (id=206387) [edit]
> MLTE adaptation v6
> 
> What's changed:
> 1. Japanese goes through Quickdraw rather than MLTE
> Now Hiragino boldface is heavier, but this one is not without problems (these
> have been present before the patch):

I haven't done much testing with the latest patch (thanks foor the build :-)). 
I few notes about Japanese font-rendering
- it is a bit less smooth than the previous patch, but not worse than the current rendering in Camino (trunk)/Firefox (trunk).
Bold is back.
- Some special characters don't render correctly when Hiragino Kaku Gothic w6 is specified (see my testfile in comment 171). 
-Some characters render very small: see the squares in this testfile
<http://dev.l-c-n.com/Gecko-bug121540/test3-japanese.php>
I've seen this with Webkit in the past . In Safari 2.0.2 it seems to be fixed, I still see that sometimes in SubEthaEdit (texteditor, uses webkit for text rendering). But they always render correctly in Gecko.

> 
> 2. More greedy ASCII optimisation
>  resolves the problem reported in comment 171, 4.
> 
That works fine now, tested on a whole range of fonts commonly availlable on OS X

Comment 179

[Spundun]

I have narrowed down the problem that I am having on gu.wikipedia.org to the following thing.
These are three simple pages with only a text area to type in. 

http://www.isi.edu/~spundun/tmp/test-en.html
http://www.isi.edu/~spundun/tmp/test-hi.html
http://www.isi.edu/~spundun/tmp/test-gu.html

The en and hi page work fine but when typing in the gu page I get pixel dirt. Just type about 5 ='s (=====) you will only see 5 and will also see pixel dirt if you press backspace. My guess is that the text renderer thinks it needs larger character sizes (just because the lang="gu" parameter in the html) and it makes the text area larger but the cursor is still moving at the regular speed.

I dont understand why firefox treats lang="gu" parameter specially by making everything large while safari doesnt do anything like that.

Thanks
Spundun
p.s. : I did validate the pages with w3.org

Comment 180

[YAMASHITA Makoto]

(In reply to comment #178)
> - Some special characters don't render correctly when Hiragino Kaku Gothic w6
> is specified (see my testfile in comment 171).
Which characters are affected? 
> -Some characters render very small: see the squares in this testfile
> <http://dev.l-c-n.com/Gecko-bug121540/test3-japanese.php>
This is "a feature not a problem", because Lucida's BLACK SQUARE is rather small compared to Hiragino's. If you set lang="ja", you get bigger squares thanks to the Hiragino priority.

(In reply to comment #179)
I guess this problem is caused by a font inconsistency between text measurement time and drawing time. When I set fonts for Gujarati manually (Times, Lucida Grande, Monaco) at the pref pane, the things looks ok to me. Though I'm not sure what happens when actual Gujarati characters are typed/Gujarati fonts are specified...

Comment 181

[Spundun]

(In reply to comment #180)
> (In reply to comment #179)
> I guess this problem is caused by a font inconsistency between text measurement
> time and drawing time. When I set fonts for Gujarati manually (Times, Lucida
> Grande, Monaco) at the pref pane, the things looks ok to me. Though I'm not
> sure what happens when actual Gujarati characters are typed/Gujarati fonts are
> specified...
Hmm.. I looked into this. It works for me also just like you said.
Specifically  only font.name.monospace.x-gujr needed to change to Monaco or Monospace or Courier (I tried only these three) and it works. The default value for this parameter is empty. But looks like the hi equivalent doesnt have this value either but that one works. I dont know why it doesnt work with gu. But once I set that value.. the rest seems perfect... Editing gu.wikipedia.org is fine. If this patch makes it into the trunk, I will make firefox my default browser again. :) . It would be nice to get this defaullt parameter thing sorted out though.

Also, this patch will work for thunderbird also, right? Can crot0 post a thuderbird build with this patch also please?

Thanks a lot for the great work
Spundun

Comment 182

[Hiro]

MLTE patch v6 test build of Tb:
http://www.bekkoame.ne.jp/~h-sek/thunderbird-1.6a1.MLTE-v6-testbuild.dmg

Comment 183

[Spundun]

(In reply to comment #182)
> MLTE patch v6 test build of Tb:
> http://www.bekkoame.ne.jp/~h-sek/thunderbird-1.6a1.MLTE-v6-testbuild.dmg
Works like a charm! Thanks crot0, you've been very helpful.

What needs to be done before this can go into cvs? (can it make it into the 1.5 branch?) What should be done to get the module owner's attention to the fact that we have a patch that gets in a major functionality without a performance hit?

Spundun

Comment 184

[Jungshik Shin]

I'd love to see this in, but it isn't likely to happen given the current status of Gfx (see sfraser's comment #138). It'd have been really great if this work had been done before 1.5. I guess one option for South/SouthAsian users at this point is to patch 1.5 source with the latest and greatest patch, build a binary and put it up at ftp.mozilla.org (contrib directory). 

Comment 185

[Greg K.]

(In reply to comment #184)
> I'd love to see this in, but it isn't likely to happen given the current status
> of Gfx (see sfraser's comment #138). It'd have been really great if this work
> had been done before 1.5. I guess one option for South/SouthAsian users at this
> point is to patch 1.5 source with the latest and greatest patch, build a binary
> and put it up at ftp.mozilla.org (contrib directory). 

Don't be too pessimistic. All the talk of Cairo is still just that at the moment. Cairo on Mac is likely at least two years away. The port has not even begun, they're still looking for help.

This helps solve a lot of existing bugs in Mac text, so there's a good chance it can be checked into the trunk and let bake there for a while. In a few months maybe we can look at the 1.8 branch again (if allowed).

Comment 186

[Spundun]

(In reply to comment #184)
> I'd love to see this in, but it isn't likely to happen given the current status
> of Gfx (see sfraser's comment #138). 
comment #138 seems to mean that this work will eventually be replaced by cairo. It doesn't talk about this work hindering cairo or any other reason why this work (if finished) shouldn't make it into the code.

Look at the following commment #139. 

I have waited about 2 years already for getting indic support in Thunderbird and Firefox. Waiting another 2 years just for cairo backend while this backend is finally ready is very unreasonable. 

Comment 187

[Simon Fraser [no longer active]]

I think this has a good chance of getting if:
1. We have solid performance data to show that it doesn't hurt performance much, if at all.
2. We have thorough testing on 10.2, 10.3 and 10.4, focussing both on visual appearance, and stability and lack of leaks.
3. We have a clear description of what the changes are doing (to help with review).

Comment 188

[Jungshik Shin]

Ok, guys. Just in case, I do care about Indic script rendering very much (see, for instance, bug 215219). In addition, the patch here fixes several other bugs as well (some of which affect CJK users). 

(In reply to comment #187)
> I think this has a good chance of getting if:

That's great to hear. I'll give my time (however short it may be) to help making those if's reality. I began to regret upgrading to 10.4 :-)
 

Comment 189

[YAMASHITA Makoto]

Then I'm going to prepare a review ready version.  And let's write a long petition when it's done...  Before that, a couple of notes on current "obstacles."

- I'm going to get the Japanese handling back to that of v5; v6 is reintroducing the bug 208037.  See this attachment, the first word in the third line from the bottom wWhen the Cyrillic serif font is set to TITUS in the pref.
I doubt if TITUS is actually used in the screenshot of attachment 206579 [details].  They might be falling back to Times CY or something else set for default Cyrillic serif font.  Please check that out again.
In any case there remains a similar problem when I try explicitly specify Times New Roman for Cyrillic strings (maybe you can check this by setting the default Cyrillic serif font to an irrelevant one).
After all it looks like there's a Cyrillic glyph representability bug outside this patch,which should be more comprehensively investigated in bug 208037.
Let's consider Hiragino boldface issue when this bug is done (the special casing is not so difficult anyway).

- Launch time crash: I did confirm that (that happens when  the pref files by the official is present).  But I can't say this patch is guilty for that.  It's not touching the layout lib and there is a similar issue for the chevron trunk build.

- Performance: I don't think this patch could hurt pageload scores: it's doing ASCII only optimization when possible, and the current measurement scheme we are using is limited to English pages.  I think the approach by this patch outperform the old one, but it's not captured too...

Comment 190

[YAMASHITA Makoto]

Attachment: Cyrillic glyph representability issue testcase

> See this attachment, the first word in the third
I meant this testcase. how could I forget to put this together? so sorry for confusing comment.

Comment 191

[Greg K.]

(In reply to comment #189)
> I doubt if TITUS is actually used in the screenshot of attachment 206579 [details] [edit].  They
> might be falling back to Times CY or something else set for default Cyrillic
> serif font.  Please check that out again.

Ah, you’re right. TITUS and Times Cyrillic glyphs look very similar. If you paste that line of text into TextEdit and set it to Times you’ll see the same bad letter Yat as in Gecko. So, FF isn’t even trying to use TITUS Cyberbit Basic, it seems.

Comment 192

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7

The changes are mostly of brush-up kind. Here's a concise description of what the patch changes at last:
nsATSUIUtils.cpp
- DrawString is changed to use MLTE
- nsATSUIToolkit has a new member var, mMLTEOption, to be fed to MLTE calls.
- The code path in GetTextLayout to create an ATSUStyle with given text style (font, sizse, bold and italic) is put in a separate function, as it is also referred from DrawString.

nsUnicodeFontMappingMac.cpp
Script code based conversion is cut off so that the conversion happens only for symbolic fonts.  Anyway it's too naive to assume a modern font is associated to a particular script...

nsUnicodeRenderingToolkit.*
- The *TextSegment funcs first checks if the primary font has the converter (this only happens when it is a symbolic font)
- If that is the case, converted data is fed to QD as it used to be.
- If not, try ASCII conversion. If it succeeds the data is fed to QD.
- Otherwise the data is fed to nsATSUIToolkit.
- fallback code paths beneath *TextSegment funcs are cut off.
- ascii optimization helper funcs (IsASCII, GetASCII) are added.
- another helper func (GetFontRuns) that splits the string into font runs are added.  Strings in ltr scripts (Hebrew and Arabic) are split char by char to accomodate the visual ordering by the core.  For ASCII optimization consecutive ASCII chars form  separate segments apart from non-ASCII chars.

My perf measurement is on 10.3.9. jshin, could you do it on 10.4?

Comment 193

[Simon Montagu :smontagu]

(In reply to comment #192)
> Strings in ltr scripts (Hebrew and Arabic) are split char by char to
> accomodate the visual ordering by the core.

Is there no way to avoid this? I understand that there wasn't when Hebrew support was first ported to Mac [1], but it causes serious performance problems [2]. If you set NS_RENDERING_HINT_BIDI_REORDERING as Windows and Pango do, the visual ordering by the core will be turned off.

[1] Bug 117098 comment 20
[2] Bug 188294 comment 17 and references there

Comment 194

[Julian D. A. Wiseman]

Bug 212745 references this, on the subject of ♦ and other suit characters not being correctly rendered on Mac under 1.5. 

Comment 195

[YAMASHITA Makoto]

Attachment: Bidi support

Modified GetHints of nsRenderingContextMac.cpp, added direction awareness to nsUnicodeRenderingToolkit.cpp and nsATSUIUtils.cpp. (and coding refinements, like unnecessary var, or "new char[...]" to "nsMemory::Alloc", etc.)
I had to introduce a bad hack in DrawString of nsATSUIUtils, because ASCII chars rendering need to be aware of the text direction. An open parenthesis should be rendered as ")" for rtl script.
Similarly ", " (arising from e.g. <a>... u05d0</a>, <b>u05d3 ...</b>) should appear as " ,".
Judging from what's working and what's not, ATSUI counts whitespace at the edge only when the string is just one char whitespace, so the current approach is a bit different from the one in my comment 137.
The perf. meas. (for English pages, of course) hasn't changed since my comment 158.

Comment 196

[Jungshik Shin]

Yamashita-san, as a part of what's suggested in comment #187, have you compared the performance for pages with a small number of non-ASCII characters. What I have in mind is pages in such languages as German, French, Spanish, Swedish. I'm not sure how much important those cases should be, but would be interesting to see how it works. Well, I can do that myself. I'll try it on 10.4.x as asked for in comment 192 (pls, wait for a few days, though). Is there anyone still with 10.2.x to try a test build? 

Comment 197

[YAMASHITA Makoto]

(In reply to comment #196)
> the performance for pages with a small number of non-ASCII characters. What I
I've browsed through wikipedias and a few others (governments, newspapers...) with no conceivable regression. Yet I don't have precise data. Is there a way to get it?
Anyway I think my approach can be faster than the offical build's for non-ASCII western scripts, because MLTE/ATSUI is probably accessing glyphs via unicode mapping tables rather than doing Unicode->script specific encoding conversion.

Comment 198

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #196)
> Is there anyone still with 10.2.x to try a test build? 

The only people I know still with 10.2.x machines are Torben and Jo Hermans; I've cc'd them here to see if they can help.

Comment 199

[Jo Hermans]

(In reply to comment #196)
> ... I'll try it on 10.4.x as asked for in comment 192 (pls, wait for a few days,
> though). Is there anyone still with 10.2.x to try a test build? 

I'm on 10.2.8, using an old 300 MHz iBook, and I have surfed for a few hours with the build from comment 177 (MLTE adaptation v6). I've mainly surfed on Dutch, Spanish, French and German newspapers sites and on wikipedia. I'm using the default install of this OS, so I have Arabic, Korean, Russian, Japenese, Chinese, ... but no Hindi, Tamil, Telugu, etc ...

This build looks very nice, ATSUI improves the text-rendering a lot. But it feels as little slower than an official Firefox 1.5 build (could also be because of the 1.5/1.6 differences). But this is an old OS-release, on very slow hardware. The text-rendering perforcement has been improved a lot since (extra caches in 10.3 if I remember correctly), so I'm not surprised that people on 10.3 or 10.4 don't report this. I can see that swapping tabs with different charsets (I have a dozen different pages on wikipedia open at the same time) is very slow, but I can't see if this is due to general swapping, or to mucking around with the charset caches in either Firefox or ATSUI. Nobody expects that you use a dozen charsets at the same time, except maybe on wikipedia (at the bottom or lower-left). I would not be surprised that various caches are too slow to handle them all together.

Don't let this block further development though. My iBook was already slow, having it a little slower don't hurt so much, especially when you see the improvements in rendering. I'm a real Mac user, who prefers beauty over speed :-)

problems :

- http://vi.wikipedia.org/wiki/Trang_Ch%C3%ADnh looks weird (Vietnamese accented characters are below baseline). It has also has a bold-problem like previously reported for Japanese

- http://fa.wikipedia.org/wiki/%D8%B5%D9%81%D8%AD%D9%87%D9%94_%D8%A7%D8%B5%D9%84%DB%8C (Persian) and http://ar.wikipedia.org/wiki/%D8%A7%D9%84%D8%B5%D9%81%D8%AD%D8%A9_%D8%A7%D9%84%D8%B1%D8%A6%D9%8A%D8%B3%D9%8A%D8%A9
 (Arabic) scroll much slower, but they seem to be using different fonts too.

- http://cs.wikipedia.org/wiki/Hlavn%C3%AD_strana (Czech) has no accent on the first character of Česká (first paragraph). But when I selected it and copied it in another app, it had one again, so it's some kind of display issue. Firefox 1.5 and Safari 1.0.3 show it correct.

Comment 200

[Torben]

Attachment: Erroneous display of Norwegian letters

I finally got time do do some quick testing, iMac G4/800, 512MB, OS X 10.2.8:

I don't notice any speed differences between the MLTE v.6 test build (from comment 177) and a current Firefox trunk nightly (but then I always find Firefox slow compared to Camino). However, on some pages (eg. http://www.aftenposten.no) Norwegian characters (æ,ø,å) are shown erroneously (almost bolded, see bottom of the attachment), refreshing the display by selecting the text fixes the display.

Will try to get time to make a Camino-build with version 7 for more testing later this week.

Comment 201

[YAMASHITA Makoto]

Attachment: Expand optimization target from ASCII to MacRoman + MLTE clip hack

looks like we have a problem in rendering accented uppercase letters in Times and Helvetica. Those fonts declare shorter ascents so that accent parts is beyond "maximum y line" as you can see by selecting the strings to highlight the text rectangle. Maybe this is a bug of MLTE. On the contrary Times New Roman, Century, Arial and Lucida Grande, Courier and Monaco are not affected by this problem.
As a way round, I expanded the target of optimization from ASCII to MacRoman. Doing this in nsUnicodeRenderingToolkit, because we don't want that "optimization" when we are on right to left script.
And another bad hack in nsATSUIUtils.cpp to evade the clipping off by setting the nonzero (yet very small) rotation. With this, "something" is visible on top of C-caron glyph of Times. However I couldn't make Helvetica work anyway.
After all, the changes haven't changed the speed on my machine.
Given the apparent baseline/rendering quality problems for non-MacRoman characters in the current releases, I think this patch in its current state is worth landing on the trunk in favor of the benefits it brings in (consistent Lucida rendering and the whole availability to the asian scripts).

Comment 202

[Hiro]

(In reply to comment #201)
> Created an attachment (id=207814) [edit]
> Expand optimization target from ASCII to MacRoman + MLTE clip hack

This has been used before FloatToFixed is defined. 
Therefore, it becomes build erro. 

+  mMLTEOption.rotation = FloatToFixed(0.01);
+  mMLTEOption.options = NULL;
+
+  // ltr is default
+  mRightToLeftText = PR_FALSE;
 }
 
 
 //------------------------------------------------------------------------
 //     GetTextLayout
 //
 //------------------------------------------------------------------------
 
 #define FloatToFixed(a)                ((Fixed)((float)(a) * fixed1))


build error log:
-config.h -Wp,-MD,.deps/nsATSUIUtils.pp nsATSUIUtils.cpp
nsATSUIUtils.cpp: In constructor `nsATSUIToolkit::nsATSUIToolkit()':
nsATSUIUtils.cpp:259: error: `FloatToFixed' undeclared (first use this 
   function)
nsATSUIUtils.cpp:259: error: (Each undeclared identifier is reported only once 
   for each function it appears in.)
make[6]: *** [nsATSUIUtils.o] Error 1
make[6]: Leaving directory `/Users/sek/Documents/mozilla-current/camino/mozilla/gfx/src/mac'
make[5]: *** [libs] Error 2

Comment 203

[YAMASHITA Makoto]

(In reply to comment #202)
> This has been used before FloatToFixed is defined. 
> Therefore, it becomes build erro. 
oops, really? I didn't encounter this and there's a *system* func named FloatToFixed declared.
http://developer.apple.com/documentation/Carbon/Reference/Mathematical_al_Utilities/Reference/reference.html#//apple_ref/doc/uid/TP30000236-CH1g-BABCCCCA
I'm going to look on this, but could you move that macro above for the time being?

Comment 204

[Hiro]

(In reply to comment #203)
> oops, really? I didn't encounter this and there's a *system* func named
> FloatToFixed declared.

Build was tried with Camino. 
The build environment is as follows. 
  Mac OS X 10.3.9
  Xcode 1.5 + latest update

> I'm going to look on this, but could you move that macro above for the time being?
Macro was moved and build succeeded. 
And it was confirmed that the same definition was in system. 
But in Camino build, the setting of system might not be referred to to specify "ac_add_options --with-macos-sdk=/Developer/SDKs/MacOSX10.2.8.sdk".
Perhaps, I think Mac OS X 10.2.x to be a similar occurrence of the error. 

http://developer.apple.com/documentation/Carbon/Reference/Mathematical_al_Utilities/Reference/reference.html#//apple_ref/doc/uid/TP30000236-CH1g-BABCCCCA
From the explanation of FloatToFixed:

Availability

    * Available in Mac OS X version 10.3 and later. 

Comment 205

[Mark Mentovai]

(In reply to comment #203)
> oops, really? I didn't encounter this and there's a *system* func named
> FloatToFixed declared.

From your referenced URL:

Availability
 * Available in Mac OS X version 10.3 and later.

Sorry, please don't rely on this macro being predefined by the system includes.  For the time being, we need to be able to build using only 10.2 APIs.

Comment 206

[YAMASHITA Makoto]

Attachment: MLTE adaptation v9.1

Just a few writing refinements from the previous one: in particular renamed that FloatToFixed macro to FLOAT_TO_FIXED because it collides with the sys func and does not comply with the coding style guide.

Comment 207

[Hiro]

Attachment: screen shot of MLTE adaptation v9.1

MLTE adaptation v9.1 was tried. 
Japanese parentheses and "Y" overlap like the screen shot . 
URL:
http://www.watch.impress.co.jp/akiba/

Comment 208

[Hiro]

Attachment: screen shot of MLTE adaptation v9.1 with arena.nikkeibp

Similarly, Japanese overlaps with the alphabet. 
URL:
http://arena.nikkeibp.co.jp/stage/av/

Comment 209

[YAMASHITA Makoto]

Attachment: MLTE adapatation v9.2

Looks like it's caused by a hanging-quotes related inconsistency between ATSUI (measurement) and MLTE. Counting that.

Comment 210

[Hiro]

(In reply to comment #209)
> Created an attachment (id=207967) [edit]
> MLTE adapatation v9.2
This patch was tried. 
The problem of commnet#207 and comment#208 does FIXED. 

Comment 211

[Hiro]

FYI

MLTE V9.1 test build:
http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-V9.1-testbuild.mac.dmg

Comment 212

[philippe (part-time)]

(In reply to comment #211)
> FYI
> 
> MLTE V9.1 test build:
> http://www.bekkoame.ne.jp/~h-sek/firefox-1.6a1.MLTE-V9.1-testbuild.mac.dmg
> 

With this build, after some light surfing around
* on http://blog.fawny.org/2006/01/09/leslieville/#recent,
under the heading 'recent postings' next to start the character ⇑ fails to display on my side, it works fine in the latest nightly Camino trunk build.
* on Japanese pages, is there a difference in line-height or baseline adjustment ? On http://www.weathermap.co.jp/monkeyweather/region/053.php3,
there is a significant difference (screenshot coming) between the latest Camino trunk build and this Firefox build (my settings should be identical). Also the  select menus on the right are much (too) tall. 
Japanese text looks good otherwise (much smoother than the default builds), except for the alteady mentioned problem with bold text.

Comment 213

[philippe (part-time)]

Attachment: screenshot for comment #212

Camino (latest trunk build) next to the Firefox build in comment #211.
Firefox is on the right side.
Both browsers have the same settings (font-sizes, etc).
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060109 Camino/1.0+

Comment 214

[philippe (part-time)]

Attachment: line-height comparison

Test case: http://dev.l-c-n.com/Gecko-bug121540/test3-japanese-lineHeight.php

Further on that line-height difference: in the screenshot, Camino on the left, Firefox on the right.
It seems only to affect the 'normal' keyword (that is: the default value in the UA stylesheet). The test case has 3 boxes. The first one leaves the line-height to the UA (normal keyword), the second box (test 2) sets the line-height to 'normal' in the author stylesheet. The third box (test 3) sets the line-height to '1.5'. In this last case, the display is identical (and correct !) between Camino trunk build and this Firefox build.

Comment 215

[YAMASHITA Makoto]

(In reply to comment #212)
> under the heading 'recent postings' next to start the character ⇑ fails to
> display on my side, it works fine in the latest nightly Camino trunk build.
Isn't AppleGothic used for that char? That font is erroneously mapping the open square glyph to that character. What happens when you explictly specify Symbol (or Apple Symbols) for that?

Comment 216

[philippe (part-time)]

(In reply to comment #215)
> (In reply to comment #212)
> > under the heading 'recent postings' next to start the character ⇑ fails to
> > display on my side, it works fine in the latest nightly Camino trunk build.
> Isn't AppleGothic used for that char? That font is erroneously mapping the open
> square glyph to that character. What happens when you explictly specify Symbol
> (or Apple Symbols) for that?
> 
Hmm, you right, it is a font problem. Camino trunk displays it correctly though. I picked up that page, knowing that the author (Joe Clark) has good knowledge of html.
If I check this reference chart, &uArr and &dArr are the only two affected.
<http://www.digitalmediaminute.com/reference/entity/index.php>
Safari displays those two correctly, only when the font-family is set to 'serif'.

Little test-case, if you feel like comparing with other browsers, forcing 'symbol' in one case.
<http://dev.l-c-n.com/Gecko-bug121540/test-5-entities.php>

Comment 217

[Hiro]

Attachment: screen shot of MLTE V9.1 with JM manual page

There is an overlap in some parts. 
URL:
http://www.linux.or.jp/JM/html/GNU_patch/man1/patch.1.html

Comment 218

[YAMASHITA Makoto]

Attachment: MLTE adaptation v9.3

Looks like an ascii (segment) measurement issue; the English words and the Japanese chars come separately in the measurement while the whole line comes in one string in the drawing process.
Now the code is doing:
  When the font is in Roman script: use QD for MacRoman chars
  When it is not: use QD for ASCII chars
  Otherwise MLTE/ATSUI

Comment 219

[YAMASHITA Makoto]

Now I have a performance measurement on Tiger: Avg. Median is 1476 msec by mine vs. 1504 msec by the official trunk build.
As this work left open here is becoming an obstacle for my work at bug 162431 and thebes/mac things, I'd like to see some sort of descision on the fate of it by the end of January.
If this one is considered to be too risky to put into the official tree anytime soon, I accept that and may be able to offer a "contribution" build for anybody interested in...

Comment 220

[Simon Fraser [no longer active]]

If you now have a patch that you believe is good enough quality to check in, you should seek review and super-review. When those are given, it can be landed on the trunk, where it can be tested. If it tests well, then it may be migrated onto the various branches.

Comment 221

[Ali Rastegar]

There are some bugs with BiDi text and Firefox 1.6 MLTE v 9.1 patched test build. are there any newer patched versions so I can test BiDi texts with?
Thanks

Comment 222

[YAMASHITA Makoto]

(In reply to comment #221)
I don't think there is any difference between v9.1 and v9.3 for Arabic text handling. Is your problem something like the ones in 153, 168, 179-181?

Comment 223

[Ali Rastegar]

(In reply to comment #222)
On some webpages such as http://irmug.org/portal/index.php , Mirroring Characters are not correct. you can find more information about mirroring character here:
http://www.abisource.com/help/en-US/howto/howtobidi.html#mirroring
and:
http://www.unicode.org/Public/UNIDATA/BidiMirroring.txt
some examples of incorrect mirroring characters:
http://irmug.org/incorrect.jpg
some examples of correct mirroring characters:
http://irmug.org/correctmirror.jpg
Thanks

Comment 224

[YAMASHITA Makoto]

(In reply to comment #223)
> (In reply to comment #222)
> On some webpages such as http://irmug.org/portal/index.php , Mirroring
> Characters are not correct. you can find more information about mirroring
Hmm... in more simple cases like this
http://people.w3.org/rishida/scripts/samples/arabic.html
we don't encounter this problem. I tried in vain to isolate the problem by just copying a code chunk from your link (the extracted one was rendered ok). Could you provide a simple testcase for this?

Comment 225

[Ali Rastegar]

(In reply to comment #224)
1.It seems that Verdana causes this problem!! please go to this page:
http://people.w3.org/rishida/scripts/samples/arabic.html
at the settings section, change the font name to Verdana then click apply. as you can see same problem happens. click justify, everything seems to be OK with justify!
here is also one simple page with body style set to verdana:
http://irmug.org/mirror2.html
2.As you can see on mirror2.html H have typed 2 lines of Persian text. in the first line first character is missing! second line seems to be correct, the only difference between them is: In the first line there is one space between "داشتم" and "!". I think it is not related to verdana issue.
Thanks

Comment 226

[YAMASHITA Makoto]

(In reply to comment #225)
Mirroring chars: Looks like Verdana and Arial are among the ones affected, while Lucida Grande and Helvetica are not. Does anyone have idea what's behind this? I have no idea what could cause such a font-specific anomaly.
Line width problem when the whitespace is present: This might be another measurement-drawing inconsistency reported in comment 171.
If there is no quick fix for these clear regressions, maybe we have to rollback to v7 that didn't exhibit the problems other than as-in-the chevron awkward bidi support due to the visual ordering which is *not* a regression.

Simon, could you review v7 in that case? I wonder if the fix here can go into 1.8.0.2. Or if you find the patch too risky/big too review anytime soon, could you mark this explicitly WONTFIX? Leaving the patch hang in the air is making all the contributions irrelevant, particularly because this gfx/src/mac is deemed to be abondoned once the ongoing cairo based implementation is done. If this bug cannot be fixed then at least we can salvage something from the patch for the sake of people who want Asian scripts support.

Comment 227

[John Dent]

(In reply to comment #226)
> Does anyone have idea what's behind
> this? I have no idea what could cause such a font-specific anomaly.

I don't know if it is related, but I used to use Safari and Mozilla some time ago (pre 10.3 days). There was a TrueType Hindi font floating around which appeared to work well on some Hindi websites. (I guess the TT font had all the correct glyphs.) But I noticed that it wasn't offering "pukka" support - you had to order the text in glyph order (eg., in Hindi, hindi would be ihMdI, because the leading i prefixes the h).

In the end, I deleted the font because, while it made some Hindi websites displayable, it confused the browser into thinking it could render all Hindi websites. The reality was that it required author support - in reordering the glyphs.

I got as far as understanding that TT's equivalent of Apple's ATSUI was incompatible.

Perhaps you we falling fowl of the same problem? Perhaps the presence of the correct glyphs in the TT font is confusing the renderer to think it can use the TT font. When, in fact, ATSUI doesn't fully support rendering complex Unicode text using TT fonts.

Comment 228

[Simon Fraser [no longer active]]

I won't be able to review this patch until the weekend.

Comment 229

[Jungshik Shin]

(In reply to comment #226)

> 
> Simon, could you review v7 in that case? I wonder if the fix here can go into
> 1.8.0.2. Or if you find the patch too risky/big too review anytime soon, could
> you mark this explicitly WONTFIX? Leaving the patch hang in the air is making

There's no way this can go in for 1.8.0.2 (firefox 1.5.0.x) for which only security related patches (and patches for regression) will be accepted in principle. However, if it's reviewed and baked in the trunk (comment #220), it has a good chance of going in for gecko 1.8.1 (for firefox 2.0). Firefox 3.0 will come out of the current trunk (gecko 1.9.x).

Comment 230

[YAMASHITA Makoto]

(In reply to comment #227)
> Perhaps you we falling fowl of the same problem? Perhaps the presence of the
> correct glyphs in the TT font is confusing the renderer to think it can use the
> TT font. When, in fact, ATSUI doesn't fully support rendering complex Unicode
> text using TT fonts.
I'm not sure if this and mirroring issue is directly related (IMU TrueType fonts does not know anything about mirroring).
On the other hand, exclamation + whitespace issue is apparently a sibling of the one in comment 171. But we can't just go to QD here, because we need the very mirroring now.

Simon, please review v7 (immune to the subsequent problems) instead.

Comment 231

[Hiro]

Attachment: screen shot of MLTE v7

V7patch is roughly excellent. 
But a part of quotation mark overlaps on the following pages. 
http://arena.nikkeibp.co.jp/tokushu/av/

Do you correct it later as another bug?

Comment 232

[YAMASHITA Makoto]

(In reply to comment #231)
It looks less hazardous than the ones in comment 207 and 208. Let me fix this later.

Comment 233

[Simon Fraser [no longer active]]

Comment on attachment 206833 [details] [diff] [review]
MLTE adaptation v7

> Index: nsATSUIUtils.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/gfx/src/mac/nsATSUIUtils.cpp,v
> retrieving revision 1.20
> diff -u -8 -p -r1.20 nsATSUIUtils.cpp
> --- nsATSUIUtils.cpp	6 Nov 2005 19:01:32 -0000	1.20
> +++ nsATSUIUtils.cpp	26 Dec 2005 03:07:43 -0000
> @@ -237,16 +237,23 @@ PRBool nsATSUIUtils::IsAvailable()
>  
>  //------------------------------------------------------------------------
>  //	nsATSUIToolkit
>  //
>  //------------------------------------------------------------------------
>  nsATSUIToolkit::nsATSUIToolkit()
>  {
>  	nsATSUIUtils::Initialize();
> +
> +  // default MLTE setting to be used in DrawString
> +  mMLTEOption.optionTags =
> +    kTXNSetJustificationMask | kTXNUseFontFallBackMask | kTXNDontWrapTextMask;
> +  mMLTEOption.flushness = kATSUStartAlignment;
> +  mMLTEOption.justification = kATSUFullJustification;
> +  mMLTEOption.options = NULL;

This needs to initialize the 'rotation' member.

> +	CreateATSUStyle(theStyle, aSize, aFontNum, aBold, aItalic, aColor);

You need to check the return value from CreateATSUStyle. Also, since theStyle
is an "out" param, it would be clearer if it was passed as a pointer, rather than
a reference, and also be the last parameter, not the first.


> @@ -524,33 +451,114 @@ nsATSUIToolkit::DrawString(
>  	PRBool aBold, PRBool aItalic, nscolor aColor)
>  {
>    oWidth = 0;
>    if (!nsATSUIUtils::IsAvailable())
>    	return NS_ERROR_NOT_INITIALIZED;
>    	
>    StPortSetter    setter(mPort);
>  
> -  ATSUTextLayout aTxtLayout;
> -  
> -  StartDraw(aCharPt, aLen, aSize, aFontNum, aBold, aItalic, aColor, aTxtLayout);
> -  if (nsnull == aTxtLayout)
> -    return NS_ERROR_FAILURE;
> +  Rect textRect;
> +  ATSUStyle theStyle;
> +  nsTextDimensions textDim;
> +
> +  GetTextDimensions(aCharPt, aLen, textDim,
> +                    aSize, aFontNum, aBold, aItalic, aColor);
> +  oWidth = textDim.width;
> +
> +  ::SetRect(&textRect,
> +            x, y - textDim.ascent, x + textDim.width, y + textDim.descent);

Rather than calling CreateATSUStyle here, wouldn't it be better to get
the same ATSUTextLayout object that GetTextDimensions() fetched (via StartDraw),
and get the style from it (which has been cached already)? You'd have to
factor the guts of GetTextDimensions() into a new method that returns the
ATSUTextLayout.

> +  CreateATSUStyle(theStyle, aSize, aFontNum, aBold, aItalic, aColor);

Need to check the return value from CreateATSUStyle here too.

> +  TXNTextBoxOptionsData* optPtr = &mMLTEOption;
> +  ::TXNDrawUnicodeTextBox(aCharPt, aLen, &textRect, theStyle, optPtr);

What if TXNDrawUnicodeTextBox returns an error?

> +  return NS_OK;
> +}

> +nsresult
> +nsATSUIToolkit::CreateATSUStyle(ATSUStyle& aStyle,
> +    short aSize, short aFontNum,
> +	PRBool aBold, PRBool aItalic, nscolor aColor)

So this returns an nsresult...

> +{
> +	OSStatus err;
> +	err = ::ATSUCreateStyle(&aStyle);
> +	if(noErr != err) {
> +		NS_WARNING("ATSUCreateStyle failed");
> +		return nsnull;
> +	}
>  
> -  oWidth = FixRound(iAfter);
> -  // aTxtLayout is cached and does not need to be disposed
> -  return NS_OK;
> +	ATSUAttributeTag 		theTag[ATTR_CNT];
> +	ByteCount				theValueSize[ATTR_CNT];
> +	ATSUAttributeValuePtr 	theValue[ATTR_CNT];
> +
> +	//--- Font ID & Face -----		
> +	ATSUFontID atsuFontID;
> +	
> +	// The use of ATSUFONDtoFontID is not recommended, see
> +	// http://developer.apple.com/documentation/Carbon/Reference/ATSUI_Reference/atsu_reference_Reference/chapter_1.2_section_19.html
> +	FMFontStyle inStyle = 0, fbStyle;
> +   if (aBold) inStyle += bold;
> +   if (aItalic) inStyle += italic;
> +	if (::FMGetFontFromFontFamilyInstance(aFontNum, inStyle, &atsuFontID, &fbStyle) == kFMInvalidFontErr) {
> +		NS_WARNING("FMGetFontFromFontFamilyInstance failed");
> +    // goto errorDoneDestroyStyle;
> +  	err = ::ATSUDisposeStyle(aStyle);
> +    return nsnull;

Return an nsresult (like NS_ERROR_FAILURE), not null (which is equivalent to
a _success_ error value).

> +	theTag[0] = kATSUFontTag;
> +	theValueSize[0] = (ByteCount) sizeof(ATSUFontID);
> +	theValue[0] = (ATSUAttributeValuePtr) &atsuFontID;
> +	//--- Font ID & Face  -----		
> +	
> +	//--- Size -----		
> +	float  dev2app;
> +	short fontsize = aSize;
> +
> +	dev2app = mContext->DevUnitsToAppUnits();
> +
> +  Fixed size = FloatToFixed( (float) rint(float(fontsize) / dev2app));
> +  // is here appropriate to do this?
> + 	if( FixRound ( size ) < 9  && !nsFontUtils::DisplayVerySmallFonts())
> + 		size = Long2Fix(9);
> +
> +	theTag[1] = kATSUSizeTag;
> +	theValueSize[1] = (ByteCount) sizeof(Fixed);
> +	theValue[1] = (ATSUAttributeValuePtr) &size;
> +	//--- Size -----		
> +	
> +	//--- Color -----		
> +	RGBColor color;
> +
> +	#define COLOR8TOCOLOR16(color8)	 ((color8 << 8) | color8) 		
> +
> +	color.red = COLOR8TOCOLOR16(NS_GET_R(aColor));
> +	color.green = COLOR8TOCOLOR16(NS_GET_G(aColor));
> +	color.blue = COLOR8TOCOLOR16(NS_GET_B(aColor));				
> +	theTag[2] = kATSUColorTag;
> +	theValueSize[2] = (ByteCount) sizeof(RGBColor);
> +	theValue[2] = (ATSUAttributeValuePtr) &color;
> +	//--- Color -----		
> +
> +	//--- Bold -----
> + 	Boolean isBold = (aBold && !(fbStyle & bold)) ? true : false;
> +// 	Boolean isBold = (aBold) ? true : false;
> +	theTag[3] = kATSUQDBoldfaceTag;
> +	theValueSize[3] = (ByteCount) sizeof(Boolean);
> +	theValue[3] = (ATSUAttributeValuePtr) &isBold;
> +	//--- Bold -----
> +
> +	//--- Italic -----
> + 	Boolean isItalic = (aItalic && !(fbStyle & italic)) ? true : false;
> +// 	Boolean isItalic = (aItalic) ? true : false;
> +	theTag[4] = kATSUQDItalicTag;
> +	theValueSize[4] = (ByteCount) sizeof(Boolean);
> +	theValue[4] = (ATSUAttributeValuePtr) &isItalic;
> +	//--- Italic -----
> +
> +	err =  ::ATSUSetAttributes(aStyle, ATTR_CNT, theTag, theValueSize, theValue);
> +	if(noErr != err) {
> +		NS_WARNING("ATSUSetAttributes failed");
> +    // goto errorDoneDestroyStyle;
> +  	err = ::ATSUDisposeStyle(aStyle);
> +    return nsnull;

Bad return value!

> +	}
>  }

Missing return value!


> Index: nsUnicodeRenderingToolkit.cpp
> ===================================================================

> +inline nsresult GetASCII(const PRUnichar* aSrcString, char* aDstPtr,
> +                         PRUint32 aLength)

> +  for (PRUint32 i = 0; i < aLength; i++) {
> +    aDstPtr[i] = (char) ASCII_MASK & aSrcString[i];
>    }
> +  return NS_OK;

Why bother with a return value?


>  #if MOZ_MATHML
>  //------------------------------------------------------------------------
>  void nsUnicodeRenderingToolkit::GetScriptTextBoundingMetrics(
> -    const char* buf,
> -    ByteCount aLen,
> -    ScriptCode aScript,
> -    nsBoundingMetrics& oBoundingMetrics)
> -{
> -    Point scale = { 1, 1 };
> -    Fixed stackWidths[STACK_TRESHOLD], *widths;
> -    Fixed stackLefts[STACK_TRESHOLD], *lefts;
> -    Rect stackRects[STACK_TRESHOLD], *rects;
> -    OSStatus err;
> -
> -    NS_PRECONDITION(aLen > 0, "length must be greater than 0");
> -
> -    if(aLen > STACK_TRESHOLD)
> -    {
> -        widths = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed));
> -        lefts = (Fixed*) nsMemory::Alloc(aLen * sizeof(Fixed));
> -        rects = (Rect*) nsMemory::Alloc(aLen * sizeof(Fixed));
> -        
> -        // if any of the allocations failed the 'else' case below will be executed
> +                                                             const char* buf,
> +                                                             ByteCount aLen,
> +                                                             ScriptCode aScript,
> +                                                             nsBoundingMetrics& oBoundingMetrics)
> +{
> +  Point scale = { 1, 1 };
> +  Fixed stackWidths[STACK_TRESHOLD], *widths;
> +  Fixed stackLefts[STACK_TRESHOLD], *lefts;
> +  Rect stackRects[STACK_TRESHOLD], *rects;
> +	SInt16 stackYMaxes[STACK_TRESHOLD], *yMaxes;
> +	SInt16 stackYMins[STACK_TRESHOLD], *yMins;
> +  OSStatus err;

These should use nsAutoBuffer.

Why are you collecting yMaxes and yMins now? The code doesn't seem to use them,
and the docs say that it's OK to pass NULL.


>  //------------------------------------------------------------------------
>  void nsUnicodeRenderingToolkit :: DrawScriptText(

Feel free to nuke the spaces around the :: in all methods that have them.

> +	ByteCount processBytes, outLen, bufLen = STACK_TRESHOLD;
> +  char* buf = new char[aLength * 3];

bufLen is unused. Maybe use an nsAutoBuffer for 'buf'?

>  nsresult
>  nsUnicodeRenderingToolkit::GetTextSegmentBoundingMetrics(
>        const PRUnichar *aString, PRUint32 aLength,
>        short fontNum, nsUnicodeFontMappingMac& fontMapping,
>        nsBoundingMetrics& oBoundingMetrics)
>  {
>    oBoundingMetrics.Clear();
>    if(aLength == 0) 
>      return NS_OK;
>    NS_PRECONDITION(BAD_FONT_NUM != fontNum, "illegal font num");
> -  PRBool firstTime = PR_TRUE;
> -  PRUint32 processLen = 0;
>    nsBoundingMetrics segBoundingMetrics;
> -  char *heapBuf = nsnull;
> -  PRUint32 heapBufSize = 0;
> -  char stackBuf[STACK_TRESHOLD];
> -  char *buf;
> -  ByteCount processBytes;
> -  ByteCount outLen;
> +  char buf[STACK_TRESHOLD];;
> +  ByteCount outLen, bufLen = STACK_TRESHOLD, processBytes;
>    
>    ::TextFont(fontNum);
>    ScriptCode script = ::FontToScript(fontNum);
> -  
> -  // find buf from stack or heap. We only need to do this once in this function.
> -  // put this out of the loop for performance...
> -  ByteCount bufLen = aLength * 2 + 10;
> -  if (bufLen > STACK_TRESHOLD)
> -  {
> -    if (bufLen > heapBufSize)
> -    {
> -      if (heapBuf)
> -        nsMemory::Free(heapBuf);
> -      heapBuf = (char*) nsMemory::Alloc(bufLen);
> -      heapBufSize = bufLen;
> -      if (nsnull == heapBuf) 
> -        return NS_ERROR_OUT_OF_MEMORY;
> -    } 
> -    buf = heapBuf;
> -  } 
> -  else 
> -  {
> -    bufLen = STACK_TRESHOLD;
> -    buf = stackBuf;
> -  }

bufLen is unused (making this confusing). Why not use nsAutoBuffer here?

Also, we used to allocate (aLength * 2 + 10) bytes, and now use (aLength * 3).
Will this always be big enough for short strings?

>  nsresult 
>  nsUnicodeRenderingToolkit::GetWidth(const PRUnichar *aString, PRUint32 aLength, 
>                                      nscoord &aWidth, PRInt32 *aFontID)
>  {
>    nsTextDimensions dim;
> @@ -1735,89 +436,70 @@ nsresult 
>  nsUnicodeRenderingToolkit::GetTextDimensions(const PRUnichar *aString, PRUint32 aLength, 
>                                                nsTextDimensions &aDim, PRInt32 *aFontID)
>  {
>    nsresult res = NS_OK;
>    nsFontMetricsMac *metrics = (nsFontMetricsMac*) mGS->mFontMetrics;
>    nsUnicodeFontMappingMac* fontmap = metrics->GetUnicodeFontMapping();
>  
>    PRUint32 i;
> -  short fontNum[2];
> -  fontNum[0] = fontNum[1] = BAD_FONT_NUM;
> -  PRUint32 start;
> -    
>    nsTextDimensions textDim;
>    nsTextDimensions thisDim;
> -  
> -  for(i =0, start = 0; i < aLength; i++)
> -  {
> -    fontNum[ i % 2] = fontmap->GetFontID(aString[i]);
> -    if ((fontNum[0] != fontNum[1]) && (0 != i))
> -    {  // start new font run...
> -      thisDim.Clear();
> -      res =  GetTextSegmentDimensions(aString+start, i - start, fontNum[(i + 1) % 2], *fontmap, thisDim);
> -      if (NS_FAILED (res)) 
> -        return res;    
> -      textDim.Combine(thisDim);
> -      start = i;
> -    }
> +  PRUint32* rangeHeads = new PRUint32[aLength + 1];
> +  short* fontNums = new short[aLength];
> +  PRUint32 segCount = aLength;
> +  GetFontRuns(aString, aLength, fontmap, segCount, rangeHeads, fontNums);
> +  rangeHeads[segCount] = aLength;
> +
> +  for (i = 0; i < segCount; i++) {
> +	// start new font run...
> +	  thisDim.Clear();
> +	  res =  GetTextSegmentDimensions(&aString[rangeHeads[i]], rangeHeads[i + 1] - rangeHeads[i],
> +				fontNums[i], *fontmap, thisDim);
> +	  if (NS_FAILED (res)) 
> +		return res;    
> +	  textDim.Combine(thisDim);
>    }
> -  res = GetTextSegmentDimensions(aString+start, aLength - start, fontNum[(i + 1) % 2], *fontmap, thisDim);
> -  if (NS_FAILED (res)) 
> -    return res;    
> -  textDim.Combine(thisDim);
>  
>    aDim.width = NSToCoordRound(float(textDim.width) * mP2T);
>    aDim.ascent = NSToCoordRound(float(textDim.ascent) * mP2T);
>    aDim.descent = NSToCoordRound(float(textDim.descent) * mP2T);
>    return res;  
>  }

You're leaking rangeHeads and fontNums. Another use for nsAutoBuffer!

>  //------------------------------------------------------------------------
>  
>  #ifdef MOZ_MATHML
>  nsresult
>  nsUnicodeRenderingToolkit::GetTextBoundingMetrics(const PRUnichar *aString, PRUint32 aLength,
>                                                    nsBoundingMetrics &aBoundingMetrics, PRInt32 *aFontID)
>  {
>    nsresult res = NS_OK;
>    nsFontMetricsMac *metrics = (nsFontMetricsMac*) mGS->mFontMetrics;
>    nsUnicodeFontMappingMac* fontmap = metrics->GetUnicodeFontMapping();
> -
> -  PRUint32 i;
> -  short fontNum[2];
> -  fontNum[0] = fontNum[1] = BAD_FONT_NUM;
> -  PRUint32 start;
>    PRBool firstTime = PR_TRUE;
>    nsBoundingMetrics thisBoundingMetrics;
> -
> -  for(i =0, start = 0; i < aLength; i++)
> -  {
> -    fontNum[ i % 2] = fontmap->GetFontID(aString[i]);
> -    if ((fontNum[0] != fontNum[1]) && (0 != i))
> -    {  // start new font run...
> -      res = GetTextSegmentBoundingMetrics(aString+start, i - start, fontNum[(i + 1) % 2], *fontmap, thisBoundingMetrics);
> -      if (NS_FAILED (res))
> -        return res;
> -      if (firstTime) {
> -        firstTime = PR_FALSE;
> -        aBoundingMetrics = thisBoundingMetrics;
> -      }
> -      else
> -        aBoundingMetrics += thisBoundingMetrics;
> -      start = i;
> -    }
> +  PRUint32* rangeHeads = new PRUint32[aLength + 1];
> +  short* fontNums = new short[aLength];

These are leaked too.

> +  PRUint32 i, segCount = aLength;
> +  GetFontRuns(aString, aLength, fontmap, segCount, rangeHeads, fontNums);
> +  rangeHeads[segCount] = aLength;
> +
> +  for (i = 0; i < segCount; i++) {
> +	// start new font run...
> +	res = GetTextSegmentBoundingMetrics(&aString[rangeHeads[i]], rangeHeads[i + 1] - rangeHeads[i],
> +			fontNums[i], *fontmap, thisBoundingMetrics);
> +	if (NS_FAILED (res))
> +		return res;
> +	if (firstTime) {
> +		firstTime = PR_FALSE;
> +		aBoundingMetrics = thisBoundingMetrics;
> +	} else {
> +		aBoundingMetrics += thisBoundingMetrics;
> +	}
>    }
> -  res = GetTextSegmentBoundingMetrics(aString+start, aLength - start, fontNum[(i + 1) % 2], *fontmap, thisBoundingMetrics);
> -  if (NS_FAILED (res))
> -    return res;
> -  if (firstTime)
> -    aBoundingMetrics = thisBoundingMetrics;
> -  else
> -    aBoundingMetrics += thisBoundingMetrics;
> -
>    aBoundingMetrics.leftBearing = NSToCoordRound(float(aBoundingMetrics.leftBearing) * mP2T);
>    aBoundingMetrics.rightBearing = NSToCoordRound(float(aBoundingMetrics.rightBearing) * mP2T);
>    aBoundingMetrics.ascent = NSToCoordRound(float(aBoundingMetrics.ascent) * mP2T);
>    aBoundingMetrics.descent = NSToCoordRound(float(aBoundingMetrics.descent) * mP2T);
>    aBoundingMetrics.width = NSToCoordRound(float(aBoundingMetrics.width) * mP2T);
>  
>    return res;
>  }
> @@ -1848,71 +530,55 @@ nsUnicodeRenderingToolkit::DrawString(co
>      for (i = 0; i < aLength; )
>      {
>        PRUint32 drawLen;
>        short curFontNum = fontmap->GetFontID(aString[i]);
>  
>        for (drawLen = 1; (i + drawLen) < aLength; drawLen++)
>        {
>          PRUnichar uc = aString[i+drawLen];
> -      	if(! (IS_CONTEXTUAL_CHARS(uc) || 
> -      		    IS_FORMAT_CONTROL_CHARS(uc) ||
> -      	      IS_COMBINING_CHARS(uc)) ) {
> -      	  break;
> -      	}
> +        if(! (IS_CONTEXTUAL_CHARS(uc) || 
> +              IS_FORMAT_CONTROL_CHARS(uc) ||
> +              IS_COMBINING_CHARS(uc)) ) {
> +          break;
> +        }
>        }
>  
>        PRInt32 transformedX = currentX, ignoreY = 0;
>        mGS->mTMatrix.TransformCoord(&transformedX, &ignoreY);
>        res = DrawTextSegment(aString+i, drawLen, curFontNum, *fontmap, transformedX, transformedY, thisWidth);
> -	    if (NS_FAILED(res))
> -	 		  return res;
> +      if (NS_FAILED(res))
> +        return res;
>        
>        for (PRUint32 j = 0; j < drawLen; j++)
> -	   	  currentX += aSpacing[i + j];
> +        currentX += aSpacing[i + j];
>  	   	  
> -	   	i += drawLen;
> +      i += drawLen;
>      }
>    }
>    else    // no spacing array
>    {
> -    short thisFont, nextFont;
> -    
> -    PRUint32 start;
> -
> -      // normal left to right
> -        thisFont = fontmap->GetFontID(aString[0]);
> -	    for (i = 1, start = 0; i < aLength; i++)
> -	    {
> -	    	PRUnichar uch = aString[i];
> -	    	if(! IS_FORMAT_CONTROL_CHARS(uch))
> -	    	{
> -	    		nextFont = fontmap->GetFontID(uch);
> -	    		if (thisFont != nextFont) 
> -	        {
> -	          // start new font run...
> -	          PRInt32 transformedX = currentX, ignoreY = 0;
> -	          mGS->mTMatrix.TransformCoord(&transformedX, &ignoreY);
> +    PRUint32* rangeHeads = new PRUint32[aLength + 1];
> +    short* fontNums = new short[aLength];

More leaks here.

> +    PRUint32 i, segCount = aLength;
> +    GetFontRuns(aString, aLength, fontmap, segCount, rangeHeads, fontNums);
> +    rangeHeads[segCount] = aLength;
> +
> +    for (i = 0; i < segCount; i++) {
> +      // start new font run...
> +      PRInt32 transformedX = currentX, ignoreY = 0;
> +      mGS->mTMatrix.TransformCoord(&transformedX, &ignoreY);
>  	          
> -            res = DrawTextSegment(aString + start, i - start, thisFont, *fontmap, transformedX, transformedY, thisWidth);
> -	    	  	if (NS_FAILED(res))
> -	    	 		  return res;
> +      res = DrawTextSegment(&aString[rangeHeads[i]], rangeHeads[i + 1] - rangeHeads[i], 
> +                            fontNums[i], *fontmap, transformedX, transformedY, thisWidth);
> +      if (NS_FAILED(res))
> +        return res;
>  	    	 		
> -	    		  currentX += NSToCoordRound(float(thisWidth) * mP2T);
> -	    		  start = i;
> -	    		  thisFont = nextFont;
> -	    		}
> -	    	}
> -	    }
> -
> -	    PRInt32 transformedX = currentX, ignoreY = 0;
> -	    mGS->mTMatrix.TransformCoord(&transformedX, &ignoreY);
> -      res = DrawTextSegment(aString+start, aLength-start, thisFont, *fontmap, transformedX, transformedY, thisWidth);
> -    if (NS_FAILED(res))
> -      return res;
> +      currentX += NSToCoordRound(float(thisWidth) * mP2T);
> +    }
>    }
>    
>  	return NS_OK;
>  }


r- based on some error handling and memory leak issues.

I also wonder how much TXNDrawUnicodeTextBox is doing for us that simply setting ATSUI font fallback flags would take care of.

Comment 234

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7.1

Fixes the problem in comment 231 and incorporates Simon's suggestions except for:
> Rather than calling CreateATSUStyle here, wouldn't it be better to get
> the same ATSUTextLayout object that GetTextDimensions() fetched (via
> StartDraw),
> and get the style from it (which has been cached already)? You'd have to
> factor the guts of GetTextDimensions() into a new method that returns the
> ATSUTextLayout.
Do we really need ATSUTextLayout caching then? We have to capture whitespaces at edges of the string but there's no way to retrieve the text set to the layout. In this ver I'm trying ATSUStyle-based modularization. I don't see any significant performance loss by this. Please let me know if there's any. Anyway ASCII rendering (that goes through optimization path) should not be affected. 

> bufLen is unused. Maybe use an nsAutoBuffer for 'buf'?
I'm now using buflen for EnsureElemCapacity and the conversion to unify the buf size management which was my first intention (I guess why wasn't the code so?).

> Also, we used to allocate (aLength * 2 + 10) bytes, and now use (aLength * 3).
aLength * 3 was a lemniscent of Japanese optimization. Now that we are using this only for symbolic fonts and ASCII optimization, "aLength" may be enough.

> I also wonder how much TXNDrawUnicodeTextBox is doing for us that simply
> setting ATSUI font fallback flags would take care of.
IMU we cant set reasonable fonts for Indic scripts et al. (bug 246527), this provides the last fallback in those cases.

Comment 235

[Simon Fraser [no longer active]]

Comment on attachment 211800 [details] [diff] [review]
MLTE adaptation v7.1

>Index: nsATSUIUtils.cpp
>===================================================================

>+  // default MLTE setting to be used in DrawString
>+  mMLTEOption.optionTags = kTXNRotateTextMask | kTXNUseFontFallBackMask |
>+    kTXNDontWrapTextMask;
>+  mMLTEOption.flushness = kATSUStartAlignment;
>+  mMLTEOption.justification = kATSUFullJustification;
>+  mMLTEOption.rotation = FLOAT_TO_FIXED(0.01);

Why not zero?

I don't have the resources to do a performance test with this version of the patch.

Comment 236

[YAMASHITA Makoto]

(In reply to comment #235)
> >+  mMLTEOption.rotation = FLOAT_TO_FIXED(0.01);
> 
> Why not zero?
This way we can get accent portion of the glyph otherwise lost in some cases (comment 201). Please wait a bit for perf. measurement on 10.4. I'm no longer running Panther. Do we need one on that?

Comment 237

[YAMASHITA Makoto]

performance score of v7.1 hasn't changed much: Avg. Median : 1449 msec against the trunk's Avg. Median : 1475 msec.

Comment 238

[YAMASHITA Makoto]

Simon, please continue your review. Who can I ask for sr?

Comment 239

[Hiro]

(In reply to comment #234)
> Created an attachment (id=211800) [edit]
> MLTE adaptation v7.1
This patch rejects by checkin of bug205387.
Please update.

Comment 240

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7.2

Reflects the fix for bug 205387 and fixes the broken "whitespace hack" in nsATSUIToolkit::DrawString.

Comment 241

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7.3 (w/ font management)

Incorporates the font manager proposed at bug 246527 to fix
  - Some indic fonts (e.g. Devanagari MT) missing (enumerate the fonts with ATSUI, rather than FMFontFamily enumeration)
  - Titus cyberbit glyph representability issue (use font's CMAP instead of script code based judgement)
  - Hiragino KakuGo Pro W3 and HiraKakuPro W6 should be gathered in single family "Hiragino KakuGo Pro" and they should have appropriate weights (i.e. 500 and 700), instead of split into different families
  - Partial fix (i.e. for non-english) for bug 972

On some occasions the layout is broken.

Comment 242

[Hiro]

(In reply to comment #241)
> Created an attachment (id=215099) [edit]
> MLTE adaptation v7.3 (w/ font management)
> 
> Incorporates the font manager proposed at bug 246527 to fix
There is no nsMacFonts(.cpp|.h). 
If bug246527 is mended with this bug, should you include source?
(Should you solve it one by one?)

Comment 243

[YAMASHITA Makoto]

(In reply to comment #242)
> There is no nsMacFonts(.cpp|.h). 
> If bug246527 is mended with this bug, should you include source?
> (Should you solve it one by one?)
> 
I put them (attachment 215095 [details] and 215096) at bug 246527 along with a minimal patch against the current (non-MLTE) gfxmac. However that patch is just a illustration of what the new font manager itself does, which is insufficient to really solve the bug as I wrote there. So if there's no alternative MLTE v7.3 serves as one (and the only, I believe,) solution for 246527.

Comment 244

[Hiro]

Attachment: screen shot of MLTE adaptation v7.3 (w/ font management)

I think that Japanese bold is better than before.

Comment 245

[Hiro]

Attachment: A bad rendering

But the display is confused.
Test page: http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2429

Comment 246

[Hiro]

It rejects by Makefile.in.
It corrected as follows and has built.

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/Makefile.in,v
retrieving revision 1.50
diff -u -r1.50 Makefile.in
--- Makefile.in 18 Mar 2006 06:11:48 -0000      1.50
+++ Makefile.in 19 Mar 2006 09:22:34 -0000
@@ -81,6 +81,7 @@
         nsUnicodeMappingUtil.cpp \
         nsUnicodeRenderingToolkit.cpp \
         nsFontUtils.cpp \
+               nsMacFonts.cpp \
         $(NULL)
 
 EXPORTS                = \
@@ -103,6 +104,7 @@
        $(EXTRA_DSO_LIBS) \
        $(MOZ_COMPONENT_LIBS) \
        $(MOZ_UNICHARUTIL_LIBS) \
+       $(MOZ_UCVUTIL_LIBS) \
        $(DIST)/lib/$(LIB_PREFIX)gfxshared_s.$(LIB_SUFFIX) \
        $(DEPTH)/modules/libutil/src/$(LIB_PREFIX)mozutil_s.$(LIB_SUFFIX) \
        $(TK_LIBS) \

Comment 247

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7.4

removes Makefile.in contamination, fixes layout problem a la comment 245.

Comment 248

[YAMASHITA Makoto]

Attachment: nsMacFonts.cpp

update for attachment 215096 [details] of bug 246527.

Comment 249

[YAMASHITA Makoto]

Attachment: nsMacFonts.h

replaces attachment 215095 [details].

Comment 250

[Hiro]

Attachment: screen shot of font panel

Comment 251

[Hiro]

In font panel, ".Aqua かな" is chosen by the default.
If this font is chosen after changing into another font, almost all Japanese will be displayed unjustly.
What ".Aqua かな" ?

In <strong></strong>, Japanese is not displayed by bold.
http://www.tohoho-web.com/html/strong.htm

Comment 252

[YAMASHITA Makoto]

(In reply to comment #250)
> Created an attachment (id=215648) [edit]
> screen shot of font panel
".Aqua Kana" (maybe one of hidden fallback fonts) is in list because the new font manager does not filter those "hidden" fonts yet, and it is selected in the popup because default "HiraginoKakuGo Pro W3" (set by /modules/libpref/src/init/all.js?) is replaced by "HiraginoKakuGo Pro" in the listing. Anyway you can select "KakuGo Pro" and "Osaka-mono" (hack for Osaka-Tofuku that is otherwise put into Osaka family), right?

(In reply to comment #251)
> In <strong></strong>, Japanese is not displayed by bold.
> http://www.tohoho-web.com/html/strong.htm
Please note that ATSUI's bold synth is *really* slight for Hiragino  ideographs. Maybe you can see the difference if you do something like <strong>Kyocho</strong>Kyocho.

Comment 253

[Hiro]

Attachment: The difference between MLTEv7.4 and Safari

Comment 254

[Hiro]

(In reply to comment #252)
> (In reply to comment #250)
> > Created an attachment (id=215648) [edit]
> > screen shot of font panel
> ".Aqua Kana" (maybe one of hidden fallback fonts) is in list because the new
> font manager does not filter those "hidden" fonts yet, and it is selected in
> the popup because default "HiraginoKakuGo Pro W3" (set by
> /modules/libpref/src/init/all.js?) is replaced by "HiraginoKakuGo Pro" in the
> listing.
o.k, understanding.

> (In reply to comment #251)
> Please note that ATSUI's bold synth is *really* slight for Hiragino 
> ideographs. Maybe you can see the difference if you do something like
> <strong>Kyocho</strong>Kyocho.
Except "KakuGo Pro", a difference is a few.
But in "KakuGo Pro", there is a being clear difference compared with Safari.
If possible, it will be good to display it as Safari similarly.

Comment 255

[YAMASHITA Makoto]

Attachment: MLTE adaptation v7.5

Adds hidden font filtering, fixes font-weight estimation and MathML bustage.

> The difference between MLTEv7.4 and Safari
It's ultimately the difference of what CSS style you associate to <strong> markup. While WebKit puts font-weight:bold, Gecko does font-weight:401 and MLTEv7.4's rendering is a better reflection of that situation.

Comment 256

[YAMASHITA Makoto]

Attachment: nsMacFonts.cpp v3

Comment 257

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Do I understand correctly that these latest changes (MLTE v7.x and associated nsMacFonts patches) have Gecko using the Cocoa font names instead of the Carbon font names now?

Comment 258

[YAMASHITA Makoto]

(In reply to comment #257)
To be precise we are going to use fontnames by ATSUI (ATSUGetIndFontName) (prefering the sys's language whenever available, otherwise in roman script) with a few exceptions for TeX fonts and Osaka-Mono, but they look identical to Cocoa's "localized fontnames" (if we don't introduce the exceptions).

Comment 259

[Wayne Woods]

Out of interest, what's special about Osaka-mono? I was just wondering about whatever reason you have for treating it differently... surely there are other odd fonts out there that will also fit that reason?  And will having exceptions make it messier when we move to Cocoa widgets and don't require this "middle" code anymore? Will this code have to remain to handle the exceptions?

Cheers

Comment 260

[YAMASHITA Makoto]

(In reply to comment #259)
> Out of interest, what's special about Osaka-mono? I was just wondering about
> whatever reason you have for treating it differently... surely there are other
> odd fonts out there that will also fit that reason? 
The problem is, the Core assumes the monospace fonts to consist separate families like Courier, not to be included in a 'usual' font family together with regular/italic typefaces (nsFonts.h). If we stick to Osaka alone, we have to add "generic monospace font"->"monospace typeface" special casing in generic font resolution code, which might be insufficient (if "monospace" becomes Osaka anywhere else, we never know if it should be monospace or not).
Any font that has monospace typeface should be affected this way. Once we register the typeface as a separate family with "-Mono", no more special casing
will be needed.
> And will having exceptions
> make it messier when we move to Cocoa widgets and don't require this "middle"
> code anymore? Will this code have to remain to handle the exceptions?
With a few modifications nsMacFons.* can be braught into thebes. I'm planning to put the codes at bug 325165.

Comment 261

[Wayne Woods]

Thanks for the explanation! :)

(In reply to comment #260)
> The problem is, the Core assumes the monospace fonts to consist separate
> families like Courier, not to be included in a 'usual' font family together
> with regular/italic typefaces (nsFonts.h).

This sounds like a very bad assumption on the part of the Core code. Is there a bug filed for it? That'd hopefully solve the problems once and for all, and you can then just use the standard ATSUI or Cocoa names for everything. Or is it not that simple?

Comment 262

[YAMASHITA Makoto]

(In reply to comment #261)
Probably instead we have to blame CSS spec, in which monospace occurs only as a generic font family, not as a style of font. Come to think of it, cursive and fantasy might be affected the same way, although I'm not sure if that's needed in practice. (Let's forget about the possibility of serif and sans-serif packed into one family! ;)
Perhaps Osaka is the sole exception for this design, so that we'd be better off with this hack...

Comment 263

[Wayne Woods]

(In reply to comment #262)
> Probably instead we have to blame CSS spec, in which monospace occurs only as a
> generic font family, not as a style of font. Come to think of it, cursive and

If it's indeed a shortcoming of the CSS spec, then other browsers should also be facing the same problem. I wonder how WebKit handles it. Or how the Windows and Linux side of our text code handle it.

Comment 264

[Bill McGonigle (not currently reading bugmail; please contact directly)]

I notice this bug is blocking bug 306902 which is access restricted.  Is this a security bug (such that this bug needs fixing to address a security issue?)  Or maybe it was just misentered.

Comment 265

[Julik Tarkhanov]

I would absolutely love this to go into trunk ASAP (before Cairo), I can't evaluate my site designs on Firefox because I don't get platform-equivalent fonts for Russian pages.

Comment 266

[Håkan Waara]

What's the status of these patches? Makotoy, are they usable & final?

This bug is affecting a lot of users.

Comment 267

[YAMASHITA Makoto]

(In reply to comment #266)
> What's the status of these patches? Makotoy, are they usable & final?
> 
I'm not hoping the fix to land on the official trees (too late for Fx2 and not needed for Fx3).  I wonder if we can make it one of "Contribution Builds" listed at the bottom of release note page. For now, my static build is at
http://www.ms.u-tokyo.ac.jp/~makotoy/ffmath/firefox-2.0b2.en-US.mac.dmg
I believe it's quite stable and efficient, but not sure how to make this a Contribution Build now.

Comment 268

[Josh Aas]

YAMASHITA Makoto - I'm sorry that the timing didn't work out such that we were able to use your patches. Thank you for all of your hard work on this, and I hope you can help us with our Gecko 1.9 (trunk) ATSUI!

Comment 269

[Josh Aas]

fixed on trunk

Comment 270

[inkyyeu]

(In reply to Josh Aas from comment #268)

YAMASHITA Makoto - I'm sorry that the timing didn't work out such that we
were able to use your patches. Thank you for all of your hard work on this,
and I hope you can help us with our Gecko 1.9 (trunk) ATSUI!

<a href="http://inkyyeu.net/">in kỷ yếu</a>

Comment 271

[inkyyeu]

http://inkyyeu.net/