677920 - With GL Layers, changing device orientation causes black flash and sometimes total device freeze

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ali Juma [:ajuma]]

Attachment: Prevent device orientation changes from causing stop/restart events.

With GL layers enabled, a change of device orientation causes the display to momentarily appear entirely black, before the screen is redrawn in the new orientation. 

With our current setup, a change of device orientation results in several Android events, including the Android activity being stopped, the Android surface being destroyed, the Android activity being restarted, and a new Android surface being created. This means that the OpenGL context and surface need to be unbound and re-bound during an orientation change, causing the brief black screen.

The patch changes the setup so that a change of device orientation only causes an Android surfaceChanged event; effectively, this means that a change of orientation is simply a resize (which we already handle correctly).

Comment 1

[Matt Brubeck (:mbrubeck)]

This was previously removed in bug 602976; looking into the reasons...

Comment 2

[Matt Brubeck (:mbrubeck)]

I couldn't figure out why "orientation" was removed from android:configChanges in bug 602976.  Can you explain, mwu?

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 552087 [details] [diff] [review]
Prevent device orientation changes from causing stop/restart events.

See https://bugzilla.mozilla.org/show_bug.cgi?id=602976#c6

We don't handle orientation so our surface can be destroyed and recreated. This is necessary to support switching to fullscreen mode at runtime.

Comment 4

[Michael Wu [:mwu]]

(In reply to Ali Juma [:ajuma] from comment #0)
> With our current setup, a change of device orientation results in several
> Android events, including the Android activity being stopped, the Android
> surface being destroyed, the Android activity being restarted, and a new
> Android surface being created. This means that the OpenGL context and
> surface need to be unbound and re-bound during an orientation change,
> causing the brief black screen.
> 

Android generally locks the screen and gives you a chance to draw something before it draws a black screen. Any flash of black is likely to be a bug where we aren't drawing when Android is giving us a chance, with the exception of keyboard resizing where the api is simply screwed up there. See the 2d drawing path for the various ways the java wrapper will either block the thread to wait for gecko to draw, or avoid locking the canvas at all when we would fail to draw anything.

Comment 5

[Ali Juma [:ajuma]]

(In reply to Michael Wu [:mwu] from comment #4)
> Android generally locks the screen and gives you a chance to draw something
> before it draws a black screen. Any flash of black is likely to be a bug
> where we aren't drawing when Android is giving us a chance, with the
> exception of keyboard resizing where the api is simply screwed up there. See
> the 2d drawing path for the various ways the java wrapper will either block
> the thread to wait for gecko to draw, or avoid locking the canvas at all
> when we would fail to draw anything.

Thanks for the pointers! A quick implementation of this approach eliminates almost all of the black flash. I'm working on eliminating the rest.

Comment 6

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a ReleaseSurface method to GLContextEGL.

We need to be able to release the GL surface when Android surfaceDestroyed events occur. This patch gives us that ability.

Comment 7

[Ali Juma [:ajuma]]

Attachment: Part 2: Fix handling of Android surface lifecycle events when using GL layers.

This patch ensures that we draw before returning from the Android surfaceChanged event, ensures that we do not draw when there is no Android surface (i.e. in between surfaceDestroyed and surfaceCreated), and releases the GL surface when the Android surface is destroyed.

This prevents the black flash and GL errors we've been getting when changing device orientation.

As an added bonus, it also resolves the black-screen-on-restart problem from Bug 672574.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 553877 [details] [diff] [review]
Part 2: Fix handling of Android surface lifecycle events when using GL layers.

Passing review to Brad Lassey, who is a better person to review this (or to find another reviewer).

Comment 9

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

Comment on attachment 553877 [details] [diff] [review]
Part 2: Fix handling of Android surface lifecycle events when using GL layers.

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

::: widget/src/android/nsAppShell.cpp
@@ +327,5 @@
>              mozilla::services::GetObserverService();
>          NS_NAMED_LITERAL_STRING(minimize, "heap-minimize");
>          obsServ->NotifyObservers(nsnull, "memory-pressure", minimize.get());
>  
> +        nsWindow::OnGlobalAndroidEvent(curEvent);

if you generate a SURFACE_DESTROYED event in PostEvent you won't need to send the ACTIVITY_STOPPING event to the window to be handled

@@ +426,5 @@
> +        if (ae->Type() == AndroidGeckoEvent::ACTIVITY_STOPPING ||
> +            ae->Type() == AndroidGeckoEvent::SURFACE_DESTROYED) {
> +            // Give priority to this event, and discard any pending 
> +            // SURFACE_CREATED events.
> +            mEventQueue.InsertElementAt(0, ae);

inserting the ACTIVITY_STOPPING event at the top of the queue concerns me. We're supposed to get a ACTIVITY_PAUSING before ACTIVITY_STOPPING, which will no longer be true.

If this needs to happen right away, I think I'd prefer you create a SURFACE_DESTROYED event to stick at the front of the queue and append the ACTIVITY_STOPPING.

::: widget/src/android/nsWindow.cpp
@@ +846,4 @@
>              break;
>  
>          case AndroidGeckoEvent::SURFACE_DESTROYED:
> +        case AndroidGeckoEvent::ACTIVITY_STOPPING:

if you generate the SURFACE_DESTROYED event in postEvent, you won't need to handle the ACTIVITY_STOPPING event here

Comment 10

[Ali Juma [:ajuma]]

Attachment: Part 2: Fix handling of Android surface lifecycle events when using GL layers, v2

Patch re-based and updated to address review comments.

Comment 11

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a ReleaseSurface method to GLContextEGL, v2

Patch updated to fix compile errors on Maemo.

We can't release or re-create the surface on QT since we don't create the surface ourselves, so I added #ifdefs to leave out the new surface release and re-creation code on QT.

Comment 12

[Benoit Girard (:BenWa)]

We suspect this bug also causes a total device freeze. If landing this patch doesn't resolve this please file a separate bug for that issue.

Comment 13

[Ali Juma [:ajuma]]

Attachment: Part 2: Fix handling of Android surface lifecycle events when using GL layers, v2 rebased

Rebased, carrying forward r=blassey.

Comment 14

[Benoit Girard (:BenWa)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6f12501fde07
http://hg.mozilla.org/integration/mozilla-inbound/rev/6e7449c449ba

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/6e7449c449ba
http://hg.mozilla.org/mozilla-central/rev/6f12501fde07

Comment 16

[Andreea Pod]

I don't see black screen when changing orientation on Nightly: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110901 Firefox/9.0a1 Fennec/9.0a1
Device: LG Optimus 2X

so I can mark this as verified fixed?

Comment 17

[Ali Juma [:ajuma]]

(In reply to Andreea Pod from comment #16)
 > so I can mark this as verified fixed?

Yes (as long as you were testing with GL layers enabled, by setting layers.acceleration.disabled=false and layers.acceleration.force-enabled=true in about:config).

Comment 18

[Andreea Pod]

I thought that I'd better make a video for you to see if it's alright: http://www.youtube.com/watch?v=ZOTrLIkk6aI&feature=channel_video_title

Comment 19

[Ali Juma [:ajuma]]

Thanks, great idea! That looks fine.