sonyxperiadev / MultimediaForAndroidLibrary

88 stars 33 forks source link

Release Method #8

Open bicoi opened 9 years ago

bicoi commented 9 years ago

Hi,

I have a question related to "release" method of MediaPlayer and Player class.

  1. In MediaPlayer class, when we call release() we will release Player and set this value to null (mPlayer = null).
  2. In Player class we will send message "MSG_STOP" to EventHandler stop other threads,

But EventHandle will handle "MSG_STOP" in another thread, so have a case a command "mPlayer = null" is run while we are processing "MSG_STOP". When "mPlayer = null" is set I think app will be crashed, because we are referencing to mPlayer in EventHandle.

Could you give your advice about this situation? Or I have misunderstood anything?

In the case it has problem, could I use "sendMessageAndAwaitResponse(nMsg)" instead of "mEventHandler.sendMessageAtFrontOfQueue(nMsg)" in release method of Player?

Thanks,

ghost commented 9 years ago

Hi.

In MediaPlayer the state is changed to mState = State.END; in release() function. This will guard all calls coming into MediaPlayer, meaning no calls from MediaPlayer to Player will be made after release.

What EventHandler is it that mean when you say that it keeps a reference to Player instance?

/Jimmy

bicoi commented 9 years ago

Hi Jimmy,

Thanks for your quick answer, I meant the problem is in class Player not in MediaPlayer. In method release of Player has following code:

        Message nMsg = mEventHandler.obtainMessage(MSG_STOP, 1, 0);
        mEventHandler.sendMessageAtFrontOfQueue(nMsg);
        sendMessageAndAwaitResponse(nMsg);
EventHandler which I have mentioned is mEventHandler.

mEventHandler will send message "MSG_STOP" in release method of Player, and as I know this message will be process on another thread (we call Thread2), which does not the same thread release() method called (we call Thread1).
Class EventHandle keep a weak reference to Player class, and I think have a case.
1. Thread1 call mPlayer.release() in MediaPlayer class
2. Thread2 process MSG_STOP message
3. While Thread2 is processing, Thread1 call mPlayer = null
4. When mPlayer is set to null, have no strong reference to this object, maybe it will be destroy by garbage collection.
5. Thread2 continue process, and this time mPlayer object is destroyed.

Sorry if my explanation is not clear :)
Regards,
ghost commented 9 years ago

Hi.

Yes there might be a timing related problem. However in the top of HandleMessage we fetch the reference: Player thiz = mPlayer.get();

But yes, if the WeakRefrence has been GC:ed before we enter handleMessage the fetched reference might be null. That is true. We have however not ever seen this happen. Do you have a case where you can trigger the problem? Could you in that case attach the StackTrace?

/Jimmy

bicoi commented 9 years ago

Hi Jimmy,

We saw this case when we run multiple Unit Tests. Bellow is log output:

junit.framework.AssertionFailedError: UncaughtException: nulljava.lang.NullPointerException: Attempt to read from field 'com.sonymobile.android.media.internal.Player$EventHandler com.sonymobile.android.media.internal.Player.mEventHandler' on a null object reference
at com.sonymobile.android.media.internal.Player.access$200(Player.java:55)
at com.sonymobile.android.media.internal.Player$EventHandler.handleMessage(Player.java:786)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:145)
at android.os.HandlerThread.run(HandlerThread.java:61)
java.lang.NullPointerException: Attempt to read from field 'com.sonymobile.android.media.internal.Player$EventHandler com.sonymobile.android.media.internal.Player.mEventHandler' on a null object reference
at com.sonymobile.android.media.internal.Player.access$200(Player.java:55)
at com.sonymobile.android.media.internal.Player$EventHandler.handleMessage(Player.java:786)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:145)
at android.os.HandlerThread.run(HandlerThread.java:61)

at com.sonymobile.android.media.ApiTest.shutDown(ApiTest.java:2261)
at com.sonymobile.android.media.ApiTest.setOnPreparedListener(ApiTest.java:1462)
at com.sonymobile.android.media.ApiTestCase.testSetOnPreparedListener(ApiTestCase.java:727)
at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1873)

In this case could I use "sendMessageAndAwaitResponse(nMsg)" instead of "mEventHandler.sendMessageAtFrontOfQueue(nMsg)" in release method of Player? Or we check "null" in handleMessage method of EventHandler class?

            if (thiz == null) {
                return;
            }

Thanks,

ghost commented 9 years ago

Hi.

I think the preferred way would be to use sendMessageAndAwaitResponse.

If you like please upload a change via a pullRequest and we will check it.

/Jimmy

bicoi commented 9 years ago

Hi Jimmy,

Thanks you very much. I will create a pullRequest soon.

Regards,