Bug 58907 - [chromium] Don't unnecessarily resize skia/cg canvases when painting in compositor
Summary: [chromium] Don't unnecessarily resize skia/cg canvases when painting in compo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-19 11:07 PDT by Adrienne Walker
Modified: 2011-05-02 16:13 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.47 KB, patch)
2011-04-19 11:09 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2011-04-27 14:01 PDT, Adrienne Walker
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-04-19 11:07:16 PDT
[chromium] Don't unnecessarily resize skia/cg canvases when painting in compositor
Comment 1 Adrienne Walker 2011-04-19 11:09:25 PDT
Created attachment 90220 [details]
Patch
Comment 2 Adrienne Walker 2011-04-19 11:10:44 PDT
James brought up this issue during a code inspection of how LayerTilerChromium worked, but when looking into fixing it, it looked like the save and restore context logic wasn't correct either.
Comment 3 Adrienne Walker 2011-04-19 11:12:05 PDT
Comment on attachment 90220 [details]
Patch

Actually, I'd like to write a layout test for this.  I only caught the context save/restore logic snafu because I manually tested this two line patch.

So, I'm taking off request for review until I do that.
Comment 4 Adrienne Walker 2011-04-27 14:01:03 PDT
Created attachment 91343 [details]
Patch
Comment 5 James Robinson 2011-04-27 14:08:04 PDT
I'm not sure I understand the save/restore context stuff here - could you explain?  What was wrong with the previous code?
Comment 6 Adrienne Walker 2011-04-27 14:11:36 PDT
The previous code (and the reason for including this test case) put the save and restore in the wrong place.  In LayerTilerChromium, there's a translation of the canvas as well which wasn't getting popped.  So, multiple redraws of the same canvas would accumulate an incorrect transform.  This was masked because we were always recreating the canvas.  Oops.

The save/restore was moved to be in the Painter functor itself, where it can wrap all state changing operations.
Comment 7 James Robinson 2011-04-28 15:38:18 PDT
Comment on attachment 91343 [details]
Patch

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

R=me, few things to address before landing.

> LayoutTests/compositing/repaint/same-size-invalidation.html:22
> +        setTimeout("changeBackground1();", 0);

nit: setTimeout(changeBackground1, 0) is a bit better. move doTest below the declarations of the functions.

> LayoutTests/platform/chromium/test_expectations.txt:3973
> +BUGENNE : compositing/repaint/same-size-invalidation.html = FAIL

= FAIL isn't sufficient - these will fail with MISSING on windows and mac since there is no baseline for those platforms.

This test should render the same on all platforms so I'd just check same-size-invalidation-expected.png/checksum into platform/chromium-gpu/ and not modify test_expectations at all.

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:84
> +        // Destructor restores canvas to pre-construction state.

does the Painter manipulate any state on the PlatformCanvas?  Painter::Painter() and Painter::~Painter() only seem to mutate their m_context, which doesn't do anything to the PlatformCanvas other than render into it.
Comment 8 Adrienne Walker 2011-04-28 19:38:27 PDT
(In reply to comment #7)
> (From update of attachment 91343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91343&action=review
> 
> R=me, few things to address before landing.
> 
> > LayoutTests/compositing/repaint/same-size-invalidation.html:22
> > +        setTimeout("changeBackground1();", 0);
> 
> nit: setTimeout(changeBackground1, 0) is a bit better. move doTest below the declarations of the functions.

Will do.  Using a string is just sloppy.  Also, I don't think I need to reorder anything because Javascript hoisting will take care of it.

> > LayoutTests/platform/chromium/test_expectations.txt:3973
> > +BUGENNE : compositing/repaint/same-size-invalidation.html = FAIL
> 
> = FAIL isn't sufficient - these will fail with MISSING on windows and mac since there is no baseline for those platforms.

Yeah, I've changed this locally already.  I learned that the hard way just yesterday.

> This test should render the same on all platforms so I'd just check same-size-invalidation-expected.png/checksum into platform/chromium-gpu/ and not modify test_expectations at all.

Too much gardening experience lately has made me cautious.  I was going to let the rebaseline tool tell me that.

> > Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:84
> > +        // Destructor restores canvas to pre-construction state.
> 
> does the Painter manipulate any state on the PlatformCanvas?  Painter::Painter() and Painter::~Painter() only seem to mutate their m_context, which doesn't do anything to the PlatformCanvas other than render into it.

Perhaps the comment should be clarified to be "restores canvas context" instead.
Comment 9 Adrienne Walker 2011-05-02 15:13:49 PDT
Committed r85527: <http://trac.webkit.org/changeset/85527>
Comment 10 Adrienne Walker 2011-05-02 16:13:35 PDT
Committed r85539: <http://trac.webkit.org/changeset/85539>