consider using background color fade-in rather than checkerboarding

VERIFIED FIXED in Firefox 11

Status

()

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: akeybl, Assigned: pcwalton)

Tracking

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

Firefox Tracking Flags

(firefox11 verified, firefox12 verified, firefox13 verified, fennec11+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

We should consider using a fast background color to web content fade-in rather than checkerboarding.
The checkerboard doesn't actually cost us any speed to render; it's all on the GPU.
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.
if this is free either way, what does UX think.
Assignee: nobody → madhava
In XUL fennec we switched from checkerboard to page background color in bug 694706.  In that case, it actually improved performance.
Duplicate of this bug: 711897
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 .
OS: Linux → Android
Hardware: Other → ARM
P3 for Madhava to make a decision here, we'll reevaluate priority once he's weighed in.
Priority: -- → P2
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.
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.
tracking-fennec: --- → 11+
Posted patch Proposed patch. (obsolete) — Splinter Review
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.
Attachment #588599 - Flags: review?(chrislord.net)
Attachment #588599 - Flags: feedback?(madhava)
Posted patch Proposed patch, version 2. (obsolete) — Splinter Review
Forgot to remove the old checkerboard.png resource (it's procedurally generated now so no longer needed).
Attachment #588599 - Attachment is obsolete: true
Attachment #588599 - Flags: review?(chrislord.net)
Attachment #588599 - Flags: feedback?(madhava)
Attachment #588600 - Flags: review?(chrislord.net)
Attachment #588600 - Flags: feedback?(madhava)
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.
Attachment #588600 - Flags: review?(chrislord.net) → review-
Would like to see it in action, but generally -- sounds good.
Posted patch Proposed patch, version 3. (obsolete) — Splinter Review
Patch part 3 addresses review comments.
Assignee: madhava → pwalton
Attachment #588600 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #588600 - Flags: feedback?(madhava)
Attachment #590097 - Flags: review?(chrislord.net)
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.
Attachment #590097 - Flags: review?(chrislord.net) → review-
Patch part 4 addresses review comments.
Attachment #590097 - Attachment is obsolete: true
Attachment #590252 - Flags: review?(chrislord.net)
Comment on attachment 590252 [details] [diff] [review]
Proposed patch, version 4.

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

Looks good to me.
Attachment #590252 - Flags: review?(chrislord.net) → review+
(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
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.
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.
Re-landed with try-catch and green run on try:

http://hg.mozilla.org/integration/mozilla-inbound/rev/33ab41faf05e
https://hg.mozilla.org/mozilla-central/rev/33ab41faf05e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Patrick, please request aurora approval
Comment on attachment 590252 [details] [diff] [review]
Proposed patch, version 4.

[Approval Request Comment]
The patch for Bug 712687 is dependent on this patch.
Attachment #590252 - Flags: approval-mozilla-beta?
Attachment #590252 - Flags: approval-mozilla-aurora?
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.
Attachment #590252 - Flags: approval-mozilla-beta?
Attachment #590252 - Flags: approval-mozilla-beta+
Attachment #590252 - Flags: approval-mozilla-aurora?
Attachment #590252 - Flags: approval-mozilla-aurora+
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.
This is the patch rebased to apply on mozilla-beta.
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)
You need to log in before you can comment on or make changes to this bug.