[webkit-reviews] review denied: [Bug 39461] Need to implement Tiling for large compositing layers : [Attachment 56719] Replacement patch with Simon's comments addressed
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 21 11:25:10 PDT 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 39461: Need to implement Tiling for large compositing layers
https://bugs.webkit.org/show_bug.cgi?id=39461
Attachment 56719: Replacement patch with Simon's comments addressed
https://bugs.webkit.org/attachment.cgi?id=56719&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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-expe
cted.txt
> ===================================================================
Same.
> Index:
LayoutTests/platform/win/compositing/huge-layer-with-layer-children-expected.tx
t
> ===================================================================
Same
> Index: LayoutTests/platform/win/compositing/huge-layer-resize-expected.txt
> ===================================================================
Same.
r- because I think WebTiledLayer::setNeedsDisplay() is wrong.
More information about the webkit-reviews
mailing list