725428 - content area goes black and then dark checkerboard before pageload during startup

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

i'm seeing some flashing, then a black content area for up to 3 seconds and then a dark checkerboard and then a white background and finally the page content loads.

while there's clearly a performance issue here, this bug is about design:

i picked a common page (mobile wikipedia) and loaded it in both Fennec and in the stock browser, cold starting both. the stock browser took longer, but *felt far faster*.

here's the timeline of visual events for the stock browser. events that occurred at basically the same time are coalesced into a single timeline event.

1. touch icon
2. app fills screen. all navigation controls are visible. content area is white. title is loaded. favicon is the stop button.
3. progress meter moves from left to right, located between navigation and content area.
4. page content loads. progress meter disappears. favicon replaces stop button.

timeline of visual events for fennec:

1. touch icon
2. there's a flash of a black screen (i think - it's really fast)
3. there's a flash of my homescreen
4. app fills screen. navigation controls are visible, except for new-tab "+". content area is black. title is loaded. favicon is progress ring.
5. content area turns into dark checkerboard. new-tab "+" appears.
6. content area turns white. some times the white only covers 2/3 of the content area - it's split vertically, with the other 1/3 being a light grey.
7. page content loads. favicon replaces progress ring.

there's a far lower amount of visual activity during the load of the stock browser. the only visual change is the progress meter - a direct visual indication of that the browser is busy doing what you asked.

fennec has a high amount of visual activity, almost all of which is superfluous to the requested task. it's feels busy, jumbly and heavy.

the net effect is that while fennec finished loading the page faster by the clock, it felt far slower because all this crazy stuff happened on screen while it did so.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

dup of bug 716581? if not part of bug 717774?

Comment 2

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

Dietrich, do you still see this?

Comment 3

[Dietrich Ayala (:dietrich)]

Just updated to latest nightly, and did a cold start:

1. content area is solid grey
2. then it's dark checkerboard
3. then it's light checkerboard
4. then content shows up

I can do a more detailed breakdown about how data fills into the nav area if it helps.

Comment 4

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

Dietrich, can you record what's happening and post it?

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: checkerboard video

Here's a screencast of it on my phone. The checkerboards flash quickly, but they're there.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: WIP patch

This patch doesn't draw the background/checkerboard/root layers if the root layer hasn't yet been set. This improves the problem by showing a white screen instead of the background/checkerboard layers before we connect the software layer client. Unfortunately, there's still a quick flash when the software layer client gets hooked up. I don't know what the cause is.

Let me know if there's a better way to fix this; I'm not too familiar with any of the gfx classes.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: WIP patch

Oops, removed a few unnecessary changes in GeckoApp.

Comment 10

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

Comment on attachment 605613 [details] [diff] [review]
WIP patch

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

In general this looks fairly sane, but the Maple landing tomorrow will change some of this code, so you might as well wait until tomorrow before continuing on this patch. Also it would be a good idea to first decide exactly what we want to be displaying during the period of time in question. By the looks of it you just want white (or the placeholder if it's there), and that's fine - just want to make sure we're clear on that first.

Comment 11

[JP Rosevear [:jpr]]

Not seeing the checkboarding currently but I do see light/dark flashing still now that maple has landed, so we should probably revisit this patch.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: patch

A somewhat hacky patch that simply sets the screen to white by setting the LayerView background color. Using glClear(), as in the previous patch, still flickered a black screen between setting the view and the glClear() call.

With this approach, we must set the LayerView back to a transparent background once we've drawn a page or else the screen will stay white. Setting a flag in setFirstPaintViewport() indicates that we're about to do a first paint; we then set the background back to transparent once we've ended the first draw.

Comment 13

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

Comment on attachment 608163 [details] [diff] [review]
patch

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

Overall the approach looks fine, but there's a couple of problems as noted below.

::: mobile/android/base/GeckoApp.java
@@ +1748,2 @@
>              mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
> +            if (!mPlaceholderLayerClient.loadScreenshot()) {

This is effectively calling loadScreenshot() twice, because it gets called once from the constructor in PlaceholderLayerClient. Might be better to just call it the one time in the constructor, and save the success value in a class variable, and then query that here.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +366,5 @@
>              // sends the request after aborting the animation. The display port request is actually
>              // a full viewport update, which is fine because if browser.js has somehow moved to
>              // be out of sync with this first-paint viewport, then we force them back in sync.
>              mLayerController.abortPanZoomAnimation();
> +            mLayerController.getView().setFirstPaint(true);

setFirstPaintViewport gets called more than just once during the lifetime of fennec. In fact it will get called every time you switch tabs or load a new page. Setting the firstPaint flag on the view back to true every time this happens means we're going to be doing a lot of unnecessary work (i.e. setting the background back to transparent multiple times). Really you want something like a tri-state flag, so that once you set the background back to transparent you go to a final state and never go back into the first-paint state.

Comment 14

[Brian Nicholson (:bnicholson)]

> ::: mobile/android/base/GeckoApp.java
> @@ +1748,2 @@
> >              mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
> > +            if (!mPlaceholderLayerClient.loadScreenshot()) {
> 
> This is effectively calling loadScreenshot() twice, because it gets called
> once from the constructor in PlaceholderLayerClient. Might be better to just
> call it the one time in the constructor, and save the success value in a
> class variable, and then query that here.
> 

Oops, I intended to remove the loadScreenshot() call from the constructor. Will that also work?

Comment 15

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

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> Oops, I intended to remove the loadScreenshot() call from the constructor.
> Will that also work?

Sure, that works too.

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Addressed above comments.

Comment 17

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

Comment on attachment 608460 [details] [diff] [review]
patch v2

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +366,5 @@
>              // sends the request after aborting the animation. The display port request is actually
>              // a full viewport update, which is fine because if browser.js has somehow moved to
>              // be out of sync with this first-paint viewport, then we force them back in sync.
>              mLayerController.abortPanZoomAnimation();
> +            mLayerController.getView().setPaintState(LayerView.PAINT_BEFORE_FIRST);

This still has the same problem as before; the state will keep bouncing between PAINT_BEFORE_FIRST and PAINT_AFTER_FIRST and you'll keep setting the color back to transparent unnecessarily. At some point there has to be a guard such that once you're in AFTER_FIRST you can't go back to BEFORE_FIRST. I'd prefer the guard to be in the LayerView class since that's more encapsulated.

::: mobile/android/base/gfx/LayerView.java
@@ +244,5 @@
>      public LayerRenderer getLayerRenderer() {
>          return mRenderer;
>      }
> +    
> +    public void setPaintState(int paintState) {

Add a comment here saying the parameter must be one of the PAINT_xxx constants. Or use an enum instead of ints. Either is fine by me.

@@ +248,5 @@
> +    public void setPaintState(int paintState) {
> +        mPaintState = paintState;
> +    }
> +
> +    public int getPaintState() {

Add a comment here saying the return value is one of the PAINT_xxx constants. Alternatively, use an enum instead of ints. Either is fine by me.

Comment 18

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

Fixed problems noted in comments.

Comment 19

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

Comment on attachment 608487 [details] [diff] [review]
patch v3

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

Nice, r=me with nit addressed

::: mobile/android/base/gfx/LayerView.java
@@ +250,5 @@
> +    /* paintState must be a PAINT_xxx constant. The state will only be changed
> +     * if paintState represents a state that occurs after the current state. */
> +    public void setPaintState(int paintState) {
> +        if (paintState > mPaintState)
> +            mPaintState = paintState;

nit: add braces around the if body.

Comment 20

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/017f6dd98dc0

Comment 21

[Brian Nicholson (:bnicholson)]

Backed out for robocop failures.

http://hg.mozilla.org/integration/mozilla-inbound/rev/4595ab8f1cd0

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Not sure if you knew robocop had an issue if the test was extending PixelBase : bug 737411 

This should be fixed in inbound, though.

Comment 23

[Brian Nicholson (:bnicholson)]

(In reply to Naoki Hirata :nhirata from comment #22)
> Not sure if you knew robocop had an issue if the test was extending
> PixelBase : bug 737411 
> 
> This should be fixed in inbound, though.

Nope, I didn't realize that. It looks like the other rc failures were from another bug, so relanding:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3d3aff127856

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/3d3aff127856

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 608487 [details] [diff] [review]
patch v3

[Approval Request Comment]
Prevents flashing of black screen at startup.

Comment 26

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

This will need to wait for maple to be uplifted before it can land on aurora.

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 608487 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only - approved for Aurora 13.

Comment 28

[Paul Feher]

Verified/fixed on:
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6