Last Comment Bug 799617 - Screen goes black when starting Fennec from external URL or resuming session
: Screen goes black when starting Fennec from external URL or resuming session
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 19
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
Depends on: 817064
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-09 11:56 PDT by Brian Nicholson (:bnicholson)
Modified: 2012-12-06 09:37 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
Set SurfaceView background to white before drawing (3.45 KB, patch)
2012-10-09 12:19 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Set SurfaceView background to white before drawing, v2 (6.97 KB, patch)
2012-10-09 14:52 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Set SurfaceView background to white before drawing, v3 (6.61 KB, patch)
2012-10-10 11:41 PDT, Brian Nicholson (:bnicholson)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Wait until second draw to clear white background (2.25 KB, patch)
2012-10-10 11:42 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2012-10-09 11:56:52 PDT
This was fixed awhile ago with bug 725428 by setting a white background color on the LayerView. Since then, this change got lost somewhere: https://hg.mozilla.org/mozilla-central/rev/3d3aff127856#l1.19. It won't work anyway, though, because the LayerView has been changed to be a FrameLayout, and the SurfaceView is now a child of LayerView rather than the LayerView itself. Setting the LayerView's background color won't help since the child sits on top of it, so we have to change the background of the child view instead.
Comment 1 Brian Nicholson (:bnicholson) 2012-10-09 12:05:37 PDT
STR:

From a cold start, launch Fennec from an external URL.
*Or* kill Fennec (either sigkill or OOM) and reopen it.

In both of these cases, the screen is black for over 1 second before anything is drawn.
Comment 2 Brian Nicholson (:bnicholson) 2012-10-09 12:19:32 PDT
Created attachment 669689 [details] [diff] [review]
Set SurfaceView background to white before drawing

I've noticed that with just the background fix applied, there is still a flash of black before the first draw (though it's very brief). I remember noticing the same thing back when bug 799617 was working. Does the endDrawing() callback get executed before the actual draw happens?

I was able to work around this by adding another state, PAINT_AFTER_SECOND, to the LayerView, which clears the white background after the second draw instead of the first. This doesn't seem like the best fix, but it works.
Comment 3 Brian Nicholson (:bnicholson) 2012-10-09 12:21:28 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> I've noticed that with just the background fix applied, there is still a
> flash of black before the first draw (though it's very brief). I remember
> noticing the same thing back when bug 799617 was working. Does the
> endDrawing() callback get executed before the actual draw happens?

s/bug 799617/bug 725428/
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-09 12:34:15 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Created attachment 669689 [details] [diff] [review]
> Set SurfaceView background to white before drawing
> 
> I've noticed that with just the background fix applied, there is still a
> flash of black before the first draw (though it's very brief). I remember
> noticing the same thing back when bug 799617 was working. Does the
> endDrawing() callback get executed before the actual draw happens?
> 

endDrawing should only be getting called after the composite is complete. Most likely this is caused by us compositing an empty frame or something like that.

> I was able to work around this by adding another state, PAINT_AFTER_SECOND,
> to the LayerView, which clears the white background after the second draw
> instead of the first. This doesn't seem like the best fix, but it works.

I'd like to dig a little to see if we can figure out why that empty frame is being drawn. Adding BenWa to see if he has any ideas.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-09 12:37:20 PDT
Also, just to clarify: if it turns out that the empty frame will always be there, then I'm fine with this patch. I just don't want to end up in a situation where that empty frame is only *sometimes* there, because then we'll leave the view with a white background until the next frame is drawn, which might not be for a while.
Comment 6 Benoit Girard (:BenWa) 2012-10-09 12:51:46 PDT
I'm not familiar with that part of the code. pcwalton worked on the android views bits.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-09 13:10:40 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Created attachment 669689 [details] [diff] [review]
> Set SurfaceView background to white before drawing
> 
> I've noticed that with just the background fix applied, there is still a
> flash of black before the first draw (though it's very brief). I remember
> noticing the same thing back when bug 799617 was working.

Do you have a specific page you're loading when you see this? I applied the background fix parts of your patch and ran:

am start -a android.intent.action.VIEW -n org.mozilla.fennec_kats/.App -d "https://staktrace.com"

and I don't see a black flash at all (whereas I do see a black flash on stock m-c without your patch). If I can repro the short flash that you're seeing I can try to attach gdb and see why the compositor is giving us a frame at that point.
Comment 8 Brian Nicholson (:bnicholson) 2012-10-09 14:52:30 PDT
Created attachment 669753 [details] [diff] [review]
Set SurfaceView background to white before drawing, v2

Cleaned up the patch a bit (in case we decide to keep this approach). I think this check only needs to be done at startup, so there's no need to reset the paint state for every setFirstPaintViewport() call.
Comment 9 Brian Nicholson (:bnicholson) 2012-10-09 15:04:52 PDT
(In reply to Kartikaya Gupta (:kats) from comment #7)
> Do you have a specific page you're loading when you see this? I applied the
> background fix parts of your patch and ran:
> 
> am start -a android.intent.action.VIEW -n org.mozilla.fennec_kats/.App -d
> "https://staktrace.com"

I see it when I run this command (same site), but it doesn't always appear. I had to try 5 times before I finally saw it (where I did menu > Quit between each attempt).

This is on a Droid RAZR, non-debug build.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-10 06:24:14 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> > am start -a android.intent.action.VIEW -n org.mozilla.fennec_kats/.App -d
> > "https://staktrace.com"
> 
> I see it when I run this command (same site), but it doesn't always appear.
> I had to try 5 times before I finally saw it (where I did menu > Quit
> between each attempt).

The fact that it doesn't happen every time worries me, because of what I said in comment #5.

> 
> This is on a Droid RAZR, non-debug build.

Is this the same mythical device you were seeing bug 751948 on? :) Do you see this flash on any other device?

I think to be safe I would like to split this patch into two - one that just sets the background color on the child element, and a second one that does the state changes. We can land the first one right away but I'd like to investigate a bit more to see if the second one is really needed.

Ideally what I want is the backtrace from the first composite to see what's triggering it but since that happens so early it's going to be hard to attach gdb in time. We might have to insert a sleep call somewhere to allow enough time to attach gdb.
Comment 11 Brian Nicholson (:bnicholson) 2012-10-10 11:41:11 PDT
Created attachment 670053 [details] [diff] [review]
Set SurfaceView background to white before drawing, v3

Sets the background color on LayerView's child.
Comment 12 Brian Nicholson (:bnicholson) 2012-10-10 11:42:33 PDT
Created attachment 670054 [details] [diff] [review]
Wait until second draw to clear white background

Workaround for black screen flash on Droid RAZR.
Comment 13 Brian Nicholson (:bnicholson) 2012-10-10 13:52:24 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/46b4fcdd20e6
Comment 14 Ed Morley [:emorley] 2012-10-11 12:09:09 PDT
https://hg.mozilla.org/mozilla-central/rev/46b4fcdd20e6
Comment 15 Brian Nicholson (:bnicholson) 2012-10-11 14:41:17 PDT
Comment on attachment 670053 [details] [diff] [review]
Set SurfaceView background to white before drawing, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: screen can flash black at startup
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 16 Alex Keybl [:akeybl] 2012-10-12 16:04:11 PDT
Comment on attachment 670053 [details] [diff] [review]
Set SurfaceView background to white before drawing, v3

Unless this is a recent regression in Fennec Native, let's only uplift this fix to FF18 on Aurora.
Comment 17 Brian Nicholson (:bnicholson) 2012-10-15 15:37:46 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c932b7b0c55d
Comment 18 Brian Nicholson (:bnicholson) 2012-10-15 23:38:25 PDT
(In reply to Alex Keybl [:akeybl] from comment #16)
> Comment on attachment 670053 [details] [diff] [review]
> Set SurfaceView background to white before drawing, v3
> 
> Unless this is a recent regression in Fennec Native, let's only uplift this
> fix to FF18 on Aurora.

It's worth mentioning that this was in fact introduced in FF17. We've had this same issue before 17 (fixed by bug 725428), but something in 17 regressed it by changing the layout.
Comment 19 Brian Nicholson (:bnicholson) 2012-10-15 23:43:29 PDT
Comment on attachment 670053 [details] [diff] [review]
Set SurfaceView background to white before drawing, v3

Re-noming to be sure (see comment 18).
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-16 07:31:26 PDT
That is worth mentioning, I'll approve for beta uplift.
Comment 21 Brian Nicholson (:bnicholson) 2012-10-16 11:36:04 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/27eed0f4867f
Comment 22 Brian Nicholson (:bnicholson) 2012-11-30 11:08:54 PST
Filed bug 817064 separately for the Droid RAZR issue.
Comment 23 Andreea Pod 2012-12-06 09:37:58 PST
Verified fixed on:
- build: Firefox for Android 18.0 Beta 3 (2012-12-06), Firefox for Android 19.0a2 (2012-12-06), Firefox for Android 17.0
- deviceL Samsung Galaxy Nexus 
- OS: Android 4.0.2

Note You need to log in before you can comment on or make changes to this bug.