17.78 KB, image/png
1.12 KB, application/xhtml+xml
13.46 KB, image/png
22.72 KB, image/png
2.83 KB, patch
|Details | Diff | Splinter Review|
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
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.
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
Second patch looks good Paul, let's just get things building again. firstname.lastname@example.org
Status: NEW → ASSIGNED
I checked in the temp fix.
Created attachment 71809 [details] [diff] [review] Implementation of the GetBoundingMetrics methods Actual implementation
Attachment #68703 - Attachment is obsolete: true
+ 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.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.
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.
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
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
+ // 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.
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.
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.
>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.
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?
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.
> 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.
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)
>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.
Code that is painting the bboxes: http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLContainerFrame.cpp#770
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.
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.
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
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 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);
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
+ 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; 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]
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 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 on attachment 73496 [details] [diff] [review] updated patch, as per comments a=roc+moz
Attachment #73496 - Flags: approval+
Patch checked in. Marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.