745177 - Retain and re-use uploaded tiles

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

We should implement the tiled surface in such a way that when content has been uploaded in the Shadow TiledThebesLayerOGL, in the absence of more recent data, it can be re-used.

This will mitigate some checkerboarding when quickly changing directions, or when zooming in, then out again, which should in turn allow us to be more aggressive with our viewport prediction.

For reference, I implemented this in the Java compositor, but we turned it off as it could possibly allow the user to see inconsistent rendering. Since then, drawing performance has improved such that the user is far less likely to see inconsistent rendering for any length of time, and being Gecko-side, we have more opportunity to correctly invalidate if we need to.

I think correct invalidation should be dealt with as a separate bug, however, if we need to do that.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: WIP patch

Here's a WIP that seems to work (it currently draws non-retained tiles at half-opacity to aid testing). This will work *much* better when we have the display-port aligned with the tile-boundaries.

This doesn't currently reconcile transform differences, so zooming in/out isn't helped (and in fact is just more likely to display invalid content). I'm not sure the best way to go about that yet, I spent quite a while trying to get it right and failed. So far at least.

I chose going this way over the simpler way of just expanding the valid region before update in TiledLayerBufferOGL::Upload, as I couldn't think of a simple way to stop the tile-store growing too large, and I think doing it that way would make it impossible to reconcile transform differences. I also like that this separates the valid and invalid content, in case we ever wanted to render differently somehow to signify invalid content (for example, desaturated, blurred, with highlight/shadow, etc.)

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 616127 [details] [diff] [review]
WIP patch

This has stood up quite well to my testing. I see problems with flinging while zoomed, but even that is surprisingly minor.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: WIP patch 2

This patch adds the necessary machinery to keep track of tile resolution and uses that information to reconcile differences during zooming. Retaining tiles over zoom changes works, but it seems my heuristics for retaining tiles aren't quite right yet...

This may just work, or work better after bug 737510, but I'll try to get it working better without too. Feedback still appreciated.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Retain and reuse uploaded, invalid GL tiles

Here's a final-ish patch. This retains only whole tiles that fall outside of the valid region (theoretically - though it still looks like this isn't working quite as I'd expect, so I've probably missed something), and retains all tiles that even partially fall outside of the valid region when the render resolution changes (as we know in this case that the whole layer will be invalidated).

This makes zooming in, then immediately zooming back out not display any 'checkerboard', and can also help when flinging and changing direction quickly (though this seems a bit flaky...)

There are two TODOs I've left:
- Define the fraction that determines the size of the invalid tile store somewhere (currently, it just uses the same size as the tile-store for the valid region, so you'll end up with twice as many uploaded tiles as you have buffered)
- Clip/scissor invalid tiles to make sure they don't get drawn underneath valid tiles in any situation. This ought to only be necessary for layers with opacity, and I'm not sure if this is a priority at the moment... Need feedback/testing.

Also, this will need to be rebased on the latest tiling work/m-c as I've not updated in a while in a bid to actually get some work done :)

Comment 5

[Chris Lord [:cwiiis]]

I realise now that the panning doesn't work well due to the way partial tiles are drawn/uploaded - I don't think it's worth investing the time to fix this, it'd be non-trivial - instead, I'm just marking this as blocking on bug 737510 (which will fix this).

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Retain and reuse uploaded, invalid GL tiles (rebased)

Same patch, rebased on top of latest patches from all dependent bugs.

I've also stuck an apk up: http://chrislord.net/files/mozilla/fennec-tiles-snaptiles-retaintiles.apk

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 616567 [details] [diff] [review]
Retain and reuse uploaded, invalid GL tiles (rebased)

Changing reviewer to ajuma, as discussed.

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 616567 [details] [diff] [review]
Retain and reuse uploaded, invalid GL tiles (rebased)

Review of attachment 616567 [details] [diff] [review]:
-----------------------------------------------------------------

Here's the feedback draft I had, I'll post that now and give you some additional feedback soon. Having Ali look at it is still a good idea if he has a bit of time to spare.

::: gfx/layers/TiledLayerBuffer.h
@@ +49,5 @@
>    T GetTile(int x, int y) const;
>  
>    uint16_t GetTileLength() const { return TILEDLAYERBUFFER_TILE_SIZE; }
>  
> +  unsigned int NTiles() const { return mRetainedTiles.Length(); }

Needs a better name

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +59,5 @@
> +
> +  // Iterate over the tiles and decide which ones we're going to keep in our
> +  // separate, invalid/old tile store.
> +  uint16_t tileSize = GetTileLength();
> +  nsIntRect validBounds = mValidRegion.GetBounds();

We need a better way to iterate over the tiles since we're already going to have 3 copies of this code with my patch and soon 4. 

Either we fix it here or I'll post a patch for my stuff shortly after it lands.

@@ +220,5 @@
>  
>  }
>  
>  void
> +TiledThebesLayerOGL::RenderQuad(TiledTexture aTile,

We currently support drawing all the way through in tiling except that the end. We need to fix that. This should be handled here rather then outside of this function so that we avoid state change for each rect of the region.

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +52,5 @@
>    GLuint mTextureHandle;
>    GLenum mFormat;
>  };
>  
> +class InvalidTiledTextureOGL

This needs documentation. My first though is 'Why keep a tiled texture if it's invalid'. Maybe we can find a better name too?

::: layout/base/nsDisplayList.cpp
@@ +245,5 @@
>    }
>  
>    metrics.mScrollId = aScrollId;
> +
> +  nsIPresShell* presShell = presContext->GetPresShell();

These changes should ideally be in another patch. I'm also not the best person to review this change.

Comment 9

[Ali Juma [:ajuma]]

Comment on attachment 616567 [details] [diff] [review]
Retain and reuse uploaded, invalid GL tiles (rebased)

Review of attachment 616567 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good on the whole. r+ with comments (and BenWa's comments) addressed, and with the change to layout/base/nsDisplayList.cpp reviewed separately by someone familiar with that code.

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +57,5 @@
> +  gfxSize scaleFactor = gfxSize(aResolution.width / mResolution.width,
> +                                aResolution.height / mResolution.height);
> +
> +  // Iterate over the tiles and decide which ones we're going to keep in our
> +  // separate, invalid/old tile store.

Add a comment describing (at a high level) how we're deciding which tiles to keep.

@@ +107,5 @@
> +    x += w;
> +  }
> +
> +  // Now prune our invalid tile store of its oldest tiles if it gets too large.
> +  // TODO: Define invalid tile-store relative size fraction somewhere...

Something in the range [1.0, 2.0] seems reasonable to start with.

@@ +209,5 @@
> +  for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
> +    const FrameMetrics& metrics = parent->GetFrameMetrics();
> +    resolution.width *= metrics.mResolution.width;
> +    resolution.height *= metrics.mResolution.height;
> +  }

For large layer trees, computing this once top-down rather than bottom-up for every leaf will be more efficient. We don't need to fix this now (since this isn't going to hurt us in practice right now), but do add a comment.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Retain and reuse uploaded, invalid GL tiles (revised)

This addresses most review comments, carrying r=ajuma. There is still tidy-up that needs to land after this, hopefully all of which is documented.

Comment 11

[Chris Lord [:cwiiis]]

Landed on inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/4114c654f5d0

Comment 12

[Matt Brubeck (:mbrubeck)]

Sorry, backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/549616263fd1

because it failed to build on Mac:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11179459&tree=Mozilla-Inbound

In file included from ../../../../gfx/layers/opengl/ReusableTileStoreOGL.cpp:6:
../../../../gfx/layers/opengl/ReusableTileStoreOGL.h:90: error: 'mTiles' was not declared in this scope
../../../../gfx/layers/opengl/ReusableTileStoreOGL.h:90: error: '>>' should be '> >' within a nested template argument list

Comment 13

[Chris Lord [:cwiiis]]

(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Sorry, backed out on inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/549616263fd1
> 
> because it failed to build on Mac:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=11179459&tree=Mozilla-
> Inbound
> 
> In file included from
> ../../../../gfx/layers/opengl/ReusableTileStoreOGL.cpp:6:
> ../../../../gfx/layers/opengl/ReusableTileStoreOGL.h:90: error: 'mTiles' was
> not declared in this scope
> ../../../../gfx/layers/opengl/ReusableTileStoreOGL.h:90: error: '>>' should
> be '> >' within a nested template argument list

erk, this was due to lack of spaces in a nested template... I've pushed a fixed patch to try, in case: https://tbpl.mozilla.org/?tree=Try&rev=efa29be43980 - will re-land when the mac build turns green.

Comment 14

[Chris Lord [:cwiiis]]

For whatever reason, Mac wasn't getting built on that try run... I've taken a risk and pushed to inbound again, as we're trying to get stuff done quickly:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b98060a2d11c

Comment 15

[(no longer active)]

(Please don't add the [inbound] tag to the whiteboard -- thanks!)

https://hg.mozilla.org/mozilla-central/rev/4114c654f5d0

Comment 16

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/549616263fd1

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Backed out: https://hg.mozilla.org/mozilla-central/rev/549616263fd1

This was the old back-out, it's currently sitting on inbound, green: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b98060a2d11c

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/b98060a2d11c

Comment 19

[JP Rosevear [:jpr]]

Comment on attachment 618140 [details] [diff] [review]
Retain and reuse uploaded, invalid GL tiles (revised)

Mobile beta blocker.  Low risk of impacting desktop.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4e462ad50a2