There is excessive checkerboarding when panning, especially on single-core devices

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
Firefox 11
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

6 years ago
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.
Attachment #580932 - Flags: review?(pwalton)
(Assignee)

Comment 3

6 years ago
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.
Attachment #580934 - Flags: review?(pwalton)
(Assignee)

Comment 4

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

Comment 8

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

Updated

6 years ago
Depends on: 708307
(Assignee)

Comment 10

6 years ago
Created attachment 582242 [details] [diff] [review]
1 - Union dirty rectangles in the compositor (revised, rebased)
Attachment #580931 - Attachment is obsolete: true
Attachment #582242 - Flags: review+
(Assignee)

Comment 11

6 years ago
Created attachment 582244 [details] [diff] [review]
2 - Coalesce multiple viewport updates (revised, rebased)
Attachment #580932 - Attachment is obsolete: true
Attachment #582244 - Flags: review+
(Assignee)

Comment 12

6 years ago
Created attachment 582245 [details] [diff] [review]
3 - Add a bias in the scroll direction for the viewport offset
Attachment #580934 - Attachment is obsolete: true
Attachment #582245 - Flags: review+
(Assignee)

Comment 13

6 years ago
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.
Status: NEW → ASSIGNED

Comment 14

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

Comment 15

6 years ago
Ignore that try, I pushed on a bad revision... Another one incoming.

Comment 16

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

Comment 17

6 years ago
And again, pushed on a busted revision... My Mercurial foo is failing me, will get there eventually.

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 18

6 years ago
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.
Attachment #582242 - Attachment is obsolete: true
Attachment #582769 - Flags: review?(pwalton)
Comment on attachment 582769 [details] [diff] [review]
1 - Union dirty rectangles in the compositor (revised again)

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

r+
Attachment #582769 - Flags: review?(pwalton) → review+
(Assignee)

Comment 20

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

Comment 21

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

Comment 22

6 years ago
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.
Attachment #582245 - Attachment is obsolete: true
Attachment #583523 - Flags: review+
Duplicate of this bug: 710510
(Assignee)

Comment 24

6 years ago
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.
Whiteboard: [leave open after inbound merge]
Target Milestone: --- → Firefox 12
Attachment #582244 - Flags: approval-mozilla-aurora?
Attachment #582769 - Flags: approval-mozilla-aurora?
Attachment #583523 - Flags: approval-mozilla-aurora?
If all goes well on trunk, we'll want these on aurora
https://hg.mozilla.org/mozilla-central/rev/28a81df3d02b
https://hg.mozilla.org/mozilla-central/rev/943ac1d43ad4
https://hg.mozilla.org/mozilla-central/rev/f75ee6fa2587
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.
Attachment #582244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #582769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #583523 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Duplicate of this bug: 710510
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?
status-firefox11: --- → fixed
Target Milestone: Firefox 12 → Firefox 11
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

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

Comment 32

6 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [leave open after inbound merge]
tracking-fennec: --- → 11+
Depends on: 716581
You need to log in before you can comment on or make changes to this bug.