Closed Bug 725428 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox14 --- verified
blocking-fennec1.0 --- +

People

(Reporter: dietrich, Assigned: bnicholson)

References

Details

(Whiteboard: startup-ux)

Attachments

(2 files, 4 obsolete files)

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.
tracking-fennec: --- → ?
Dietrich, do you still see this?
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.
Dietrich, can you record what's happening and post it?
blocking-fennec1.0: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: startup-ux
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → +
Priority: -- → P1
Attached video checkerboard video
Here's a screencast of it on my phone. The checkerboards flash quickly, but they're there.
Duplicate of this bug: 729980
tracking-fennec: ? → ---
Duplicate of this bug: 730299
Attached patch WIP patch (obsolete) — Splinter Review
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.
Attachment #605612 - Flags: feedback?(bugmail.mozilla)
Attached patch WIP patch (obsolete) — Splinter Review
Oops, removed a few unnecessary changes in GeckoApp.
Attachment #605612 - Attachment is obsolete: true
Attachment #605612 - Flags: feedback?(bugmail.mozilla)
Attachment #605613 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #605613 - Flags: feedback?(bugmail.mozilla) → feedback+
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #605613 - Attachment is obsolete: true
Attachment #608163 - Flags: review?(bugmail.mozilla)
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.
Attachment #608163 - Flags: review?(bugmail.mozilla) → review-
> ::: 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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Addressed above comments.
Attachment #608163 - Attachment is obsolete: true
Attachment #608460 - Flags: review?(bugmail.mozilla)
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.
Attachment #608460 - Flags: review?(bugmail.mozilla) → review-
Attached patch patch v3Splinter Review
Fixed problems noted in comments.
Attachment #608460 - Attachment is obsolete: true
Attachment #608487 - Flags: review?(bugmail.mozilla)
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.
Attachment #608487 - Flags: review?(bugmail.mozilla) → review+
Not sure if you knew robocop had an issue if the test was extending PixelBase : bug 737411 

This should be fixed in inbound, though.
(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
https://hg.mozilla.org/mozilla-central/rev/3d3aff127856
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Comment on attachment 608487 [details] [diff] [review]
patch v3

[Approval Request Comment]
Prevents flashing of black screen at startup.
Attachment #608487 - Flags: approval-mozilla-aurora?
This will need to wait for maple to be uplifted before it can land on aurora.
Comment on attachment 608487 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only - approved for Aurora 13.
Attachment #608487 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified/fixed on:
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.