MathML "extra" functions not implemented in nsRenderingContextBeos.cpp

VERIFIED FIXED

Status

()

Core
MathML
--
blocker
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Paul, Assigned: Daniel Switkin)

Tracking

Trunk
x86
BeOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Comment 1

17 years ago
Created attachment 68701 [details] [diff] [review]
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.
(Reporter)

Comment 2

17 years ago
Created attachment 68703 [details] [diff] [review]
Temporary fix for build errors

Oops, I should have compiled this before I posted it.  Cut and past error
fixed.
Attachment #68701 - Attachment is obsolete: true
(Assignee)

Comment 3

17 years ago
Second patch looks good Paul, let's just get things building again. r=mozilla@switkin.com
Status: NEW → ASSIGNED
I checked in the temp fix.

(Reporter)

Comment 5

17 years ago
Created attachment 71809 [details] [diff] [review]
Implementation of the GetBoundingMetrics methods

Actual implementation
Attachment #68703 - Attachment is obsolete: true

Comment 6

17 years ago
+       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.

(Reporter)

Comment 7

17 years ago
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.
(Reporter)

Comment 8

17 years ago
Created attachment 72738 [details] [diff] [review]
implementation of GetBoundingMetrics for MathML

A "good" implementation of the GetBoundingMetrics methods used by the MathML
renderer.
Attachment #71809 - Attachment is obsolete: true
(Reporter)

Comment 9

17 years ago
Created attachment 72742 [details]
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

17 years ago
+       // 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.
(Reporter)

Comment 11

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

17 years ago
Created attachment 72746 [details]
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

17 years ago
>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.
(Reporter)

Comment 14

17 years ago
Created attachment 72902 [details] [diff] [review]
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?
Attachment #72738 - Attachment is obsolete: true
Attachment #72742 - Attachment is obsolete: true

Comment 15

17 years ago
Created attachment 72918 [details]
testcase to exercise the implementation

Comment 16

17 years ago
Created attachment 72919 [details]
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

17 years ago
> 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.
(Reporter)

Comment 18

17 years ago
Created attachment 72927 [details]
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

17 years ago
>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 21

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

17 years ago
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. 
(Reporter)

Comment 23

17 years ago
Created attachment 73468 [details] [diff] [review]
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.
Attachment #72902 - Attachment is obsolete: true
(Reporter)

Comment 24

17 years ago
Created attachment 73469 [details]
beos rendering of test case

beos rendering of test case showing that it now works.
Attachment #72927 - Attachment is obsolete: true

Comment 25

17 years ago
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);
(Reporter)

Comment 26

17 years ago
Created attachment 73485 [details] [diff] [review]
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.
Attachment #73468 - Attachment is obsolete: true

Comment 27

17 years ago
+		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]
(Reporter)

Comment 28

17 years ago
Created attachment 73496 [details] [diff] [review]
updated patch, as per comments

updated to try and keep the utf8 conversion on the stack for single char
strings
Attachment #73485 - Attachment is obsolete: true

Comment 29

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

Updated

17 years ago
Attachment #73496 - Flags: review+
Comment on attachment 73496 [details] [diff] [review]
updated patch, as per comments

a=roc+moz
Attachment #73496 - Flags: approval+
(Reporter)

Comment 31

17 years ago
Patch checked in.  Marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.