124543 - MathML "extra" functions not implemented in nsRenderingContextBeos.cpp

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Paul]

nsRenderContextBeOS needs implementations for the following:

#ifdef MOZ_MATHML
  /**
   * Returns bounding metrics (in app units) of an 8-bit character string
   * @param aString string to measure
   * @param aLength number of characters in string
   * @return aBoundingMetrics struct that contains various metrics (see below)
   * @return error status
   */
  NS_IMETHOD
  GetBoundingMetrics(const char*        aString,
                     PRUint32           aLength,
                     nsBoundingMetrics& aBoundingMetrics) = 0;
  /**
   * Returns bounding metrics (in app units) of an Unicode character string
   * @param aString string to measure
   * @param aLength number of characters in string
   * @param aFontID an optional out parameter used to store a
   *        font identifier that can be passed into the GetBoundingMetrics()
   *        methods to speed measurements
   * @return aBoundingMetrics struct that contains various metrics (see below)
   * @return error status
   */
  NS_IMETHOD
  GetBoundingMetrics(const PRUnichar*   aString,
                     PRUint32           aLength,
                     nsBoundingMetrics& aBoundingMetrics,
                     PRInt32*           aFontID = nsnull) = 0;
#endif

Comment 1

[Paul]

Attachment: Temporary fix to the build errors

Daniel, I assigned this to you, since you had done work on the gfx package 
recently.  I hope you don't mind.
The attachment should at least allow the Tinderbox to build again, until we 
actually get time to implement these.

Comment 2

[Paul]

Attachment: Temporary fix for build errors

Oops, I should have compiled this before I posted it.  Cut and past error
fixed.

Comment 3

[Daniel Switkin]

Second patch looks good Paul, let's just get things building again. r=[e-mail]

Comment 4

[hacker formerly known as [e-mail]]

I checked in the temp fix.

Comment 5

[Paul]

Attachment: Implementation of the GetBoundingMetrics methods

Actual implementation

Comment 6

[rbs]

+       mCurrentFont->GetHeight(height);
+       aBoundingMetrics.ascent = height->ascent;
+       aBoundingMetrics.descent = height->descent;

The bounding metrics' ascent & descent are not the same as the font' ascent & 
descent

+       mCurrentFont->GetEdges(aString,aLength,bearings);
+       aBoundingMetrics.width = mCurrentFont->StringWidth(aString);
+       aBoundingMetrics.leftBearing = bearings[0].left;
+       aBoundingMetrics.rightBearing = bearings[aLength -1].right;

Unless GetEdges(() which I know not is adding things up, the right bearing is 
the width of aString[0..length-1] + the right bearking of last character -- a 
slightly more complicated formula can take care of kerning effets too. See the 
code in nsFontMetricsWin for example.

A good way to test your implementation is to enable #define SHOW_BOUNDING_BOX 1:
http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsIMathMLFrame.h#
21

Then zoom while vieweing to check that, on MathML content, the bounding boxes 
that will be drawn cover exactly the text. For example, the bbox of <msup> 
should appear as the smallest rectangle that exactly encloses the base and the 
superscript.

Comment 7

[Paul]

Oops, my bad.  I should have read the description in the header a little closer before just 
wacking out some code.  I'll fix as soon as I find out how to get the proper info.

Comment 8

[Paul]

Attachment: implementation of GetBoundingMetrics for MathML

A "good" implementation of the GetBoundingMetrics methods used by the MathML
renderer.

Comment 9

[Paul]

Attachment: screen shot of torture test page with bounding boxes

screen shot of torture test page, zoomed to 200%, with bounding boxes shown,
using the above patch

Comment 10

[rbs]

+       // The origin in beos is the top left. so we switch these to give what
MathML wants, bottom left
+       aBoundingMetrics.ascent = (nscoord)rect.bottom;
+       aBoundingMetrics.descent = (nscoord)rect.top;


Drawing operations are now made on the baseline. I remember to have patched the
BeOS code sometimes ago about that -- but it may have been reversed -- din't
check. The screenshot shows that the metrics are still not returned correctly.
The baseline is needed to allow you to compute:
ascent = bottom - baseline
descent = height - ascent (where height = bottom - top --per your comment above)

I will attach a reference screenshot for your information.

Comment 11

[Paul]

btw, how does the MathML load the fonts it needs?  i.e. where does it do this? 
I just installed the fonts, and now I'm getting blocks instead of characters.

Comment 12

[rbs]

Attachment: screenshot - expected bounding boxes

note how at each level of nesting, elements are embedded in an hierarchy of
precise bounding boxes in a way that mirrors the XML parenting relashionship
per level.

Comment 13

[rbs]

>btw, how does the MathML load the fonts it needs?  i.e. where does it do this? 

The fonts are loaded in the same way as mainline layout does (i.e., using 
nsIRenderingContext::SetFont(), etc).

But for the fonts to be recognized properly, the ucvmath module needs to be 
hooked -- see similar bug 107146 - ucvmath not working with Fizzilla.

Comment 14

[Paul]

Attachment: seems to return proper boundnig box for chars, but not for equation

Well, I know seem to have it drawing the proper bounding boxes for the 
characters/strings, and the width is correct for the entire equation, but the
height is wrong.  Any ideas?

Comment 15

[rbs]

Attachment: testcase to exercise the implementation

Comment 16

[rbs]

Attachment: reference rendering of the testcase

The testcase contrast the bounding metrics vs. the usual CSS metrics. The solid
blue borders are done with the bounding metrics while the dashed red borders
are done with the CSS metrics.

Comment 17

[rbs]

> but the height is wrong.  Any ideas?

The ascent and descent need to be relative to the baseline as I hinted earlier.
The cross-platform origin expected higher-up in consumers of the 
GetBoundingMetrics() function is at the baseline.

Comment 18

[Paul]

Attachment: beos rendering of testcase

as you can see, I am generating the proper bounding boxes for all of the fonts.
 The only bounding boxes that aren't correct, are the boxes generated based on
my bounding boxes, the "global" boxes in the first 2 mathml test cases.  If it
is able to draw the proper bounding boxes for the strings, then it seems to me
to be a problem with mathml calculating the "global" bounding box.  

Here's the debug output I in my patch for the test case:
GetBoundingMetrics for: j (-7.000000,-76.000000,37.000000,22.000000) ->
(-7.000000,37.000000,-22.000000,-76.000000)
GetBoundingMetrics for: i (7.000000,-76.000000,36.000000,-1.000000) ->
(7.000000,36.000000,1.000000,-76.000000)
GetBoundingMetrics for: f (9.000000,-77.000000,48.000000,-1.000000) ->
(9.000000,48.000000,1.000000,-77.000000)
GetBoundingMetrics for: j (-5.000000,-54.000000,25.000000,15.000000) ->
(-5.000000,25.000000,-15.000000,-54.000000)
GetBoundingMetrics for: i (5.000000,-54.000000,25.000000,-1.000000) ->
(5.000000,25.000000,1.000000,-54.000000)
GetBoundingMetrics for: f (6.000000,-54.000000,33.000000,-1.000000) ->
(6.000000,33.000000,1.000000,-54.000000)
GetBoundingMetrics for: jif (-7.000000,-77.000000,96.000000,22.000000) ->
(-7.000000,96.000000,-22.000000,-77.000000)
GetBoundingMetrics for: jif (-5.000000,-54.000000,67.000000,15.000000) ->
(-5.000000,67.000000,-15.000000,-54.000000)

Comment 19

[rbs]

>If it is able to draw the proper bounding boxes for the strings, then it seems
>to me to be a problem with mathml calculating the "global" bounding box.

The blue bounding boxes are drawn with something like (simplyfing...) :
DrawRect(0, 0, ascent + descent, rightBearing - leftBearing)

So although, ascent + descent may sum up to the height, it doesn't mean that
ascent and descent are correct if taken indivually. As I mentioned earlier,
these values need to be set relative to the baseline since this is what the
other MathML rendering operations expect.

Comment 20

[rbs]

Code that is painting the bboxes:
http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLContainerFrame.cpp#770

Comment 21

[rbs]

Just did a lookup in google and it now seems to me that there is possibly 
another hole in your patch. 
(http://www.bespecific.com/dialog/bedevtalk/archive/990308/00000103.htm)

+       // Use the printing metric to get more detail
+       mCurrentFont->GetBoundingBoxesForStrings(&aString, 1, B_SCREEN_METRIC, 
&delta, &rect);

Are you getting the rect of the *first* character only or what?
To test this, you can edit the testcase and change "jif" to "ijf" throughout.

Comment 22

[rbs]

Errata... I was messed up with the names. The function that you are using 
applies to the full string indeed...

GetBoundingBoxesForStrings():
   returns an array of BRect objects indicating the bounding rectangles for an
   array of strings, one BRect per string. These rectangles enclose the entire
   string they represent. 

Comment 23

[Paul]

Attachment: Hopefully the final patch

Seems to be workign properly now.  Still switching to top and bottom values,
but now negating the descent instead of the ascent, to match how MathML expects

things.  I then had to offset the values by the total of ascent and descent,
to move them to the proper cooridinate space.

Comment 24

[Paul]

Attachment: beos rendering of test case

beos rendering of test case showing that it now works.

Comment 25

[rbs]

Comment on attachment 73468 [details] [diff] [review]
Hopefully the final patch

+	// BeOS origin at top left, so the bottom and top values are switched
+	// for ascent and descent, but we then need to offset those values
+	// by there totals to obtain the proper bounding box.
+	float offset = rect.bottom + rect.top;
+	
+	// Flip sign of descent for cross-platform compatibility
+	aBoundingMetrics.ascent =  (nscoord)(rect.bottom - offset);
+	aBoundingMetrics.descent = (nscoord)(0.0 - rect.top + offset);

It appears that the documentation (comment?) is buggy. Indeed the
formulas are simply reducing to:

ascent = rect.bottom - offset
       = rect.bottom - (rect.bottom + rect.top)
       = -rect.top

descent = - rect.top + offset;
	= - rect.top + (rect.bottom + rect.top)
	= rect.bottom

[I just checked that you didn't try this combination in your earlier patch...]

So the origin is still at the baseline on BeOS, ...but it is the orientation
that is _downward_ for _all_ vertical metrics (ascent & descent). The MathML
engine expects what X Windows does: the orientation of the ascent is upward
(i.e., a postive ascent means the glyph extends above
the baseline), while the descent is oriented downward (i.e., a positive descent
means the glyph extends below the baseline).

So flipping the sign of the ascent on BeOS turns the ascent from downward to
upward and this brings things to parity.
[On Win32, the orientation is upward for all vertical metrics and that's
why I had to flip its ascent to turn it from upward to downward. Hope this
quick explantation makes sense...]

+	aBoundingMetrics.rightBearing = (nscoord)rect.right + 1;

Why '+1' ? have you encouuntered fonts with bad metrics that make this
necessary? On the screenshot, there seems to be little gaps as a
result of this.

Since BeOS is using floating arithmetic for its device values, you don't need
the casting that may cause even more truncation errors, you could directly use:

+	aBoundingMetrics.leftBearing = NSToCoordRound(rect.left * mP2T);
+	aBoundingMetrics.rightBearing = NSToCoordRound(rect.right * mP2T);
+	aBoundingMetrics.width = NSToCoordRound(width * mP2T);
+	aBoundingMetrics.ascent = NSToCoordRound(-rect.top * mP2T);
+	aBoundingMetrics.descent = NSToCoordRound(rect.bottom * mP2T);

Comment 26

[Paul]

Attachment: Updated patch, as per comments from above

I think I was a bit too excited that it was drawing properly, that I did not
notice the uglyness of my code.  It is now cleaned up, and hopefully the
comment explains things better.

Comment 27

[rbs]

+		uint8 *utf8str = new uint8[aLength * 4 + 1];
+		uint8 *utf8ptr = utf8str;

I was concentratimg on the correctness and didn't pay much attention to other 
parts... More often than not, there is only a single character passed to the 
function, and you will be |new|ing all the time. It might be best to use a 
stack variable and only |new| if the length exceeds the stack. With this, the 
|new| is rarely going to happen. So:

  uint8 utf8buf[1024];
  uint8* utf8str = &utf8buf;
  if (aLength * 4 + 1 > 1024)
    utf8str = new uint8[aLength * 4 + 1];

  ...use it...

  if (utf8str != utf8buf)
    delete[] utf8str;

[There is similar code along these lines in GfxWin, BTW]

Comment 28

[Paul]

Attachment: updated patch, as per comments

updated to try and keep the utf8 conversion on the stack for single char
strings

Comment 29

[rbs]

Comment on attachment 73496 [details] [diff] [review]
updated patch, as per comments

Need some null check in case |new| fails, but just looked up and noted that
file is not bothering doing null checks for GetWidth/DrawString/...
and to be meaningful, null checks would have to be scattered everywhere
throughout that file at some stage.

So r=rbs for the patch

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 73496 [details] [diff] [review]
updated patch, as per comments

a=roc+moz

Comment 31

[Paul]

Patch checked in.  Marking fixed.

Comment 32

[Kai Lahmann (is there, where MNG is)]

v