538571 - checkerboard appears after slow pan

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Fabrice Desré]

Here are the STR :

1) go to eg http://www.fsf.org/licensing/licenses/agpl-3.0.html
2) zoom in by double clicking
3) start a slow vertical pan, stop near the top of page without releasing the mouse/finger
4) wait a little bit (1/2 sec)

Result :
The page becomes a full checkerboard, even if it was perfectly displayed when the panning stopped. 
Releasing the mouse/finger leads to a repaint.

Expected result :
No checkerboard

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

From IRC:


[10:47am] vingtetun: fabrice: killing this part of the code seems to resolve the problem
[10:47am] fabrice: vingtetun, killing the idle crawler ?
[10:47am] vingtetun: can you try commenting/removing http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#2904
[10:47am] vingtetun: fabrice: yes
[10:48am] vingtetun: (that's just for testing and find something to patch)
[10:48am] vingtetun: i want to know if you also see the problem dissapear with that
[10:48am] fabrice: ok, let me check
[10:53am] fabrice: vingtetun, looks like commenting browser.js#2904 fixes the issue. I'm gonna take a look at this setAggressive() call

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Fabrice think that the problem came from here :
http://mxr.mozilla.org/mobile-browser/source/chrome/content/TileManager.js.in#368

Maybe the criticalRect is wrong.

Comment 3

[Benjamin Stover (:stechz)]

Nothing like that.  The last critical rect is actually released when you start a pan, so when idle crawler has a go at it the critical tiles can be evicted.

There's two things that would really help here:
1) No idle crawler during pan
2) Don't release those tiles until we've stopped panning (basically once the screen is repainted).

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> Nothing like that.  The last critical rect is actually released when you start
> a pan, so when idle crawler has a go at it the critical tiles can be evicted.
> 
> There's two things that would really help here:
> 1) No idle crawler during pan
> 2) Don't release those tiles until we've stopped panning (basically once the
> screen is repainted).

I pretty sure we should not do any idle crawling while we are panning.

Comment 5

[Benjamin Stover (:stechz)]

Attachment: Patch

WIP, for some reason idle crawler is not really queuing up tiles outside of viewport.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch based on stechz one

This patch seems to correct the issue for me, any other problems related to the TileManager and Idle Crawler should be tracked in an other bug i think

Comment 7

[Benjamin Stover (:stechz)]

Comment on attachment 420851 [details] [diff] [review]
Patch based on stechz one

Looks good!  I'm glad you toned this patch up :) I am going to make a case for a few things you took out.  Looking at the diff from first version to second version of patch:

> -      // XXX cacheSize still can be -1 even though new profile sets it to a positive value
> -      if (cacheSize == -1)
> -        cacheSize = kBrowserViewCacheSize;
> -      

Not related to checkerboarding, but my fennec would have infinite capacity and would eventually crash on me, every single time.  Perhaps we should create a new bug?

Either way, I feel this belongs in RC3.  Low risk, tons to gain.

> +  /**
> +   * @param dx Guess delta to destination x coordinate
> +   * @param dy Guess delta to destination y coordinate
> +   */
> +  onBeforeVisibleMove: function onBeforeVisibleMove(dx, dy) {
> +    let vs = this._browserViewportState;
> +    let vr = this.getVisibleRect();
> +
> +    let destCR = BrowserView.Util.visibleRectToCriticalRect(vr.translate(dx, dy), vs);
> +
> +    this._tileManager.beginCriticalMove(destCR);
> +  },

Good call.  I did get carried away with this.  With some profiling and talking with Roy, I think this is the right thing to do.  Eventually.

> -    } else {
> -      // The critical rect hasn't changed, but there are possibly more tiles lounging about offscreen,
> -      // waiting to be rendered. Make sure crawler knows about them.
> -      this.recenterCrawler();

This is more change, but recenterCrawler and its instances should be added back.  They help with checkerboarding.  For this snipplet, you can see checkerboarding with twitter or anything that grows--no matter how long you wait before panning.

>    dirtyRects: function dirtyRects(rects, doCriticalRender) {
> -    let outsideIsDirty = false;
>      let criticalIsDirty = false;
>      let criticalRect = this._criticalRect;
>      let tc = this._tileCache;
> @@ -275,88 +269,143 @@ 
>  
>        BEGIN_FOREACH_IN_RECT(rect, tc, tile)
>  
> - 
> -      if (!tile.boundRect.intersects(criticalRect)) {
> -        // Dirty tile outside of viewport. Just remove and redraw later.
> -        outsideIsDirty = true;
> +      if (!tile.boundRect.intersects(criticalRect))
>          this._removeTileSafe(tile);
> -      } else {
> +      else
>          criticalIsDirty = true;
> -        crawler.enqueue(tile.i, tile.j);
> -      }

I see my mistake with this code now.  The tile should be crawl-queued if (and really, only if) the tile is outside the bounding rectangle.  I have the opposite here.  This change should be kept, so that tiles in the critical region don't get queued in the crawler.

>  
>        tile.markDirty(rects[i]);
>  
> +      if (crawler)
> +        crawler.enqueue(tile.i, tile.j);
> +
>        END_FOREACH_IN_RECT
>      }
> 
>      if (criticalIsDirty && doCriticalRender)
>        this.criticalRectPaint();
> -    if (outsideIsDirty)
> -      this.restartPrefetchCrawl();
>    },
 
Again, should be added back.  Here, dirtyRects queues up newly dirtied rectangles but never does anything about it afterwards!  If the crawler has stopped offscreen dirty tiles will never be redrawn.  And then--checkerboard.

>    criticalRectPaint: function criticalRectPaint() {
>      let cr = this._criticalRect;
> -    let newCr = this._newCriticalRect;
>  
> -    if (!newCr.isEmpty()) {
> -      // This is the first paint since the last critical move.
> -      let tc = this._tileCache;
> -      BEGIN_FOREACH_IN_RECT(cr, tc, tile)
> -        tc.releaseTile(tile);
> -      END_FOREACH_IN_RECT
> +    //let start = Date.now();
>  
> -      cr.copyFrom(newCr);
> -      newCr.setRect(0, 0, 0, 0);
> +    if (!cr.isEmpty()) {
> +      let ctr = cr.center().map(Math.round);
>  
> -      this.recenterEvictionQueue(cr.center().map(Math.round));
> -      this.recenterCrawler();
> -    }
> +      if (!this._ctr.equals(ctr)) {
> +        this._ctr.set(ctr);
> +        this.recenterEvictionQueue(ctr);
> +      }
> +
> +      //let start = Date.now();
>  
> -    if (!cr.isEmpty())
>        this._renderAppendHoldRect(cr);
> +
> +      //dump("  render, append, hold: " + (Date.now() - start) + "\n");
> +    }
> +
> +    //dump(" paint: " + (Date.now() - start) + "\n");
> +  },

The change here and endCriticalMove does look scary!  You're right, let's leave this alone for now.

This code makes sure that the last known rendered critical rect is held until the new critical rect is rendered.  Because of the change in the dragging code, this is no longer a huge problem.  But it still can be a problem during loading.  Here's what can happen:

1) Page begins loading, first critical rect is drawn and held on to.
2) Idle timer begins
3) onAfterVisibleMove is called and the hold on critical rect is let go.  This can happen if sidebars are hidden, or a notification pops up, or many other things.  The new critical rect isn't immediately rerendered because of beginBatchOp during load.
4) Crawler jumps in and starts painting, evicting perfectly good currently-painted critical tiles in the process.

The change is risky enough and this particular "checkerboard surprise attack" seems rare enough that we should probably leave this change for Fennec.next.

> +    // XXX the conjunction with doCriticalPaint may cause tiles to disappear
> +    // (be evicted) during a (relatively slow) move as no tiles will be "held"
> +    // until a critical paint is requested.  Also, while we have this
> +    // && doCriticalPaint then we don't need this loop altogether, as
> +    // criticalRectPaint will hold everything for us (called below)

This is what I was talking about above.

> +  _prefetch: false,
>    setPrefetch: function setPrefetch(prefetch) {

This is fine for singletons like BrowserView, but normally member variables go in the constructor of objects.

> -  restartPrefetchCrawl: function restartPrefetchCrawl() {
> -    if (this._prefetch && !this._idleTileCrawlerTimeout) {
> +
> +  restartPrefetchCrawl: function restartPrefetchCrawl(startRectOrQueue) {
> +    if (startRectOrQueue instanceof Array) {
> +      this._crawler = new TileManager.CrawlIterator(this._tileCache);
> +
> +      if (startRectOrQueue) {
> +        for (let k = 0, len = startRectOrQueue.length; k < len; ++k)
> +          this._crawler.enqueue(startRectOrQueue[k].i, startRectOrQueue[k].j);
> +      }
> +    } else {
>        let cr = this._criticalRect;
> -      this._idleTileCrawlerTimeout = setTimeout(this._idleTileCrawler, kTileCrawlComeAgain, this);
> +      this._crawler = new TileManager.CrawlIterator(this._tileCache,
> +                                                    startRectOrQueue || (cr ? cr.clone() : null));
>      }
> -  },

We should keep most of this change.  This change ensures that when crawling is restarted, that _prefetch flag has been set to true.  And it removes all this startRectOrQueue junk that isn't used anymore.  At the very least keep the extra flag check.

> -  /** Crawler will recalculate the tiles it is supposed to fetch in the background. */
> -  recenterCrawler: function recenterCrawler() {
> -    let cr = this._criticalRect;
> -    this._crawler = new TileManager.CrawlIterator(this._tileCache, (cr.isEmpty() ? cr.clone() : null));

Keep recenterCrawl.  It's useful when for when the crawler needs to recalibrate but eviction queue is fine (for viewport change handler).

>    _idleTileCrawler: function _idleTileCrawler(self) {
> -    Util.dumpLn('IDLE');
>      if (!self) self = this;
>  
>      let start = Date.now();
> @@ -472,10 +520,9 @@ 
>          comeAgain = false;
>          break;
>        }
> -
> -      self._appendTileSafe(tile);
> -      self._renderTile(tile);
> -      Util.dumpLn(' ', tile);
> +      if (tile.isDirty()) {
> +        self._renderTile(tile);
> +      }

Keep appendTileSafe--otherwise the tile is just repainted and not in the DOM, completely unprepared for when the user pans.  beginCriticalRect could catch it, but this guarantees it's there.

tile.isDirty() check is part of _renderTile.  IMO it's sloppy to keep checking that.

Comment 8

[Benjamin Stover (:stechz)]

Attachment: The average of the above patches

Makes most of the changes I said above in the code review.  I think this covers the important things of my bigger patch in bug 538669: less checkerboarding, more careful about interfering with pans, no more infinite tile capacity.

What do you think?  Did any of my explanations above convince you some of the extra changes are worth having?

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I've to admit that I'm not very confortable with changing a lot the tile cache code before RC3, reading your comment in bug 538669 i understand why you want all these.

I hope to have my n900 back on monday, otherwise i will test that on n810 only where it can be hard to see the perfs improvements :(

Let me think about it and get times to do a full review/testing session on monday. (i hope on n900)

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

As far as I can see there is 4 differents stuffs in this patch:
 * Check the infinite tile capacity - fine for me in RC3, low risk

 * Correct this bug by allowing a "pause" to the tile crawling mechanism - fine for me in RC3

 * some code refactoring for old unuseful code (restartPrefectCrawl/stopPrefectCrawl) - it its unuseful let do it here for RC3


 * a patch to correct the behavior of dirty tiles outside viewport - Even if i'm fine with the code i would rather prefer to create an other bug and track this part of the code there. This way if there is any regression we can back it out and leave this bug resolved.
I can r+ the other bug if needed, but I will be more confident by tracking these 2 bugs with differents patches.

Comment 11

[Benjamin Stover (:stechz)]

> it its unuseful let do it here for RC3

OK.  There's not any reason this needs to land now.

> a patch to correct the behavior of dirty tiles outside viewport - Even if
i'm fine with the code i would rather prefer to create an other bug and track
this part of the code there

Sure, I can split that out into a new bug.  Maybe we can still get it for RC3?  It makes a big difference with seeing checkerboards.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Minimal changes for this bug

This fixes infinite tile capacity and ensures fetcher is stopped when panning.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 421086 [details] [diff] [review]
Minimal changes for this bug

nit: as seen on IRC, remove the infinite tile cache patch and track that in an other bug cause the bug is more serious than expected

Comment 14

[Roy Frostig :froystig]

Comment on attachment 421086 [details] [diff] [review]
Minimal changes for this bug

Not that Ben asked for this review, but I didn't get a chance to review the previous version that's now marked obsolete and wanted to mention these two.


>+  _updateTileManager: function _execute() {

Mismatching prototype property name and function name.


>   restartPrefetchCrawl: function restartPrefetchCrawl(startRectOrQueue) {
>     if (startRectOrQueue instanceof Array) {
>       this._crawler = new TileManager.CrawlIterator(this._tileCache);
> 
>       if (startRectOrQueue) {
>         for (let k = 0, len = startRectOrQueue.length; k < len; ++k)
>           this._crawler.enqueue(startRectOrQueue[k].i, startRectOrQueue[k].j);
>       }
>     } else {
>       let cr = this._criticalRect;
>       this._crawler = new TileManager.CrawlIterator(this._tileCache,
>-                                                    startRectOrQueue || (cr ? cr.clone() : null));
>+                                                    startRectOrQueue || (!cr.isEmpty() ? cr.clone() : null));
>     }

Yeah, at some more convenient point the "then"-block of this |if| code can go.  The function used to be called in different ways (sometimes queuing up an array of stuff and other times with an initial rectangle), but I don't think that's the case any longer.  Unnecessary complexity.

Comment 15

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/4114098a2af3

Comment 16

[Aakash Desai [:aakashd]]

I don't see the checkboarding with the testcase from the reporter's comments on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100113 Firefox/3.6pre Fennec/1.1a1pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20100113 Firefox/3.7a1pre Fennec/1.1a1pre

Other instances tried: slashdot.org, webhamster.com and yahoo.com