droid: remove the stateful life-cycle manager #2278

Merged
merged 2 commits into from Mar 1, 2013

Projects

None yet

2 participants

@theuni
Member
theuni commented Feb 22, 2013

This fixes random black-screens, fixes, and crashes when starting XBMC as a
launcher.

Simply properly react to messages from appglue. This ends up being much cleaner and
more straightforward.

There were many problems with the old life-cycle code:

  • It based all decisions on gain/loss of focus, which is incorrect, as that
    pertains to input rather than windowing.
  • Due to the above, there were many race conditions.
  • The startup/teardown were convoluted. Now we start at startup and exit at exit :)
  • It was a mix of examples, a stateful one where the app keeps track of status,
    and a stateless one mainly handle by appglue. Since we really only care about
    a handful of events (startup/exit/newwindow/destroywindow mainly), stateless
    is the way to go for us.
Cory Fields droid: remove the stateful life-cycle manager
Simply properly react to messages instead. This ends up being much cleaner and
more straightforward.

There were many problems with the old life-cycle code:
- It based all decisions on gain/loss of focus, which is incorrect, as that
  pertains to input rather than windowing.
- Due to the above, there were many race conditions.
- The startup/teardown were convoluted
- It was a mix of examples, a stateful one where the app keeps track of status,
  and a stateless one mainly handle by appglue. Since we really only care about
  a handful of events (startup/exit/newwindow/destroywindow mainly), stateless
  is the way to go for us.

This fixes random black-screens, fixes, and crashes when starting XBMC as a
launcher.
026ac1a
@Montellese Montellese commented on an outdated diff Feb 22, 2013
xbmc/android/activity/XBMCApp.cpp
+ // been destroyed.
+ if (!m_exiting)
+ {
+ XBMC_Stop();
+ pthread_join(m_thread, NULL);
+ android_printf(" => XBMC finished");
+ }
+
+ if (m_wakeLock != NULL && m_activity != NULL)
+ {
+ JNIEnv *env = NULL;
+ m_activity->vm->AttachCurrentThread(&env, NULL);
+
+ env->DeleteGlobalRef(m_wakeLock);
+ m_wakeLock = NULL;
+ }
@Montellese
Montellese Feb 22, 2013 Team Kodi member

You need to detach again.

@Montellese
Member

Apart from the missing detach I commented on, there are a few cosmetics (missing spaces before/after "=" and between "if" and "(") but didn't spot anything else otherwise.

It's hard to see if this actually works in all cases as well as the android lifecycle is very complicated (and confusing). The old code certainly wasn't ideal.
IIRC one of the odd use cases in the lifecycle is when long-pressing Home to get to the task manager. The app receives an OnPause() (and should stop doing anything in the GUI) but it doesn't receive an OnDestroyWindow() because the window is still visible in the background of the task manager.

I'll give this a test run when I got some time and I'll try to cover all use cases I can think of (including the task manager scenario, being Home'd etc).

@theuni
Member
theuni commented Feb 22, 2013

I tested every possible scenario (I hope!) and it was rock-solid. One thing to keep in mind is that we now have the ability to destroy the window on the spot, which we did not have at the time this was written.

We don't need to react to OnPause() or any of the others... as long as we have the window we can render into it. Besides, we really want to be rendering then, because the thumbnail-view in the task-switcher is a live view of the app.

I think the cycle is simpler than we made it out to be, because we were responding to the wrong events. Mainly because we were paying attention to focus, which is specified for input and not windowing. It broadcasts many events that are not of much use to us since we run in the background.

The diff here is pretty unreadable, but I think the change is easy enough to understand. The logic is ripped out of the EventLoop, which now just calls our functions as it gets messages. It kicks off the app at OnStart(), then hackishly waits for the window creation and it's on its way. This part will be removed when I can get EGL smart enough to start headless then switch to visible.

We call the Android finish command when XBMC is done executing, which forces the shutdown sequence of: onPause(), onLostFocus(), onDestroyWindow(), onStop(), onDestroy(). We skip onDestroyWindow() since we've already done it, catch onDestroy, and cleanup. If we catch onDestroy when XBMC has not been shut down yet, we assume android is forcing an exit, so we tear down manually.

@theuni
Member
theuni commented Feb 22, 2013

Thinking about it a bit more, we may want to pause playback for some of those events, but not stop rendering. If it's safe to call XBMC_Pause() when we're already paused, and likewise when we're not, it may just be easiest to blast out pause/resume events for onPause/onStop/onResume/OnStart/etc.

@Montellese
Member

In the current/old code, video and audio playback is always paused when we are Home'd or when the task manager is open so that would be a regression if we want to keep that behaviour.

@theuni
Member
theuni commented Feb 27, 2013

Inadvertently tested this at SCALE all weekend, worked without issue. We were paused correctly as far as I can tell.

Queuing this one up for March (after fixing the detach you pointed out).

@theuni theuni was assigned Feb 27, 2013
@theuni
Member
theuni commented Mar 1, 2013

@Montellese Any objection to seeing this go in? Even if we do discover some corner-case that's missed, it should be very easy to fix. And this fixes several bugs.

@Montellese
Member

Go ahead. I know who to blame from now on ;-)

@theuni
Member
theuni commented Mar 1, 2013

Haha, fair enough. I'm going to wait til tomorrow to test+push. That detach makes me uncomfortable as it looks like it wasn't there before either. Makes me wonder if we had an extra somewhere.

@theuni
Member
theuni commented Mar 1, 2013

Ok, it works fine. I'm leaving that detach commit as separate to help with the conflicts that will arise with #2319

@theuni theuni merged commit f42873c into xbmc:master Mar 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment