Bug 72765 – [chromium] Avoid pushing dirty tiles to the impl layer

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Avoid pushing dirty tiles to the impl layer

[James Robinson]

Created attachment 115889 [details]
Patch

[Nat Duca]

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.

[James Robinson]

BTW the meat of the change is this in TiledLayerChromium::pushPropertiesTo:

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

The rest is test harness hookup

[Adrienne Walker]

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?

[James Robinson]

(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.

[James Robinson]

Created attachment 115925 [details]
fix comment

[WebKit Review Bot]

Comment on attachment 115925 [details]
fix comment

Attachment 115925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10516337

[Adrienne Walker]

(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.

[James Robinson]

Created attachment 116132 [details]
Defer tiler creation until prepareToUpdate

[James Robinson]

(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.

[Adrienne Walker]

Comment on attachment 116132 [details]
Defer tiler creation until prepareToUpdate

Looks good to me.

[Kenneth Russell]

Comment on attachment 116132 [details]
Defer tiler creation until prepareToUpdate

Sounds good. Big thumbs up on the test. rs=me

[James Robinson]

Committed r100958: <http://trac.webkit.org/changeset/100958>