Closed Bug 1350253 Opened 7 years ago Closed 7 years ago

[Fennec][HLS] HLSDemuxer/HLSTrackDemuxer in Gecko should be able to get media & sample information from GeckoHlsPlayer.java

Categories

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

enhancement
Not set
normal

Tracking

(firefox55 disabled)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- disabled

People

(Reporter: kikuo, Assigned: kikuo)

References

Details

Attachments

(1 file)

A JNI Wrapper should be provided for HLSDemuxer/HLSTrackDemuxer for Gecko to obtain metadata information and samples from GeckoHLSPlayer singleton instance.
Summary: [Fennec][HLS] HLSDemuxer/HLSTrackDemuxer should obtain media & sample information from GeckoHlsPlayer.java → [Fennec][HLS] HLSDemuxer/HLSTrackDemuxer in Gecko should be able to get media & sample information from GeckoHlsPlayer.java
Attachment #8864771 - Flags: review?(jyavenard)
Attachment #8864771 - Flags: review?(jolin)
NOTE: The call sites for this java wrapper will be provided in Bug 1350246.
I'll look into this in Taipei on Monday...

it's a lot to digest...
Comment on attachment 8864771 [details]
Bug 1350253 - Provide GeckoHlsDemuxerWrapper & corresponding generated JNI files as the glue for HLSDemuxer and GeckoHlsPlayer.

https://reviewboard.mozilla.org/r/136432/#review141400

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:23
(Diff revision 1)
> +
> +public final class GeckoHlsDemuxerWrapper {
> +    private static final String LOGTAG = "GeckoHlsDemuxerWrapper";
> +    private static final boolean DEBUG = true;
> +
> +    // NOTE : These TRACK definition should be synced with Gecko.

definitions

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:31
(Diff revision 1)
> +    private static final int TRACK_VIDEO = 2;
> +    private static final int TRACK_TEXT = 3;
> +
> +    private GeckoHlsPlayer player = null;
> +    // A flag to avoid using the native object that has been destroyed.
> +    private boolean destroyed;

do we need this?

isn't testing if player == null sufficient?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:112
(Diff revision 1)
> +             */
> +            assertTrue(!MimeTypes.AUDIO_RAW.equals(fmt.sampleMimeType));
> +            aInfo.bitDepth = 16;
> +            aInfo.mimeType = fmt.sampleMimeType;
> +            aInfo.duration = player.getDuration();
> +            // For HLS content, csd-0 is enough.

hmmm that would be true for AAC only.

but likely fine here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:129
(Diff revision 1)
> +        if (DEBUG) Log.d(LOGTAG, "[getVideoInfo] formatIndex : " + index);
> +        Format fmt = player.getVideoTrackFormat(index);
> +        GeckoVideoInfo vInfo = null;
> +        if (fmt != null) {
> +            vInfo = new GeckoVideoInfo();
> +            vInfo.displayX = fmt.width;

the choice of members name displayX/displayY is not good.

X and Y typically indicates coordinates.
That's different to width and height

Can those members be rename to say displayWidth, displayHeight etc?

this also assume that all HLS video have a PAR of 1:1 is that always the case?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:183
(Diff revision 1)
> +    private long getNextKeyFrameTime() {
> +        assertTrue(player != null);
> +        return player.getNextKeyFrameTime();
> +    }
> +
> +    @WrapForJNI // Called when natvie object is destroyed.

native
Attachment #8864771 - Flags: review?(jyavenard) → review+
Comment on attachment 8864771 [details]
Bug 1350253 - Provide GeckoHlsDemuxerWrapper & corresponding generated JNI files as the glue for HLSDemuxer and GeckoHlsPlayer.

https://reviewboard.mozilla.org/r/136432/#review141508

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:24
(Diff revision 1)
> +public final class GeckoHlsDemuxerWrapper {
> +    private static final String LOGTAG = "GeckoHlsDemuxerWrapper";
> +    private static final boolean DEBUG = true;
> +
> +    // NOTE : These TRACK definition should be synced with Gecko.
> +    private static final int TRACK_UNDEFINED = 0;

`enum`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:33
(Diff revision 1)
> +
> +    private GeckoHlsPlayer player = null;
> +    // A flag to avoid using the native object that has been destroyed.
> +    private boolean destroyed;
> +
> +    public static class HlsDemuxerCallbacks extends JNIObject

Please make sure that native callbacks won't use native objects after free.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:41
(Diff revision 1)
> +        @WrapForJNI(calledFrom = "gecko")
> +        HlsDemuxerCallbacks() {}
> +
> +        @Override
> +        @WrapForJNI(dispatchTo = "gecko")
> +        public native void onInitialized(boolean hasAudio, boolean hasVideo);;

s/;;/;/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:60
(Diff revision 1)
> +            throw new AssertionError("Expected condition to be true");
> +        }
> +    }
> +
> +    private GeckoHlsPlayer.Track_Type getPlayerTrackType(int trackType) {
> +        if (trackType == 1) {

s/1/TRACK_AUDIO/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:79
(Diff revision 1)
> +    }
> +
> +    @WrapForJNI(calledFrom = "gecko")
> +    public static GeckoHlsDemuxerWrapper create(GeckoHlsPlayer player,
> +                                                GeckoHlsPlayer.DemuxerCallbacks callback) {
> +        GeckoHlsDemuxerWrapper wrapper = new GeckoHlsDemuxerWrapper(player, callback);

`return new ...`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:97
(Diff revision 1)
> +        assertTrue(player != null);
> +
> +        if (DEBUG) Log.d(LOGTAG, "[getAudioInfo] formatIndex : " + index);
> +        Format fmt = player.getAudioTrackFormat(index);
> +        GeckoAudioInfo aInfo = null;
> +        if (fmt != null) {

Nit:
```
if (fmt == null) {
    return null;
}

GeckoAudioInfo aInfo = new GeckoAudioInfo();
...
```

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsDemuxerWrapper.java:127
(Diff revision 1)
> +        assertTrue(player != null);
> +
> +        if (DEBUG) Log.d(LOGTAG, "[getVideoInfo] formatIndex : " + index);
> +        Format fmt = player.getVideoTrackFormat(index);
> +        GeckoVideoInfo vInfo = null;
> +        if (fmt != null) {

ditto
Attachment #8864771 - Flags: review?(jolin) → review+
Comment on attachment 8864771 [details]
Bug 1350253 - Provide GeckoHlsDemuxerWrapper & corresponding generated JNI files as the glue for HLSDemuxer and GeckoHlsPlayer.

https://reviewboard.mozilla.org/r/136430/#review146428

Addressed all issues.
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d5a071e4404
Provide GeckoHlsDemuxerWrapper & corresponding generated JNI files as the glue for HLSDemuxer and GeckoHlsPlayer. r=jolin,jya
https://hg.mozilla.org/mozilla-central/rev/0d5a071e4404
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1368925
This was reverted for Beta 55 as part of bug 1368925. It remains enabled on Trunk, which is now tracking Gecko 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

Created:
Updated:
Size: