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

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
Audio/Video
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: kikuo, Assigned: kikuo)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox55 disabled)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

6 months 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
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8864771 - Flags: review?(jyavenard)
Attachment #8864771 - Flags: review?(jolin)
(Assignee)

Comment 2

6 months 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

5 months 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

5 months 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

5 months 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)
(Assignee)

Comment 8

5 months ago
Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17df38ae5194fcf212d26239ee7fd1d36a1bc15b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f26a08867793000e9fd2628b82492d266ca2ed1

Comment 9

5 months 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d5a071e4404
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
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.
status-firefox55: fixed → disabled
You need to log in before you can comment on or make changes to this bug.