Bug 58833

Created attachment 98785 [details] Patch

Comment on attachment 98785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98785&action=review I'm really excited that you're cleaning this up, because it's been a long time coming. Did you sort out why changing the tile size was causing rendering issues? > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:178 > +bool ImageLayerChromium::drawsContent() const > { > - if (!m_textureUpdater) > - m_textureUpdater = ImageLayerTextureUpdater::create(layerRendererContext(), layerRenderer()->contextSupportsMapSub()); > + return m_contents; I'm curious why you needed an implementation of drawsContent for ImageLayer. I could see adding a check for m_contents as an early out. However, I think you might need to call the base class version too to catch cases like GraphicsLayerChromium::drawsContent() returning false or a really huge image layer being used as a mask that can't be drawn. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > +// When tiling is enabled, use tiles of this dimension squared. > +static int defaultTileSize = 256; I suppose that this could wait for a follow-up patch, but I think the TiledLayerChromium-derived classes should manage their own tile sizes, so that image layers and content layers can have different behavior.

(In reply to comment #2) > (From update of attachment 98785 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98785&action=review > > I'm really excited that you're cleaning this up, because it's been a long time coming. > > Did you sort out why changing the tile size was causing rendering issues? No, but I'm pretty sure it's orthogonal to this patch. I'll try to investigate that in parallel. > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:178 > > +bool ImageLayerChromium::drawsContent() const > > { > > - if (!m_textureUpdater) > > - m_textureUpdater = ImageLayerTextureUpdater::create(layerRendererContext(), layerRenderer()->contextSupportsMapSub()); > > + return m_contents; > > I'm curious why you needed an implementation of drawsContent for ImageLayer. I could see adding a check for m_contents as an early out. However, I think you might need to call the base class version too to catch cases like GraphicsLayerChromium::drawsContent() returning false or a really huge image layer being used as a mask that can't be drawn. drawsContent() was returning false for simple image layers that should render, I need to do some more digging to figure out why. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > > +// When tiling is enabled, use tiles of this dimension squared. > > +static int defaultTileSize = 256; > > I suppose that this could wait for a follow-up patch, but I think the TiledLayerChromium-derived classes should manage their own tile sizes, so that image layers and content layers can have different behavior. Agree on both points.

Comment on attachment 98785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98785&action=review lgtm > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:164 > +LayerTextureUpdater* ImageLayerChromium::textureUpdater() const nit: move this to the header file > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:382 > +static void paintLayerHelper(LayerChromium* layer, const IntRect& targetRenderSurface) nit: paintLayerContentsIfDirty may be a better name > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:276 > + m_textureOrientation = textureUpdater->orientation(); ASSERT if all tiles are not being updated and orientation and texelformat changes.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 98785 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=98785&action=review > > > > I'm really excited that you're cleaning this up, because it's been a long time coming. > > > > Did you sort out why changing the tile size was causing rendering issues? > > No, but I'm pretty sure it's orthogonal to this patch. I'll try to investigate that in parallel. > It seems that the tiler can't handle non-square tiles smaller than the layer bounds. If I make the default tile size 256x306 then I get weird distortion on compositing/color-matching/image-color-matching.html. Interesting, but not directly related to this patch.

Comment on attachment 98785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98785&action=review >> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:164 >> +LayerTextureUpdater* ImageLayerChromium::textureUpdater() const > > nit: move this to the header file no can do, ImageLayerTextureUpdater is declared in the .cpp so in the header there's no way for the compiler to know whether a ImageLayerTextureUpdater* actually is a LayerTextureUpdater* or not. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:382 >> +static void paintLayerHelper(LayerChromium* layer, const IntRect& targetRenderSurface) > > nit: paintLayerContentsIfDirty may be a better name good idea >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:276 >> + m_textureOrientation = textureUpdater->orientation(); > > ASSERT if all tiles are not being updated and orientation and texelformat changes. sure

(In reply to comment #3) > > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:178 > > > +bool ImageLayerChromium::drawsContent() const > > > { > > > - if (!m_textureUpdater) > > > - m_textureUpdater = ImageLayerTextureUpdater::create(layerRendererContext(), layerRenderer()->contextSupportsMapSub()); > > > + return m_contents; > > > > I'm curious why you needed an implementation of drawsContent for ImageLayer. I could see adding a check for m_contents as an early out. However, I think you might need to call the base class version too to catch cases like GraphicsLayerChromium::drawsContent() returning false or a really huge image layer being used as a mask that can't be drawn. > > drawsContent() was returning false for simple image layers that should render, I need to do some more digging to figure out why. > Ah, the issue is that GraphicsLayer::drawsContent() returns false for simple composited images, as it should, because only the image layer draws. It would make sense to return false for the other cases, though. I'll see how reasonable that is.

Created attachment 98989 [details] Patch

Addressed most review feedback: - renamed the helper fn in LayerRendererChromium to paintLayerContentsIfDirty - made ImageLayerChromium::drawsContent() check m_contents and the parent class' ::drawsContent() to check for missing owner, bad tiling assumptions, etc. To do this I added a ContentLayerChromium::drawsContent() override to check for m_owner->drawsContent() since that check is only applicable to content layers There's no good way to ASSERT() that the texel format or texture orientation only change if all of the tiles are updated since there's no simple way to define 'all the tiles'. The code behaves the same way with this patch as it did before, anyhow, since the draw() implementation assumes that all tiles have the texture orientation and texel format as the updater.

Comment on attachment 98989 [details] Patch Looks really good! If you don't mind holding off landing it until the memory reduction patches are in, it will make backporting them to M13 a lot simpler.

Created attachment 99890 [details] Patch

Merge up to ToT. I think https://bugs.webkit.org/show_bug.cgi?id=64009 is the last bug that Vangelis wanted to hold off on, and it shouldn't conflict with this patch, so as soon as that lands we should be good to go for this one.

Comment on attachment 99890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99890&action=review Unofficially, this looks good to me, other than that one nit. > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:142 > void CCLayerImpl::draw() > { > - return m_owner->draw(); > + ASSERT_NOT_REACHED(); > } Nit: If you don't want anyone to call this particular function, consider making it abstract.

Comment on attachment 99890 [details] Patch Looks good based on cursory review and Adrienne's and Vangelis's unofficial reviews. r=me

Committed r90842: <http://trac.webkit.org/changeset/90842>

(In reply to comment #13) > (From update of attachment 99890 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99890&action=review > > Unofficially, this looks good to me, other than that one nit. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:142 > > void CCLayerImpl::draw() > > { > > - return m_owner->draw(); > > + ASSERT_NOT_REACHED(); > > } > > Nit: If you don't want anyone to call this particular function, consider making it abstract. Note for posterity: This can't be abstract since we do instantiate CCLayerImpls (the base class), but the draw() call should be unreached since drawsContent() always returns false for these.

Patch was reverted, I was being a little sloppy with drawsContent() handling. Updating patch on the way...

Created attachment 100599 [details] Patch

Patch updated. The change between the previous version that fixes the bug is changing LayerRendererChromium::updateCompositorResources() to check drawsContent() on the LayerChromium* instead of the CCLayerImpl*, since that bit hasn't been synchronized at the time the function is called. Other changes to make things a bit safer/more reliable are: - adding invalid values for the LayerTextureUpdater enums and initializing the values on LayerTilerChromium explicitly - initializing CCLayerImpl::m_drawsContent explicitly (oops). passes all tests on linux in release _and_ debug so should be good to go.

(In reply to comment #20) > Patch updated. The change between the previous version that fixes the bug is changing LayerRendererChromium::updateCompositorResources() to check drawsContent() on the LayerChromium* instead of the CCLayerImpl*, since that bit hasn't been synchronized at the time the function is called. Other changes to make things a bit safer/more reliable are: > - adding invalid values for the LayerTextureUpdater enums and initializing the values on LayerTilerChromium explicitly > - initializing CCLayerImpl::m_drawsContent explicitly (oops). > > passes all tests on linux in release _and_ debug so should be good to go. Those changes unofficially look good to me.

Comment on attachment 100599 [details] Patch Sounds fine. rs=me

Comment on attachment 100599 [details] Patch Clearing flags on attachment: 100599 Committed r90963: <http://trac.webkit.org/changeset/90963>

All reviewed patches have been landed. Closing bug.