Bug 54143 – [chromium] Modify LayerTilerChromium to use TilingData.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Modify LayerTilerChromium to use TilingData.

[Adrienne Walker]

Created attachment 81873 [details]
Patch

[WebKit Review Bot]

Attachment 81873 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7884034

[Adrienne Walker]

This is another incremental step towards tiling layers.  Because layers can have transforms on them, we need to handle bilinear filtering across tiles.  This code changes the compositor tiling code to use scheib's TilingData class that is currently used for canvas images.

I realize there are a lot of changes in this patch, so here's the few changes from which everything else falls out.

1) To handle scrollbars before, I was changing the tile size to be exactly the size of the scrollbar.  TilingData only has one maximum texture dimension.  Rather than modifying that code to handle two texture dimensions (which in retrospect may have been a better option), I ended up just adding a maximum texture size option to LayerTilerChromium.  For example, this prevents a vertical scrollbar's texture from never taking more than its width.

2) TilingData needed to be changed to allow the page to be resized and to add an option for not truncating the right-most and bottom-most tiles to sub-tile texture sizes.  Canvas images don't change in size, but root layers can certainly get bigger and it seems reasonable to have a constant tile size rather than trying to have smaller textures on the edges of pages.

3) Because tile textures now have border texels, I had to add a texture matrix to the ContentLayerChromium shader.  However, because our use of shaders in the compositor is entangled by all going through the same draw routine, I had to add a bunch of parameters to the drawing code in all the other layers.  If there are ways to make this better, I'd love suggestions.

This patch feels really messy when I look at the diff, so I'd love any suggestions about design, clarity, or even splitting this patch up to be simpler.

(I also need to add more unit tests for the TilingData changes before I commit this and to fix the unit test build errors above.)

[Vincent Scheib]

Comment on attachment 81873 [details]
Patch

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

I looked at the use of TilingData and changes to it. Discussed with Adrienne in person some, and think some code can be simplified, at noted points.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:184
> +    if (m_textureClampX > 0 && layerRect.width() > m_textureClampX)

could just set m_textureClampX to max texture size and always run a min() instead of if

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:193
> +    int width = m_textureClampX ? std::min(m_tileSize, m_textureClampX) : m_tileSize;

ditto "could just set m_textureClampX to max texture size and always run a min() instead of if"

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:437
> +            if (m_textureClampX > 0 && tileRect.width() > m_textureClampX)

just a min() needed if textureClamp always set

> Source/WebCore/platform/graphics/gpu/TilingData.h:75
> +    IntPoint textureOffset(int xIndex, int yIndex) const;

Perhaps rename to make a little clearer that this is returning the boarder on top or left? e.g. have two that say tileTopBorder() ...?

> Source/WebCore/platform/graphics/gpu/TilingData.h:85
> +    bool m_clipEdgeTiles;

Can be pulled out

[Vangelis Kokkevis]

Comment on attachment 81873 [details]
Patch

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

Overall it looks reasonable.  A couple of starter comments from a quick glance.  I'll look into more details later..

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:213
> +                                 int textureMatrixLocation);

I would recommend instead of changing the existing drawTexturedQuad method and forcing all layer types to pass additional parameters, to create a new method, preferably in the tiler that deals with drawing these quads.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:447
> +            texMatrix.scale(tileRect.width(), tileRect.height());

It would be a bit more efficient if instead of using a matrix, you actually passed in a single quad the two offsets and scale factors.

[Adrienne Walker]

(In reply to comment #5)
> (From update of attachment 81873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81873&action=review
> 
> Overall it looks reasonable.  A couple of starter comments from a quick glance.  I'll look into more details later..

Please wait for the next patch before spending any time looking at it more.  After talking with Vince about his feedback above, a lot of the code gets cleaned up and simpler, so it should be easier to read.

> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:213
> > +                                 int textureMatrixLocation);
> 
> I would recommend instead of changing the existing drawTexturedQuad method and forcing all layer types to pass additional parameters, to create a new method, preferably in the tiler that deals with drawing these quads.

Hmm.  I guess I can create a new shader and set of shared values in the tiler and eventually the ContentLayerChromium and ImageLayerChromium will be switched over to use it.  We'll just have some duplication in the meantime.

> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:447
> > +            texMatrix.scale(tileRect.width(), tileRect.height());
> 
> It would be a bit more efficient if instead of using a matrix, you actually passed in a single quad the two offsets and scale factors.

Great point.  Will do.

[Vangelis Kokkevis]

> 
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:213
> > > +                                 int textureMatrixLocation);
> > 
> > I would recommend instead of changing the existing drawTexturedQuad method and forcing all layer types to pass additional parameters, to create a new method, preferably in the tiler that deals with drawing these quads.
> 
> Hmm.  I guess I can create a new shader and set of shared values in the tiler and eventually the ContentLayerChromium and ImageLayerChromium will be switched over to use it.  We'll just have some duplication in the meantime.

I thought that Content and Image layers will get tiled only if they are larger than a certain size (or maybe if they have overflow).  I would expect the simpler, smaller layers to go through the existing path.  Isn't that the case?

[James Robinson]

(In reply to comment #7)
> > 
> > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:213
> > > > +                                 int textureMatrixLocation);
> > > 
> > > I would recommend instead of changing the existing drawTexturedQuad method and forcing all layer types to pass additional parameters, to create a new method, preferably in the tiler that deals with drawing these quads.
> > 
> > Hmm.  I guess I can create a new shader and set of shared values in the tiler and eventually the ContentLayerChromium and ImageLayerChromium will be switched over to use it.  We'll just have some duplication in the meantime.
> 
> I thought that Content and Image layers will get tiled only if they are larger than a certain size (or maybe if they have overflow).  I would expect the simpler, smaller layers to go through the existing path.  Isn't that the case?

I'm not sure this is obvious - we may want to just tile everything to improve memory locality and to make the upload bitmaps smaller, which could make allocation of the transfer buffers easier (lots of small uniformly-size allocations fragment a lot less than large irregularly-sized allocations).  We'd probably have to experiment.

The mac compositor avoids tiling on layers whenever possible due to performance problems that we can avoid.

[Adrienne Walker]

Created attachment 82073 [details]
Patch

[James Robinson]

Comment on attachment 82073 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:49
> +PassOwnPtr<LayerTilerChromium> LayerTilerChromium::create(LayerRendererChromium* layerRenderer, const IntSize& tileSize, bool hasBorderTexels)

a bool is unfortunate here - it's impossible to tell what the values 'true' and 'false' mean when looking at a callsite.  how about enum { HasBorderTexels, NoBorderTexels } ?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:59
> +    , m_tilingData(std::max(tileSize.width(), tileSize.height()), 0, 0, hasBorderTexels)

WebKit style is to add a 'using namespace std' declaration and just call max()

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:50
> +    static PassOwnPtr<LayerTilerChromium> create(LayerRendererChromium*, const IntSize& tileSize, bool hasBorderTexels);

same here - the bool is pretty opaque

[Adrienne Walker]

Created attachment 82076 [details]
Patch

[Adrienne Walker]

(In reply to comment #11)
> Created an attachment (id=82076) [details]
> Patch

I fixed the above things that jamesr mentions.

I'm not terribly happy with the copied fragment shader from ContentLayerChromium, but I don't see a great way to share it without the two shared value classes becoming very awkwardly coupled together.

[Vangelis Kokkevis]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=82076) [details] [details]
> > Patch
> 
> I fixed the above things that jamesr mentions.
> 
> I'm not terribly happy with the copied fragment shader from ContentLayerChromium, but I don't see a great way to share it without the two shared value classes becoming very awkwardly coupled together.

Here's one possible solution: Create a base SharedValue class that uses static members to store shader strings shared across the derived SharedValues.  I think for example our vertex shader string is shared among most of our shaders.  I've also been meaning to start using the macro trick to define shader strings to make them easier to read and also get rid of all the unnecessary spaces we currently have.

[Adrienne Walker]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created an attachment (id=82076) [details] [details] [details]
> > > Patch
> > 
> > I fixed the above things that jamesr mentions.
> > 
> > I'm not terribly happy with the copied fragment shader from ContentLayerChromium, but I don't see a great way to share it without the two shared value classes becoming very awkwardly coupled together.
> 
> Here's one possible solution: Create a base SharedValue class that uses static members to store shader strings shared across the derived SharedValues.  I think for example our vertex shader string is shared among most of our shaders.  I've also been meaning to start using the macro trick to define shader strings to make them easier to read and also get rid of all the unnecessary spaces we currently have.

That sounds like a great plan.  I think I'd prefer to make this change and then clean up all the compositor shaders in a separate patch immediately following this one.

[Vincent Scheib]

Comment on attachment 82076 [details]
Patch

LGTM (TilingData code reviewed)

[James Robinson]

Comment on attachment 82076 [details]
Patch

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

It looks like this patch doesn't apply to ToT and so the EWS bots can't process it.  Would you mind posting a new patch that does apply just to be on the safe side?

> Source/WebCore/ChangeLog:7
> +        [chromium] Modify LayerTilerChromium to use TilingData.
> +        https://bugs.webkit.org/show_bug.cgi?id=54143
> +

this needs some sort of description of the patch what's going on here

[Adrienne Walker]

Created attachment 82180 [details]
Patch

[James Robinson]

Comment on attachment 82180 [details]
Patch

R=me

[Adrienne Walker]

Committed r78484: <http://trac.webkit.org/changeset/78484>

[Chris Guillory]

Created attachment 85406 [details]
Fix for Coverity discovered NO_EFFECT (self-assign) defect.

[James Robinson]

Comment on attachment 85406 [details]
Fix for Coverity discovered NO_EFFECT (self-assign) defect.

Hah!  Good catch.

Is there any way our testing could be extended to catch this, enne?

[Adrienne Walker]

Sure.  It looks like there are some TilingData unit tests that should have been added to when I added that method in the first place.

[WebKit Commit Bot]

Comment on attachment 85406 [details]
Fix for Coverity discovered NO_EFFECT (self-assign) defect.

Clearing flags on attachment: 85406

Committed r80824: <http://trac.webkit.org/changeset/80824>

[Adrienne Walker]

Created attachment 85519 [details]
Patch

[Kenneth Russell]

Comment on attachment 85519 [details]
Patch

The test looks good. r=me

[WebKit Commit Bot]

Comment on attachment 85519 [details]
Patch

Clearing flags on attachment: 85519

Committed r80894: <http://trac.webkit.org/changeset/80894>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.