Last Comment Bug 667947 - Canvas measureText loses precision
: Canvas measureText loses precision
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: David Baron :dbaron: ⌚️UTC-10
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 698185
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-28 10:27 PDT by Andreas Gal :gal
Modified: 2011-10-29 03:00 PDT (History)
10 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (495 bytes, text/html)
2011-06-28 16:30 PDT, Andreas Gal :gal
no flags Details
mac screenshot (9.27 KB, image/tiff)
2011-06-28 16:42 PDT, Andreas Gal :gal
no flags Details
patch, fixes the issue (2.58 KB, patch)
2011-06-28 16:58 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
reftest (2.21 KB, patch)
2011-06-28 17:36 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
dbaron: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-06-28 10:27:26 PDT
The code uses nscoord (PRInt32), which rounds and loses precision. This is hurting pdf.js (badly).
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-06-28 12:47:22 PDT
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?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-28 15:52:56 PDT
Can you be more precise with a testcase?
Comment 3 Andreas Gal :gal 2011-06-28 15:56:21 PDT
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.
Comment 4 David Baron :dbaron: ⌚️UTC-10 2011-06-28 16:27:28 PDT
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.
Comment 5 Andreas Gal :gal 2011-06-28 16:30:20 PDT
Created attachment 542660 [details]
test case
Comment 6 Andreas Gal :gal 2011-06-28 16:31:19 PDT
Works in webkit, but not in gecko.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2011-06-28 16:34:06 PDT
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.
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-06-28 16:34:46 PDT
But I think it's worth testing to see if the change in comment 4 helps the problem you're having.
Comment 9 Andreas Gal :gal 2011-06-28 16:35:00 PDT
Works in opera, too (and boy are opera fonts ugly).
Comment 10 Andreas Gal :gal 2011-06-28 16:35:26 PDT
Kerning is fine, I meant the significant with difference.
Comment 11 Andreas Gal :gal 2011-06-28 16:39:08 PDT
Ok, this is massively different per platform. It looks great on Linux, its totally messed up on mac.
Comment 12 Andreas Gal :gal 2011-06-28 16:42:02 PDT
Created attachment 542663 [details]
mac screenshot
Comment 13 David Baron :dbaron: ⌚️UTC-10 2011-06-28 16:58:32 PDT
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.
Comment 14 Andreas Gal :gal 2011-06-28 17:00:32 PDT
Testing.
Comment 15 Andreas Gal :gal 2011-06-28 17:03:56 PDT
Fixed!
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 17:36:59 PDT
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 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-28 17:49:02 PDT
Comment on attachment 542669 [details] [diff] [review]
patch, fixes the issue

Review of attachment 542669 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-06-28 18:04:55 PDT
I can confirm that Chris' reftest fails on mac without dbaron's patch but passes with it.
Comment 19 Andreas Gal :gal 2011-06-28 18:35:01 PDT
The test is npotb so I don't think you need review for it. Just land it.
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 18:35:58 PDT
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.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 18:37:41 PDT
(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 22 David Baron :dbaron: ⌚️UTC-10 2011-06-28 21:09:17 PDT
Comment on attachment 542691 [details] [diff] [review]
reftest

r=dbaron, but in future please include useful commit messages in patches for review
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 21:22:34 PDT
Sorry about that, wasn't sure if the test would work.

I'm happy to land both patches; about to push to try.
Comment 26 David Baron :dbaron: ⌚️UTC-10 2011-06-29 14:45:45 PDT
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
Comment 28 David Baron :dbaron: ⌚️UTC-10 2011-06-30 17:50:53 PDT
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.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2011-06-30 20:17:54 PDT
And it was just hg added, not hg copied, so it lost blame too.  :(
Comment 30 Marco Bonardo [::mak] 2011-07-02 02:34:42 PDT
merged comment 28 http://hg.mozilla.org/mozilla-central/rev/d2372623b561
Comment 31 Simona B [:simonab ] 2011-08-29 05:18:05 PDT
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.
Comment 32 Jonathan Kew (:jfkthame) 2011-10-29 03:00:07 PDT
This broke the rendering of mixed-direction strings in canvas; filed bug 698185.

Note You need to log in before you can comment on or make changes to this bug.