491595 - Canvas: drawing text outside of the visible canvas is very slow

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Staś Małolepszy :stas]

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.

Comment 1

[Staś Małolepszy :stas]

Attachment: Testcase

Comment 2

[Christopher Blizzard (:blizzard)]

That's a pretty awesome set of test cases.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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..

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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???

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you on 10.5?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: drop glyphs that we know aren't visible

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.)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: use undocumented cache-size functions

This patch makes us do the cache size mojo that Leopard's Webkit does.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: one cycle of the glyph cache thrashing

Oops, I forgot to attach this glyph cache dump.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: use path filling for large font sizes

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 376659 [details] [diff] [review]
use path filling for large font sizes

This patch is a no-brainer, I reckon.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

at your service, recompiling

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 376659 [details] [diff] [review]
use path filling for large font sizes

I'll take that as Vlad's r+

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Gandalf: it sounds like you aren't rebuilding properly... you did a full build?

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

No, I just rebuilt the components modified by the patch. Should I do a full rebuild?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, that's the no-brainer way to fix things. You're on Mac so a full build shouldn't take long.

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: screenshot comparing Gecko with patches and WebKit

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!

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which patches was that for?

Have you tried just the patch in comment #21 and no other patches?

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

No, this was with patches from Comment 21 and from Comment 13

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you try just the patch in comment #21, are the results good?

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

yes! it works perfectly fine with the same performance with patch from comment #21 only.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: follow-up patch

Comment 37

[Hiroyuki Ikezoe (:hiro)]

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

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 377281 [details] [diff] [review]
follow-up patch

Oops! Fixed already.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.)

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 376659 [details] [diff] [review]
use path filling for large font sizes

a=shaver

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9d0e60c8b7d0

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f87bd9b7fdf1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/54aeae69b3bc