Closed
Bug 491595
Opened 15 years ago
Closed 15 years ago
Canvas: drawing text outside of the visible canvas is very slow
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: stas, Assigned: roc)
References
()
Details
(Keywords: fixed1.9.1, perf)
Attachments
(7 files, 1 obsolete file)
14.45 KB,
text/html
|
Details | |
1.12 KB,
patch
|
Details | Diff | Splinter Review | |
12.84 KB,
patch
|
Details | Diff | Splinter Review | |
1.25 KB,
patch
|
Details | Diff | Splinter Review | |
5.01 KB,
text/plain
|
Details | |
2.18 KB,
patch
|
roc
:
review+
shaver
:
approval1.9.1+
|
Details | Diff | Splinter Review |
171.78 KB,
image/png
|
Details |
Canvas method fillText() is very slow when a big text is drawn and it doesn't fit into the visible part of canvas. It seems that we're drawing the whole text instead of just clipping the path to fit in the viewport. Attached is a suite of testcases that I used to test the performance.
Reporter | ||
Comment 1•15 years ago
|
||
Comment 2•15 years ago
|
||
That's a pretty awesome set of test cases.
Comment 3•15 years ago
|
||
It's confirmed macos only. On Linux clip() is fastest, on Windows source-in is. On Safari/Webkit increasing font instead of scaling is way faster and it seems they never redraw what's outside of the viewport.
Convolving and integrating in my RGB? It's more likely than you think! 0.0% 79.0% CoreGraphics CGContextShowGlyphsWithAdvances 0.0% 79.0% CoreGraphics draw_glyphs 0.0% 79.0% libRIP.A.dylib ripc_DrawGlyphs 0.0% 78.8% CoreGraphics CGGlyphLockLockGlyphBitmaps 0.0% 78.8% CoreGraphics create_missing_bitmaps 0.0% 78.7% CoreGraphics CGFontCreateGlyphBitmaps 0.0% 78.7% CoreGraphics CGFontCreateGlyphBitmaps32 0.0% 78.7% CoreGraphics CGFontGetGlyphPaths 0.0% 78.7% CoreGraphics glyph_path_end 1.3% 77.8% CoreGraphics CGSScanconverterRenderMask 76.4% 76.4% CoreGraphics CGSScanConvolveAndIntegrateRGB
Assignee | ||
Comment 5•15 years ago
|
||
It definitely looks like the CG glyph cache is not working. I thought perhaps the problem might be that we're drawing the same CGFont at different scales, and possibly CG only caches glyphs for a single CTM per CGFont. But the test that changes the font size uses separate CGFonts with a constant CTM and that is just as slow.
Assignee | ||
Comment 6•15 years ago
|
||
One thing that's clear is that if you shorten the string to just "A", there is no problem. So CG is definitely rasterizing all glyphs without looking at the clip extents to see if a glyph is visible. It might be worthwhile for us to do that; even if the glyph cache is working, if we're doing rotation so that a lot of (glyph, transform) pairs are never actually visible, we'd be wasting a lot of work rasterizing those glyphs.
Comment 7•15 years ago
|
||
Yea, I confirm that shortening the string solves the problem, so it has to be CG rasterising glyphs each frame. I'm not sure if not rasterizing off-screen glyphs will solve the problem because we still can do a lot of harm by just rotating/scaling/moving the very same glyph at the enormous scale inside the viewport. Should we narrow this down to not caching glyphs and threat ignoring off-screen glyphs as a nice addition?
(In reply to comment #7) > Should we narrow this down to not caching glyphs and threat ignoring off-screen > glyphs as a nice addition? The problem is that "not caching glyphs" isn't something we can fix, whereas "ignoring off-screen glyphs" is something we can do in our code..
Assignee | ||
Comment 9•15 years ago
|
||
As I tried to say in comment #6, even if the glyphs are properly cached, there are lots of examples where ignoring glyphs outside the clip region would still save a lot of work. So we should do that. I'll give it a go, I think it should be fairly straightforward since we already have an efficient "loose" glyph bounds cache that will be fine for this. I'd kind of like to know why CG glyph caching is behaving the way it is, for future reference, but figuring it out would be a huge pain and I don't think we have anyone volunteering to do that. The best approach would probably be to write a small test program equivalent to what we do, and see if we can get someone at Apple to explain why it's not receiving the benefits of glyph caching.
Assignee: nobody → roc
Flags: wanted1.9.1?
Assignee | ||
Comment 10•15 years ago
|
||
This patch sets doUseIntermediateSurface to always be true for text fills. It makes us uniformly fast for all the fillText tests! WHY???
Hmm. I wonder if there's just no clip set on the canvas surface, which is confusing CG? Though with a group there isn't a clip either, so.. hm.
Comment 12•15 years ago
|
||
(In reply to comment #10) > Created an attachment (id=376553) [details] > always PushGroup when drawing text > > This patch sets doUseIntermediateSurface to always be true for text fills. It > makes us uniformly fast for all the fillText tests! WHY??? I could not reproduce the improvement on my mozilla-central build with this patch applied. My results were the same with and without the patch.
Assignee | ||
Comment 13•15 years ago
|
||
Are you on 10.5?
Assignee | ||
Comment 14•15 years ago
|
||
I think this patch is reasonable. If we have glyph extents information, then we use it to drop glyphs that are outside the clip region. The overhead in the Draw path looks pretty low. We do have to add TEXT_NEEDS_BOUNDING_BOX to nsCanvasRenderingContext2D's textruns, to make that extents data available. This will slow down canvas text drawing a little bit in general, but this tradeoff is unavoidable as far as I can see. Extents are cached aggressively so I expect that heavy canvas text users like Bespin won't notice anything. With this patch, the testcase gives about the same results whether the test string is "A" or some long string. That's good. However, source-over fillText() is still slower than source-in and mozPathText() by nearly a factor of two, which suggests to me that the CG glyph cache is still not working. (I wonder whether source-in is fast here because CG gives up on regular glyph rasterization for some reason and just fills the path instead.)
Assignee | ||
Comment 15•15 years ago
|
||
I revived my dormant 133t sk1llz and started reverse-engineering CoreGraphics a bit. CGFontCachePrint is a useful little function, and running it at every fillText call shows us a clear picture of what's going on. Basically the glyph cache size is fixed at 1000000 bytes (at least, I think they're bytes), and we're thrashing it.
Assignee | ||
Comment 16•15 years ago
|
||
Indeed, calling CGFontCacheSetMaxSize(..., 0xd0eedb0, 10*1024*1024) stops us thrashing and makes the test run about twice as fast. CGFontCacheSetMaxSize is undocumented of course, but WKSetUpFontCache in WebKitSystemInterface.h (Webkit's private interface to undocumented OS X functionality) uses it. In Safari 3.2 with the standard Leopard system Webkit, WKSetUpFontCache calls CGFontCacheSetMaxSize with a parameter of 6MB, which explains why that Webkit would beat us. Curiously, on trunk Webkit WKSetUpFontCache does not call CGFontCacheSetMaxSize, it just calls CGFontSetShouldUseMulticache(true), and I'm not sure what that does. Of course Safari 4 itself might still be calling CGFontCacheSetMaxSize. The call to CGFontCacheSetMaxSize was removed in this commit: http://trac.webkit.org/changeset/32530 which I don't understand yet. I guess we should try calling CGFontSetShouldUseMulticache to see if that helps, and if it doesn't, we could consider wrangling CGFontCacheSetMaxSize. Although maybe it doesn't exist in Snow Leopard?
Assignee | ||
Comment 17•15 years ago
|
||
CGFontSetShouldUseMulticache doesn't seem to make a difference. Calling CGFontCacheSetMaxSize to set the size above 3MB helps a lot. So I don't know of any undocumented stuff that *trunk* Webkit is doing that would make it faster.
Assignee | ||
Comment 18•15 years ago
|
||
This patch makes us do the cache size mojo that Leopard's Webkit does.
Assignee | ||
Comment 19•15 years ago
|
||
Another thing we could consider doing is making all text above some size threshold (in device space) be drawn using path filling, on the assumption that the glyphs will be so large that we'll win due to not having to fill the whole path due to clipping, and due to not busting glyph caches.
Assignee | ||
Comment 20•15 years ago
|
||
Oops, I forgot to attach this glyph cache dump.
Assignee | ||
Comment 21•15 years ago
|
||
Looks like someone in cairo already had this idea! This patch just lets us use a much more reasonable font size threshold if the surface supports native fills (in which case we assume filling is reasonably fast). This patch alone, without the other patches, nails the testcase in this bug.
Assignee | ||
Comment 22•15 years ago
|
||
I lost my URL to the "real" demo that inspired this bug. It would be great if someone could re-test that demo with the patch in comment #21, the patch in comment #14, and both patches.
Assignee | ||
Comment 23•15 years ago
|
||
Comment on attachment 376659 [details] [diff] [review] use path filling for large font sizes This patch is a no-brainer, I reckon.
Attachment #376659 -
Flags: review?(jmuizelaar)
Comment 24•15 years ago
|
||
at your service, recompiling
No-brainer patch looks good to me; maybe kick off a try server run with the glyph cache magic thing, see if it makes a difference on Tp?
Comment 26•15 years ago
|
||
I recompiled with one and I recompiled with the other and both gave no signs of improvement for me. I'm wondering if I might recompile improperly after applying patches.
Assignee | ||
Comment 27•15 years ago
|
||
Comment on attachment 376659 [details] [diff] [review] use path filling for large font sizes I'll take that as Vlad's r+
Attachment #376659 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 28•15 years ago
|
||
Gandalf: it sounds like you aren't rebuilding properly... you did a full build?
Whiteboard: [needs landing]
Comment 29•15 years ago
|
||
No, I just rebuilt the components modified by the patch. Should I do a full rebuild?
Assignee | ||
Comment 30•15 years ago
|
||
Yeah, that's the no-brainer way to fix things. You're on Mac so a full build shouldn't take long.
Comment 31•15 years ago
|
||
Nice!!!! Roc, you rock! :) We're faster in all tests and the default behavior is the fastest one now!!! We're getting back on track with the demo, thanks!
Assignee | ||
Comment 32•15 years ago
|
||
Which patches was that for? Have you tried just the patch in comment #21 and no other patches?
Comment 33•15 years ago
|
||
No, this was with patches from Comment 21 and from Comment 13
Assignee | ||
Comment 34•15 years ago
|
||
If you try just the patch in comment #21, are the results good?
Comment 35•15 years ago
|
||
yes! it works perfectly fine with the same performance with patch from comment #21 only.
Comment 36•15 years ago
|
||
Comment 37•15 years ago
|
||
There is a build error: cairo-gstate.c c:\cygwin\home\user\hg\mozilla-central\gfx\cairo\cairo\src\cairo-platform.h : wa rning C4819: The file contains a character that cannot be represented in the cur rent code page (932). Save the file in Unicode format to prevent data loss c:\cygwin\home\user\hg\mozilla-central\gfx\cairo\cairo\src\cairo-platform.h(53) : warning C4005: 'cairo_public' : macro redefinition c:\cygwin\home\user\hg\mozilla-central\gfx\cairo\cairo\src\cairoint.h(54 ) : see previous definition of 'cairo_public' c:/cygwin/home/user/hg/mozilla-central/gfx/cairo/cairo/src/cairo-gstate.c(1692) : error C2143: syntax error : missing ';' before 'type' c:/cygwin/home/user/hg/mozilla-central/gfx/cairo/cairo/src/cairo-gstate.c(1694) : error C2065: 'path_fill_threshold' : undeclared identifier make[1]: *** [cairo-gstate.obj] Error 2
Comment 38•15 years ago
|
||
Comment on attachment 377281 [details] [diff] [review] follow-up patch Oops! Fixed already.
Attachment #377281 -
Attachment is obsolete: true
Assignee | ||
Comment 39•15 years ago
|
||
Checked into trunk: http://hg.mozilla.org/mozilla-central/rev/fe1c87fa95a3 Bustage fixes: http://hg.mozilla.org/mozilla-central/rev/6b797a9da0c8 http://hg.mozilla.org/mozilla-central/rev/621b6607bc80 Those weren't quite just bustage fixes .... I wasn't sure what was going on so I moved the size threshold calculation code into cairo-surface.c, where it seems to be better belong. Hijoyuki's patch makes it obvious the problem was declaring a variable not at the begining of a function in a C file. (Too bad the compiler error message is completely useless.)
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Updated•15 years ago
|
Attachment #376659 -
Flags: approval1.9.1?
Comment on attachment 376659 [details] [diff] [review] use path filling for large font sizes a=shaver
Attachment #376659 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs 191 approval] → [needs 191 landing]
Assignee | ||
Comment 41•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9d0e60c8b7d0
Assignee | ||
Comment 42•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f87bd9b7fdf1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/54aeae69b3bc
You need to log in
before you can comment on or make changes to this bug.
Description
•