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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kikuo, Assigned: kikuo)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 disabled)

Details

Attachments

(1 attachment)

A JNI Wrapper should be provided for HLSDemuxer/HLSTrackDemuxer for Gecko to obtain metadata information and samples from GeckoHLSPlayer singleton instance.
Assignee

Updated

2 years ago
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
Assignee

Updated

2 years ago
Attachment #8864771 - Flags: review?(jyavenard)
Attachment #8864771 - Flags: review?(jolin)
Assignee

Comment 2

2 years ago
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 4

2 years ago
mozreview-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/#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 5

2 years ago
mozreview-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+
Assignee

Comment 6

2 years ago
mozreview-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.
Comment hidden (mozreview-request)

Comment 9

2 years ago
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

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d5a071e4404
Status: NEW → RESOLVED
Closed: 2 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.