Closed Bug 1350253 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

()

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: 3 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.
You need to log in before you can comment on or make changes to this bug.