Improve the Reusable Tile Store eviction strategy

RESOLVED FIXED

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

24.75 KB, patch
cwiiis
: review-
Details | Diff | Splinter Review
21.49 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 667700 [details] [diff] [review]
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.
(Assignee)

Comment 2

6 years ago
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

6 years ago
(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...
(Assignee)

Comment 4

6 years ago
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.

Updated

6 years ago
Blocks: 795259
(Assignee)

Comment 6

6 years ago
Created attachment 670458 [details] [diff] [review]
Part 1:New tiles will replace equivilent tiles
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #670458 - Flags: review?(chrislord.net)

Comment 7

6 years ago
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.
Attachment #670458 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 670494 [details] [diff] [review]
New tile store tiless will replace equivalent tiles. r=Cwiiis

Forgot to delete the texture
Attachment #670458 - Attachment is obsolete: true
Attachment #670494 - Flags: review+
(Assignee)

Comment 9

6 years ago
Created attachment 670495 [details] [diff] [review]
New tile store tiless will replace equivalent tiles. r=Cwiiis
Attachment #670494 - Attachment is obsolete: true
Attachment #670495 - Flags: review+
(Assignee)

Comment 10

6 years ago
Created attachment 670961 [details] [diff] [review]
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.
Attachment #670495 - Attachment is obsolete: true
Attachment #670961 - Flags: review?(chrislord.net)
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.
Attachment #670961 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 671478 [details] [diff] [review]
Keep tiles when changing resolution
Attachment #667700 - Attachment is obsolete: true
Attachment #671478 - Flags: review?(chrislord.net)
(Assignee)

Comment 14

6 years ago
Created attachment 671480 [details] [diff] [review]
Keep tiles when changing resolution
Attachment #671478 - Attachment is obsolete: true
Attachment #671478 - Flags: review?(chrislord.net)
Attachment #671480 - Flags: review?(chrislord.net)
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.
Attachment #671480 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 16

6 years ago
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.
(Assignee)

Comment 17

6 years ago
Created attachment 671538 [details] [diff] [review]
Keep tiles when changing resolution
Attachment #671480 - Attachment is obsolete: true
Attachment #671538 - Flags: review+

Updated

6 years ago
Depends on: 802143
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.
Whiteboard: [leave-open]
(Assignee)

Comment 22

6 years ago
Depends on bug 795674. With the change in atachment 671705 we no longer lose the layer.
Depends on: 795674
(Assignee)

Comment 23

6 years ago
I need bug 802143 to land before I can reland part 1/2 without regressing mochitest.
(Assignee)

Comment 25

6 years ago
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.
(Assignee)

Comment 26

6 years ago
Created attachment 672946 [details] [diff] [review]
WIP

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

Updated

6 years ago
No longer blocks: 795259
(Assignee)

Updated

6 years ago
Depends on: 804801
(Assignee)

Comment 27

6 years ago
Created attachment 674661 [details] [diff] [review]
Updated WIP

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

Comment 28

6 years ago
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.
(Assignee)

Comment 29

6 years ago
Created attachment 675470 [details] [diff] [review]
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.
Attachment #670961 - Attachment is obsolete: true
Attachment #671538 - Attachment is obsolete: true
Attachment #672946 - Attachment is obsolete: true
Attachment #674661 - Attachment is obsolete: true
Attachment #675470 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #675470 - Flags: review? → review?(chrislord.net)
(Assignee)

Comment 30

6 years ago
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?
(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 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.
Attachment #675470 - Flags: review?(chrislord.net) → review-
(Assignee)

Comment 33

6 years ago
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.
Created attachment 675615 [details] [diff] [review]
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.
(Assignee)

Comment 35

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 805907
(Assignee)

Updated

6 years ago
Depends on: 805909
Blocks: 792131
Created attachment 676165 [details] [diff] [review]
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.
Attachment #675615 - Attachment is obsolete: true
Attachment #676165 - Flags: review?(bgirard)
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.
(Assignee)

Comment 38

6 years ago
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.
Attachment #676165 - Flags: review?(bgirard) → review+
(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.
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.
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.
(Assignee)

Comment 44

5 years ago
Marking has fixed since we landed some improvements but wont do any more since tile store has been replaced.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.