Bug 96053 – Don't copy canvas texture during composite if the canvas content didn't change

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Canvas

[KangYuan]

During the process of composite, the canvas element would copy its latest content from back texture to front one, which would display on screen.
We could check whether canvas content is modified since last composite, and save the copy if the content doesn't change.

This can help a little on performance for some cases that typically have a canvas element as background.
One case is FishIETank (http://ie.microsoft.com/testdrive/Performance/FishIETank/), the background canvas is only drawn once when the window size changed. 

-Kang Yuan

[KangYuan]

Created attachment 162650 [details]
suggested patch

[KangYuan]

Created attachment 163041 [details]
patch set 1

rebase

[Wei James (wistoch)]

Comment on attachment 163041 [details]
patch set 1

please add a ChangeLog entry for this patch. http://www.webkit.org/coding/contributing.html 

you can use Tools/Scripts/webkit-patch upload to upload a patch and the script will add ChangeLog entry for you automatically.

[KangYuan]

Created attachment 163059 [details]
Patch

[James Robinson]

This looks nice!  Could you please merge the patch up to the most recent changes so it applies cleanly and the bots can run?  Also please describe more thoroughly what the change is in the ChangeLog.

Do we have any layout tests that exercise this optimization?

[Stephen White]

Comment on attachment 163059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163059&action=review

We're hoping to remove double-buffering from canvas soon.  I'm not sure it's worth pursuing this approach, especially since the code this change touches is dead code in newer versions of WebKit.

> Source/WebCore/platform/graphics/ImageBuffer.h:40
> +#include "HTMLCanvasElement.h"

This is a layering violation.  Files in platform/ should not refer to files outside of platform/.

[Justin Novosad]

This change might have made a difference in the past, but not anymore.
All code paths that depend on m_useDoubleBuffering are almost dead code since deferred canvas rendering turns off double buffering.

I see no need to improve a code path that we are in the process of removing.

[KangYuan]

I exercised the webkit fast layout tests. There is no regression.

For the layer violation, I am going to move class canvasobserver from HTMLCanvasElemnt.h to a new header file that placed in platform directory. Can I do this?

For the double buffer going to dead. After all, the patch still can help in some cases for a short coming time. And it doesn't have any negative impact. If there is no other technical concerns, I hope our optimization could be merged, as it still can benefit those who adopts WebKit in the time frame when that code path will be or was still used.

-Kang Yuan

[James Robinson]

Comment on attachment 163059 [details]
Patch

This patch has no effect right now since we don't ship the double-buffered mode anywhere and we don't plan to ship the double-buffered mode anywhere in the future.  Deferred canvas is on by default and it's the future.  I think trying to change this path is likely to cause more harm than good.