Closed Bug 1376966 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kikuo, Assigned: JamesCheng)

References

Details

Attachments

(1 file)

Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.
59 bytes, text/x-review-board-request
kikuo
: review+
Details
Summary: [Fennec][HLS] Fix detects in GeckoHlsPlayer.java → [Fennec][HLS] Fix defects in GeckoHlsPlayer.java
Priority: -- → P3
Assignee: nobody → jacheng
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 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 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 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 on attachment 8884208 [details]
Bug 1376966 - [Fennec][HLS] Fix defects in GeckoHlsPlayer.java.

https://reviewboard.mozilla.org/r/155162/#review160576
Attachment #8884208 - Flags: review?(kikuo) → review+
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b561b725de2
[Fennec][HLS] Fix defects in GeckoHlsPlayer.java. r=kikuo
https://hg.mozilla.org/mozilla-central/rev/1b561b725de2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: