[chromium] Tile content and image layers
Created attachment 86975 [details] Patch
Comment on attachment 86975 [details] Patch This doesn't need a formal review, because I'm going to wait for bug 56793 to land first. I'll need to do some minor merging. I would appreciate any general feedback, though. :)
Hmm, so masks can't be tiled? What happens when somebody sets a giant layer as a mask layer (I sure hope they don't)? Oh btw I gave you a ton of merge conflicts with http://trac.webkit.org/changeset/82006 - hopefully most are insignificant. Ask me about anything that looks tricky!
(In reply to comment #3) > Hmm, so masks can't be tiled? What happens when somebody sets a giant layer as a mask layer (I sure hope they don't)? It's also that render surfaces can't be tiled either. I wanted to put off solving the render surface issue, because it uses two textures to draw (the drawn surface and an opacity mask). Then the question is what do you do if one is tiled and the other isn't (or do we always just tile both?). I'm not totally convinced that render surfaces need tiling. I mean, it'd be nice, but it doesn't seem at all as high of a priority as content and image layers. I can file a feature bug if this is needed. In terms of what happens, if a render surface is too big, it fails the texture reserve step and the entire render surface layer is skipped. I'll add some code to the "drawsContent()" in case the mask texture is too big and the tiler is marked as skipped. > Oh btw I gave you a ton of merge conflicts with http://trac.webkit.org/changeset/82006 - hopefully most are insignificant. Ask me about anything that looks tricky! My kingdom for a C++ virtual override keyword. :(
Created attachment 87643 [details] Patch
I see that this patch didn't apply cleanly and will reupload a new one. However, more importantly, it looks like I might need to apply some of this patch to fix http://crbug.com/76001 (namely the split of the paint/upload steps in the tiler code). I would still appreciate feedback on this patch, as I don't have any more changes intended for it.
Comment on attachment 87643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87643&action=review Looks good overall and I like how the tiler gets used by both root and content layers. I have some concerns about what happens with really large layers if they render into a render surface (not convinced that we won't be creating tiles for the entire layer) and also whether the texture manager will play well with the tiler. > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:88 > +void ContentLayerChromium::paintContentsIfDirty(const IntRect& contentRect) contentRect -> targetSurfaceRect ? > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:128 > +IntRect ContentLayerChromium::enclosingRectInLayerSpace(const IntRect& rect) I find some of the variable names in this method misleading or not descriptive enough. As far as I can tell, the rect that gets passed as a parameter is the region in the target surface that layer is allowed to draw in. So maybe the parameter should be renamed from rect to targetSurfaceRect? The whole function could probably use a different name too: paintableLayerRect()? meh... > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:137 > + IntRect layerInPageSpace = transform.mapRect(boundsRect); The term Page is misleading here. Some layers render in RenderSurfaces and their transforms are expressed in the render surface coordinate system, not the page's. Maybe PageSpace -> targetSurfaceSpace ? > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:146 > + IntRect viewportInLayerSpace = inverse.mapRect(rect); Again, the term Viewport is misleading. rect is the portion of the target render surface that the layer can draw into so here you're computing which portion of the layer this maps to. > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:179 > +void ContentLayerChromium::draw(const IntRect& contentRect) contentRect -> targetSurfaceRect ? > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:186 > + m_tiler->unreserveTextures(); If the textures get unreserved here what happens if a tile texture gets recycled but the tile doesn't get dirtied? When are we going to recreate the tile texture? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:914 > + layer->draw(layer->scissorRect()); The layer's scissor rect isn't particularly tight and isn't limited to only the visible portions of the viewport. Does this mean that really large layers will be fully tiled and populated?
Thanks for the review and all the good suggestions. :) (In reply to comment #7) > (From update of attachment 87643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87643&action=review > > Looks good overall and I like how the tiler gets used by both root and content layers. I have some concerns about what happens with really large layers if they render into a render surface (not convinced that we won't be creating tiles for the entire layer) and also whether the texture manager will play well with the tiler. I'll double check what happens with large layers in a render surface and see if I need to add a test there. I think the texture manager should play fine with the tiler. Right now, the tiler recycles tiles and tries to reuse them before creating new ones. That should minimize texture recreation. Additionally, if the texture manager ever fails to create a texture because of memory limitations, the tiler gets skipped, so it should be safe. > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:88 > > +void ContentLayerChromium::paintContentsIfDirty(const IntRect& contentRect) > > contentRect -> targetSurfaceRect ? I will do that here and in other places. > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:128 > > +IntRect ContentLayerChromium::enclosingRectInLayerSpace(const IntRect& rect) > > I find some of the variable names in this method misleading or not descriptive enough. As far as I can tell, the rect that gets passed as a parameter is the region in the target surface that layer is allowed to draw in. So maybe the parameter should be renamed from rect to targetSurfaceRect? The whole function could probably use a different name too: paintableLayerRect()? meh... You're correct about what this function is trying to do. I'll clean up the naming and try to make it more clear. > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:137 > > + IntRect layerInPageSpace = transform.mapRect(boundsRect); > > The term Page is misleading here. Some layers render in RenderSurfaces and their transforms are expressed in the render surface coordinate system, not the page's. Maybe PageSpace -> targetSurfaceSpace ? Yeah, that's much more clear. > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:186 > > + m_tiler->unreserveTextures(); > > If the textures get unreserved here what happens if a tile texture gets recycled but the tile doesn't get dirtied? When are we going to recreate the tile texture? If a texture gets recycled by the texture manager, the LayerTilerChromium::update function will catch it. When unifying all the dirty rects, it checks if a tile texture is valid and marks it as dirty if it's not. This is also where we check if a tile hasn't been created yet as well. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:914 > > + layer->draw(layer->scissorRect()); > > The layer's scissor rect isn't particularly tight and isn't limited to only the visible portions of the viewport. Does this mean that really large layers will be fully tiled and populated? I'll look into this and make sure I'm passing the correct bound down to the paint and draw functions.
In looking at the code for which rect to use for drawing layers, I am pretty sure the scissor rect is the correct rect to be using. For layers rendering into the default render surface, the scissor rect is the viewport. For layers rendering into some other target render surface, the scissor rect is set to be the drawable content rect of that target render surface. As render surfaces themselves are not tiled as a part of this patch, if there's a render surface that is really large, then it will get skipped. If there's a very large content layer that's rendering into a render surface, then it will only fill in enough tiles to fill the render surface. The worst case here is a render surface that is very large (but not over the max texture size) and is partially clipped by the main viewport. In that case, we'd render more of the layers in the render surface than we needed to. However, this is no worse than the situation before this patch. I agree that it could be better, but I'd like to land this patch and not hold up James and Nat anymore and I can file a bug to improve this situation.
Created attachment 87898 [details] Patch
(In reply to comment #10) > Created an attachment (id=87898) [details] > Patch Wait, why do I get a "patch: **** Only garbage was found in the patch input." error?
Comment on attachment 87898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87898&action=review > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:161 > + const IntSize tileSize(defaultTileSize, defaultTileSize); if a layer is particularly tall+narrow or wide+short this seems inefficient (i'm thinking about scrollbars in particular which will often be around 15x1000). can this be min(defaultTileSize, layerSize) in each dimension? whenever we resize a layer we'll pretty much always have to repaint the whole thing > Source/WebCore/platform/graphics/chromium/LayerChromium.h:116 > + virtual void invalidateRect(const FloatRect& dirtyRect) {}; no ; > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:244 > - if (m_horizontalScrollbarTiler) > - m_horizontalScrollbarTiler->draw(m_viewportVisibleRect); > + if (m_verticalScrollbarTiler) { > + m_verticalScrollbarTiler->uploadCanvas(); > + m_verticalScrollbarTiler->draw(m_viewportVisibleRect, scroll, 1.0f); > + m_verticalScrollbarTiler->unreserveTextures(); > + } > + > + if (m_horizontalScrollbarTiler) { > + m_horizontalScrollbarTiler->uploadCanvas(); > + m_horizontalScrollbarTiler->draw(m_viewportVisibleRect, scroll, 1.0f); > + m_horizontalScrollbarTiler->unreserveTextures(); > + } all this will soon be gone (meaning the patches will conflict, ah well!) > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:363 > + GLC(m_context.get(), m_context->blendFunc(GraphicsContext3D::ONE, GraphicsContext3D::ONE_MINUS_SRC_ALPHA)); why's this change in here? at first glance i can't tell why it's related
Comment on attachment 87898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87898&action=review > Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:81 > + explicit Painter(PlatformCanvas*, bool drawingToImageBuffer); no bool prz, callsites are opaque now. enum { GreyscaleText, SubpixelText } maybe? also with 2 arguments this constructor doesn't need "explicit" any more
(In reply to comment #12) > (From update of attachment 87898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87898&action=review > > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:161 > > + const IntSize tileSize(defaultTileSize, defaultTileSize); > > if a layer is particularly tall+narrow or wide+short this seems inefficient (i'm thinking about scrollbars in particular which will often be around 15x1000). can this be min(defaultTileSize, layerSize) in each dimension? whenever we resize a layer we'll pretty much always have to repaint the whole thing Excellent point. Scrollbars are doing this right now, so content layers should be changed to behave similarly so that scrollbar-like layers will continue to have that behavior. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:363 > > + GLC(m_context.get(), m_context->blendFunc(GraphicsContext3D::ONE, GraphicsContext3D::ONE_MINUS_SRC_ALPHA)); > > why's this change in here? at first glance i can't tell why it's related Perhaps this should go in another bug, but it doesn't appear to get set anywhere else. I ran into a bug where layers wouldn't draw the first time through the draw loop because the only place the blend func was getting set was in the HUD (which was incorrectly always on). So, I moved that call to a more reasonable place. I don't know how it ever worked before.
Comment on attachment 87898 [details] Patch Clearing from review queue. This patch looks pretty much good to go except for the issues already raised, as far as I can see.
Created attachment 88835 [details] Patch
Comment on attachment 88835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88835&action=review > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:-74 > - // FIXME: Remove this test when tiled layers are implemented. > - if (requiresClippedUpdateRect()) { > - // Use the base version of updateContents which draws a subset of the > - // image to a bitmap, as the pixel contents can't be uploaded directly. > - ContentLayerChromium::paintContentsIfDirty(); > - return; > - } > - does this mean huge images aren't tiled? > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:255 > + PlatformCanvas::Painter::TextOption textOption = m_tilingData.borderTexels() ? PlatformCanvas::Painter::GrayscaleText : PlatformCanvas::Painter::SubpixelText; might be worth a comment explaining why border texels determines whether the text is grayscale or subpixel AA. I can't actually think of why.
(In reply to comment #17) > does this mean huge images aren't tiled? No. The image layer code is now more unified with the content layer code. Huge images are tiled in the same way that a huge layer is tiled. If you think we should have the same old image layer behavior (use a single texture for images if we can), I could override the tile size setting in ImageLayerChromium. > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:255 > > + PlatformCanvas::Painter::TextOption textOption = m_tilingData.borderTexels() ? PlatformCanvas::Painter::GrayscaleText : PlatformCanvas::Painter::SubpixelText; > > > might be worth a comment explaining why border texels determines whether the text is grayscale or subpixel AA. I can't actually think of why. Done.
Created attachment 88872 [details] Patch
Comment on attachment 88872 [details] Patch Giving official R+. If/when Vangelis is happy, I'm happy :_}
Comment on attachment 88872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88872&action=review Happy here with the exception of one question. > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:139 > + IntRect layerInPageSpace = transform.mapRect(layerBoundRect); nit: layerInPageSpace -> layerInRenderSurfaceSpace > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:249 > if (dirtyLayerRect.isEmpty()) Shouldn't we be locking the tile textures in the loop above? What is it that prevents a tile's texture from getting recycled between this point and when you call uploadCanvas()? Am I missing something in the logic here?
(In reply to comment #21) > (From update of attachment 88872 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88872&action=review > > Happy here with the exception of one question. > > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:139 > > + IntRect layerInPageSpace = transform.mapRect(layerBoundRect); > > nit: layerInPageSpace -> layerInRenderSurfaceSpace Will do. > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:249 > > if (dirtyLayerRect.isEmpty()) > > Shouldn't we be locking the tile textures in the loop above? What is it that prevents a tile's texture from getting recycled between this point and when you call uploadCanvas()? Am I missing something in the logic here? Nice catch! I wonder if that's the issue behind http://crbug.com/64769.
Created attachment 89058 [details] Patch
Created attachment 89059 [details] Patch
(In reply to comment #24) > Created an attachment (id=89059) [details] > Patch Sorry to ditch the R+, but I made a few minor edits to texture reserving behavior in LayerTilerChromium.cpp that I wanted another eye on before committing. Textures now get reserved during painting if they're valid and then always reserved (if they weren't previously) during the upload pass. As before, all textures are unreserved via the unreserveTextures() call, usually done after painting. I also updated the variable name that Vangelis suggested.
Comment on attachment 89059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89059&action=review > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:246 > + tile->texture()->reserve(m_tileSize, GraphicsContext3D::RGBA); I think you need to call reserve regardless of whether isValid succeeds or fails. isValid only moves the texture to the top of the MRU list but doesn't actually reserve it.
(In reply to comment #26) > (From update of attachment 89059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89059&action=review > > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:246 > > + tile->texture()->reserve(m_tileSize, GraphicsContext3D::RGBA); > > I think you need to call reserve regardless of whether isValid succeeds or fails. isValid only moves the texture to the top of the MRU list but doesn't actually reserve it. If it's not valid, then there's no need to reserve it until later because we're going to redraw the entire tile. The code as it stands will reserve that texture just prior to upload.
Comment on attachment 89059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89059&action=review Unofficial R+ from me... >>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:246 >>> + tile->texture()->reserve(m_tileSize, GraphicsContext3D::RGBA); >> >> I think you need to call reserve regardless of whether isValid succeeds or fails. isValid only moves the texture to the top of the MRU list but doesn't actually reserve it. > > If it's not valid, then there's no need to reserve it until later because we're going to redraw the entire tile. The code as it stands will reserve that texture just prior to upload. Ah, yes, you're right!
Committed r83500: <http://trac.webkit.org/changeset/83500>
Reverted r83500 for reason: Regresses huge-layer-rotated test Committed r83511: <http://trac.webkit.org/changeset/83511>
Created attachment 89327 [details] Patch
Maybe the next R+ will stick. Changes from the last patch: * visibleLayerRect has been modified to be less wrong. I was using mapRect instead of projectQuad to find the projection of the target surface rect in the layer space. mapRect is a projection onto a z=0 plane. projectQuad is a projection onto an arbitrary plane, which is what this is trying to do. * LayerTilerChromium now has a minimum texture dimension of 16 pixels. That seemed reasonable to me, but I could make it as small as 4 if somebody feels otherwise. This fixes a crash from the 1x1 image layer test that Vangelis just added. * I also rebaselined a few other images (not included in this patch due to bug 35208).
(In reply to comment #32) > Maybe the next R+ will stick. Changes from the last patch: > > * LayerTilerChromium now has a minimum texture dimension of 16 pixels. That seemed reasonable to me, but I could make it as small as 4 if somebody feels otherwise. This fixes a crash from the 1x1 image layer test that Vangelis just added. Why do we need a minimum at all? 16 minimum is a little bit wasteful for scrollbars, and having a minimum is potentially wasteful for small layers people might use for whatnot.
(In reply to comment #33) > (In reply to comment #32) > > Maybe the next R+ will stick. Changes from the last patch: > > > > * LayerTilerChromium now has a minimum texture dimension of 16 pixels. That seemed reasonable to me, but I could make it as small as 4 if somebody feels otherwise. This fixes a crash from the 1x1 image layer test that Vangelis just added. > > Why do we need a minimum at all? 16 minimum is a little bit wasteful for scrollbars, and having a minimum is potentially wasteful for small layers people might use for whatnot. We need a minimum because of the resolution to bug 58364. If borders are requested and the tiling size is too small (1 or 2 pixels), then TilingData returns 0 tiles. You're right that 16 is wasteful. How's 4?
Comment on attachment 89327 [details] Patch Temporarily taking off my request for review. For whatever reason, this patch at ToT (but not previous ones at previous ToTs) now causes a bunch of egregious fast/canvas failures on Mac.
Comment on attachment 89327 [details] Patch False alarm. canvas tests now just fail when run in NRWT over ssh. This patch is fine.
Created attachment 89500 [details] Patch
(In reply to comment #37) > Created an attachment (id=89500) [details] > Patch After some discussion, same as before but now with size 4 minimum texture size. I can follow up with more cleanup later if needed.
Created attachment 89665 [details] Patch
(In reply to comment #39) > Created an attachment (id=89665) [details] > Patch Given that bug 58634 landed, I removed the minimum texture size change. I also merged in the changes from the scrollbars-as-layers change. Otherwise, no changes.
Comment on attachment 89665 [details] Patch Bombs away!
Committed r83915: <http://trac.webkit.org/changeset/83915>
Committed r84035: <http://trac.webkit.org/changeset/84035>
http://trac.webkit.org/changeset/84035 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/html/details-add-summary-1-and-click.html fast/html/details-add-summary-1.html fast/html/details-add-summary-10-and-click.html fast/html/details-add-summary-10.html fast/html/details-add-summary-2-and-click.html fast/html/details-add-summary-2.html fast/html/details-add-summary-3-and-click.html fast/html/details-add-summary-3.html fast/html/details-add-summary-4-and-click.html fast/html/details-add-summary-4.html fast/html/details-add-summary-5-and-click.html fast/html/details-add-summary-5.html fast/html/details-add-summary-6-and-click.html fast/html/details-add-summary-6.html fast/html/details-add-summary-7-and-click.html fast/html/details-add-summary-7.html fast/html/details-add-summary-8-and-click.html fast/html/details-add-summary-8.html fast/html/details-add-summary-9-and-click.html fast/html/details-add-summary-9.html fast/html/details-no-summary1.html fast/html/details-no-summary2.html fast/html/details-no-summary3.html fast/html/details-no-summary4.html fast/html/details-open-javascript.html fast/html/details-open1.html fast/html/details-open2.html fast/html/details-open3.html fast/html/details-open4.html fast/html/details-open5.html fast/html/details-open6.html fast/html/details-position.html fast/html/details-remove-summary-1-and-click.html fast/html/details-remove-summary-1.html fast/html/details-remove-summary-2-and-click.html fast/html/details-remove-summary-2.html fast/html/details-remove-summary-3-and-click.html fast/html/details-remove-summary-3.html fast/html/details-remove-summary-4-and-click.html fast/html/details-remove-summary-4.html fast/html/details-remove-summary-5-and-click.html fast/html/details-remove-summary-5.html fast/html/details-remove-summary-6-and-click.html fast/html/details-remove-summary-6.html fast/html/details-writing-mode.html