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."
Created attachment 69516 [details] Patch
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.
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).
(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.
Created attachment 69865 [details] Patch
(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.
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()?
Whoops, I totally misinterpreted the purpose of the image. My bad!
Comment on attachment 69865 [details] Patch Resetting enne's flags
(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.
Created attachment 69957 [details] Patch
(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.
Created attachment 70097 [details] Patch
(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?
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)
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.
(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?
Created attachment 70173 [details] Patch
(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).
Created attachment 70201 [details] Patch
(In reply to comment #20) > Created an attachment (id=70201) [details] > Patch Rebaseling against ToT.
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()
Created attachment 70407 [details] Patch
(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.
> > > > 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.
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.
Created attachment 70535 [details] Patch
(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.
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
Created attachment 70669 [details] Patch
(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.
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.
Created attachment 70688 [details] Patch
Comment on attachment 70688 [details] Patch Cool. Could you file a bug about the layer sizes/offsets being slightly different?
(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
Comment on attachment 70688 [details] Patch Clearing flags on attachment: 70688 Committed r69747: <http://trac.webkit.org/changeset/69747>
All reviewed patches have been landed. Closing bug.