1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, enhancement, P3)

Description

[Kilik Kuo [:kikuo] (inactive)]

see defects on https://scan4.coverity.com/reports.htm#v27745/p11134/fileInstanceId=21856840

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

Review commit: https://reviewboard.mozilla.org/r/155162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155162/

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160224

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:144
(Diff revision 1)
>  
> -        if (DEBUG) {
> +        if (DEBUG && mTracksInfo != null) {
>              Log.d(LOGTAG, "[checkInitDone] VReady:" + mTracksInfo.videoReady() +
>                      ",AReady:" + mTracksInfo.audioReady() +
>                      ",hasV:" + mTracksInfo.hasVideo() +
>                      ",hasA:" + mTracksInfo.hasAudio());
>          }
> -        if (mTracksInfo.videoReady() && mTracksInfo.audioReady()) {
> +        if (mTracksInfo != null && mTracksInfo.videoReady() && mTracksInfo.audioReady()) {
> +            if ( mDemuxerCallbacks != null) {
> -            mDemuxerCallbacks.onInitialized(mTracksInfo.hasAudio(), mTracksInfo.hasVideo());
> +                mDemuxerCallbacks.onInitialized(mTracksInfo.hasAudio(), mTracksInfo.hasVideo());
> +            }
>              mIsDemuxerInitDone = true;

if mTracksInfo is null, we could not do anything.
if mDemuxerCallbacks is null, that means the an important setup procedure is not completed yet.

Could you centralized these checks together and add debug trace for it ?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java
(Diff revision 1)
> -        assertTrue(mPlayer != null);
>          if (isLiveStream()) {
>              return 0L;
>          }
>          // Value returned by getDuration() is in milliseconds.
> -        long duration = Math.max(0L, mPlayer.getDuration() * 1000L);
> +        long duration = mPlayer == null ? 0 : Math.max(0L, mPlayer.getDuration() * 1000L);
>          if (DEBUG) { Log.d(LOGTAG, "getDuration : " + duration  + "(Us)"); }

Could you merge these few lines so that we can see the debug message in every cases ? e.g.

(isLiveStream() || mPlayer == null) ? 0 : ...

or other expression ?

Comment 3

[James Cheng[:JamesCheng]]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155162/diff/1-2/

Comment 4

[James Cheng[:JamesCheng]]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160224

> if mTracksInfo is null, we could not do anything.
> if mDemuxerCallbacks is null, that means the an important setup procedure is not completed yet.
> 
> Could you centralized these checks together and add debug trace for it ?

Sure, I remain the check of callback since we explicit set it to null by release with another thread.

So if release is called before, we should skip the callback.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160274

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:156
(Diff revision 2)
> +            if (mDemuxerCallbacks != null) {
> -            mDemuxerCallbacks.onInitialized(mTracksInfo.hasAudio(), mTracksInfo.hasVideo());
> +                mDemuxerCallbacks.onInitialized(mTracksInfo.hasAudio(), mTracksInfo.hasVideo());
> +            }

I noticed that '''checkInitDone''' is called inside ComponentListener's methods which are all synchronized with this inner class  ComponentListener.

By changing the code from 

public synchronized void onDataArrived(...) {
    ...
    checkInitDone();
}

to

public void onDataArrived(...) {
    synchronized(GeckoHlsPlayer.this) {
       ...
       checkInitDone();
    }
}

'''checkInitDone()''' & '''release()'''/'''init()''' won't race each other, right ?

Is that make sense to you ?
In that case, there seems no need to check mTrackInfo != null in '''checkInitDone'''

Comment 6

[James Cheng[:JamesCheng]]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155162/diff/2-3/

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160572

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:220
(Diff revisions 2 - 3)
> +                if (mTracksInfo == null) {
> +                    Log.e(LOGTAG, "Encounter a abnormal calling sequence. mTracksInfo is null");
> +                    return;
> +                }
> -            mTracksInfo.onDataArrived(trackType);
> +                mTracksInfo.onDataArrived(trackType);
> +                if (mResourceCallbacks != null) {
> -            mResourceCallbacks.onDataArrived();
> +                    mResourceCallbacks.onDataArrived();
> +                }

Since you're returning here if mTracksInfo is null, supposing the 2 assertTture above are to make sure that Fennec gets crash in DEBUG cases,

in the other hand, for RELEASE cases, you could directly check, i.e.

if (mTracksInfo == null || mResourceCallbacks == null) {
 // Log the information indicating an abnormal situation.
}

and remove the ResourceCallback check below.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:221
(Diff revisions 2 - 3)
> -            }
> +                }
> -            assertTrue(mResourceCallbacks != null);
> +                assertTrue(mResourceCallbacks != null);
> -            assertTrue(mTracksInfo != null);
> +                assertTrue(mTracksInfo != null);
> +
> +                if (mTracksInfo == null) {
> +                    Log.e(LOGTAG, "Encounter a abnormal calling sequence. mTracksInfo is null");

s/a/an/

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160576

Comment 9

[James Cheng[:JamesCheng]]

Comment on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155162/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b561b725de2
[Fennec][HLS] Fix defects in GeckoHlsPlayer.java. r=kikuo

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b561b725de2