Bug 47016 – [chromium] GPU compositor cannot handle large layers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Adrienne Walker]

This corresponds to http://code.google.com/p/chromium/issues/detail?id=48019.

From the original bug: "There's a limit to the maxium texture size that GPUs can handle and therefore there's currently a hard limit to how large layers can be (it's set to 4Kx4K).  A proper solution would be to implement tiled layers to be more friendly with vram usage."

[Adrienne Walker]

Created attachment 69516 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 69516 [details]
Patch

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

Looks good, other than the couple of comments.

> WebCore/ChangeLog:9
> +        large are not drawn, as before.

nit but for clarity you might want to make the distinction between layers that are transformed in 3D and those that appear flat on the page (instead of 2D and 3D layers).

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:183
> +          m_skipsDraw = true;

4 space indent

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:190
> +        const IntSize layerPos(position().x() - m_bounds.width() / 2,

position() is not necessarily the center point of the layer as other matrices get layered on on it.  The center of the layer is always the translation component of the drawTransform()

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:200
> +        m_largeLayerDrawRect = drawRect;

It took me a bit of time to decipher the transformation logic here. Adding some inline comments describing what each rect and pos really is and in which space, would really help a lot.

[Vangelis Kokkevis]

I also wonder whether we can split some of this code into its own function so that we can use it for large images which end up in an ImageLayerChromium (a derived class).

[Adrienne Walker]

(In reply to comment #3)
> I also wonder whether we can split some of this code into its own function so that we can use it for large images which end up in an ImageLayerChromium (a derived class).

That's reasonable.  I will just go ahead and update ImageLayerChromium with this same change while I'm at it.

[Adrienne Walker]

Created attachment 69865 [details]
Patch

[Adrienne Walker]

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

Addressed the above feedback.  Abstracted the transform logic into its own function.  Updated ImageLayerChromium to just use the ContentLayerChromium logic for creating partial textures. Added a test with a large (500x20000) image to test this.

Additionally, after some thought, given that large layers will need to reallocate pixels every time the content rect changes during scrolling, the canvas and pixel data is cached and reused if it is the same size.

[James Robinson]

Comment on attachment 69865 [details]
Patch

For a .dumpAsText() or .layerTreeAsText() test there should be no expected .png, just the layer output (as a .txt).  Do you need to specify both .dumpAsText() and .layerTreeAsText()?

[James Robinson]

Whoops, I totally misinterpreted the purpose of the image. My bad!

[James Robinson]

Comment on attachment 69865 [details]
Patch

Resetting enne's flags

[Adrienne Walker]

(In reply to comment #9)
> (From update of attachment 69865 [details])
> Resetting enne's flags

Per an in-person conversation with jamesr, I am going to look into seeing if there's some way to create an image layer without checking in a PNG.

[Adrienne Walker]

Created attachment 69957 [details]
Patch

[Adrienne Walker]

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

Image file removed.  The image src is now programmatically generated from a canvas.

[Adrienne Walker]

Created attachment 70097 [details]
Patch

[Adrienne Walker]

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

This last patch is just a tiny addition of a sanity check for the case where clipping against the content rect still yields a texture that is too large.

I'd like to land this patch today if possible, given that I'm out on vacation Friday through Monday.  If it's at all possible, could I get a review today?

[Vangelis Kokkevis]

Comment on attachment 70097 [details]
Patch

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

Looks good overall.  Some comments are inline. Also, while I like the improvement done with caching the update bitmaps so that they can be reused I feel that that change should go on a separate CL as it's orthogonal to the large layer stuff.

> LayoutTests/compositing/tiling/huge-layer-img.html:22
> +            var context = canvas.getContext('2d');

Might be better to try to achieve the same effect with absolutely positioned <div> elements with a background color style property. I'm worried that <canvas> adds an unnecessary unknown to the test.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:204
>      IntRect dirtyRect(m_dirtyRect);

Since m_dirtyRect isn't used by the large layers, setting the dirtyRect to m_dirtyRect should be done for small layers only for clarity.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:353
> +                         m_largeLayerDrawRect.width() / 2);

Since largeLayers only kick in if their draw transform is identity or translation, we can completely throw out the drawTransform and do:
TransformationMatrix transform;
transform.translate3d(m_largeLayerDrawRect.center().x(), m_largeLayerDrawRect.center().y())

> WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:81
> +        // image to a canvas, as the pixel contents can't be uploaded directly.

canvas -> bitmap  (it's only a "canvas" in the skia case)

[Adrienne Walker]

Thanks for the quick review.

(In reply to comment #15)
> (From update of attachment 70097 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70097&action=review
> 
> Looks good overall.  Some comments are inline. Also, while I like the improvement done with caching the update bitmaps so that they can be reused I feel that that change should go on a separate CL as it's orthogonal to the large layer stuff.

That's reasonable.  No need for this patch to be overly complex.  I'll cherry-pick that change out into another patch and file a bug once this has landed.
 
> > LayoutTests/compositing/tiling/huge-layer-img.html:22
> > +            var context = canvas.getContext('2d');
> 
> Might be better to try to achieve the same effect with absolutely positioned <div> elements with a background color style property. I'm worried that <canvas> adds an unnecessary unknown to the test.

This is a test for ImageLayerChromium.  The huge-layer.html page already does exactly what you specify.

I changed this test to use canvas to satisfy jamesr's request above that I not commit a large binary image.  I could use a data URL, but that seemed less flexible for somebody else modifying the test in the future.  Is that preferable to canvas?

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:204
> >      IntRect dirtyRect(m_dirtyRect);
> 
> Since m_dirtyRect isn't used by the large layers, setting the dirtyRect to m_dirtyRect should be done for small layers only for clarity.

I'll clean this up.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:353
> > +                         m_largeLayerDrawRect.width() / 2);
> 
> Since largeLayers only kick in if their draw transform is identity or translation, we can completely throw out the drawTransform and do:
> TransformationMatrix transform;
> transform.translate3d(m_largeLayerDrawRect.center().x(), m_largeLayerDrawRect.center().y())

Ah, that's a much more elegant way to express that.

> > WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:81
> > +        // image to a canvas, as the pixel contents can't be uploaded directly.
> 
> canvas -> bitmap  (it's only a "canvas" in the skia case)

Quite right.

[Adrienne Walker]

(In reply to comment #16)

> > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:353
> > > +                         m_largeLayerDrawRect.width() / 2);
> > 
> > Since largeLayers only kick in if their draw transform is identity or translation, we can completely throw out the drawTransform and do:
> > TransformationMatrix transform;
> > transform.translate3d(m_largeLayerDrawRect.center().x(), m_largeLayerDrawRect.center().y())
> 
> Ah, that's a much more elegant way to express that.

Actually, what about the original z-component? Are we not still using depth buffers?

[Adrienne Walker]

Created attachment 70173 [details]
Patch

[Adrienne Walker]

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

Removed the caching of update bitmaps.

After an in-person discussion with Vangelis, I left the re-use of the drawTransform (as the z-component of the translation may not be non-zero) and the use of canvas (as using a data URL would likely be larger on disk than using a PNG in the first place).

[Adrienne Walker]

Created attachment 70201 [details]
Patch

[Adrienne Walker]

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

Rebaseling against ToT.

[James Robinson]

Comment on attachment 70201 [details]
Patch

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

Needs a touch more cleanup.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> +    , m_allocatedTextureSize(0, 0)
> +    , m_skipsDraw(false)
> +    , m_largeLayerDrawRect(0, 0, 0, 0)
> +    , m_largeLayerDirtyRect(0, 0, 0, 0)

The default constructor should do the right thing for all of these except for m_skipsDraw.  No need to list them out.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:149
> +bool ContentLayerChromium::largeLayer() const

This function name isn't very clear - it actually looks more like a getter for some large layer member variable.  Maybe something like requiresClippedUpdateRect() or something?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:155
> +void ContentLayerChromium::calculateLargeLayerViewRect(IntRect* dirtyRect,
> +                                                       IntRect* drawRect) const

nit: don't line wrap this

WebKit typically uses reference parameters for out params

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:164
> +    // 1) The minimal texture space rectangle to be uploaded, returned in
> +    // dirtyRect.
> +    // 2) The content rect-relative rectangle to draw this texture in, returned
> +    // in drawRect.

nit: this is over-wrapped. There's not a firm column limit in WebKit but this would be more readable if (1) and (2) were each only on one line.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:175
> +    const IntSize layerPos(transform.m41() - m_bounds.width() / 2,
> +                           transform.m42() - m_bounds.height() / 2);
> +    // Transform the contentRect into the space of the layer.
> +    IntRect contentRectInLayerSpace(IntPoint(0, 0) - layerPos,
> +                                    contentRect.size());

I don't fully grok the math here, but it seems like the negation in contentRectInLayerSpace's initialization could be done in the layerPos calculation and that layerPos is really an IntPoint and not an IntSize.  Or in other words something like:

const IntPoint layerPos(m_bounds.width() - transform.m41() / 2, m_bounds.height() - transform.m42() / 2);
IntRect contentRectInLayerSpace(layerPos, contentRect.size());

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:375
> +        transform.setM41(m_largeLayerDrawRect.center().x());
> +        transform.setM42(m_largeLayerDrawRect.center().y());

is this just doing a translate?  If so please use the appropriate translate() function on TransformationMatrix to be clearer

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:95
> +    bool largeLayer() const;
> +    void calculateLargeLayerViewRect(IntRect* dirtyRect,
> +                                     IntRect* drawRect) const;
>  
>      unsigned m_contentsTexture;
>      IntSize m_allocatedTextureSize;
>      bool m_skipsDraw;
> -
> +    IntRect m_largeLayerDrawRect;
> +    IntRect m_largeLayerDirtyRect;

It looks like the newly added functions/members should all be private: except for largeLayer()

[Adrienne Walker]

Created attachment 70407 [details]
Patch

[Adrienne Walker]

(In reply to comment #22)
> (From update of attachment 70201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70201&action=review
> 
> Needs a touch more cleanup.

Thanks for the review.  I appreciate all the nits, as I'm still learning WebKit style.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> > +    , m_allocatedTextureSize(0, 0)
> > +    , m_skipsDraw(false)
> > +    , m_largeLayerDrawRect(0, 0, 0, 0)
> > +    , m_largeLayerDirtyRect(0, 0, 0, 0)
> 
> The default constructor should do the right thing for all of these except for m_skipsDraw.  No need to list them out.

The default constructor is "IntRect() {}" so far as I can tell, so I've left these initializations.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:149
> > +bool ContentLayerChromium::largeLayer() const
> 
> This function name isn't very clear - it actually looks more like a getter for some large layer member variable.  Maybe something like requiresClippedUpdateRect() or something?

Good suggestion.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:155
> > +void ContentLayerChromium::calculateLargeLayerViewRect(IntRect* dirtyRect,
> > +                                                       IntRect* drawRect) const
> 
> nit: don't line wrap this
> 
> WebKit typically uses reference parameters for out params

Done.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:164
> > +    // 1) The minimal texture space rectangle to be uploaded, returned in
> > +    // dirtyRect.
> > +    // 2) The content rect-relative rectangle to draw this texture in, returned
> > +    // in drawRect.
> 
> nit: this is over-wrapped. There's not a firm column limit in WebKit but this would be more readable if (1) and (2) were each only on one line.

Done.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:175
> > +    const IntSize layerPos(transform.m41() - m_bounds.width() / 2,
> > +                           transform.m42() - m_bounds.height() / 2);
> > +    // Transform the contentRect into the space of the layer.
> > +    IntRect contentRectInLayerSpace(IntPoint(0, 0) - layerPos,
> > +                                    contentRect.size());
> 
> I don't fully grok the math here, but it seems like the negation in contentRectInLayerSpace's initialization could be done in the layerPos calculation and that layerPos is really an IntPoint and not an IntSize.

It's really more of a relative position than an absolute one, which is why I used IntSize originally.  I've changed it as you suggested.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:375
> > +        transform.setM41(m_largeLayerDrawRect.center().x());
> > +        transform.setM42(m_largeLayerDrawRect.center().y());
> 
> is this just doing a translate?  If so please use the appropriate translate() function on TransformationMatrix to be clearer

It's a translate, but preserving the z-coordinate.  I've modified this to call translate3d now.
 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.h:95
> > +    bool largeLayer() const;
> > +    void calculateLargeLayerViewRect(IntRect* dirtyRect,
> > +                                     IntRect* drawRect) const;
> >  
> >      unsigned m_contentsTexture;
> >      IntSize m_allocatedTextureSize;
> >      bool m_skipsDraw;
> > -
> > +    IntRect m_largeLayerDrawRect;
> > +    IntRect m_largeLayerDirtyRect;
> 
> It looks like the newly added functions/members should all be private: except for largeLayer()

Good catch.  I had originally intended to use them in ImageLayerChromium, but I found a simpler solution that didn't require that.

[James Robinson]

> 
> > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> > > +    , m_allocatedTextureSize(0, 0)
> > > +    , m_skipsDraw(false)
> > > +    , m_largeLayerDrawRect(0, 0, 0, 0)
> > > +    , m_largeLayerDirtyRect(0, 0, 0, 0)
> > 
> > The default constructor should do the right thing for all of these except for m_skipsDraw.  No need to list them out.
> 
> The default constructor is "IntRect() {}" so far as I can tell, so I've left these initializations.

True, but IntRect has two members of type IntPoint and IntSize, both of which have default constructors that zero-initialize.  It's one of the nice properties of the WebCore geometric primitives.

[James Robinson]

Comment on attachment 70407 [details]
Patch

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

R=me

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> +    , m_allocatedTextureSize(0, 0)
> +    , m_skipsDraw(false)
> +    , m_largeLayerDrawRect(0, 0, 0, 0)
> +    , m_largeLayerDirtyRect(0, 0, 0, 0)

nit: IntSize and IntRect zero-initialize by default.

[Adrienne Walker]

Created attachment 70535 [details]
Patch

[Adrienne Walker]

(In reply to comment #26)
> (From update of attachment 70407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70407&action=review
> 
> R=me
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> > +    , m_allocatedTextureSize(0, 0)
> > +    , m_skipsDraw(false)
> > +    , m_largeLayerDrawRect(0, 0, 0, 0)
> > +    , m_largeLayerDirtyRect(0, 0, 0, 0)
> 
> nit: IntSize and IntRect zero-initialize by default.

Oh, you're quite right.

Since I'm not a committer yet, I uploaded another patch with that change.

[WebKit Commit Bot]

Comment on attachment 70535 [details]
Patch

Rejecting patch 70535 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
k
All tests successful.
Files=14, Tests=304,  1 wallclock secs ( 0.72 cusr +  0.17 csys =  0.89 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21536 test cases.
compositing/tiling/huge-layer-img.html -> failed

Exiting early after 1 failures. 984 tests run.
38.60s total testing time

983 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://queues.webkit.org/results/4370024

[Adrienne Walker]

Created attachment 70669 [details]
Patch

[Adrienne Walker]

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

As it turns out, the compositing directory isn't run at all on chromium and I missed that fact while grepping through test expectations.  So, no need for platform-specific expectations at this point.  The new -expected.txt file is from mac and it passes there.

[James Robinson]

Comment on attachment 70669 [details]
Patch

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

Still looks good, just need some expectations tweaks.

> LayoutTests/compositing/tiling/huge-layer-img-expected.txt:1
> +The yellow box should be large enough to scroll off the bottom. There should be a red box on the first page and a blue box near the bottom of the yellow box. This tests that we can support very large composited image layers.

If this is a mac-specific expectation and other platforms produce a different result, then it really should go into platform/mac

If we have the chromium results why not check them in as well?  We're working on enabling those on the bots and will have to check in expectations at some point one way or the other.

[Adrienne Walker]

Created attachment 70688 [details]
Patch

[James Robinson]

Comment on attachment 70688 [details]
Patch

Cool. Could you file a bug about the layer sizes/offsets being slightly different?

[Adrienne Walker]

(In reply to comment #34)
> (From update of attachment 70688 [details])
> Cool. Could you file a bug about the layer sizes/offsets being slightly different?

https://bugs.webkit.org/show_bug.cgi?id=47638

[WebKit Commit Bot]

Comment on attachment 70688 [details]
Patch

Clearing flags on attachment: 70688

Committed r69747: <http://trac.webkit.org/changeset/69747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.