Canvas measureText loses precision

VERIFIED FIXED in mozilla7

Status

()

Core
Canvas: 2D
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: gal, Assigned: dbaron)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

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

Comment 3

6 years ago
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.
(Assignee)

Comment 4

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

Comment 5

6 years ago
Created attachment 542660 [details]
test case
(Reporter)

Comment 6

6 years ago
Works in webkit, but not in gecko.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
But I think it's worth testing to see if the change in comment 4 helps the problem you're having.
(Reporter)

Comment 9

6 years ago
Works in opera, too (and boy are opera fonts ugly).
(Reporter)

Comment 10

6 years ago
Kerning is fine, I meant the significant with difference.
(Reporter)

Comment 11

6 years ago
Ok, this is massively different per platform. It looks great on Linux, its totally messed up on mac.
(Reporter)

Comment 12

6 years ago
Created attachment 542663 [details]
mac screenshot
(Assignee)

Comment 13

6 years ago
Created attachment 542669 [details] [diff] [review]
patch, fixes the issue

I didn't compile this.

I also find the mix of float and gfxFloat in DrawOrMeasureText to be a bit sketchy.
(Reporter)

Comment 14

6 years ago
Testing.
(Reporter)

Comment 15

6 years ago
Fixed!
(Reporter)

Updated

6 years ago
Attachment #542669 - Attachment description: untested patch → patch, fixes the issue
(Reporter)

Updated

6 years ago
Attachment #542669 - Flags: review?(roc)
Created attachment 542691 [details] [diff] [review]
reftest

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)
(Reporter)

Comment 19

6 years ago
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.
(Assignee)

Comment 22

6 years ago
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/85e5b0ed7081
http://hg.mozilla.org/integration/mozilla-inbound/rev/dc710b3aa902
Whiteboard: [inbound]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/dc710b3aa902
http://hg.mozilla.org/mozilla-central/rev/85e5b0ed7081
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
(Assignee)

Comment 26

6 years ago
Backed out and relanded with the correct bug number:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6749b5e99460
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54ad22cbf88
http://hg.mozilla.org/mozilla-central/rev/6749b5e99460
http://hg.mozilla.org/mozilla-central/rev/c54ad22cbf88
(Assignee)

Comment 28

6 years ago
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.  :(
merged comment 28 http://hg.mozilla.org/mozilla-central/rev/d2372623b561
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.