Last Comment Bug 709152 - There is excessive checkerboarding when panning, especially on single-core devices
: There is excessive checkerboarding when panning, especially on single-core de...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: Firefox 11
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
: 710510 (view as bug list)
Depends on: 708307 716581
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-09 09:38 PST by Chris Lord [:cwiiis]
Modified: 2016-07-29 14:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
1 - Union dirty rectangles in the compositor (4.66 KB, patch)
2011-12-12 08:56 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Splinter Review
2 - Coalesce multiple viewport updates (9.42 KB, patch)
2011-12-12 08:58 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Splinter Review
3 - Add a bias in the scroll direction for the viewport offset (8.01 KB, patch)
2011-12-12 09:01 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Splinter Review
1 - Union dirty rectangles in the compositor (revised, rebased) (3.74 KB, patch)
2011-12-16 04:32 PST, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Splinter Review
2 - Coalesce multiple viewport updates (revised, rebased) (8.49 KB, patch)
2011-12-16 04:33 PST, Chris Lord [:cwiiis]
chrislord.net: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
3 - Add a bias in the scroll direction for the viewport offset (8.57 KB, patch)
2011-12-16 04:34 PST, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Splinter Review
1 - Union dirty rectangles in the compositor (revised again) (5.61 KB, patch)
2011-12-19 02:12 PST, Chris Lord [:cwiiis]
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
3 - Add a bias in the scroll direction for the viewport offset (fix too-new API usage) (8.72 KB, patch)
2011-12-21 08:55 PST, Chris Lord [:cwiiis]
chrislord.net: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2011-12-09 09:38:30 PST
This is hard to quantify and probably requires a multi-part solution, but there is a lot of checkerboarding when panning, especially after fixing bug #705092 and on single-core devices.

Ideally, I think we should have minimal/no checkerboarding when panning quickly downwards on a page like engadget.com.
Comment 1 Chris Lord [:cwiiis] 2011-12-12 08:56:40 PST
Created attachment 580931 [details] [diff] [review]
1 - Union dirty rectangles in the compositor

It's a rare situation that if Gecko can update fast enough to send us two draw updates, we want to handle them as two separate uploads, especially if we're widening all sub-image uploads to the entire width of the buffer.

This patch replaces mDirtyRects with mDirtyRect, and squashes multiple sub-image uploads into a single upload. This applies over the patch in bug #708307
Comment 2 Chris Lord [:cwiiis] 2011-12-12 08:58:36 PST
Created attachment 580932 [details] [diff] [review]
2 - Coalesce multiple viewport updates

If we send viewport updates to Gecko fast enough that it has multiple updates in its event queue, we only really want to handle the last one (as it's the most up-to-date). This patch splits viewport updates into their own separate event, and coalesces them in nsAppShell, in the same way that draw events are coalesced.
Comment 3 Chris Lord [:cwiiis] 2011-12-12 09:01:40 PST
Created attachment 580934 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset

This patch biases the viewport offset to buffer more area in the direction that is being scrolled. It also fixes the logic of making sure the displayport stays within the page boundaries.

It would be great to see bug #524925 fixed for this, but I see marked improvement, even considering that bug. With this in mind, I've made some concessions to not change the viewport offset too often.
Comment 4 Chris Lord [:cwiiis] 2011-12-12 09:03:24 PST
Note that these three patches help the problem, but do not entirely alleviate it.

We could still really do with faster texture upload, having bug #524925 fixed, and just some better metrics to help us tune our constants to achieve best performance on an average Android device.
Comment 5 Patrick Walton (:pcwalton) 2011-12-12 10:47:25 PST
Comment on attachment 580931 [details] [diff] [review]
1 - Union dirty rectangles in the compositor

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

r+ with comments

::: mobile/android/base/gfx/TileLayer.java
@@ +55,5 @@
>   */
>  public abstract class TileLayer extends Layer {
>      private static final String LOGTAG = "GeckoTileLayer";
>  
> +    private final Rect mDirtyRect;

nit: import java.util.ArrayList; is no longer necessary and can be removed.

@@ +133,5 @@
>          // Reallocate the texture if the size has changed
>          validateTexture(gl);
>  
> +        // Don't do any work if the image has an invalid size or there's
> +        // to upload.

nit: "there's nothing to upload", I assume

@@ +134,5 @@
>          validateTexture(gl);
>  
> +        // Don't do any work if the image has an invalid size or there's
> +        // to upload.
> +        if (mDirtyRect.isEmpty() || mImage.getSize().equals(new IntSize(0, 0)))

Maybe add this as a method to IntSize? It seems kinda wasteful to be allocating a new size here.
Comment 6 Patrick Walton (:pcwalton) 2011-12-12 10:54:16 PST
Comment on attachment 580932 [details] [diff] [review]
2 - Coalesce multiple viewport updates

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

So overall I like this idea, but I almost feel like the way we squash draw (and now viewport) events is a violation of the semantics of event queuing generally. I'd like to see what happens if we make draw and viewport change not events but rather globals in widget that get updated. Whenever it's the case that there's something to draw or a viewport to be updated, those take priority over all other events.

This doesn't have to be done now, it can be left to a followup (and I'm not totally sure it will work).

::: widget/src/android/nsAppShell.cpp
@@ +472,5 @@
>          if (ae->Type() == AndroidGeckoEvent::DRAW) {
>              if (--mNumDraws == 0)
>                  mLastDrawEvent = nsnull;
> +        } else if (ae->Type() == AndroidGeckoEvent::VIEWPORT) {
> +            mNumViewports --;

nit: no space before --
Comment 7 Patrick Walton (:pcwalton) 2011-12-12 10:58:09 PST
Comment on attachment 580934 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset

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

I don't see where the bias is being set -- wouldn't the PanZoomController have to be involved somehow?

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +122,5 @@
>          //  as much as possible)
>          if (viewport.left - optimumOffset.x < 0)
>            optimumOffset.x = viewport.left;
> +        else if ((bufferSpace.width - optimumOffset.x) + viewport.right > mPageSize.width)
> +          optimumOffset.x = bufferSpace.width - (int)(mPageSize.width - viewport.right);

Why truncation instead of rounding?
Comment 8 Chris Lord [:cwiiis] 2011-12-12 11:04:33 PST
(In reply to Patrick Walton (:pcwalton) from comment #7)
> Comment on attachment 580934 [details] [diff] [review]
> 3 - Add a bias in the scroll direction for the viewport offset
> 
> Review of attachment 580934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see where the bias is being set -- wouldn't the PanZoomController
> have to be involved somehow?

It is, by calling LayerController.scrollBy, which in turn calls ViewportMetrics.setOrigin. Whenever the origin is moved, the bias changes and the bias is used in the viewport offset next time a viewport adjustment is set.

> ::: mobile/android/base/gfx/ViewportMetrics.java
> @@ +122,5 @@
> >          //  as much as possible)
> >          if (viewport.left - optimumOffset.x < 0)
> >            optimumOffset.x = viewport.left;
> > +        else if ((bufferSpace.width - optimumOffset.x) + viewport.right > mPageSize.width)
> > +          optimumOffset.x = bufferSpace.width - (int)(mPageSize.width - viewport.right);
> 
> Why truncation instead of rounding?

Mistake from legacy, well-spotted.
Comment 9 Patrick Walton (:pcwalton) 2011-12-13 11:43:49 PST
Comment on attachment 580934 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset

Oh, I see about the bias setting. Clever. Could you add a comment to the ViewportMetrics when the bias is being set explaining the series of events that lead to the bias calculation? r+ with that.
Comment 10 Chris Lord [:cwiiis] 2011-12-16 04:32:23 PST
Created attachment 582242 [details] [diff] [review]
1 - Union dirty rectangles in the compositor (revised, rebased)
Comment 11 Chris Lord [:cwiiis] 2011-12-16 04:33:39 PST
Created attachment 582244 [details] [diff] [review]
2 - Coalesce multiple viewport updates (revised, rebased)
Comment 12 Chris Lord [:cwiiis] 2011-12-16 04:34:33 PST
Created attachment 582245 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset
Comment 13 Chris Lord [:cwiiis] 2011-12-16 04:36:43 PST
Being a bit more cautious now, so I'll land these when a try run shows that it doesn't cause crashing. Works fine for me though.
Comment 14 Mozilla RelEng Bot 2011-12-16 06:10:45 PST
Try run for 138b0539cd7f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=138b0539cd7f
Results (out of 26 total builds):
    exception: 6
    success: 1
    warnings: 13
    failure: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/chrislord.net@gmail.com-138b0539cd7f
Comment 15 Chris Lord [:cwiiis] 2011-12-16 06:38:53 PST
Ignore that try, I pushed on a bad revision... Another one incoming.
Comment 16 Mozilla RelEng Bot 2011-12-16 07:10:34 PST
Try run for 295a94fbb8a9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=295a94fbb8a9
Results (out of 25 total builds):
    exception: 7
    success: 1
    warnings: 11
    failure: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/chrislord.net@gmail.com-295a94fbb8a9
Comment 17 Chris Lord [:cwiiis] 2011-12-16 07:18:59 PST
And again, pushed on a busted revision... My Mercurial foo is failing me, will get there eventually.
Comment 18 Chris Lord [:cwiiis] 2011-12-19 02:12:31 PST
Created attachment 582769 [details] [diff] [review]
1 - Union dirty rectangles in the compositor (revised again)

This patch was causing failures, I've revised it a bit and a try run completes green now (https://tbpl.mozilla.org/?tree=Try&rev=e63240c9018b). Worth getting a very quick review in again I think.

The bit that was causing failures was the code that discards an upload if the dirty rect is empty. It wasn't properly considering the case when the image size changes and the texture is recreated. Even correcting that mistake, however, resulted in a single failure, so I've just left this behaviour as it was before the patch.

If we have an empty dirty rect and are asked to do an upload, maybe it's just better to err on the side of caution and upload everything anyway.
Comment 19 Patrick Walton (:pcwalton) 2011-12-19 23:01:38 PST
Comment on attachment 582769 [details] [diff] [review]
1 - Union dirty rectangles in the compositor (revised again)

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

r+
Comment 20 Chris Lord [:cwiiis] 2011-12-20 02:16:04 PST
This is proving to be much trickier than envisioned - Patch 1 is fine, but patch 3 causes all tests to fail and patch 2 causes a couple of failures in xul fennec. Looking into them. Patch 3 is the most important of all of these patches I think, so I'll concentrate on getting this working first.
Comment 21 Chris Lord [:cwiiis] 2011-12-20 11:32:13 PST
Just to add, patch 2 is fine, the failures were random and rebuilding them resulted in success: https://tbpl.mozilla.org/?tree=Try&rev=0814720aa34c
Comment 22 Chris Lord [:cwiiis] 2011-12-21 08:55:36 PST
Created attachment 583523 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset (fix too-new API usage)

Oh, the problem was the use of Math.copySign I believe - I've fired off another try run, will commit this if it's green.
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2011-12-22 09:23:52 PST
*** Bug 710510 has been marked as a duplicate of this bug. ***
Comment 24 Chris Lord [:cwiiis] 2011-12-23 02:05:18 PST
Whoops, forgot to hit post yesterday... Pretend the following is written a day before the date listed.

Finally, all green: https://tbpl.mozilla.org/?tree=Try&rev=cdcb2d8e9134 (separate try runs show it doesn't break xul-fennec either)

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/28a81df3d02b
http://hg.mozilla.org/integration/mozilla-inbound/rev/943ac1d43ad4
http://hg.mozilla.org/integration/mozilla-inbound/rev/f75ee6fa2587

Note, these only help with checkerboarding. I would consider that this bug still exists even with these patches, so this shouldn't be closed just yet.
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-23 18:57:26 PST
If all goes well on trunk, we'll want these on aurora
Comment 27 Alex Keybl [:akeybl] 2011-12-26 11:06:54 PST
Comment on attachment 582244 [details] [diff] [review]
2 - Coalesce multiple viewport updates (revised, rebased)

[Triage Comment]
Haven't heard of any issues, just tested on nightly myself. Let's land these on Aurora and get the fix out to testers.
Comment 28 Alex Keybl [:akeybl] 2011-12-26 11:10:53 PST
*** Bug 710510 has been marked as a duplicate of this bug. ***
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-27 11:17:46 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc90db2bca9
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d642175458d
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce658a689f5d

With the complexity of tracking in m-c and m-a, I think we should file a new bug instead of leaving this one open. But honestly, I don't know what these patches fixed versus what the bug was originally about.

Chris, you wanted this bug left open. Can you file a new bug to handle the remaining issues and close this bug?
Comment 30 Aaron Train [:aaronmt] 2011-12-28 07:25:15 PST
Need a way for QA to verify this. Should we target Adreno 200/Qualcomm -- QSD 8250 Snapdragon devices? (i.e, Nexus One), and a URL (i.e, timecube.com) ?
Comment 31 Manoj 2012-01-03 16:03:44 PST
(In reply to Mark Finkle (:mfinkle) from comment #29)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc90db2bca9
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2d642175458d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/ce658a689f5d
> 
> With the complexity of tracking in m-c and m-a, I think we should file a new
> bug instead of leaving this one open. But honestly, I don't know what these
> patches fixed versus what the bug was originally about.
> 
> Chris, you wanted this bug left open. Can you file a new bug to handle the
> remaining issues and close this bug?

This blog post might answer the question about what this patch fixes and what the bug was originally about: http://masalalabs.ca/2012/01/measuring-reduced-checkerboarding-in-mobile-fennec/
Comment 32 Chris Lord [:cwiiis] 2012-01-04 06:43:58 PST
(In reply to Manoj from comment #31)
> (In reply to Mark Finkle (:mfinkle) from comment #29)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc90db2bca9
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/2d642175458d
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/ce658a689f5d
> > 
> > With the complexity of tracking in m-c and m-a, I think we should file a new
> > bug instead of leaving this one open. But honestly, I don't know what these
> > patches fixed versus what the bug was originally about.
> > 
> > Chris, you wanted this bug left open. Can you file a new bug to handle the
> > remaining issues and close this bug?
> 
> This blog post might answer the question about what this patch fixes and
> what the bug was originally about:
> http://masalalabs.ca/2012/01/measuring-reduced-checkerboarding-in-mobile-
> fennec/

Cool, I hadn't seen this blog post so it's great to see this has measurably helped things. Personally, I still think the amount of checkerboarding we have is unacceptable, but if it makes things easier to track, I'll close this and open a new bug when I have work that'll help things further.

Note You need to log in before you can comment on or make changes to this bug.