Bug 96053 - Don't copy canvas texture during composite if the canvas content didn't change
Summary: Don't copy canvas texture during composite if the canvas content didn't change
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-06 19:08 PDT by KangYuan
Modified: 2022-07-18 14:23 PDT (History)
6 users (show)

See Also:


Attachments
suggested patch (3.23 KB, patch)
2012-09-06 19:12 PDT, KangYuan
no flags Details | Formatted Diff | Diff
patch set 1 (6.75 KB, patch)
2012-09-09 22:41 PDT, KangYuan
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2012-09-10 00:57 PDT, KangYuan
jamesr: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KangYuan 2012-09-06 19:08:08 PDT
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
Comment 1 KangYuan 2012-09-06 19:12:34 PDT
Created attachment 162650 [details]
suggested patch
Comment 2 KangYuan 2012-09-09 22:41:13 PDT
Created attachment 163041 [details]
patch set 1

rebase
Comment 3 Wei James (wistoch) 2012-09-10 00:23:41 PDT
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.
Comment 4 KangYuan 2012-09-10 00:57:12 PDT
Created attachment 163059 [details]
Patch
Comment 5 James Robinson 2012-09-10 08:45:52 PDT
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?
Comment 6 Stephen White 2012-09-10 12:45:38 PDT
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/.
Comment 7 Justin Novosad 2012-09-10 12:53:38 PDT
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.
Comment 8 KangYuan 2012-09-12 01:56:09 PDT
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
Comment 9 James Robinson 2012-09-12 01:58:41 PDT
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.