From 63545be6bcdc8aa2d10810e30cc8a67d0332adfd Mon Sep 17 00:00:00 2001 From: Chris Lord Date: Wed, 21 Nov 2012 22:34:19 +0000 Subject: [PATCH] Bug 783368 - Fix progressive tile update coherency issues. r=bgirard Fix some progressive tile updating coherency issues caused by aborting at inopportune times and tile draw ordering. --- gfx/layers/basic/BasicTiledThebesLayer.cpp | 57 +++++++++++++------ gfx/layers/basic/BasicTiledThebesLayer.h | 5 ++ mobile/android/base/gfx/GeckoLayerClient.java | 11 ---- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/gfx/layers/basic/BasicTiledThebesLayer.cpp b/gfx/layers/basic/BasicTiledThebesLayer.cpp index dee9d6452ef9..6b013242ec0d 100644 --- a/gfx/layers/basic/BasicTiledThebesLayer.cpp +++ b/gfx/layers/basic/BasicTiledThebesLayer.cpp @@ -269,6 +269,7 @@ BasicTiledThebesLayer::ComputeProgressiveUpdateRegion(BasicTiledLayerBuffer& aTi const nsIntRegion& aOldValidRegion, nsIntRegion& aRegionToPaint, const gfx3DMatrix& aTransform, + const nsIntRect& aCompositionBounds, const gfx::Point& aScrollOffset, const gfxSize& aResolution, bool aIsRepeated) @@ -281,18 +282,16 @@ BasicTiledThebesLayer::ComputeProgressiveUpdateRegion(BasicTiledLayerBuffer& aTi bool drawingLowPrecision = aTiledBuffer.IsLowPrecision(); // Find out if we have any non-stale content to update. - nsIntRegion freshRegion; - if (!mFirstPaint) { - freshRegion.And(aInvalidRegion, aOldValidRegion); - freshRegion.Sub(aInvalidRegion, freshRegion); - } + nsIntRegion staleRegion; + staleRegion.And(aInvalidRegion, aOldValidRegion); // Find out the current view transform to determine which tiles to draw // first, and see if we should just abort this paint. Aborting is usually // caused by there being an incoming, more relevant paint. gfx::Rect viewport; float scaleX, scaleY; - if (BasicManager()->ProgressiveUpdateCallback(!freshRegion.IsEmpty(), viewport, + if (BasicManager()->ProgressiveUpdateCallback(!staleRegion.Contains(aInvalidRegion), + viewport, scaleX, scaleY, !drawingLowPrecision)) { SAMPLE_MARKER("Abort painting"); aRegionToPaint.SetEmpty(); @@ -304,10 +303,16 @@ BasicTiledThebesLayer::ComputeProgressiveUpdateRegion(BasicTiledLayerBuffer& aTi RoundedTransformViewportBounds(viewport, aScrollOffset, aResolution, scaleX, scaleY, aTransform); - // Paint tiles that have no content before tiles that only have stale content. - bool drawingStale = freshRegion.IsEmpty(); + // Paint tiles that have stale content or that intersected with the screen + // at the time of issuing the draw command in a single transaction first. + // This is to avoid rendering glitches on animated page content, and when + // layers change size/shape. + nsIntRect criticalViewportRect = roundedTransformedViewport.Intersect(aCompositionBounds); + aRegionToPaint.And(aInvalidRegion, criticalViewportRect); + aRegionToPaint.Or(aRegionToPaint, staleRegion); + bool drawingStale = !aRegionToPaint.IsEmpty(); if (!drawingStale) { - aRegionToPaint = freshRegion; + aRegionToPaint = aInvalidRegion; } // Prioritise tiles that are currently visible on the screen. @@ -317,6 +322,10 @@ BasicTiledThebesLayer::ComputeProgressiveUpdateRegion(BasicTiledLayerBuffer& aTi paintVisible = true; } + // Paint area that's visible and overlaps previously valid content to avoid + // visible glitches in animated elements, such as gifs. + bool paintInSingleTransaction = paintVisible && (drawingStale || mFirstPaint); + // The following code decides what order to draw tiles in, based on the // current scroll direction of the primary scrollable layer. NS_ASSERTION(!aRegionToPaint.IsEmpty(), "Unexpectedly empty paint region!"); @@ -364,11 +373,12 @@ BasicTiledThebesLayer::ComputeProgressiveUpdateRegion(BasicTiledLayerBuffer& aTi // The region needed to paint is larger then our progressive chunk size // therefore update what we want to paint and ask for a new paint transaction. - // If we're drawing stale, visible content, make sure that it happens - // in one go by repeating this work without calling the painted - // callback. The remaining content is then drawn tile-by-tile in - // multiple transactions. - if (!drawingLowPrecision && paintVisible && drawingStale) { + // If we need to draw more than one tile to maintain coherency, make + // sure it happens in the same transaction by requesting this work be + // repeated immediately. + // If this is unnecessary, the remaining work will be done tile-by-tile in + // subsequent transactions. + if (!drawingLowPrecision && paintInSingleTransaction) { repeatImmediately = true; } else { BasicManager()->SetRepeatTransaction(); @@ -384,6 +394,7 @@ BasicTiledThebesLayer::ProgressiveUpdate(BasicTiledLayerBuffer& aTiledBuffer, nsIntRegion& aInvalidRegion, const nsIntRegion& aOldValidRegion, const gfx3DMatrix& aTransform, + const nsIntRect& aCompositionBounds, const gfx::Point& aScrollOffset, const gfxSize& aResolution, LayerManager::DrawThebesLayerCallback aCallback, @@ -399,6 +410,7 @@ BasicTiledThebesLayer::ProgressiveUpdate(BasicTiledLayerBuffer& aTiledBuffer, aOldValidRegion, regionToPaint, aTransform, + aCompositionBounds, aScrollOffset, aResolution, repeat); @@ -497,12 +509,20 @@ BasicTiledThebesLayer::PaintThebes(gfxContext* aContext, resolution.height *= metrics.mResolution.height; } - // Calculate the scroll offset since the last transaction. + // Calculate the scroll offset since the last transaction, and the + // composition bounds. + nsIntRect compositionBounds; gfx::Point scrollOffset(0, 0); Layer* primaryScrollable = BasicManager()->GetPrimaryScrollableLayer(); if (primaryScrollable) { const FrameMetrics& metrics = primaryScrollable->AsContainerLayer()->GetFrameMetrics(); scrollOffset = metrics.mScrollOffset; + gfxRect transformedViewport = transform.TransformBounds( + gfxRect(metrics.mCompositionBounds.x, metrics.mCompositionBounds.y, + metrics.mCompositionBounds.width, metrics.mCompositionBounds.height)); + transformedViewport.RoundOut(); + compositionBounds = nsIntRect(transformedViewport.x, transformedViewport.y, + transformedViewport.width, transformedViewport.height); } // Only draw progressively when the resolution is unchanged. @@ -528,8 +548,8 @@ BasicTiledThebesLayer::PaintThebes(gfxContext* aContext, } if (!ProgressiveUpdate(mTiledBuffer, mValidRegion, invalidRegion, - oldValidRegion, transform, scrollOffset, resolution, - aCallback, aCallbackData)) + oldValidRegion, transform, compositionBounds, + scrollOffset, resolution, aCallback, aCallbackData)) return; } else { mTiledBuffer.SetFrameResolution(resolution); @@ -594,7 +614,8 @@ BasicTiledThebesLayer::PaintThebes(gfxContext* aContext, updatedLowPrecision = ProgressiveUpdate(mLowPrecisionTiledBuffer, mLowPrecisionValidRegion, lowPrecisionInvalidRegion, oldValidRegion, transform, - scrollOffset, resolution, aCallback, aCallbackData); + compositionBounds, scrollOffset, resolution, aCallback, + aCallbackData); } // Re-add the high-precision valid region intersection so that we can diff --git a/gfx/layers/basic/BasicTiledThebesLayer.h b/gfx/layers/basic/BasicTiledThebesLayer.h index 10fe1522ad09..afc1f2e6e754 100644 --- a/gfx/layers/basic/BasicTiledThebesLayer.h +++ b/gfx/layers/basic/BasicTiledThebesLayer.h @@ -230,6 +230,8 @@ private: * which indicates that there is no more work to do. * aTransform is the transform required to convert from screen-space to * layer-space. + * aCompositionBounds is the composition bounds from the primary scrollable + * layer, transformed into layer coordinates. * aScrollOffset is the current scroll offset of the primary scrollable layer. * aResolution is the render resolution of the layer. * aIsRepeated should be true if this function has already been called during @@ -243,18 +245,21 @@ private: const nsIntRegion& aOldValidRegion, nsIntRegion& aRegionToPaint, const gfx3DMatrix& aTransform, + const nsIntRect& aCompositionBounds, const gfx::Point& aScrollOffset, const gfxSize& aResolution, bool aIsRepeated); /** * Performs a progressive update of a given tiled buffer. + * See ComputeProgressiveUpdateRegion above for parameter documentation. */ bool ProgressiveUpdate(BasicTiledLayerBuffer& aTiledBuffer, nsIntRegion& aValidRegion, nsIntRegion& aInvalidRegion, const nsIntRegion& aOldValidRegion, const gfx3DMatrix& aTransform, + const nsIntRect& aCompositionBounds, const gfx::Point& aScrollOffset, const gfxSize& aResolution, LayerManager::DrawThebesLayerCallback aCallback, diff --git a/mobile/android/base/gfx/GeckoLayerClient.java b/mobile/android/base/gfx/GeckoLayerClient.java index 315820fb80d9..c0551c9fd110 100644 --- a/mobile/android/base/gfx/GeckoLayerClient.java +++ b/mobile/android/base/gfx/GeckoLayerClient.java @@ -412,17 +412,6 @@ public class GeckoLayerClient return mProgressiveUpdateData; } - // There's no new content (where new content is considered to be an - // update in a region that wasn't previously visible), and we've sent a - // more recent display-port. - // Aborting in this situation helps us recover more quickly when the - // user starts scrolling on a page that contains animated content that - // is slow to draw. - if (!aHasPendingNewThebesContent) { - mProgressiveUpdateData.abort = true; - return mProgressiveUpdateData; - } - return mProgressiveUpdateData; }