Closed Bug 1011020 Opened 6 years ago Closed 4 years ago

The bars of fractions, radicals etc are not visible at some zoom levels

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: fredw, Assigned: jkitch)

References

Details

Attachments

(5 files, 3 obsolete files)

Hi Raniere, have you solved your problem with the FirefoxOS build and are you able to work on this again?
Flags: needinfo?(ra092767)
Hi Frédéric,

> have you solved your problem with the FirefoxOS build and are you able to work on this again?

Will try again soon.
Flags: needinfo?(ra092767)
Frédéric,

The WebBrowser app is very unstable right now. Firefox OS is crashing every time that I try to zoom in or out with Flatfish.

https://github.com/mozilla-b2g/B2G.git

commit 9260e00c890c1cd385fd281296d5037fa7e4dece
Merge: fe0184b a50b995
Author: Dave Hylands <dhylands@gmail.com>
Date:   Tue Mar 24 16:06:04 2015 -0700

    Merge pull request #424 from dhylands/bug-1141635-recovery
    
    Bug 1141635 - Updated flash.sh to flash recovery image when doing a full flashfor aries/shinano. r=lissyx+mozillians

https://git.mozilla.org/releases/gecko.git

commit 8d01823d19a0011661d95799e1aba47c7edd0c8f
Merge: b8b1677 4d9c8a3
Author: Phil Ringnalda <philringnalda@gmail.com>
Date:   Sat Mar 28 21:33:32 2015 -0700

    Merge f-t to m-c, a=merge

https://git.mozilla.org/releases/gaia

commit be25b16efa19bab8d54be08f8fe45dcc93bf93d0
Merge: 67ad91f 7716352
Author: autolander <bug.autolander@gmail.com>
Date:   Sun Mar 29 03:19:00 2015 -0700

    Bug 994357 - merge pull request #28784 from stasm:994357-dom-overlays to mozilla-b2g:master
Depends on: 1148909
The zoom level increase from left to right, starting with level 0.
The zoom level is decreasing from left to right, starting with level 0.
Frédéric,

I will try to understand Gecko code during the Easter.
Frédéric,

this bug is at layout/mathml/nsMathMLmrootFrame.cpp or gfx/*?
Flags: needinfo?(fred.wang)
These bars are drawn with DisplayBar:

https://dxr.mozilla.org/mozilla-central/search?q=DisplayBar

The thickness is determined in various places (including in gfx/* where we read OpenType MATH parameters). As discussed on bug 961365, there are two things to try:

1) At various places (e.g. https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#342) we do 

     onePixel = nsPresContext::CSSPixelsToAppUnits(1)
     ...
     if (mRuleThickness < onePixel) {
       mRuleThickness = onePixel;
     }

   To ensure that the rule thickness is at least 1px. This does not seem to work well so perhaps onePixel = nsFontMetrics::AppUnitsPerDevPixel will give better results.

2) As Jonathan wrote, a 1px bar is "ugly" at small zoom levels, so we might want to allow thickness < 1px but draw an antialiased bar instead. Currently, we draw a black rectangle that has height the desired thickness (see https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.cpp#361). Maybe we should do as in menclose (https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#771) and draw normal lines with the desired thickness instead. We probably want "butt" style for the bar ends (https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-linecap) but I don't know how to do that with the gfx drawing primitives. Karl or roc know that better than me.

Note: IIUC, this happens also on Desktop at small zoom levels, so you can directly try it on your computer before testing on a Firefox OS / Android device.
Flags: needinfo?(fred.wang)
Frédéric,

thanks for all the information.

I'm trying how I can get information of that happen at this part of the code for the test case of this bug but I fail. I triedg GDB but Firefox was too slow and others problems. I tried printf at the start of every function/method of layout/mathml/nsMathMLmrootFrame.cpp but I didn't get any printf when running Firefox. I considered https://developer.mozilla.org/en-US/docs/Debugging_Frame_Reflow but I don't have much background knowledge to understand it. Can you suggest me a way to see the execution of the functions/methors of layout/mathml/nsMathMLmrootFrame.cpp?
Flags: needinfo?(fred.wang)
(In reply to Raniere Silva from comment #10)
> Can you suggest me a way to
> see the execution of the functions/methors of
> layout/mathml/nsMathMLmrootFrame.cpp?

Are you sure printf does not work? I think you need to use a new line to be sure that the text is printed in the terminal:

printf("myvalue=%d\n", 17);
Flags: needinfo?(fred.wang)
> Are you sure printf does not work?

Using https://github.com/r-gaia-cs/gecko-dev/commits/1011020 I ran

$ python2 mach build
$ ./obj-x86_64-unknown-linux-gnu/dist/bin/firefox https://bugzilla.mozilla.org/attachment.cgi?id=8524909

and didn't get any information.
This is because <msqrt> uses nsMathMLmenclose not nsMathMLmroot, I guess.
(In reply to Raniere Silva from comment #12)
> > Are you sure printf does not work?
> 
> Using https://github.com/r-gaia-cs/gecko-dev/commits/1011020 I ran
> 
> $ python2 mach build
> $ ./obj-x86_64-unknown-linux-gnu/dist/bin/firefox
> https://bugzilla.mozilla.org/attachment.cgi?id=8524909
> 
> and didn't get any information.

try a fprintf to stderr and see if that works better.
> This is because <msqrt> uses nsMathMLmenclose not nsMathMLmroot, I guess.

Your guess is correct.

And sorry for my delay in reply. I did small progress: http://blog.rgaiacs.com/2015/04/07/hacking_gecko.html.
Attached patch Patch (obsolete) — Splinter Review
The problem lies in nsDisplayMathMLBar::Paint(...)

https://hg.mozilla.org/mozilla-central/annotate/01ae99b53561a2c3b40533d8c1c92bd3efc42d00/layout/mathml/nsMathMLFrame.cpp#l366

The rest of the layout code ensures that mRect will have a minimum height equal to one pixel in appunits.  It is the process of snapping the bar to the pixel grid (NSRectToSnappedRect) that causes it to round down to 0.

The attached patch fixes it by setting a height of 1.0 whenever it rounds to zero, but I'm not convinced that manual adjustments after snapping is wise.  

Other alternatives:
* Fix the snapping process so that it cannot round to 0
* Pass in a height guaranteed not to round to 0.
However I do not know the scaling algorithm well enough for either of them.

Tests to follow.  I can't produce a single test that will reliably work, but with 5 or so zoom levels across every reftest platform the problem will hopefully be triggered - but no promises.

Try builds:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/jkitch.bug@gmail.com-6ec756886cb3
Raniere, would you mind testing with the platforms you have available?
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Flags: needinfo?(ra092767)
(In reply to James Kitchener (:jkitch) from comment #17)
> The rest of the layout code ensures that mRect will have a minimum height
> equal to one pixel in appunits.  It is the process of snapping the bar to
> the pixel grid (NSRectToSnappedRect) that causes it to round down to 0.
> 

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> IMO, when zoomed out we really -should- allow the fraction bar (etc) to
> become less than one device pixel thick, otherwise at small sizes it'll be
> much too heavy in comparison to the glyphs. However, to avoid it simply
> disappearing, we'd need to render such a fractional-pixel-wide line with
> antialiasing, so that it'd still be present, just fainter than a 1-devpx
> solid line. But I don't know how readily we could achieve that... if we
> can't antialias it, then maintaining a minimum thickness of 1 dev px is our
> safest option.

So comparing with what is done for menclose:
https://hg.mozilla.org/mozilla-central/file/dd2a1d737a64/layout/mathml/nsMathMLmencloseFrame.cpp#l771

We could try to use NSRectToRect instead of NSRectToSnappedRect. Additionally, we could try to do draw a line with drawTarget->StrokeLine (with appropriate thickness and "butt" linecap http://www.w3.org/TR/SVG/painting.html#StrokeLinecapProperty) instead of drawing a rectangle with drawTarget->FillRect. Hopefully this will allow to draw an antialiased line as suggested by Jonathan.
(In reply to James Kitchener (:jkitch) from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51bcf6aa60e

It seems that the remaining failures are due to the thickness change / antialiasing and can be fixed by increasing the thickness of the green covering outlines.
James,

> would you mind testing with the platforms you have available?

Unfortunately I can't. =(

For Hamachi something happened that for new images I only get a black screen. I can flash old images but not new ones. At the log I got a lot of "could not find/open".

Also, for Flatfish I flash a bad image that I only get a black screen and couldn't flash any image again.

Sorry for not be more helpful.
Flags: needinfo?(ra092767)
Attached patch Patch (obsolete) — Splinter Review
The attached patch uses the paint stroke method.  It distinguishes between horizontal and vertical strikes in case it makes a difference to rendering.

With the change away from a snapped Rect, bars are drawn in marginally different positions, however this change has the benefit of giving consistent widths - sometimes the rounding of the old algorithm would give thicker bars.

Note that antialiasing may result in the area where the radical and the horizontal bar touch not having the same colour as the rest of the symbol (but it should only be noticeable when magnifying a screenshot.

As the problem is one of rounding, reliably reproducing the bug across platforms is difficult.  Therefore the tests are probabilistic - the expectation is that should the problem occur, at least one test on at least one platform will fail.
Attachment #8659226 - Attachment is obsolete: true
Attachment #8663256 - Flags: review?(roc)
Comment on attachment 8663256 [details] [diff] [review]
Patch

Review of attachment 8663256 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should just give up on snapping. That will look ugly in common situations where the line is a multiple of device pixels in height but not vertically aligned to a device pixel boundary.

I think we should have a variant of NSRectToSnappedRect, let's say NSRectToNonEmptySnappedRect, that tries to snap but falls back to not snapping if the snapped rect is empty. For bonus points, make an independent decision to snap on each axis.

AFAIK the stroking code here is unnecessary for the not-snapped case. Rendering the unsnapped rect should antialias when it's not device pixel aligned.

> this change has the benefit of giving consistent widths

I think it's a stretch to describe solid lines rendered unaligned with device pixel boundaries, with antialiasing, as having "consistent widths". Depending on the fractional offset they can have quite different visual appearances even if the logical width is constant.

(One day we'll all have ultra-high-DPI screens and wake up from this long nightmare...)
Attachment #8663256 - Flags: review?(roc) → review-
Attached patch Patch (obsolete) — Splinter Review
The comments from the previous review are adopted.

Snapping is not performed for an axis if it would result in a length of 0.

The changes to the menclose tests have been reverted as their pass/fail status no longer changes, but otherwise the tests haven't changed
Attachment #8663256 - Attachment is obsolete: true
Attachment #8669608 - Flags: review?(roc)
Comment on attachment 8669608 [details] [diff] [review]
Patch

Review of attachment 8669608 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!
Attachment #8669608 - Flags: review?(roc) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca5e30a34dd1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af14ca96565e

There is a comment change between those try runs and the attached patch, but no changes to code.
Keywords: checkin-needed
Woops, wrong bug number in the commit. I missed it as well. Here's the commit where it landed onto m-i

https://hg.mozilla.org/integration/mozilla-inbound/rev/18d4a0ca8cc1
Keywords: checkin-needed
sorry had to back this out for causing bug 1212267
Flags: needinfo?(jkitch.bug)
I'll try to work out what went wrong.
Flags: needinfo?(jkitch.bug)
1.  The applied zoom of the failing reftest is lower than what is actually permitted by default (10% vs 30%)
2.  At 10% zoom, AppUnitsPerCSSPixel and AppUnitsPerDevPixel disagree substantially (60 vs 600) leading to a height of 0.1 which whatever does the antialiasing/rendering rounds down to 0 and which I somehow failed to notice.

Possible solutions.

1.  Inflate the rect during NSRectToNonEmptySnappedRect to something guaranteed not to eventually round to invisible during .
2.  Add a flag to gfx::Rect and friends to tell the rendering engine that we would really like something to be displayed.
3.  Demonstrate that it will always render something at the acceptable zoom levels and just change the reftests.

The question is how much "padding" needs to be added to make it visually distinguishable, as opposed to just enough to get the reftest to pass - I expect it will depend on the background.
See comment 37.

On Windows at least the actual rendering is quickly passed onto Direct2D, so option 2 looks to be impractical.

Which of the two remaining options would you prefer?
Flags: needinfo?(roc)
Option 3 sounds fine.
Flags: needinfo?(roc)
James told me he has issues with internet access at the moment and asked me to do the remaining changes here. IIUC, for option 3 it will be enough to remove the cases zoom=.1 to make the test pass...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=090520b8ae49
Attached patch PatchSplinter Review
Attachment #8669608 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2a3c9fbd5ad0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.