430873 - fast-path drawImage with a canvas as source

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

Attachment: add native fast-path

There's a bit of quite dumb code that always converts a source canvas to an image surface before rendering it when a canvas is used as a source for drawImage.  This makes all sorts of actions much slower than they need to be, especially when a separate canvas is used for buffering (e.g. when drawWindow is done to a separate canvas which is then composited according to user input).

This extends the internal canvas API so that it's possible for a canvas to request a thebes surface from another canvas, and so speeds up the drawImage operation tremendously.

I'd like to get this into RC1, as it's a quite low-risk very large perf win for extensions and other consumers of canvas.

Comment 1

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

Comment on attachment 317784 [details] [diff] [review]
add native fast-path

Did I ever tell you that I sort of hate the way that each 'context' actually has its own completely separate buffer state?

Comment 2

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

(In reply to comment #1)
> (From update of attachment 317784 [details] [diff] [review])
> Did I ever tell you that I sort of hate the way that each 'context' actually
> has its own completely separate buffer state?

I think you did, though there are good arguments for both ways of doing it!

Though with this, you could probably hack up an additional context to grab the buffer of the previously-created context or somesuch...  that is, once we support multiple contexts on one canvas.

Comment 3

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

Comment on attachment 317784 [details] [diff] [review]
add native fast-path

Requesting approval for this (I believe) low-risk performance fix.  It greatly improves the common case of rendering a canvas to another canvas using drawImage; test coverage is in the form of the existing canvas tests, as this is just a perf issue and should not change functionality at all.

Comment 4

[Damon Sicore (:damons)]

Comment on attachment 317784 [details] [diff] [review]
add native fast-path

a1.9+=damons

Comment 5

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

Attachment: minor tweak

This is a minor tweak to the previous patch -- the previous code would've caused a canvas used as a pattern to be "live", which is a change in behaviour.  With this patch, a copy is forced when a canvas is used as a pattern, but no copy happens when drawImage is used.

Comment 6

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

+                ctx->SetOperator(gfxContext::OPERATOR_OVER);

I wondered why this isn't OPERATOR_SOURCE, but I guess it's because you want to compose the surfaces if there are multiple contexts.

Another argument for having only one active buffer :-).

Comment 7

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

(In reply to comment #2)
> Though with this, you could probably hack up an additional context to grab the
> buffer of the previously-created context or somesuch...  that is, once we
> support multiple contexts on one canvas.

Right. Let there be an "active" context which has the current buffer. If someone draws through an inactive context, it takes over the buffer, copying to the right kind of surface if necessary. That would make a lot more sense to me.

Comment 8

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

Attachment: my testcase

Comment 9

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

Comment on attachment 318299 [details]
my testcase

This is very much the wrong bug.

Comment 10

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Backed out because of
Linux qm-centos5-01 dep unit test on 2008/04/29 03:15:14
*** 4847 ERROR FAIL | pixel 1,1 is 0,0,0,255; expected 0,255,0,255 +/- 0 |  | /tests/content/canvas/test/test_2d.pattern.basic.nocontext.html

Comment 11

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Note, at least locally that test failure was random, but happened pretty often.

Comment 12

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

Attachment: fix for linux

Stupid linux.  The temp surface created isn't cleared like it should be (and like it is on the other platforms); so go back to explicitly clearing it.  Waiting on tryserver build to verify this on linux.

Comment 13

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 318414 [details] [diff] [review]
fix for linux

Tested locally, works ok on 64bit linux.

Comment 14

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

Comment on attachment 318414 [details] [diff] [review]
fix for linux

Simple fix for linux, just ran tests on linux to make sure the issue is fixed.  Just needed the temp surface to be cleared (like it was before, but I thought it wasn't needed, argh).

Comment 15

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

Comment on attachment 318414 [details] [diff] [review]
fix for linux

Begging for approval again, big canvas perf win, this time without silly bugs on linux. :(

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 318414 [details] [diff] [review]
fix for linux

a1.9+=damons

Comment 17

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

Checked in again.