The LayerRendererChromium compositor currently scrolls by copying the root layer. This is quite slow. Rather than copying or redrawing when scrolling, the root layer should be drawn into tiles instead which don't have to necessarily be redrawn when the page scrolls.
Created attachment 74617 [details] Patch
I'm looking to get an initial review on this code. It still needs more performance analysis, but it is doing ok (in my opinion) on the correctness end of things. In terms of performance, when a tile is dirty, this code redraws the entire tile. It likely makes more sense to allocate a canvas per paint call that's the size of the dirty rect, but I need to do some performance analysis to test this. I also need to do some performance analysis on the optimal tile size. 256x256 was arbitrary. In terms of correctness, this code passes all the compositor tests on Linux and OS X. However it appears to introduce/unearth a Skia-only bug with incorrect filtering on images/gradients along tile boundaries when the page is scaled.
Please check the performance carefully for populating the backing store in 256x256 chunks vs all at once. In particular I'm a little worried that populating a new row of tiles on a page like gmail as a series of narrow paints instead of one full-width paint might be a lot slower. If so, we might have to do some tricks if we have a lot of adjacent invalidated tiles to update at once (maybe paint into a contiguous back buffer and use stride tricks to upload into tiles). Measure first, though :)
Same issue as grace on the providing feedback. Sigh. Lost a ton of comments. Broadly: - Page space vs Content space; Screen space vs Frame Space. Whats the right term? We use all four. - For functions that take coordinates, would it help to name the parameters to indicate what space they're in? - What's the difference between the tiler's grow and resize, or rather why are they both exposed? - Should tiles know about their position? I notice tiles track dirty coordinates in content (page?) space, but then when we go to draw using them, we construct their position using the x and y loop variables.
Comment on attachment 74617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74617&action=review First of all I should say that this is really nicely done! I have a couple of inline comments but in general I do agree with the other reviewers that the naming (especially page and layer) is confusing. I would also recommend to reserve using *Rect as part of a variable name only if you're describing a pixel rectangle and use a different suffix for the index rectangles used to group index tiles. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:177 > It seems to me that this method could be eliminated completely. The parts that make the GL calls should just be moved to drawLayers > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:255 > + ASSERT(m_verticalScrollbarTiler); I think there's an assumption that drawLayers gets called after updateRootLayerContents with the same visibleRect and contentRect. How about caching the scrollbarRect's in updateRootLayerContents and just issue their draw calls here? In a way it almost seems that drawLayers should just call updateAndDrawRootLayer() which is essentially updateRootLayerContents() + the three tiler->draws() and then go and draw the composited layers. > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:247 > + tile->clearDirty(); Any reason why you're not using the tile's dirty rect instead of redrawing the entire tile? > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:316 > +{ I thought that up until here you were using the term "page" to mean the container the tiled layer lives in, which allows you to convert from page space to layer space. If that's true then I think what you're resizing here is the layer size. If I'm not completely lost, m_pageSize should be called m_layerSize . > WebKit/chromium/src/WebViewImpl.cpp:2342 > + : m_webViewImpl(webViewImpl) I believe you need a 4-space indent here.
(In reply to comment #3) > Please check the performance carefully for populating the backing store in 256x256 chunks vs all at once. In particular I'm a little worried that populating a new row of tiles on a page like gmail as a series of narrow paints instead of one full-width paint might be a lot slower. If so, we might have to do some tricks if we have a lot of adjacent invalidated tiles to update at once (maybe paint into a contiguous back buffer and use stride tricks to upload into tiles). Measure first, though :) This is an excellent point. I did some performance measurements of the CPU-side costs of filling tiles and my conclusion at this point is to minimize calls to paint, as you suggest. I did a number of tests of different tile sizes (64, 128, 256, 512, 1024) and different types of invalidation (full page, small selections) on three different web pages of different complexity (http://webkit.org/blog/386/3d-transforms, Gmail, a spreadsheet from jamesr). The only test case where painting into multiple tiles was the strictly better strategy was full-page invalidations of the WebKit blog at some tile sizes, and even then it was 16 ms vs. 18 ms. So, I'll revise the patch to minimize paints. Also, based on these timings, 256x256 and 512x512 are tied for being the best tile size.
Created attachment 75544 [details] Patch
(In reply to comment #4) > Same issue as grace on the providing feedback. Sigh. Lost a ton of comments. > > Broadly: > - Page space vs Content space; Screen space vs Frame Space. Whats the right term? We use all four. I've renamed all "page space" references to "content space" and have tried to differentiate between "layer space" and "content space" where needed. > - For functions that take coordinates, would it help to name the parameters to indicate what space they're in? This should be done, at least in the new code that's been added. > - What's the difference between the tiler's grow and resize, or rather why are they both exposed? grow was intended to grow the layer to contain a rectangle. resize was to resize the layer to an exact size. I've renamed these for clarity. > - Should tiles know about their position? I notice tiles track dirty coordinates in content (page?) space, but then when we go to draw using them, we construct their position using the x and y loop variables. I think it's easiest for tiles to just have their position constructed implicitly rather than repeating that information in a separate variable.
(In reply to comment #5) > (From update of attachment 74617 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74617&action=review > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:177 > > > > It seems to me that this method could be eliminated completely. The parts that make the GL calls should just be moved to drawLayers Done. I simplified the API boundary between WebViewImpl and LayerRendererChromium so that there's only one function call to draw all the layers. > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:255 > > + ASSERT(m_verticalScrollbarTiler); > > I think there's an assumption that drawLayers gets called after updateRootLayerContents with the same visibleRect and contentRect. How about caching the scrollbarRect's in updateRootLayerContents and just issue their draw calls here? In a way it almost seems that drawLayers should just call updateAndDrawRootLayer() which is essentially updateRootLayerContents() + the three tiler->draws() and then go and draw the composited layers. This should be much cleaner with the above change as well. > > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:247 > > + tile->clearDirty(); > > Any reason why you're not using the tile's dirty rect instead of redrawing the entire tile? It was a design decision that I hadn't nailed down yet. After the performance analysis and the need to do row-by-row memcpy, I've modified this patch to use the dirty rect instead of the entire tile. > > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:316 > > +{ > > I thought that up until here you were using the term "page" to mean the container the tiled layer lives in, which allows you to convert from page space to layer space. If that's true then I think what you're resizing here is the layer size. If I'm not completely lost, m_pageSize should be called m_layerSize . You're quite right. I've tried to clean up the language in the latest patch. Please let me know if I could be clearer. I think I was just frustrated with the surrounding code doing a very poor job of specifying what rects were in which space, so I tried to make sure that this code was more clear. > > WebKit/chromium/src/WebViewImpl.cpp:2342 > > + : m_webViewImpl(webViewImpl) > > I believe you need a 4-space indent here. Done.
Ack. I neglected to incorporate Grace's feedback because it was in a different place. I'll do that right now and submit another patch.
Created attachment 75546 [details] Patch
Comment on attachment 75546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75546&action=review I didn't have time to give this a full review yet, but I left a bunch of nitpicks. I'm a bit confused by LayerTilerChromium's mixture of values and rectangles that represent pixel regions vs values and rectangles that represent tile indices. > WebCore/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). > + > + Refactor root layer update and drawing from LayerRendererChromium into > + LayerTilerChromium. The root layer is now drawn as multiple tiles > + rather than as one single large texture. Scrollbars are now drawn > + separately rather than as part of the root layer. > + > + https://bugs.webkit.org/show_bug.cgi?id=49947 > + Test: LayoutTests/compositing/ nit: preferred format is: 1-line description (normally same as the bug title) bug URL long form description of patch Test: blah blah > WebCore/platform/graphics/chromium/LayerTilerChromium.h:57 > + // Change the tile size. This may invalidate all the existing tiles. > + void setTileSize(const IntSize& size); Should this be public API? It seems a little cleaner if the tile size is a construction-time invariant. > WebCore/platform/graphics/chromium/LayerTilerChromium.h:66 > + Tile(unsigned int textureId) : m_textureId(textureId) { } explicit > WebCore/platform/graphics/chromium/LayerTilerChromium.h:88 > + void invalidateTiles(const IntRect& considerGroup, const IntRect& keepGroup); I tried but I can't really figure out what you mean by 'considerGroup' and 'keepGroup' here. Are these rectangles in some coordinate space or ranges of tiles? If the latter, I feel like this shouldn't be using the IntRect type since that normally implies a geometric meaning. > WebCore/platform/graphics/chromium/LayerTilerChromium.h:92 > + int tileIndex(int x, int y); The API here makes me think that x and y are offsets in some logical space but the implementation makes me think they are indices into a two-dimensional array. Which are they? I think if the latter i and j may make more sense as parameter names. Same goes for other functions here that take x, y > WebCore/platform/graphics/chromium/LayerTilerChromium.h:108 > + // Cache a tile-sized pixel buffer to draw into. > + Vector<uint8_t> m_tilePixels; OwnArrayPtr<> might be a better match here - we don't need anything out of Vector except the ability to allocate an array. > WebKit/chromium/src/WebViewImpl.cpp:2315 > + IntRect dirtyRect = view->windowToContents(rect); > + IntRect dirtyRect2(IntPoint(rect.x() + view->scrollX(), rect.y() + view->scrollY()), rect.size()); > + ASSERT(dirtyRect == dirtyRect2); > + m_layerRenderer->invalidateRootLayerRect(dirtyRect, visibleRect, contentRect); This looks like it wasn't intended to be part of this patch
(In reply to comment #12) > (From update of attachment 75546 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75546&action=review > > I didn't have time to give this a full review yet, but I left a bunch of nitpicks. I'm a bit confused by LayerTilerChromium's mixture of values and rectangles that represent pixel regions vs values and rectangles that represent tile indices. There are a number of places (invalidation, tile update, and tile drawing) that require iterating over a set of tiles. I was using IntRect because it seemed like a reasonable way to group rectangles of tile indices. However, given your feedback here and Vangelis's similar comments before, maybe I should just make some custom iterator class to handle this same behavior, unless you have some other suggestion. > > WebCore/platform/graphics/chromium/LayerTilerChromium.h:57 > > + // Change the tile size. This may invalidate all the existing tiles. > > + void setTileSize(const IntSize& size); > > Should this be public API? It seems a little cleaner if the tile size is a construction-time invariant. In this patch, when the content rect is resized, scrollbars are also resized to use the minimal amount of tiles possible. This usually ends up being just a single tile per scrollbar, unless your window size is over the max texture size. Maybe this is an unneeded optimization. > > WebCore/platform/graphics/chromium/LayerTilerChromium.h:92 > > + int tileIndex(int x, int y); > > The API here makes me think that x and y are offsets in some logical space but the implementation makes me think they are indices into a two-dimensional array. Which are they? I think if the latter i and j may make more sense as parameter names. Same goes for other functions here that take x, y That's a good point--the intention here could be confused. They are supposed to be indices, as you correctly guess. > This looks like it wasn't intended to be part of this patch Oops, that was unintentional.
Created attachment 75738 [details] Patch
(In reply to comment #14) > Created an attachment (id=75738) [details] > Patch Cleaned up the style nitpicks and as well as removed all use of IntRect to group tile indices.
Comment on attachment 75738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75738&action=review Overall this looks good to me. One minor issue needs to be fixed though to avoid introducing a regression. > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:279 > + kCGImageAlphaPremultipliedLast)); This needs to be kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host per a recent bug fix Nico did. Also, indentation is off here. > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:341 > + // Render the root layer using a quad that takes up the entire visible area of the window. This comment needs to be updated.
Comment on attachment 75738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75738&action=review Looks good on my end (except for a couple of inline comments). I like the name cleanup! > WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:349 > + GLC(context, context->colorMask(true, true, true, false)); Since the layer tiler is supposed to be more general purpose and eventually used by other layers as well, alpha masking should be done in the LayerRendererChromium and not here. > WebKit/chromium/src/WebViewImpl.cpp:2325 > + if (page()) this should probably move to after the point where we know for sure whether we're doing accelerated compositing or not. If we fail to create the GL context or the LayerRendererChromium we won't be doing accelerated compositing. > WebKit/chromium/src/WebViewImpl.cpp:2364 > +class WebViewImplTilePaintInterface : public TilePaintInterface { It is somewhat unusual to define classes in a .cpp file. A WebKit reviewer can confirm whether it's ok or not.
(In reply to comment #17) > > WebKit/chromium/src/WebViewImpl.cpp:2364 > > +class WebViewImplTilePaintInterface : public TilePaintInterface { > > It is somewhat unusual to define classes in a .cpp file. A WebKit reviewer can confirm whether it's ok or not. Personally I think it's fine. The rule I'm aware of is not declaring multiple classes in a .h.
Comment on attachment 75738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75738&action=review > WebCore/platform/ScrollView.h:326 > bool m_paintsEntireContents; > + bool m_clipsRepaints; Why do we need both of these? What's the difference? >> WebKit/chromium/src/WebViewImpl.cpp:2364 >> +class WebViewImplTilePaintInterface : public TilePaintInterface { > > It is somewhat unusual to define classes in a .cpp file. A WebKit reviewer can confirm whether it's ok or not. I think defining this class in the .cpp if it's only used in this .cpp is fine.
Comment on attachment 75738 [details] Patch Clearing from the review queue. I think the patch is pretty close but there are comments here that need to be addressed before this is 100% ready.
(In reply to comment #19) > (From update of attachment 75738 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75738&action=review > > > WebCore/platform/ScrollView.h:326 > > bool m_paintsEntireContents; > > + bool m_clipsRepaints; > > Why do we need both of these? What's the difference? It's kind of frustrating to need both. (I'm CCing kenneth@webkit.org who added this.)
(In reply to comment #21) > (In reply to comment #19) > > (From update of attachment 75738 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=75738&action=review > > > > > WebCore/platform/ScrollView.h:326 > > > bool m_paintsEntireContents; > > > + bool m_clipsRepaints; > > > > Why do we need both of these? What's the difference? > > It's kind of frustrating to need both. (I'm CCing kenneth@webkit.org (:kenne) (r) who added this.) paintsEntireContents doesn't clip repaints but it also sets the content rect to be the entire contents of the page. In other words, paintsEntireContents changes the way the page is drawn. clipsRepaints only does the first part of that, allowing offscreen invalidations. Because we still want to keep the content rect and the scrollbars where they are, I added yet another boolean. It's not ideal, but it seemed like the best approach.
Created attachment 77043 [details] Patch
(In reply to comment #23) > Created an attachment (id=77043) [details] > Patch This patch addresses the two comments by Vangelis and the two comments by Ken. The clipsRepaint logic is still there, because I think clipsRepaint is trying to do a subset of what paintsEntireContents is trying to do. Additionally, this patch adds LayerTilerChromium::invalidateEntireLayer which gets called when the layer gets set on LayerRendererChromium. This fixes an issue with offscreen tiles needing to be invalidated when switching from one tab to another.
Created attachment 77051 [details] Patch
(In reply to comment #25) > Created an attachment (id=77051) [details] > Patch Rebaselining patch due to merge conflict.
Comment on attachment 77051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77051&action=review Leaving some nits. I'm still not super duper sure about the paintsEntireContents/clipRects logic - why do we want to receive invalidations for offscreen rectangles if we aren't changing the visible region? It seems like we either only want to paint the visible region (and thus only want invalidations for the visible region), or we are painting outside what the ScrollView thinks is visible and thus need invalidations outside what the ScrollView thinks is visible. I'm not sure how we can possibly have it both ways. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:126 > + m_rootLayerTiler = 0; > + m_horizontalScrollbarTiler = 0; > + m_verticalScrollbarTiler = 0; nit: I think .clear() is a more explicit way of doing what you want to do here. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:183 > + // Mask out writes to alpha channel: ClearType via Skia results in invalid Nit: subpixel antialiasing, not ClearType
(In reply to comment #27) > (From update of attachment 77051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77051&action=review > > Leaving some nits. Thanks for pointing those out. > I'm still not super duper sure about the paintsEntireContents/clipRects logic - why do we want to receive invalidations for offscreen rectangles if we aren't changing the visible region? It seems like we either only want to paint the visible region (and thus only want invalidations for the visible region), or we are painting outside what the ScrollView thinks is visible and thus need invalidations outside what the ScrollView thinks is visible. I'm not sure how we can possibly have it both ways. Are you not sure about why we need both paintsEntireContents and clipsRepaints, why we need either, or the interaction between them? This patch was intended to be minimally intrusive to the paintsEntireContents logic. If that flag is true, then this patch should be a no-op. I agree with why your confusion about why you need to clip repaints when you set your visible rect to the entire page, but that yak shaving should be left for the port using paintsEntireContents and not this patch. I was hoping that Kenneth Christianson could chime in here. Ignoring paintsEntireContents, Chromium's tiled rendering paints offscreen of what ScrollView thinks is visible.
Seems we are gettings lots of tile implementations in WebKit :-) One in WebCore, one for WebKit2, one for EFL and now one for Chromium. > Are you not sure about why we need both paintsEntireContents and clipsRepaints, why we need either, or the interaction between them? > > This patch was intended to be minimally intrusive to the paintsEntireContents logic. If that flag is true, then this patch should be a no-op. I agree with why your confusion about why you need to clip repaints when you set your visible rect to the entire page, but that yak shaving should be left for the port using paintsEntireContents and not this patch. I was hoping that Kenneth Christianson could chime in here. > > Ignoring paintsEntireContents, Chromium's tiled rendering paints offscreen of what ScrollView thinks is visible. Could you explain me why you cannot use the paintsEntireContents? It basically means "get updates even outside viewport" which sounds pretty much like !clipRepaints. Where is it that paintsEntireContents does something you do not need, or where you want to modify it? > paintsEntireContents doesn't clip repaints but it also sets the content rect to be the entire contents of the page. In other words, paintsEntireContents changes the way the page is drawn. I do not really think it needs to do that, or does that, because we layout given the viewport meta tag, so we paint fixed elements relative to that etc.
(In reply to comment #29) > Could you explain me why you cannot use the paintsEntireContents? It basically means "get updates even outside viewport" which sounds pretty much like !clipRepaints. > > Where is it that paintsEntireContents does something you do not need, or where you want to modify it? In particular, ScrollView::visibleContentRect returns IntRect(IntPoint(0, 0), contentsSize()) if paintsEntireContents() is true. This return value ignores both the scroll offset and scrollbars. Alternatively, I could duplicate the visible region calculation from visibleContentRect() in WebViewImpl.cpp, but clipsRepaints seemed like a better solution than that. > > paintsEntireContents doesn't clip repaints but it also sets the content rect to be the entire contents of the page. In other words, paintsEntireContents changes the way the page is drawn. > > I do not really think it needs to do that, or does that, because we layout given the viewport meta tag, so we paint fixed elements relative to that etc. I'm guessing you also aren't drawing scrollbars?
Created attachment 77231 [details] Patch
Comment on attachment 77231 [details] Patch This patch addresses the rest of jamesr's nitpicks, other than the clipsRepaints vs. paintsEntireContents issue. I maintain that adding an additional flag is the simplest option among the alternatives. At this point, I would prefer to have this patch submitted and we can clean that up in a follow-on patch, if necessary.
(In reply to comment #32) > (From update of attachment 77231 [details]) > This patch addresses the rest of jamesr's nitpicks, other than the clipsRepaints vs. paintsEntireContents issue. > > I maintain that adding an additional flag is the simplest option among the alternatives. At this point, I would prefer to have this patch submitted and we can clean that up in a follow-on patch, if necessary. I agree with Enne here. paintsEntireContents() controls how much of the page is actually painted whereas clipsRepaints() controls whether invalidates get clipped or not. When tiling, it makes sense that we want to get invalidates that are outside the renderable rect so that we can update off-screen tile content accordingly.
Comment on attachment 77231 [details] Patch The revised patch looks good to me.
Committed r74568: <http://trac.webkit.org/changeset/74568>
http://trac.webkit.org/changeset/74568 might have broken GTK Linux 64-bit Debug