Bug 72765 - [chromium] Avoid pushing dirty tiles to the impl layer
Summary: [chromium] Avoid pushing dirty tiles to the impl layer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-18 15:18 PST by James Robinson
Modified: 2011-11-21 14:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.26 KB, patch)
2011-11-18 15:20 PST, James Robinson
no flags Details | Formatted Diff | Diff
fix comment (18.22 KB, patch)
2011-11-18 19:16 PST, James Robinson
no flags Details | Formatted Diff | Diff
Defer tiler creation until prepareToUpdate (20.20 KB, patch)
2011-11-21 14:05 PST, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-11-18 15:18:05 PST
[chromium] Avoid pushing dirty tiles to the impl layer
Comment 1 James Robinson 2011-11-18 15:20:59 PST
Created attachment 115889 [details]
Patch
Comment 2 Nat Duca 2011-11-18 15:35:32 PST
Comment on attachment 115889 [details]
Patch

I <3 the tests.

Enne probably wants to look through the meat of the change, its a little beyond my comfort zone.
Comment 3 James Robinson 2011-11-18 15:52:02 PST
BTW the meat of the change is this in TiledLayerChromium::pushPropertiesTo:

248        if (tile->isDirty())
249            continue;

The rest is test harness hookup
Comment 4 Adrienne Walker 2011-11-18 16:28:06 PST
Comment on attachment 115889 [details]
Patch

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

Nice test.  Making LayerChromium more testable is fantastic.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:160
> +void TiledLayerChromium::createTiler(CCLayerTilingData::BorderTexelOption borderTexelOption)
> +{
> +    m_tiler = CCLayerTilingData::create(IntSize(defaultTileSize, defaultTileSize), borderTexelOption);
>  }

It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size.  Maybe this function should take both tile size and texel option?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:64
> +    // Exposed to subclasses for testing.
> +    void setTileSize(const IntSize&);

Maybe not just for testing.  This'd probably be useful if we wanted to have image layers not tile unless they were really large, for instance.  Just sayin'.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:117
> +    // The tile size is 100x100, so this invalidates and then paints three tiles.

3? 100 x 200 pixels = 1 x 2 tiles?
Comment 5 James Robinson 2011-11-18 17:56:13 PST
(In reply to comment #4)
> (From update of attachment 115889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115889&action=review
> 
> Nice test.  Making LayerChromium more testable is fantastic.
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:160
> > +void TiledLayerChromium::createTiler(CCLayerTilingData::BorderTexelOption borderTexelOption)
> > +{
> > +    m_tiler = CCLayerTilingData::create(IntSize(defaultTileSize, defaultTileSize), borderTexelOption);
> >  }
> 
> It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size.  Maybe this function should take both tile size and texel option?

Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created.  Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway.  Would require a bit of careful thought, though, to not get it wrong.  How would you feel about doing this later?

> 
> 
> > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:117
> > +    // The tile size is 100x100, so this invalidates and then paints three tiles.
> 
> 3? 100 x 200 pixels = 1 x 2 tiles?

Bad comment, when I was fighting border texels I made this test use 3 tiles instead of two. Will fix the comment.
Comment 6 James Robinson 2011-11-18 19:16:21 PST
Created attachment 115925 [details]
fix comment
Comment 7 WebKit Review Bot 2011-11-18 20:03:33 PST
Comment on attachment 115925 [details]
fix comment

Attachment 115925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10516337
Comment 8 Adrienne Walker 2011-11-21 09:51:16 PST
(In reply to comment #5)
> (In reply to comment #4)

> > It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size.  Maybe this function should take both tile size and texel option?
> 
> Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created.  Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway.  Would require a bit of careful thought, though, to not get it wrong.  How would you feel about doing this later?

If it's not too complicated, deferring the creation of the tiler until the tile size is known seems like it'd make this cleaner.
Comment 9 James Robinson 2011-11-21 14:05:08 PST
Created attachment 116132 [details]
Defer tiler creation until prepareToUpdate
Comment 10 James Robinson 2011-11-21 14:06:43 PST
(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #4)
> 
> > > It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size.  Maybe this function should take both tile size and texel option?
> > 
> > Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created.  Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway.  Would require a bit of careful thought, though, to not get it wrong.  How would you feel about doing this later?
> 
> If it's not too complicated, deferring the creation of the tiler until the tile size is known seems like it'd make this cleaner.

This patch defers until prepareToUpdate() which seems like the first place where the tiler really needs to be there.  If tile size or tiling option are changed before the first prepareToUpdate() call then we just store the new values to use then. If we have a tiler and the tile size / tiling option change, then the existing tiler is thrown away and a new one created in prepareToUpdate(). I think it is cleaner this way.
Comment 11 Adrienne Walker 2011-11-21 14:14:27 PST
Comment on attachment 116132 [details]
Defer tiler creation until prepareToUpdate

Looks good to me.
Comment 12 Kenneth Russell 2011-11-21 14:15:36 PST
Comment on attachment 116132 [details]
Defer tiler creation until prepareToUpdate

Sounds good. Big thumbs up on the test. rs=me
Comment 13 James Robinson 2011-11-21 14:22:45 PST
Committed r100958: <http://trac.webkit.org/changeset/100958>