710533 - consider using background color fade-in rather than checkerboarding

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Alex Keybl [:akeybl]]

We should consider using a fast background color to web content fade-in rather than checkerboarding.

Comment 1

[Patrick Walton (:pcwalton)]

The checkerboard doesn't actually cost us any speed to render; it's all on the GPU.

Comment 2

[Alex Keybl [:akeybl]]

Do we think there would be a significant performance hit if we painted the page background color instead of the default GPU checkerboard?

I wonder if (time to paint bg color + short fade-in time) will actually do more for the user perception of smoothness/speed.

Comment 3

[Doug Turner (:dougt)]

if this is free either way, what does UX think.

Comment 4

[Matt Brubeck (:mbrubeck)]

In XUL fennec we switched from checkerboard to page background color in bug 694706.  In that case, it actually improved performance.

Comment 6

[Aaron Train [:aaronmt]]

Added comments to my dupe in dupe 711897. Stock browser on ICS does it pretty good with a fade in of content using the most dominant background colour as a transition .

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

P3 for Madhava to make a decision here, we'll reevaluate priority once he's weighed in.

Comment 8

[Patrick Walton (:pcwalton)]

Note that if we fade in we will pay a 4MB memory hit due to double buffering. I'm ok with that, because I want to pay that memory hit for other reasons (incremental off-thread texture upload), but we should be aware of this.

Comment 9

[Patrick Walton (:pcwalton)]

Note that if we pay the memory hit for off-thread texture upload, then it will cost us no additional memory to do fade-in, and it'll be a nice bit of polish. Eye candy, yes, but eye candy is delicious.

Also the fade-in question is separate from the question of whether we should display the checkerboard or the page background. We can fade in from a checkerboard as well, or we can display the page background without the fade-in.

Comment 10

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

This patch makes us draw a colored checkerboard by using the background color of the page for the "light" squares and the background color of the page mixed with 60% of halftone gray for the "dark" squares. It looks nice to me on dark pages like http://nightly.mozilla.org/ and matches what MobileSafari does.

Comment 11

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

Forgot to remove the old checkerboard.png resource (it's procedurally generated now so no longer needed).

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 588600 [details] [diff] [review]
Proposed patch, version 2.

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

I don't think that moving the checkerboard layer into LayerController is a good idea. r+ if it remains in LayerRenderer.

::: mobile/android/base/gfx/CheckerboardImage.java
@@ +45,5 @@
> +
> +/** A Cairo image that displays a tinted checkerboard. */
> +public class CheckerboardImage extends CairoImage {
> +    // The width and height of the checkerboard tile.
> +    private static final int SIZE = 16;

It would be nice if this could depend on screen dpi somewhat - the checkerboard looks ok on my Flyer, but terrible on the Galaxy Nexus (where 16x16 pixels is tiny). This is a separate bug though.

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +421,5 @@
>      }
> +
> +    // Parses a color from an RGB triple of the form "rgb([0-9]+, [0-9]+, [0-9]+)". If the color
> +    // cannot be parsed, returns white.
> +    private static int parseColor(String string) {

We should probably call this parseGeckoColor or something, to differentiate it from Android's Color.parseColor() function, to avoid someone mistaking this for a sign that Android doesn't have an alternative.

::: mobile/android/base/gfx/LayerController.java
@@ +106,5 @@
>          mPanZoomController = new PanZoomController(this);
>          mView = new LayerView(context, this);
> +
> +        mCheckerboardImage = new CheckerboardImage();
> +        mCheckerboardLayer = new SingleTileLayer(true, mCheckerboardImage);

I think these ought to live in LayerRenderer, there's too much context associated with rendering them that isn't located here. LayerController can have a getBackgroundColor function for LayerRenderer to call.

Comment 13

[Madhava Enros [:madhava]]

Would like to see it in action, but generally -- sounds good.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

Patch part 3 addresses review comments.

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 590097 [details] [diff] [review]
Proposed patch, version 3.

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

r- because of the return false issue - I'd like the rest addressed ideally, but some of it is probably a matter of opinion, so if it returns true, r+.

::: mobile/android/base/gfx/LayerController.java
@@ +349,5 @@
> +    /**
> +     * Retrieves the new color of the checkerboard, or null if the checkerboard is to remain the
> +     * same color.
> +     */
> +    public Integer getCheckerboardColor() {

I'm not too keen on the idea of a destructive getter function... Could this not just return an int and we can check for equality when setting it?

::: mobile/android/base/gfx/LayerRenderer.java
@@ +335,5 @@
>      }
>  
> +    private boolean updateCheckerboardLayer(GL10 gl, RenderContext renderContext) {
> +        Integer checkerboardColor = mView.getController().getCheckerboardColor();
> +        if (checkerboardColor == null) {

for example, here could just be:
int checkerboardColor = mView.getController().getCheckerboardColor();
if (checkerboardColor == mCheckerboardLayer.getColor()) return true;

@@ +336,5 @@
>  
> +    private boolean updateCheckerboardLayer(GL10 gl, RenderContext renderContext) {
> +        Integer checkerboardColor = mView.getController().getCheckerboardColor();
> +        if (checkerboardColor == null) {
> +            return false;

This should return true, or LayerRenderer will keep on scheduling redraws when the checkerboard colour doesn't change.

Comment 16

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 4.

Patch part 4 addresses review comments.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 590252 [details] [diff] [review]
Proposed patch, version 4.

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

Looks good to me.

Comment 18

[William Lachance (:wlach)]

(In reply to Madhava Enros [:madhava] from comment #13)
> Would like to see it in action, but generally -- sounds good.

Here's a quick video of the behaviour in action on Daring Fireball. Quite an improvement over before IMO:

http://people.mozilla.com/~wlachance/df_scroll.webm

Comment 19

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b77c0c621163

Comment 20

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/420d7b8ed59d for the sketchiest and most nebulous of reasons. https://tbpl.mozilla.org/php/getParsedLog.php?id=8779678&tree=Mozilla-Inbound is your red native ts. I see that you're in there repeatedly hitting NS_ERROR_DOM_NOT_SUPPORTED_ERR from the browser.js hunk, but I do not see what the reason for the redness (which according to the "Finished 'python run_tests.py ...' failed" bit was caused in the test running step, not any ancillary parts) might be.

Comment 21

[Patrick Walton (:pcwalton)]

I guess it would have to be one of two things: either the content document doesn't have a body yet or window.getComputedStyle() doesn't work during Talos. I hate to do this, but I'll just insert a try-catch around the call: there's no other way that I can see to detect this case.

Comment 22

[Patrick Walton (:pcwalton)]

Re-landed with try-catch and green run on try:

http://hg.mozilla.org/integration/mozilla-inbound/rev/33ab41faf05e

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/33ab41faf05e

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Patrick, please request aurora approval

Comment 25

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 590252 [details] [diff] [review]
Proposed patch, version 4.

[Approval Request Comment]
The patch for Bug 712687 is dependent on this patch.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 590252 [details] [diff] [review]
Proposed patch, version 4.

[Triage Comment]
Fixed in 12, so should only need to be uplifted to Beta 11. +'ing both just in case.

Comment 27

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33ab41faf05e

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Patrick, 2 hunks of this patch don't apply, one is trivial to fix. The other, in beginDraw is non-obvious because so much of the surrounding code has changed. Please attach a patch that applies to beta.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch rebased for mozilla-beta

This is the patch rebased to apply on mozilla-beta.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Landed on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/eab0f523de86

Comment 31

[Andreea Pod]

Verified fixed on builds:
Firefox 11 (tinderbox build): 	1328738704/	08-Feb-2012 15:44		
Firefox 12: Firefox 12.0a2 (2012-02-08)
Firefox 13: Firefox 13.0a1 (2012-02-08)

Device: LG Optimus 2X (Android 2.2.2)