Closed Bug 667947 Opened 11 years ago Closed 11 years ago

Canvas measureText loses precision

Categories

(Core :: Canvas: 2D, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: gal, Assigned: dbaron)

References

Details

Attachments

(4 files)

The code uses nscoord (PRInt32), which rounds and loses precision. This is hurting pdf.js (badly).
nscoord can express things down to 1/60px.  We use it for all of our layout calculations, including all of Gecko's text-measurement.  Why is this being particularly a problem for pdf.js?
Can you be more precise with a testcase?
We are rending PDFs which use absolute positioning. Depending on the tool used to generate the PDF, words are often chunked, i.e. we have a command to write "Hel" and then a command to render "lo". For small font sizes, a small measurement error when measuring "Hel" results in the ll stuck together too closely. There is no repositioning in between, we do fillText and then x += measureText.

We are currently working around this problem by making the font 100x larger and then measuring that, but that can be very slow when a large font has to be rasterized for that.
nsCanvasBidiProcessor::GetWidth scales to pixels while the data are still integers, and then nsCanvasRenderingContext2D::DrawOrMeasureText (with nsBidiPresUtils::ProcessText in between) converts the result to a float.  Perhaps that scaling should move out into DrawOrMeasureText.
Attached file test case
Works in webkit, but not in gecko.
On Linux, that testcase shows that you're getting kerning when "Wo" is rendered together but you don't get the kerning when you draw the letters separately.  That's expected.
But I think it's worth testing to see if the change in comment 4 helps the problem you're having.
Works in opera, too (and boy are opera fonts ugly).
Kerning is fine, I meant the significant with difference.
Ok, this is massively different per platform. It looks great on Linux, its totally messed up on mac.
Attached image mac screenshot
I didn't compile this.

I also find the mix of float and gfxFloat in DrawOrMeasureText to be a bit sketchy.
Testing.
Fixed!
Attachment #542669 - Attachment description: untested patch → patch, fixes the issue
Attachment #542669 - Flags: review?(roc)
Attached patch reftestSplinter Review
This passes for me on linux-x11 without the patch, but judging by Andreas's screenshot ought to fail on mac.
Comment on attachment 542669 [details] [diff] [review]
patch, fixes the issue

Review of attachment 542669 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542669 - Flags: review?(roc) → review+
I can confirm that Chris' reftest fails on mac without dbaron's patch but passes with it.
Attachment #542691 - Attachment description: reftest? → reftest
Attachment #542691 - Flags: review?(dbaron)
The test is npotb so I don't think you need review for it. Just land it.
Comment on attachment 542691 [details] [diff] [review]
reftest

I chose "He" because those two glyphs shouldn't be kerned, and they were spaced weirdly in Andreas's screenshot.
(In reply to comment #19)
> The test is npotb so I don't think you need review for it. Just land it.

It's part of the build.  Whether tests need review varies by module, but I haven't been told not to request review for canvas reftests yet.
Comment on attachment 542691 [details] [diff] [review]
reftest

r=dbaron, but in future please include useful commit messages in patches for review
Attachment #542691 - Flags: review?(dbaron) → review+
Sorry about that, wasn't sure if the test would work.

I'm happy to land both patches; about to push to try.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/dc710b3aa902
http://hg.mozilla.org/mozilla-central/rev/85e5b0ed7081
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
I also landed the same patch on the Azure canvas implementation:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2372623b561
since Bas pointed out to me on IRC that canvas has been forked.

This is a real pain, though, since the patch would have applied cleanly had the whitespace and bracing style of the entire file not been changed.
And it was just hg added, not hg copied, so it lost blame too.  :(
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Verified issue on Mac OS X 10.6, Win XP, Win 7 and Ubuntu 11.04 using the test case attached in Comment 5. Issue is no longer reproducible.

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Depends on: 698185
This broke the rendering of mixed-direction strings in canvas; filed bug 698185.
You need to log in before you can comment on or make changes to this bug.