Bug 39461 – Need to implement Tiling for large compositing layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Marrin]

If a compositing layer gets too big on Windows, D3D can't render it. We need to limit layer sizes to 2k x 2k like we do on Mac and do tiling above that.

Test cases will be included in forthcoming patch

[Chris Marrin]

Created attachment 56653 [details]
Patch

[WebKit Review Bot]

Attachment 56653 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:168:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.h:203:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WebTiledLayer.cpp:228:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 56653 [details]
Patch


> Index: WebCore/WebCore.vcproj/WebCore.vcproj
> ===================================================================
> --- WebCore/WebCore.vcproj/WebCore.vcproj	(revision 59629)
> +++ WebCore/WebCore.vcproj/WebCore.vcproj	(working copy)
> @@ -23867,6 +23867,22 @@
>  						>
>  					</File>

>  				</File>
>  				<File
> -					RelativePath="$(WebKitOutputDir)\obj\$(ProjectName)\DerivedSources\JSIDBDatabase.cpp"
> +					RelativePath="..\bindings\js\JSIDBAnyCustom.cpp"
>  					>

etc. Lots of cruft here that should not be committed.

> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h	(revision 59629)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h	(working copy)
> @@ -60,12 +60,13 @@
>      virtual ~WKCACFLayer();
>  
>      virtual void setNeedsRender() { }
> -    virtual void drawInContext(PlatformGraphicsContext*) { }
> +    virtual void drawInContext(PlatformGraphicsContext*, bool flipy) { }

flipy is a bad name, and boolean parameters are hard to understand at the call site. Would be better with an enum.

>      virtual void setNeedsDisplay(const CGRect* dirtyRect);
>      void setNeedsDisplay();
>  
>      // Makes this layer the root when the passed context is rendered
>      void becomeRootLayerForContext(CACFContextRef);
> +    void resignRootLayerForContext(CACFContextRef);

I don't understand the need for this method. Why isn't the CACFContextSetLayer*( call just in WKCACFLayerRenderer?

> +    // This should be overridden if the subclass needs to manipulate the layer hierarchy, along with the
> +    // protected sublayerAtIndex and indexOfSublayer functions
> +    virtual void insertSublayer(PassRefPtr<WKCACFLayer>, size_t index);
> +
> +    // These function are specified in terms of The above function
>      void addSublayer(PassRefPtr<WKCACFLayer> sublayer);
> -    void insertSublayer(PassRefPtr<WKCACFLayer>, size_t index);
>      void insertSublayerAboveLayer(PassRefPtr<WKCACFLayer>, const WKCACFLayer* reference);
>      void insertSublayerBelowLayer(PassRefPtr<WKCACFLayer>, const WKCACFLayer* reference);
>      void replaceSublayer(WKCACFLayer* reference, PassRefPtr<WKCACFLayer>);
> +
> +    virtual void setSublayersFromLayer(WKCACFLayer* source);

I'd like to see a clear separation between "core" and "wrapper" sublayer-related methods. I think the "core" methods should be private/protected, and only the wrapper methods callable from elsewhere. They should have distinct naming.

> +    virtual size_t numSublayers() const
> +    {
> +        CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> +        return sublayers ? CFArrayGetCount(sublayers) : 0;
> +    }

sublayerCount might be a better name.

> -    void setBounds(const CGRect&);
> +    virtual void setBounds(const CGRect&);
>      CGRect bounds() const { return CACFLayerGetBounds(layer()); }
>  

> -    void setFrame(const CGRect&);
> -    CGRect frame() const { return CACFLayerGetFrame(layer()); }

It looks like you did something with frame/bounds methods, but the changelog doesn't explain it.

> +    virtual void setPosition(const CGPoint& position) { CACFLayerSetPosition(layer(), position); setNeedsCommit(); }

Shame that the subclass needs to remember to all setNeedsCommit() too in these virtual methods.

> -    const WKCACFLayer* sublayerAtIndex(int) const;
> +    virtual WKCACFLayer* sublayerAtIndex(int) const;

Why is the return value no longer const?

Out of time, will resume later.

[Chris Marrin]

rdar://problem/7988188

[Chris Marrin]

...
> >      virtual void setNeedsDisplay(const CGRect* dirtyRect);
> >      void setNeedsDisplay();
> >  
> >      // Makes this layer the root when the passed context is rendered
> >      void becomeRootLayerForContext(CACFContextRef);
> > +    void resignRootLayerForContext(CACFContextRef);
> 
> I don't understand the need for this method. Why isn't the CACFContextSetLayer*( call just in WKCACFLayerRenderer?

I wanted to keep the API symmetrical and avoid more CACF calls in WKCACFRenderLayer

I've incorporated all your other changes. Patch soon

[Simon Fraser (smfr)]

(In reply to comment #5)
> ...
> > >      // Makes this layer the root when the passed context is rendered
> > >      void becomeRootLayerForContext(CACFContextRef);
> > > +    void resignRootLayerForContext(CACFContextRef);
> > 
> > I don't understand the need for this method. Why isn't the CACFContextSetLayer*( call just in WKCACFLayerRenderer?
> 
> I wanted to keep the API symmetrical and avoid more CACF calls in WKCACFRenderLayer

But it doesn't make sense to call resignRootLayerForContext() on anything other than the layer is is already the root layer, so both these should really be methods on the context, not on the layer.

[Chris Marrin]

Created attachment 56719 [details]
Replacement patch with Simon's comments addressed

[WebKit Review Bot]

Attachment 56719 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:164:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.h:199:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WebTiledLayer.cpp:221:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 56719 [details]
Replacement patch with Simon's comments addressed

> Index: WebCore/platform/graphics/win/WebTiledLayer.h
> ===================================================================
> +    virtual void setBounds(const CGRect&);
> +    virtual void setPosition(const CGPoint& position);
> +    virtual void setTransform(const CATransform3D& transform);

No need to name the arguments there.

> +    int numTiles() const;

Maybe tileCount() to match sublayerCount().

> Index: WebCore/platform/graphics/win/WKCACFLayerRenderer.cpp
> ===================================================================
>      if (m_context) {
> +        m_rootLayer->resignRootLayerForContext(m_context.get());

This still seems backwards.

> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================

> +void  WKCACFLayer::setSublayersFromLayer(WKCACFLayer* source)

I don't like the name of this method; it's really moving the sublayers, no just setting them. So it would be better as
  adoptSublayers()
or
  moveSublayers()

> Index: WebCore/platform/graphics/win/WebTiledLayer.cpp
> ===================================================================

> +PassRefPtr<WebTiledLayer> WebTiledLayer::create(const CGSize& tileSize, GraphicsLayer* owner)
> +{
> +    if (!WKCACFLayerRenderer::acceleratedCompositingAvailable())
> +        return 0;

This seems like an odd place to check acceleratedCompositingAvailable(). Surely you'd never get here if it is disabled? An assertion might be more appropriate.

> +WebTiledLayer::WebTiledLayer(const CGSize& tileSize, GraphicsLayer* owner)
> +    : WebLayer(WKCACFLayer::Layer, owner)
> +{
> +    // Tiled layers are placed in a child layer that is always the first child of the TiledLayer
> +    RetainPtr<CACFLayerRef> tiles(AdoptCF, CACFLayerCreate(kCACFLayer));
> +    CACFLayerInsertSublayer(layer(), tiles.get(), 0);

I think 'tiles' should be 'tileContainer' here, or tileParent to be consistent with your other naming.

> +void WebTiledLayer::setTransform(const CATransform3D& transform)
> +{
> +    WebLayer::setTransform(transform);
> +    updateTiles();
> +}

Why no check for transform change? You may end up updating tiles for the identity transform a lot.

> +void WebTiledLayer::setNeedsDisplay(const CGRect* dirtyRect)
> +{
> +    // FIXME: Only setNeedsDisplay for tiles that are currently visible
> +    int numTileLayers = numTiles();
> +    for (int i = 0; i < numTileLayers; ++i)
> +        CACFLayerSetNeedsDisplay(tileAtIndex(i), dirtyRect);

This seems wrong. Don't you need to map dirtyRect into the coordinate space of each tile? You should at least intersect the dirty rect with each tile to avoid dirtying tiles that don't intersect the dirty rect.

> +size_t WebTiledLayer::internalSublayerCount() const
> +{
> +    ASSERT(WebLayer::internalSublayerCount() > 0);
> +    return WebLayer::internalSublayerCount() - 1;
> +}

I'd like to see a comment here to explain the magic -1.

> +void WebTiledLayer::internalInsertSublayer(PassRefPtr<WKCACFLayer> layer, size_t index)
> +{
> +    WebLayer::internalInsertSublayer(layer, index + 1);
> +}
> +
> +WKCACFLayer* WebTiledLayer::internalSublayerAtIndex(int i) const
> +{
> +    return WebLayer::internalSublayerAtIndex(i + 1);
> +}
> +
> +int WebTiledLayer::internalIndexOfSublayer(const WKCACFLayer* layer)
> +{
> +    int i = WebLayer::internalIndexOfSublayer(layer);
> +    return (i > 0) ? i - 1 : -1;
> +}

Ditto for these.

> +CACFLayerRef WebTiledLayer::tileParent() const
> +{
> +    CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> +    ASSERT(sublayers && CFArrayGetCount(sublayers) > 0);
> +    return static_cast<CACFLayerRef>(const_cast<void*>(CFArrayGetValueAtIndex(sublayers, 0)));
> +}

Is that really the most efficient way to get the first child?

> +void WebTiledLayer::addTile()
> +{
> +    CACFLayerRef layerTileParent = tileParent();
> +    RetainPtr<CACFLayerRef> newLayer(AdoptCF, CACFLayerCreate(kCACFLayer));
> +    CACFLayerSetAnchorPoint(newLayer.get(), CGPointMake(0, 1));
> +    CACFLayerSetUserData(newLayer.get(), this);
> +    CACFLayerSetDisplayCallback(newLayer.get(), tileDisplayCallback);
> +    CACFLayerInsertSublayer(tileParent(), newLayer.get(), numTiles());

Use your layerTileParent local variable. Even the, it's a shame to have to call
tileParent() every time addTile() is called (which happens in a loop in updateTiles()). Maybe pass the tileParent in, or just keep tileParent in a member var?

> +void WebTiledLayer::removeTile()
> +{
> +    CACFLayerRemoveFromSuperlayer(tileAtIndex(numTiles() - 1));
> +}

This is weird. Why just remove the last one?

> +CACFLayerRef WebTiledLayer::tileAtIndex(int index)
> +{
> +    CFArrayRef sublayers = CACFLayerGetSublayers(tileParent());
> +    if (!sublayers || index < 0 || numTiles() <= index)
> +        return 0;

Easier to read as index < 0 || index >= numTiles()

> +void WebTiledLayer::updateTiles()
> +{
> +    // FIXME: In addition to redoing the number of tiles, we need to only render and have backing
> +    // store for visible layers
> +    CGRect layerBounds = bounds();
> +    int numTilesHorizontal = ceil(layerBounds.size.width / (float) m_tileSize.width);
> +    int numTilesVertical = ceil(layerBounds.size.height / (float) m_tileSize.height);

m_tileSize is CGFloats already. No need to cast.

> +    int numTilesTotal = numTilesHorizontal * numTilesVertical;

Chance of integer overflow here.

> +    int numTilesToChange = numTilesTotal - numTiles();
> +    if (numTilesToChange >= 0) {
> +        // Add new tiles
> +        for (int i = 0; i < numTilesToChange; ++i)
> +            addTile();
> +    } else {
> +        // Remove old tiles
> +        numTilesToChange = -numTilesToChange;
> +        for (int i = 0; i < numTilesToChange; ++i)
> +            removeTile();
> +    }

Seems that batch-add and batch-remove would be worth doing for efficiency.

> +    // Set coordinates for all tiles
> +    for (int i = 0; i < numTilesHorizontal; ++i) {
> +        for (int j = 0; j < numTilesVertical; ++j) {
> +            CACFLayerRef tile = tileAtIndex(i * numTilesVertical + j);
> +            CACFLayerSetPosition(tile, CGPointMake(i * m_tileSize.width, j * m_tileSize.height));
> +            int width = min(m_tileSize.width, layerBounds.size.width - i * m_tileSize.width);
> +            int height = min(m_tileSize.height, layerBounds.size.height - j * m_tileSize.height);
> +            CACFLayerSetBounds(tile, CGRectMake(0, 0, width, height));
> +
> +            // Flip Y
> +            CATransform3D transform = CATransform3DMakeScale(1, -1, 1);
> +            CATransform3DTranslate(transform, 0, height, 0);
> +            CACFLayerSetTransform(tile, transform);

I don't understand why we need Y-flipping here. The entire layer hierarchy is already flipped.

> +
> +            String name = "Tile (" + String::number(i) + "," + String::number(j) + ")";
> +            CACFLayerSetName(tile, RetainPtr<CFStringRef>(AdoptCF, name.createCFString()).get());

Name-setting should be debug-only.

Why doesn't updateTiles() do a setNeedsDisplay() on the tiles?
Can this be optimized to only add/remove tiles at the edges, to avoid having to redraw all the times when the size changes?

> +void WebTiledLayer::drawTile(CACFLayerRef tile, CGContextRef context)
> +{
> +    CGRect tileBounds = CACFLayerGetBounds(tile);
> +    CGPoint tilePosition = CACFLayerGetPosition(tile);
> +
> +    CGContextSaveGState(context);
> +
> +    //int y = bounds().size.height - tilePosition.y - tileBounds.size.height;

Don't commit commnted-out code.

> +    int y = tilePosition.y;

No need for this local variable.

> Index: LayoutTests/platform/win/compositing/huge-layer-expected.txt
> ===================================================================
> --- LayoutTests/platform/win/compositing/huge-layer-expected.txt	(revision 0)
> +++ LayoutTests/platform/win/compositing/huge-layer-expected.txt	(revision 0)
> @@ -0,0 +1,45 @@
> +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 compositing layers.
> +
> +(GraphicsLayer
> +  (position 0.00 0.00)
> +  (anchor 0.50 0.50)
> +  (bounds 785.00 5111.00)
> +  (opacity 1.00)
> +  (usingTiledLayer 0)
> +  (preserves3D 0)
> +  (drawsContent 0)
> +  (backfaceVisibility visible)
> +  (backgroundColor none)
> +  (transform identity)
> +  (children 1
> +    (GraphicsLayer
> +      (position 0.00 0.00)
> +      (anchor 0.50 0.50)
> +      (bounds 785.00 5111.00)
> +      (opacity 1.00)
> +      (usingTiledLayer 0)
> +      (preserves3D 0)
> +      (drawsContent 0)
> +      (backfaceVisibility visible)
> +      (backgroundColor none)
> +      (transform identity)
> +      (childrenTransform identity)
> +      (children 1
> +        (GraphicsLayer
> +          (position 8.00 68.00)
> +          (anchor 0.50 0.50)
> +          (bounds 502.00 5002.00)
> +          (opacity 1.00)
> +          (usingTiledLayer 1)
> +          (preserves3D 0)
> +          (drawsContent 1)
> +          (backfaceVisibility visible)
> +          (backgroundColor none)
> +          (transform identity)
> +          (childrenTransform identity)
> +        )
> +      )
> +    )
> +  )
> +)

Does this test really test anything useful? Would it have passed before the tiled layer changes?

If you're just testing that it doesn't crash, the test should say that.

> Index: LayoutTests/platform/win/compositing/huge-layer-add-remove-child-expected.txt
> ===================================================================

Same comments about this test. You're dumping GraphicsLayers, not WKCACALayers, so I'm not sure the test would reveal the kind of failure you need it to.

> Index: LayoutTests/platform/win/compositing/huge-layer-with-layer-children-resize-expected.txt
> ===================================================================

Same.

> Index: LayoutTests/platform/win/compositing/huge-layer-with-layer-children-expected.txt
> ===================================================================

Same

> Index: LayoutTests/platform/win/compositing/huge-layer-resize-expected.txt
> ===================================================================

Same.

r- because I think WebTiledLayer::setNeedsDisplay() is wrong.

[Chris Marrin]

(In reply to comment #9)
> ...
> 
> Does this test really test anything useful? Would it have passed before the tiled layer changes?
> 
> If you're just testing that it doesn't crash, the test should say that.
> 
> > Index: LayoutTests/platform/win/compositing/huge-layer-add-remove-child-expected.txt
> > ===================================================================
> 
> Same comments about this test. You're dumping GraphicsLayers, not WKCACALayers, so I'm not sure the test would reveal the kind of failure you need it to.
> 
> > Index: LayoutTests/platform/win/compositing/huge-layer-with-layer-children-resize-expected.txt
> > ===================================================================
> 
> Same.
> 
> > Index: LayoutTests/platform/win/compositing/huge-layer-with-layer-children-expected.txt
> > ===================================================================
> 
> Same
> 
> > Index: LayoutTests/platform/win/compositing/huge-layer-resize-expected.txt
> > ===================================================================
> 
> Same.
>

These are the best tests I can do given the current capabilities of DRT. They will fail without my changes, if only because usingTiledLayer will never be 1. No time to do any better right now.

[Chris Marrin]

(In reply to comment #9)
...
> > +CACFLayerRef WebTiledLayer::tileParent() const
> > +{
> > +    CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> > +    ASSERT(sublayers && CFArrayGetCount(sublayers) > 0);
> > +    return static_cast<CACFLayerRef>(const_cast<void*>(CFArrayGetValueAtIndex(sublayers, 0)));
> > +}
> 
> Is that really the most efficient way to get the first child?

As opposed to some sort of getFirstChild call? I don't see one in any of the CACF API.

>...
> > +void WebTiledLayer::removeTile()
> > +{
> > +    CACFLayerRemoveFromSuperlayer(tileAtIndex(numTiles() - 1));
> > +}
> 
> This is weird. Why just remove the last one?

This is done in a loop to get rid of tiles no longer needed. I'm removing them from the end to avoid needing to move existing array elements.

>...
> > +    int numTilesTotal = numTilesHorizontal * numTilesVertical;
> 
> Chance of integer overflow here.

I will be adding logic to handle max number of tiles when this patch is landed. That will take care of any overflow issues.

> 
> > +    int numTilesToChange = numTilesTotal - numTiles();
> > +    if (numTilesToChange >= 0) {
> > +        // Add new tiles
> > +        for (int i = 0; i < numTilesToChange; ++i)
> > +            addTile();
> > +    } else {
> > +        // Remove old tiles
> > +        numTilesToChange = -numTilesToChange;
> > +        for (int i = 0; i < numTilesToChange; ++i)
> > +            removeTile();
> > +    }
> 
> Seems that batch-add and batch-remove would be worth doing for efficiency.

True. But that would be more work, and I think this is sufficient for now.

> 
> > +    // Set coordinates for all tiles
> > +    for (int i = 0; i < numTilesHorizontal; ++i) {
> > +        for (int j = 0; j < numTilesVertical; ++j) {
> > +            CACFLayerRef tile = tileAtIndex(i * numTilesVertical + j);
> > +            CACFLayerSetPosition(tile, CGPointMake(i * m_tileSize.width, j * m_tileSize.height));
> > +            int width = min(m_tileSize.width, layerBounds.size.width - i * m_tileSize.width);
> > +            int height = min(m_tileSize.height, layerBounds.size.height - j * m_tileSize.height);
> > +            CACFLayerSetBounds(tile, CGRectMake(0, 0, width, height));
> > +
> > +            // Flip Y
> > +            CATransform3D transform = CATransform3DMakeScale(1, -1, 1);
> > +            CATransform3DTranslate(transform, 0, height, 0);
> > +            CACFLayerSetTransform(tile, transform);
> 
> I don't understand why we need Y-flipping here. The entire layer hierarchy is already flipped.

I added a comment about it.

>...
> 
> Why doesn't updateTiles() do a setNeedsDisplay() on the tiles?
> Can this be optimized to only add/remove tiles at the edges, to avoid having to redraw all the times when the size changes?

Everything it's called from (which is currently only setBounds) will do the appropriate setNeedsDisplay, right?

[Chris Marrin]

Created attachment 56766 [details]
Replacement patch with more issues addressed

[Simon Fraser (smfr)]

Comment on attachment 56766 [details]
Replacement patch with more issues addressed

> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================

> +    void setNeedsDisplay(const CGRect* dirtyRect)
> +    {
> +        internalSetNeedsDisplay(dirtyRect);
> +        setNeedsCommit();
> +    }
> +
> +    void setNeedsDisplay() { setNeedsDisplay(0); }

Why not just void setNeedsDisplay(const CGRect* dirtyRect = 0)? No need for another method.

> Index: WebCore/platform/graphics/win/WebTiledLayer.cpp
> ===================================================================

> +WebTiledLayer::WebTiledLayer(const CGSize& tileSize, GraphicsLayer* owner)
> +    : WebLayer(WKCACFLayer::Layer, owner)
> +{
> +    // Tiled layers are placed in a child layer that is always the first child of the TiledLayer
> +    RetainPtr<CACFLayerRef> tileParent(AdoptCF, CACFLayerCreate(kCACFLayer));
> +    CACFLayerInsertSublayer(layer(), tileParent.get(), 0);
> +
> +    m_tileSize = tileSize;

Use an intializer: m_tileSize(tileSize).

> +CACFLayerRef WebTiledLayer::tileParent() const
> +{
> +    CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> +    ASSERT(sublayers && CFArrayGetCount(sublayers) > 0);
> +    return static_cast<CACFLayerRef>(const_cast<void*>(CFArrayGetValueAtIndex(sublayers, 0)));
> +}

I think it's worth keeping tileParent in a member variable. Up to you.
r=me

[Chris Marrin]

Landed http://trac.webkit.org/changeset/59982