Bug 58833 – [chromium] Move draw implementation for ContentLayerChromium/ImageLayerChromium to the appropriate CCLayerImpl subclass

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

As title says.  I'm looking into this now.

[James Robinson]

Created attachment 98785 [details]
Patch

[Adrienne Walker]

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.

[James Robinson]

(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.

[Alok Priyadarshi]

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.

[James Robinson]

(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.

[James Robinson]

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

[James Robinson]

(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.

[James Robinson]

Created attachment 98989 [details]
Patch

[James Robinson]

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.

[Vangelis Kokkevis]

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.

[James Robinson]

Created attachment 99890 [details]
Patch

[James Robinson]

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.

[Adrienne Walker]

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.

[Kenneth Russell]

Comment on attachment 99890 [details]
Patch

Looks good based on cursory review and Adrienne's and Vangelis's unofficial reviews. r=me

[James Robinson]

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

[James Robinson]

(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.

[James Robinson]

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

[James Robinson]

http://trac.webkit.org/changeset/90859

[James Robinson]

Created attachment 100599 [details]
Patch

[James Robinson]

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.

[Adrienne Walker]

(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.

[Kenneth Russell]

Comment on attachment 100599 [details]
Patch

Sounds fine. rs=me

[WebKit Review Bot]

Comment on attachment 100599 [details]
Patch

Clearing flags on attachment: 100599

Committed r90963: <http://trac.webkit.org/changeset/90963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.