Closed Bug 677920 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: ajuma, Assigned: ajuma)

References

Details

Attachments

(2 files, 4 obsolete files)

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).
Attachment #552087 - Flags: review?(mbrubeck)
This was previously removed in bug 602976; looking into the reasons...
I couldn't figure out why "orientation" was removed from android:configChanges in bug 602976.  Can you explain, 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.
Attachment #552087 - Flags: review?(mbrubeck) → review-
(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.
(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.
We need to be able to release the GL surface when Android surfaceDestroyed events occur. This patch gives us that ability.
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.
Attachment #552087 - Attachment is obsolete: true
Attachment #553872 - Flags: review?(matt.woodrow)
Attachment #553877 - Flags: review?(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).
Attachment #553877 - Flags: review?(mbrubeck) → review?(blassey.bugs)
Attachment #553872 - Flags: review?(matt.woodrow) → review+
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
Attachment #553877 - Flags: review?(blassey.bugs) → review-
Patch re-based and updated to address review comments.
Attachment #553877 - Attachment is obsolete: true
Attachment #554409 - Flags: review?(blassey.bugs)
Attachment #554409 - Flags: review?(blassey.bugs) → review+
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.
Attachment #553872 - Attachment is obsolete: true
Attachment #554551 - Flags: review?(matt.woodrow)
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.
Summary: With GL Layers, changing device orientation causes black flash → With GL Layers, changing device orientation causes black flash and sometimes total device freeze
Attachment #554551 - Flags: review?(matt.woodrow) → review+
Rebased, carrying forward r=blassey.
Attachment #554409 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/6e7449c449ba
http://hg.mozilla.org/mozilla-central/rev/6f12501fde07
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 682327
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?
(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).
Whiteboard: [inbound]
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
Thanks, great idea! That looks fine.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.