796117 - Improve the Reusable Tile Store eviction strategy

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Benoit Girard (:BenWa)]

There are 2 ways tiles can get evicted from the reusable tile store:
1) The tiles insects with the valid region. In the case of a partial intersection we will evict useful content.
2) After a certain number of tiles, we evict the oldest tile we have. We should change this to be least recently used. This is simply a mater of reordering entries when they are drawn.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: WIP

Right now I change the reusable tile store to retain all the tiles and I've been testing with the main layer drawing disable. I'm trying to make sure we always retain the tiles we should retain and it's still not true but this patch is making things better. We now retain tiles from different resolution and retain the tiles we want on most occasions.

I'm still trying to figure out why some tiles don't get stored (or drawn?) when they should.

Comment 2

[Benoit Girard (:BenWa)]

The problem I'm running into is that we destroy the layer which causes us to lose the entire tile store and stale layer content when hitting the edge of the page :(.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> The problem I'm running into is that we destroy the layer which causes us to
> lose the entire tile store and stale layer content when hitting the edge of
> the page :(.

What page and environment are you testing on? We shouldn't just be destroying layers willy-nilly :/ I have a patch that I need to check in that will improve layer retention during scrolling, it's possible it may affect this.

I guess the alternative is that hitting the page edge is causing the sub-pixel alignment to change and causing a total invalidation, but then the layer still shouldn't be destroyed...

Comment 4

[Benoit Girard (:BenWa)]

Here's a video showing how we lose the layer. We expect old tiles to display until they have been replace but here they disappear because the layer is getting destroyed.

Comment 5

[Benoit Girard (:BenWa)]

http://people.mozilla.com/~bgirard/lostlayer.webm

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Part 1:New tiles will replace equivilent tiles

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 670458 [details] [diff] [review]
Part 1:New tiles will replace equivilent tiles

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

r+ with the second comment addressed.

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +170,5 @@
> +          // Remove any tile that is superseded by this new tile.
> +          // (same resolution, same area)
> +          for (int i = 0; i < mTiles.Length() - 1; i++) {
> +            if (mTiles[i]->mTileOrigin == nsIntPoint(x, y) &&
> +                // XXX Perhaps we should check the region instead of the origin

nit: would rather this comment was just above the if statement rather than interleaved with it.

@@ +174,5 @@
> +                // XXX Perhaps we should check the region instead of the origin
> +                //     so a partial tile doesn't replace a full older tile?
> +                mTiles[i]->mResolution == aOldResolution) {
> +              mTiles.RemoveElementAt(i);
> +              printf_stderr("Remove obsolete tile\n");

Remove this debugging comment/surround it with some kind of #ifdef.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: New tile store tiless will replace equivalent tiles. r=Cwiiis

Forgot to delete the texture

Comment 9

[Benoit Girard (:BenWa)]

Attachment: New tile store tiless will replace equivalent tiles. r=Cwiiis

Comment 10

[Benoit Girard (:BenWa)]

Attachment: New tile store tiless will replace equivalent tiles

re-requesting review. The replacement wasn't perfect and the tile cache would keep growing while scrolling taskjs. This fixes problem with the replacement:

We use the tile origin rather then the draw region origin. And we don't store tiles that are too small because the memory used to retain a small area isn't worth it.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 670961 [details] [diff] [review]
New tile store tiless will replace equivalent tiles

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

Looks fine to me.

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f31a736800

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Keep tiles when changing resolution

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Keep tiles when changing resolution

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 671480 [details] [diff] [review]
Keep tiles when changing resolution

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

r+ with the comments addressed.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +262,5 @@
>      resolution.width *= metrics.mResolution.width;
>      resolution.height *= metrics.mResolution.height;
>    }
>  
> +  if (mTiledBuffer.GetResolution() != resolution) {

Let's add a comment above this, something along the lines of

"If the resolution has changed, discard all the old tiles. They will end up being retained on the shadow side by ReusableTileStoreOGL"

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +184,5 @@
>              }
>            }
> +#ifdef GFX_TILEDLAYER_PREF_WARNINGS
> +          if (replacedATile) {
> +            printf_stderr("Replace tile at %d,%d, x%f for reuse\n", x, y, aOldResolution.width);

nit: s/Replace/Replaced/

@@ +206,5 @@
>    // Make sure we don't hold onto tiles that may cause visible rendering glitches
>    InvalidateTiles(aLayer, aNewValidRegion, aNewResolution);
>  
>    // Now prune our reused tile store of its oldest tiles if it gets too large.
> +  while (mTiles.Length() > aVideoMemoryTiledBuffer->GetTileCount() * mSizeLimit && mTiles.Length() > 500) {

500? That seems an awful lot, I assume this is just for testing.

@@ +217,5 @@
>      mTiles.RemoveElementAt(0);
>    }
>  
>  #ifdef GFX_TILEDLAYER_PREF_WARNINGS
> +  printf_stderr("max %f\n", aVideoMemoryTiledBuffer->GetTileCount() * mSizeLimit);

Would prefer a more descriptive output here, say s/max/Retained tile limit:/

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +159,5 @@
>      delete mReusableTileStore;
>      mReusableTileStore = nullptr;
>    } else if (!mReusableTileStore && !mIsFixedPosition) {
>      // XXX Add a pref for reusable tile store size
> +    mReusableTileStore = new ReusableTileStoreOGL(gl(), 3);

Arbitrary change, sounds like it should stay in a debugging patch until we nail down exactly how we're going to determine the size of the tile store.

Comment 16

[Benoit Girard (:BenWa)]

Opps these two were supposed to be changed by a qref:

(In reply to Chris Lord [:cwiiis] from comment #15)
> 
> @@ +206,5 @@
> >    // Make sure we don't hold onto tiles that may cause visible rendering glitches
> >    InvalidateTiles(aLayer, aNewValidRegion, aNewResolution);
> >  
> >    // Now prune our reused tile store of its oldest tiles if it gets too large.
> > +  while (mTiles.Length() > aVideoMemoryTiledBuffer->GetTileCount() * mSizeLimit && mTiles.Length() > 500) {
> 
> 500? That seems an awful lot, I assume this is just for testing.

I changed to a macro which is defined to 100.

> 
> ::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
> @@ +159,5 @@
> >      delete mReusableTileStore;
> >      mReusableTileStore = nullptr;
> >    } else if (!mReusableTileStore && !mIsFixedPosition) {
> >      // XXX Add a pref for reusable tile store size
> > +    mReusableTileStore = new ReusableTileStoreOGL(gl(), 3);
> 
> Arbitrary change, sounds like it should stay in a debugging patch until we
> nail down exactly how we're going to determine the size of the tile store.

I wont land this change.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Keep tiles when changing resolution

Comment 18

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5496f32f2e4

Comment 19

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9e70b44f36
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d10007609f1
:(

Comment 20

[Chris Lord [:cwiiis]]

The patches checked in aren't the end of this work I don't think, adding leave-open to the whiteboard. At the least, we have the tile-store capacity determination code to write.

Comment 21

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f3f11410a4d0

Comment 22

[Benoit Girard (:BenWa)]

Depends on bug 795674. With the change in atachment 671705 we no longer lose the layer.

Comment 23

[Benoit Girard (:BenWa)]

I need bug 802143 to land before I can reland part 1/2 without regressing mochitest.

Comment 24

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=00111ed0be98

Comment 25

[Benoit Girard (:BenWa)]

I want to experience by evicting lower resolution first. This should let the user force the page to cache by zooming out.

I also want to play with evicting based on the distance from the screen.

Comment 26

[Benoit Girard (:BenWa)]

Attachment: WIP

This is a massive improvement and works very well on taskjs + CNN but I want to fine tune the eviction.

Comment 27

[Benoit Girard (:BenWa)]

Attachment: Updated WIP

More of a backup. This has code path commented out and whatnot for testing.

Comment 28

[Benoit Girard (:BenWa)]

The culling is this incomplete. We do good at culling stuff that on the screen but we still do drawing calling for stuff that outside the screen.

Comment 29

[Benoit Girard (:BenWa)]

Attachment: patch

This include part of the changes from the previous patch plus the new stuff I'm working on. I had to disable progressing painting while zooming in cause I noticed that was some nasty zooming problems in edge cases that would stay in the tile store.

I plan on doing further improvement, particularly to the fixme features that are commented out but for now this is a significant improvement for cnn so I say we land this.

Comment 30

[Benoit Girard (:BenWa)]

Also I know you don't like the idea of having something like 100 tiles per store which is understandable. I also don't want other temporary layers stealing important tiles from the primary tile store. Maybe we could make only the primary thebes layer have a tile store or something similar?

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #30)
> Also I know you don't like the idea of having something like 100 tiles per
> store which is understandable. I also don't want other temporary layers
> stealing important tiles from the primary tile store. Maybe we could make
> only the primary thebes layer have a tile store or something similar?

It's just that 100 tiles at 32bit equates to 25 megs - a cost that would just kill us on armv6 (though maybe the tile store should just be disabled on those devices). I'd prefer a limit based on the visible size of the layer - that would at least mean that small layers don't end up with lots of tiles too.

How about maxTiles = (visibleRegionArea / tileLength^2) * 2? This ought to scale ok, as only layers for the content area will be display-port sized and the rest will be screen-sized or smaller (for the most part) - I'd be happy with this (for now).

Comment 32

[Chris Lord [:cwiiis]]

Comment on attachment 675470 [details] [diff] [review]
patch

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

I like where this is going, but I think this patch is trying to do too much in one go and I'm fairly certain this will introduce coherency issues.

I think we should split out three patches:
- Sort out tile-store size limit (I'll post a patch for this today)
- Enable progressive rendering when zooming (I think we can/should do this without relying on the tile store - may get to this today)
- Tile store eviction/drawing improvements (the important part of this patch)

It'd also be nice to sort out the issues with reusable tile store and UseIntermediateSurface at some point, but I don't think that's important as any of the above.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +399,5 @@
>      resolution.width *= metrics.mResolution.width;
>      resolution.height *= metrics.mResolution.height;
>    }
> +  // If the resolution has changed, discard all the old tiles.
> +  // They will end up being retained on the shadow side by ReusableTileStoreOGL

We can't rely on this for resolution changes without changes to ReusableTileStoreOGL - It doesn't work for fixed-position layers, for example, and it also relies on layers not getting recreated.

I think a better idea for progressive rendering on zoom is to set mFirstPaint = true so that the screen-intersecting tiles are drawn in one shot and the rest are drawn progressively. We'd also have to ignore cancelling while drawing the screen-intersecting tiles in this case (or get the front-end not to cancel, but we have more relevant context to make this decision here).

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ -80,4 @@
>      }
>  
> -    // If the tile region wasn't contained within the valid region, check if
> -    // it intersects with the currently rendered region.

This check is important - We need to remove tiles that are within the display-port but outside of the visible bounds, or layers that shrink to a size smaller than the display-port will end up drawing invalid content.

@@ +49,5 @@
>  
> +    // Check if the tile region is contained within the new valid region.
> +    if (aValidRegion.Contains(tileRect)) {
> +      // Currently doesn't work well with progressive draw
> +      release = false;

Release is already false, should this be true? This code looks like tiles will never be released as a result of not being useful/coherent?

@@ +151,5 @@
> +            // XXX Perhaps we should check the region instead of the origin
> +            //     so a partial tile doesn't replace a full older tile?
> +            if (aVideoMemoryTiledBuffer->RoundDownToTileEdge(mTiles[i]->mTileOrigin.x) == aVideoMemoryTiledBuffer->RoundDownToTileEdge(x) &&
> +                aVideoMemoryTiledBuffer->RoundDownToTileEdge(mTiles[i]->mTileOrigin.y) == aVideoMemoryTiledBuffer->RoundDownToTileEdge(y) &&
> +                abs(mTiles[i]->mResolution.width - aOldResolution.width) < 1e-5) {

Don't we have a FuzzyEquals macro in gfx?

@@ +194,5 @@
> +  while (mTiles.Length() > aVideoMemoryTiledBuffer->GetTileCount() * mSizeLimit &&
> +         mTiles.Length() > MAX_HI_TILES + MAX_LOW_TILES) {
> +    int index = mTiles.Length() - 1 - lowTiles - hiTiles;
> +    ReusableTiledTextureOGL* tile = mTiles[index];
> +    if (tile->mResolution.width >= 1.0) {

It took me a little while to get my head around this loop, I think a comment here would help.

@@ +199,5 @@
> +      if (hiTiles < MAX_HI_TILES) {
> +        hiTiles++;
> +        continue;
> +      }
> +      printf_stderr("Prune hi tile %i\n", hiTiles);

printf_stderr not wrapped in a #define

@@ +205,5 @@
> +      if (lowTiles < MAX_LOW_TILES) {
> +        lowTiles++;
> +        continue;
> +      }
> +      printf_stderr("Prune low tile %i\n", lowTiles);

Same

@@ +238,5 @@
>    // scrollable child, in conjunction with its content area and viewport offset
>    // to establish the screen coordinates to which the content area will be
>    // rendered.
> +  gfxRect contentBounds;
> +  gfx::Point scrollOffset;

Why is scrollOffset moved out here?

@@ +244,5 @@
>    for (ContainerLayer* parent = aLayer->GetParent(); parent; parent = parent->GetParent()) {
>        const FrameMetrics& parentMetrics = parent->GetFrameMetrics();
>        if (parentMetrics.IsScrollable())
>          scrollableLayer = parent;
> +      // REVIEW do we still need to check if mDisplayPort is empty? Does that imply that metrics.mContentRect is empty?

Checking if mDisplayPort is not empty checks that there's a display-port set - we want the first scrollable descendant of the nearest display-port. Realistically, this is GetPrimaryScrollableLayer, but there is the possibility it may not be.

If we're not a descendant of the primary scrollable layer, we likely don't want to have a tile store...

@@ +255,5 @@
>            float scaleY = rootTransform.GetYScale();
>  
>            // Get the content document bounds, in screen-space.
>            const FrameMetrics& metrics = scrollableLayer->GetFrameMetrics();
> +          const nsIntSize& contentSize = metrics.mCompositionBounds.Size();

I understand the change to mCompositionBounds, but the variable names and comments need to change or this is very confusing. If the composition bounds happens to be empty, it might be good to try to fall back to the content bounds. I also wonder if the composition bounds can be larger than the document bounds? In which case, we'd want to clip to the document bounds... I don't think this should happen in Firefox for Android, but perhaps b2g is different? It wouldn't hurt to check.

@@ +292,4 @@
>      nsIntRegion transformedValidRegion(aValidRegion);
>      if (aResolution != tile->mResolution)
>        transformedValidRegion.ScaleRoundOut(1.0f/scaleFactor.width,
> +                                          1.0f/scaleFactor.height);

Bad alignment change.

@@ +296,4 @@
>      nsIntRegion tileRegion;
>      tileRegion.Sub(tile->mTileRegion, transformedValidRegion);
>  
> +    // Transform the display-port from screen space to layer space.

We should stick with the same nomenclature as the variables we're using from FrameMetrics.

@@ +300,2 @@
>      // Intersect the tile region with the content area.
> +    gfx3DMatrix transformInverse = transform.Inverse();

Why has this been moved outside of the block below? Now we'd calculate the inverse even if we aren't going to use it.

@@ +314,5 @@
> +    }
> +
> +    // This code doesn't handle tile with different resolution
> +    // properly so we don't optimize away over drawing the same area
> +    // by multiple tiles yet.

I think a feasible way of doing this would be when we're drawing tiles, add each tile we draw to a list and each area it draws to a region. Make sure to clip to that region when drawing so we don't draw overlapping tiles, and also afterwards, remove any tiles that are contained in that region and not in the list.

This doesn't handle resolution prioritisation though - I'm not sure what resolution we'd want to favour tiles in... Perhaps the nearest to the layer resolution? We could sort by our favoured resolution after adding tiles (I'd assume that changing resolution will always result in adding tiles) and that would fix that.

@@ +341,5 @@
>      nsIntSize textureSize(tile->mTileSize, tile->mTileSize);
> +    aLayer->RenderTile(tile->mTexture, transform, aRenderOffset, tileRegion,
> +                       tileOffset, textureSize, aMaskLayer);
> +
> +    // FIXME properly handle scaleFactor

Looks like you were having the add-to-region idea here, in fact :)

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +135,5 @@
>                    const gfx3DMatrix& aTransform,
>                    const nsIntPoint& aOffset,
> +                  const nsIntRegion& aScreenRegion,
> +                  const nsIntPoint& aTextureOffset,
> +                  const nsIntSize& aTextureBounds,

Nice catch with these.

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 675470 [details] [diff] [review]
patch

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

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ -80,4 @@
>      }
>  
> -    // If the tile region wasn't contained within the valid region, check if
> -    // it intersects with the currently rendered region.

You're right. I'll restore this.

@@ +49,5 @@
>  
> +    // Check if the tile region is contained within the new valid region.
> +    if (aValidRegion.Contains(tileRect)) {
> +      // Currently doesn't work well with progressive draw
> +      release = false;

That's right. Removing tiles in the valid area wasn't helping much and was making debugging harder. We certainly do need to restore the code you mention. I don't think removing tiles from the valid region is very useful as they will be replaced properly now.

@@ +151,5 @@
> +            // XXX Perhaps we should check the region instead of the origin
> +            //     so a partial tile doesn't replace a full older tile?
> +            if (aVideoMemoryTiledBuffer->RoundDownToTileEdge(mTiles[i]->mTileOrigin.x) == aVideoMemoryTiledBuffer->RoundDownToTileEdge(x) &&
> +                aVideoMemoryTiledBuffer->RoundDownToTileEdge(mTiles[i]->mTileOrigin.y) == aVideoMemoryTiledBuffer->RoundDownToTileEdge(y) &&
> +                abs(mTiles[i]->mResolution.width - aOldResolution.width) < 1e-5) {

I looked and it was all private.

@@ +238,5 @@
>    // scrollable child, in conjunction with its content area and viewport offset
>    // to establish the screen coordinates to which the content area will be
>    // rendered.
> +  gfxRect contentBounds;
> +  gfx::Point scrollOffset;

I was planning on using it in this scope when fixing up the culling but ended up not needing it.

@@ +244,5 @@
>    for (ContainerLayer* parent = aLayer->GetParent(); parent; parent = parent->GetParent()) {
>        const FrameMetrics& parentMetrics = parent->GetFrameMetrics();
>        if (parentMetrics.IsScrollable())
>          scrollableLayer = parent;
> +      // REVIEW do we still need to check if mDisplayPort is empty? Does that imply that metrics.mContentRect is empty?

If we don't want to have a tile store we should have the TiledThebesLayerOGL detect that and delete this.

@@ +300,2 @@
>      // Intersect the tile region with the content area.
> +    gfx3DMatrix transformInverse = transform.Inverse();

I'll restore this.

@@ +314,5 @@
> +    }
> +
> +    // This code doesn't handle tile with different resolution
> +    // properly so we don't optimize away over drawing the same area
> +    // by multiple tiles yet.

I already have code to keep track of the region we have drawn and remove it but it only works if all the tiles have the same resolution. I plan on fixing this next week.

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +135,5 @@
>                    const gfx3DMatrix& aTransform,
>                    const nsIntPoint& aOffset,
> +                  const nsIntRegion& aScreenRegion,
> +                  const nsIntPoint& aTextureOffset,
> +                  const nsIntSize& aTextureBounds,

I'm going to land these in a separate patch to get these changes in now so we can take our time to review this patch.

Comment 34

[Chris Lord [:cwiiis]]

Attachment: WIP: Fix ReusableTileStore

Just uploading this here for reference - This mostly fixes things that were broken in ReusableTileStore, some due to changes in FrameMetrics, others due to progressive tiles breaking some assumptions it made.

With this, I end up retaining tiles, but there are quite a few gaps in content - will fix this properly over the weekend probably.

Comment 35

[Benoit Girard (:BenWa)]

I feel like this overlap a bit with the chances I'm doing. I'll work on handling memory pressure and we can sync back on monday.

Comment 36

[Chris Lord [:cwiiis]]

Attachment: Fix ReusableTileStoreOGL

This incorporates the 'replace tiles in the same position' part of BenWa's patch. It also does LRU, but differently (I didn't realise that patch had that while I was writing).

This pretty much restores ReusableTileStoreOGL to its old behaviour, I think, and also ups the tile limit. The tile limit is calculated based on the visible region size rather than the number of tiles, so isn't liable to be broken by a progressive update with no stale content.

I haven't tested this extensively, but it does basically seem to work. You can still get gaps in content, but this is mostly unavoidable without taking extra care specifically not to do this.

I'd appreciate any extra testing, I think it's worth running with this patch for a day or so to see if there's anything obviously broken.

Comment 37

[Chris Lord [:cwiiis]]

Comment on attachment 676165 [details] [diff] [review]
Fix ReusableTileStoreOGL

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

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +347,1 @@
>        // Transform the content bounds from screen space to layer space.

I've fixed this comment in my local patch, just noticed it.

Comment 38

[Benoit Girard (:BenWa)]

Comment on attachment 676165 [details] [diff] [review]
Fix ReusableTileStoreOGL

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

r+ with these 2 things fixed.

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +237,5 @@
> +  // Calculate the maximum number of tiles we should have. We base this on the
> +  // number of tiles it would take to cover the visible region.
> +  uint32_t maxTiles = 0;
> +  while (!visibleRegion.IsEmpty()) {
> +    nsIntRegionRectIterator it(aLayer->GetEffectiveVisibleRegion());

visibleRegion

@@ +315,3 @@
>            break;
>        }
> +      parentResolution.width /= parentMetrics.mResolution.width;

You're not using these. Copy and paste error?

@@ +353,5 @@
>      }
>  
>      // If the tile region is empty, skip drawing.
> +    if (tileRegion.IsEmpty()) {
> +      reorderedTiles.InsertElementAt(lastOldTile++, mTiles[i].forget());

I'm a bit worried we might destroy this object. Let me check as I'm not familiar with nsAutoPtr. Ok it nulls out mTiles[i] and returns the pointer so we should be fine.

Comment 39

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #38)
> Comment on attachment 676165 [details] [diff] [review]
> Fix ReusableTileStoreOGL
> 
> Review of attachment 676165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with these 2 things fixed.
> 
> ::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
> @@ +237,5 @@
> > +  // Calculate the maximum number of tiles we should have. We base this on the
> > +  // number of tiles it would take to cover the visible region.
> > +  uint32_t maxTiles = 0;
> > +  while (!visibleRegion.IsEmpty()) {
> > +    nsIntRegionRectIterator it(aLayer->GetEffectiveVisibleRegion());
> 
> visibleRegion

Right, shocked I didn't immediately notice this - ends up non-rectangular visible regions are quite rare - did manage to hit it occasionally when navigating away from a site after further testing.

> @@ +315,3 @@
> >            break;
> >        }
> > +      parentResolution.width /= parentMetrics.mResolution.width;
> 
> You're not using these. Copy and paste error?

Legacy from earlier versions, nice catch.

Comment 40

[Chris Lord [:cwiiis]]

Argh, noticed this is missing a bit of logic for the intermediate surface case, I must've lost track half-way through fixing it... Will attach an updated patch soon.

Comment 41

[Chris Lord [:cwiiis]]

Actually, not nearly as bad as I thought, no bother - only change is to remove the break so that the transform is calculated correctly iterating down to the root layer. Don't think this needs any extra review as the code didn't make sense with the break in it - will push.

Comment 42

[Chris Lord [:cwiiis]]

Corrected and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a1b4cc15cc

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73a1b4cc15cc

Comment 44

[Benoit Girard (:BenWa)]

Marking has fixed since we landed some improvements but wont do any more since tile store has been replaced.