[Fennec][HLS] Create a demuxer-like component based on ExoPlayer.

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kikuo, Assigned: kikuo)

Tracking

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

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
ExoPlayer is able to be customized in 4 parts [1].

We intent to create a simple GeckoHLSPlayer which is derived from ExoPlayer, and to inject customized A/V renderers.  In that case, by exposing demuxer APIs, we could return demuxed samples which is ready for playback back to gecko's MediaFormatReader.

Based on our understanding to ExoPlayer, thee reasons why we don't customize "MediaSource" are
1) The effort of handling HLSMediaSource is too much for us right now. It's also quite complicated to us. We need to control downloading / meter bandwidth / integrate the TS extractor from exoplayer / select tracks for playback ...etc.
2) All ExoPlayer components begin to work continuously once the playback start, so if we customize HLSMediaSource to get samples back, we are still required to customize A/V renderers which don't render actual frames in order to make the playback go on.


[1] http://google.github.io/ExoPlayer/doc/reference/com/google/android/exoplayer2/doc-files/exoplayer-threading-model.svg
(Assignee)

Updated

a year ago
Blocks: 1350253
(Assignee)

Updated

a year ago
Blocks: 1350250
(Assignee)

Updated

a year ago
Depends on: 1341990
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → kikuo
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
According to Bug 1016449, it seems practical to introduce ExoPlayer source code into Gecko and once Bug 1341990 has landed, we're about to roll out the HLS implementation. Here's the 1st part of it. The core component works like a demuxer for Gecko's media pipeline.
(Assignee)

Updated

a year ago
Attachment #8856550 - Flags: review?(jyavenard)
Attachment #8856550 - Flags: review?(jolin)
Attachment #8856551 - Flags: review?(jyavenard)
Attachment #8856551 - Flags: review?(jolin)
(Assignee)

Comment 6

a year ago
Should add, James Cheng and I worked on this together for months, more patches are coming soon :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128510/#review139878

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoAudioInfo.java:10
(Diff revision 2)
> +package org.mozilla.gecko.media;
> +
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +
> +//A subset of the class AudioInfo in dom/media/MediaInfo.h
> +@WrapForJNI

Could we have something in place to ensure that it matches sthe C++ Audio/VideoInfo ?

Do we have to worry if they differ?
Attachment #8856550 - Flags: review?(jyavenard) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review139888

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/EventLogger.java:2
(Diff revision 4)
> +/*
> + * Copyright (C) 2016 The Android Open Source Project

why would this be copyright the Android Open Source project?

if we copied the code from somewhere, could you indicate the source?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:57
(Diff revision 4)
> +        }
> +        if (decoderInfo == null) {
> +            return RendererCapabilities.FORMAT_UNSUPPORTED_SUBTYPE;
> +        }
> +        boolean decoderCapable = Versions.preLollipop ||
> +                                 (format.sampleRate == -1 ||

could you add a comment on why a sampleRate of -1 makes it decodable?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:64
(Diff revision 4)
> +                                 (format.channelCount == -1 ||
> +                                  decoderInfo.isAudioChannelCountSupportedV21(format.channelCount));
> +        int formatSupport = decoderCapable ?
> +            RendererCapabilities.FORMAT_HANDLED :
> +            RendererCapabilities.FORMAT_EXCEEDS_CAPABILITIES;
> +        return RendererCapabilities.ADAPTIVE_NOT_SEAMLESS | formatSupport;

some comments would go a long way really...

They are very scarce (nonexistent really)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:140
(Diff revision 4)
> +        ByteBuffer buffer = ByteBuffer.wrap(realData);
> +        inputBuffer.clear();
> +
> +        CryptoInfo cryptoInfo = bufferForRead.isEncrypted() ? bufferForRead.cryptoInfo.getFrameworkCryptoInfoV16() : null;
> +        BufferInfo bufferInfo = new BufferInfo();
> +        // Flags in DecoderInputBuffer are syned with MediaCodec Buffer flags.

synced

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:74
(Diff revision 4)
> +    private int numVideoTracks = 0;
> +    private int numAudioTracks = 0;
> +    private GeckoHlsVideoRenderer vRenderer = null;
> +    private GeckoHlsAudioRenderer aRenderer = null;
> +    private boolean enableV = true;
> +    private boolean enableA = true;

it makes it very hard to see that enableV and enableA are global variable throughout the code..

please rename. Ideally a class, with all class members starting with the letter m

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:76
(Diff revision 4)
> +    private GeckoHlsVideoRenderer vRenderer = null;
> +    private GeckoHlsAudioRenderer aRenderer = null;
> +    private boolean enableV = true;
> +    private boolean enableA = true;
> +
> +    private boolean hasVideo = false;

can't this be put into a class/structure rather than having all those private variables?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:94
(Diff revision 4)
> +        TRACK_AUDIO,
> +        TRACK_VIDEO,
> +        TRACK_TEXT,
> +    }
> +
> +    public static final int E_RESOURCE_BASE         = -100;

enum ?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:472
(Diff revision 4)
> +        return hasAudio ? aRenderer.getFormat(index) : null;
> +    }
> +
> +    public boolean seek(long positionMs) {
> +        // positionMs : milliseconds.
> +        // NOTE : 1) It's not possible to seek media by tracktype via ExoPlayer Interface.

can't we use microseconds everywhere like all the gecko code?

only using the unit required by the framework at the end?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:477
(Diff revision 4)
> +        // NOTE : 1) It's not possible to seek media by tracktype via ExoPlayer Interface.
> +        //        2) positionMs is samples PTS from MFR, we need to re-adjust it
> +        //           for ExoPlayer by subtracting sample start time.
> +        try {
> +            // TODO : Gather Timeline Period / Window information to develop
> +            //        complete timeilne, and seekTime should be inside the duration.

timeline

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:33
(Diff revision 4)
> +    protected ConcurrentLinkedQueue<GeckoHlsSample> demuxedInputSamples = new ConcurrentLinkedQueue<>();
> +
> +    protected ByteBuffer inputBuffer = null;
> +    protected Format format = null;
> +    protected ArrayList<Format> formats = new ArrayList<Format>();
> +    protected boolean initialized = false;

should follow the name name policy as the rest of gecko.

so class variable prefixed by m

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:67
(Diff revision 4)
> +        int size = demuxedInputSamples.size();
> +        GeckoHlsSample[] queuedSamples =
> +                demuxedInputSamples.toArray(new GeckoHlsSample[size]);
> +        return Math.abs(queuedSamples[size - 1].info.presentationTimeUs -
> +                        queuedSamples[0].info.presentationTimeUs) >
> +               QUEUED_INPUT_SAMPLE_DURATION_THRESHOLD ? true : false;

? true : false unnecessary

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:73
(Diff revision 4)
> +    }
> +
> +    public Format getFormat(int index) {
> +        assertTrue(index >= 0);
> +        Format fmt = index < formats.size() ? formats.get(index) : null;
> +        if (DEBUG) Log.d(LOGTAG, "getFormat : index = " + index +

missing { }

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:93
(Diff revision 4)
> +            }
> +            GeckoHlsSample sample = demuxedInputSamples.poll();
> +            samples.offer(sample);
> +        }
> +        if (samples.isEmpty()) {
> +            if (DEBUG) Log.d(LOGTAG, "getQueuedSamples isEmpty, waitingForData = true !");

missing { }

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:35
(Diff revision 4)
> +public class GeckoHlsVideoRenderer extends GeckoHlsRendererBase {
> +    private final VideoRendererEventListener.EventDispatcher eventDispatcher;
> +
> +    /*
> +     * By configuring these states, initialization data is provided for
> +     * ExoPlayer's HlsMediaSouck to parse HLS bitstream and then provide samples

HlsMediaSouck?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:39
(Diff revision 4)
> +     * By configuring these states, initialization data is provided for
> +     * ExoPlayer's HlsMediaSouck to parse HLS bitstream and then provide samples
> +     * starting with an Access Unit Delimiter including SPS/PPS for TS,
> +     * and provide samples starting with an AUD without SPS/PPS for FMP4.
> +     */
> +    private static final int RECONFIGURATION_STATE_NONE = 0;

use enum

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:100
(Diff revision 4)
> +                    decoderCapable = format.width * format.height <= MediaCodecUtil.maxH264DecodableFrameSize();
> +                } catch (MediaCodecUtil.DecoderQueryException e) {
> +                    Log.e(LOGTAG, e.getMessage());
> +                }
> +                if (!decoderCapable) {
> +                    if (DEBUG) Log.d(LOGTAG, "Check [legacyFrameSize, " +

missing { }

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:135
(Diff revision 4)
> +        initialized = true;
> +    }
> +
> +    @Override
> +    protected void resetRenderer() {
> +        if (DEBUG) Log.d(LOGTAG, "[resetRenderer] initialized = " + initialized);

missing { }

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:314
(Diff revision 4)
> +         * So we calcualte it by referring to nearby samples' timestamp.
> +         * A temporary queue |demuxedNoDurationSamples| is used to queue demuxed
> +         * samples from HlsMediaSource which have no duration information at first.
> +         * Considering there're 9 demuxed samples in the _no duration_ queue already,
> +         * e.g. |-2|-1|0|1|2|3|4|5|6|...
> +         * Once a new demuxed(No duration) sample A (10th) is put into the

what abotu streams with b frames?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:318
(Diff revision 4)
> +         * e.g. |-2|-1|0|1|2|3|4|5|6|...
> +         * Once a new demuxed(No duration) sample A (10th) is put into the
> +         * temporary queue,
> +         * e.g. |-2|-1|0|1|2|3|4|5|6|A|...
> +         * we are able to calculate the correct duration for sample 0 by finding
> +         * the most closest but greater pts than sample 0 among these 9 samples,

the closest

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:369
(Diff revision 4)
> +            //        A workaround here is to assign a close duration to it.
> +            long tmpDuration = 33333;
> +            GeckoHlsSample sample = null;
> +            for (sample = demuxedNoDurationSamples.poll(); sample != null; sample = demuxedNoDurationSamples.poll()) {
> +                if (sample.duration == Long.MAX_VALUE) {
> +                    sample.duration = tmpDuration;

rather than setting the duration of the last frame to 33ms, it would be better to set the duration to be the same as the previous one. as typically, framerate is constant

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:385
(Diff revision 4)
> +    }
> +
> +    // Return the time of first keyframe sample in the queue.
> +    // If there's no sample in the queue, return the last one we've found.
> +    public long getNextKeyFrameTime() {
> +        for (GeckoHlsSample sample : demuxedInputSamples) {

if there's no next keyframe, then the value returned should be INT_MAX; otherwise you may end up with dropped frames as the MFR believes decoding is too slow

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:436
(Diff revision 4)
> +        if (format.width == Format.NO_VALUE || format.height == Format.NO_VALUE) {
> +            // We can't infer a maximum input size without video dimensions.
> +            return Format.NO_VALUE;
> +        }
> +
> +        // Attempt to infer a maximum input size from the format.

this makes me cringe... Last time I saw android code attempting to guess the maximum size of a sample (in stagefright) we found heaps of security issue and memory overflow.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:441
(Diff revision 4)
> +        // Attempt to infer a maximum input size from the format.
> +        int maxPixels;
> +        int minCompressionRatio;
> +        switch (format.sampleMimeType) {
> +            case MimeTypes.VIDEO_H263:
> +            case MimeTypes.VIDEO_MP4V:

you won't see this with HLS

Plus we don't support those codecs anyway (h263, mp4v/divx, h265)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:470
(Diff revision 4)
> +        return (maxPixels * 3) / (2 * minCompressionRatio);
> +    }
> +
> +    private static boolean areAdaptationCompatible(Format first, Format second) {
> +        return first.sampleMimeType.equals(second.sampleMimeType)
> +               && getRotationDegrees(first) == getRotationDegrees(second);

trailing &&

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Utils.java:31
(Diff revision 4)
> +
> +    public static void logThreadSignature() {
> +        Log.d("ThreadUtils", getThreadSignature());
> +    }
> +
> +    public static void sleepForInSecs(int secs) {

unused method.. can we remove that ? very dirty

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Utils.java:39
(Diff revision 4)
> +        } catch (InterruptedException x) {
> +            throw new RuntimeException("interrupted", x);
> +        }
> +    }
> +
> +    public static String getCallStack() {

unused method
Attachment #8856551 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review139888

> why would this be copyright the Android Open Source project?
> 
> if we copied the code from somewhere, could you indicate the source?

We copied it from ExoPlayer Github repo [1].  ExoPlayer is basically a project on top of AOSP and is expected to be used on it.
[1] https://github.com/google/ExoPlayer/blob/release-v2/demo/src/main/java/com/google/android/exoplayer2/demo/EventLogger.java#L1-L15
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review139888

> what abotu streams with b frames?

Each _no_duration_ sample is one of the I or P or B frame and that is already considered.
Because the duration calculation for each sample in the _no_duration_queue_ only starts when the queued-sample size is larger than 10.  And each time with the removal of first queued sample (that's say the sample |-2| in the comment), the duration of each sample will be calculated again by comparing with all others in the window.

Hope this explanation makes things clear.

Comment 14

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128510/#review140474

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoAudioInfo.java:10
(Diff revision 2)
> +package org.mozilla.gecko.media;
> +
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +
> +//A subset of the class AudioInfo in dom/media/MediaInfo.h
> +@WrapForJNI

Assuming these info objects are just used to populate native `MediaInfo`, you could provide getter methods instead of exposing data directly to ensure constness or make all instance variables `final`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoAudioInfo.java:12
(Diff revision 2)
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +
> +//A subset of the class AudioInfo in dom/media/MediaInfo.h
> +@WrapForJNI
> +public final class GeckoAudioInfo {
> +    public byte[] codecSpecificData = null;

Use `ByteBuffer` instead of `byte[]`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoAudioInfo.java:13
(Diff revision 2)
> +
> +//A subset of the class AudioInfo in dom/media/MediaInfo.h
> +@WrapForJNI
> +public final class GeckoAudioInfo {
> +    public byte[] codecSpecificData = null;
> +    public byte[] extraData = null;

Remove this if unused.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsSample.java:26
(Diff revision 2)
> +        EOS = new GeckoHlsSample(null, eosInfo, null);
> +    }
> +
> +    // Indicate the index of format which is used by this sample.
> +    @WrapForJNI
> +    public int formatIndex;

It seems all instance variables should be `final`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsSample.java:37
(Diff revision 2)
> +    public BufferInfo info;
> +
> +    @WrapForJNI
> +    public CryptoInfo cryptoInfo;
> +
> +    private byte[] byteArray;

Use `ByteBuffer` to save extra copy.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsSample.java:57
(Diff revision 2)
> +    public boolean isKeyFrame() {
> +        return (info.flags & MediaCodec.BUFFER_FLAG_KEY_FRAME) != 0;
> +    }
> +
> +    @WrapForJNI
> +    public void dispose() {

Is this necessary?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsSample.java:75
(Diff revision 2)
> +        byteArray = bytes;
> +        this.info = info;
> +        this.cryptoInfo = cryptoInfo;
> +    }
> +
> +    public static byte[] byteArrayFromBuffer(ByteBuffer buffer, int offset, int size) {

Not needed if `byteArray` is replaced with `ByteBuffer`
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128510/#review140474

> It seems all instance variables should be `final`.

Sadly, not exactly. For TS samples, we need to calculate the duration by referring samples' PTS around.
But I'll make other variables _final_ as many as possible.
(Assignee)

Comment 16

a year ago
mozreview-review-reply
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review139888

> rather than setting the duration of the last frame to 33ms, it would be better to set the duration to be the same as the previous one. as typically, framerate is constant

In fact this is exactly doing the same thing but just starting with a default duration 33ms.
The last sample has the largest PTS, so it's not possible for the last sample to modify its duration in the for-loop above (Line#353-#362).
As a result, the duration of the last sample is set by the same duration of its previous sample through the following for-loop.

I'd rename the tmpDuration to prevDuration.  That should make this clear.

> this makes me cringe... Last time I saw android code attempting to guess the maximum size of a sample (in stagefright) we found heaps of security issue and memory overflow.

It should be safe here, this calculated max input size is used to create a place holder (A ByteBuffer for DecoderInputBuffer.data) for ExoPlayer's HlsMediaSource to put samples.  Beside the formula is the same as the one used in MediaCodecVideoRenderer in ExoPlayer.
Also, the space for writing sample into this place is also ensured in DefaultTrackOuput [1].
[1] https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/src/main/java/com/google/android/exoplayer2/extractor/DefaultTrackOutput.java#L297

An IllegalStateException will be thrown if something goes wrong, I'll add a try/catch for that.

Comment 17

a year ago
mozreview-review
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review140496

::: mobile/android/base/moz.build:443
(Diff revision 4)
>      'java/com/googlecode/eyesfree/braille/selfbraille/WriteData.java',
>  ]]
>  
>  if CONFIG['MOZ_ANDROID_HLS_SUPPORT']:
>      gvjar.sources += [geckoview_source_dir + 'java/org/mozilla/gecko/' + x for x in [
> +        'media/EventLogger.java',

If this is just to help debug, please include it only when `CONFIG['MOZ_DEBUG']`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:28
(Diff revision 4)
> +import java.nio.ByteBuffer;
> +
> +import org.mozilla.gecko.AppConstants.Versions;
> +
> +public class GeckoHlsAudioRenderer extends GeckoHlsRendererBase {
> +    private final AudioRendererEventListener.EventDispatcher eventDispatcher;

It seems this is only used for `inputFormatChanged()`, which calls `AudioRendererEventListener.onAudioInputFormatChanged()`. Why not just call `playerListener.onAudioInputFormatChanged()` directly?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:49
(Diff revision 4)
> +        if (!MimeTypes.isAudio(mimeType)) {
> +            return RendererCapabilities.FORMAT_UNSUPPORTED_TYPE;
> +        }
> +        MediaCodecInfo decoderInfo = null;
> +        try {
> +            decoderInfo = mediaCodecSelector.getDecoderInfo(mimeType, false);

It seems you can use `MediaCodecSelector.DEFAULT` directly and get rid of `mediaCodecSelector`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:67
(Diff revision 4)
> +            RendererCapabilities.FORMAT_HANDLED :
> +            RendererCapabilities.FORMAT_EXCEEDS_CAPABILITIES;
> +        return RendererCapabilities.ADAPTIVE_NOT_SEAMLESS | formatSupport;
> +    }
> +
> +    protected void handleDrmInitChanged(Format oldFormat, Format newFormat) {

this and the one in video renderer look exactly the same. Please move to superclass.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:81
(Diff revision 4)
> +        }
> +    }
> +
> +    @Override
> +    protected final void maybeInitRenderer() {
> +        if (initialized || format == null) {

Please try extracting as much as common code in override methods into superclass.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:86
(Diff revision 4)
> +        if (initialized || format == null) {
> +            return;
> +        }
> +        if (DEBUG) Log.d(LOGTAG, "Initializing ... ");
> +        inputBuffer = ByteBuffer.wrap(new byte[22048]);
> +        initialized = true;

ditto.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:99
(Diff revision 4)
> +
> +    /*
> +     * The place we get demuxed data from HlsMediaSource(ExoPlayer).
> +     * The data will then be converted to GeckoHlsSample and deliver to
> +     * GeckoHlsDemuxerWrapper for further use.
> +     */

Please add comments for return value.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:101
(Diff revision 4)
> +     * The place we get demuxed data from HlsMediaSource(ExoPlayer).
> +     * The data will then be converted to GeckoHlsSample and deliver to
> +     * GeckoHlsDemuxerWrapper for further use.
> +     */
> +    @Override
> +    protected synchronized boolean feedInputBuffersQueue() {

Part of this method looks identical to the one in video renderer. Please try extracting to superclass.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:133
(Diff revision 4)
> +        }
> +
> +        bufferForRead.flip();
> +
> +        int size = bufferForRead.data.limit();
> +        byte[] realData = new byte[size];

save this copy by offering `bufferForRead.data` directly to `GeckoHlsSample.create()`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:175
(Diff revision 4)
> +        demuxedInputSamples.clear();
> +        return true;
> +    }
> +
> +    @Override
> +    protected void onInputFormatChanged(Format newFormat) {

Please try extracting common code to superclass.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:55
(Diff revision 4)
> +    private static boolean DEBUG = false;
> +
> +    private DataSource.Factory mediaDataSourceFactory;
> +    private Timeline.Period period;
> +    private Timeline.Window window;
> +    private Handler mainHandler;

Is this Android main thread or Gecko main thread handler?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:56
(Diff revision 4)
> +
> +    private DataSource.Factory mediaDataSourceFactory;
> +    private Timeline.Period period;
> +    private Timeline.Window window;
> +    private Handler mainHandler;
> +    private EventLogger eventLogger;

`ExoPlayer.EventListener mEventLogger` to avoid reference to the non-exist `EventLogger` type in release version.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:64
(Diff revision 4)
> +    private DefaultTrackSelector trackSelector;
> +    private MediaSource mediaSource;
> +    private ComponentListener componentListener;
> +
> +    private boolean isTimelineStatic = false;
> +    private long durationUs;

Is this unused?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:87
(Diff revision 4)
> +    private DemuxerCallbacks demuxerCallbacks;
> +    private ResourceCallbacks resourceCallbacks;
> +
> +    protected String userAgent;
> +
> +    public enum Track_Type {

Nit:
s/Track_Type/TrackType/
s/TRACK_//

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:106
(Diff revision 4)
> +    public static final int E_DEMUXER_PLAYER       = E_DEMUXER_BASE + 2;
> +    public static final int E_DEMUXER_UNSUPPORTED  = E_DEMUXER_BASE + 3;
> +
> +    public interface DemuxerCallbacks {
> +        void onInitialized(boolean hasAudio, boolean hasVideo);
> +        void onDemuxerError(int errorCode);

Nit:`onError()` should be sufficient. It's a `DemuxerCallbacks` method after all.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:111
(Diff revision 4)
> +        void onDemuxerError(int errorCode);
> +    }
> +
> +    public interface ResourceCallbacks {
> +        void onDataArrived();
> +        void onResourceError(int errorCode);

ditto.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:134
(Diff revision 4)
> +            demuxerCallbacks.onInitialized(hasAudio, hasVideo);
> +            isDemuxerInitDone = true;
> +        }
> +    }
> +
> +    public final class ComponentListener implements VideoRendererEventListener,

Don't need to implement these interfaces if renderer called format change callbacks directly.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:226
(Diff revision 4)
> +        public void onMetadata(Metadata metadata) {
> +            // do nothing
> +        }
> +    }
> +
> +    public DataSource.Factory buildDataSourceFactory(Context va, DefaultBandwidthMeter bandwidthMeter) {

s/va/ctxt/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:237
(Diff revision 4)
> +        return new DefaultHttpDataSourceFactory(userAgent, bandwidthMeter);
> +    }
> +
> +    private MediaSource buildMediaSource(Uri uri, String overrideExtension) {
> +        if (DEBUG) Log.d(LOGTAG, "buildMediaSource uri[" + uri + "]" + ", overridedExt[" + overrideExtension + "]");
> +        int type = Util.inferContentType(!TextUtils.isEmpty(overrideExtension) ? "." + overrideExtension

Nit: reformat to
```
  !TextUtils.isEmpty(overrideExtension)
  ? "." + overrideExtension
  : uri.getLastPathSegment()
```
Better yet, drop the `!`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:292
(Diff revision 4)
> +        renderersList.add(vRenderer);
> +        aRenderer = new GeckoHlsAudioRenderer(MediaCodecSelector.DEFAULT,
> +                                              mainHandler,
> +                                              componentListener);
> +        renderersList.add(aRenderer);
> +        renderers = renderersList.toArray(new GeckoHlsRendererBase[renderersList.size()]);

```
  renderers = new GeckoHlsRendererBase[2];
  renderers[0] = new GeckoHlsVideoRenderer(...);
  renderers[1] = new GeckoHlsAudioRenderer(...);
```

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:304
(Diff revision 4)
> +            eventLogger = new EventLogger(trackSelector);
> +            player.addListener(eventLogger);
> +        }
> +
> +        Uri[] uris = new Uri[]{Uri.parse(url)};
> +        userAgent = Util.getUserAgent(ctx, "RemoteDecoder");

replace "RemoteDecoder" with proper app name. Perhaps "Firefox for Android"/"Fennec" or the package name?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:307
(Diff revision 4)
> +
> +        Uri[] uris = new Uri[]{Uri.parse(url)};
> +        userAgent = Util.getUserAgent(ctx, "RemoteDecoder");
> +        mediaDataSourceFactory = buildDataSourceFactory(ctx, BANDWIDTH_METER);
> +
> +        MediaSource[] mediaSources = new MediaSource[1];

Why the 1 element array? Doesn't `mediaSource = buildMediaSource(...)` work?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:350
(Diff revision 4)
> +        }
> +    }
> +
> +    @Override
> +    public synchronized void onTracksChanged(TrackGroupArray ignored, TrackSelectionArray trackSelections) {
> +        if (DEBUG) Log.e(LOGTAG, "onTracksChanged : TGA[" + ignored +

s/Log.e/Log.d/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:353
(Diff revision 4)
> +    @Override
> +    public synchronized void onTracksChanged(TrackGroupArray ignored, TrackSelectionArray trackSelections) {
> +        if (DEBUG) Log.e(LOGTAG, "onTracksChanged : TGA[" + ignored +
> +                         "], TSA[" + trackSelections + "]");
> +        if (trackGroupUpdated) {
> +            trackGroupUpdated = false;

This looks redundant: the value will be set again before leaving this method anyway.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:371
(Diff revision 4)
> +                    }
> +                }
> +            }
> +        }
> +        hasVideo = numVideoTracks > 0 ? true : false;
> +        hasAudio = numAudioTracks > 0 ? true : false;

`? true : false` is not needed

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:421
(Diff revision 4)
> +
> +    // =======================================================================
> +    // API for GeckoHlsDemuxerWrapper
> +    // =======================================================================
> +    public ConcurrentLinkedQueue<GeckoHlsSample> getVideoSamples(int number) {
> +        return vRenderer != null ? vRenderer.getQueuedSamples(number) :

Please reformat "?:"

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:426
(Diff revision 4)
> +        return vRenderer != null ? vRenderer.getQueuedSamples(number) :
> +                                   new ConcurrentLinkedQueue<GeckoHlsSample>();
> +    }
> +
> +    public ConcurrentLinkedQueue<GeckoHlsSample> getAudioSamples(int number) {
> +        return aRenderer != null ? aRenderer.getQueuedSamples(number) :

ditto

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:446
(Diff revision 4)
> +        long bufferedPos = player.getBufferedPosition() * 1000;
> +        if (DEBUG) Log.d(LOGTAG, "getBufferedPosition : " + bufferedPos + "(Us)");
> +        return bufferedPos;
> +    }
> +
> +    public synchronized int getNumberTracks(Track_Type trackType) {

Nit: s/getNumberTracks/getNumberOfTracks/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:23
(Diff revision 4)
> +
> +public abstract class GeckoHlsRendererBase extends BaseRenderer {
> +    protected static final int QUEUED_INPUT_SAMPLE_DURATION_THRESHOLD = 1000000; //1sec
> +    protected final MediaCodecSelector mediaCodecSelector;
> +    protected final FormatHolder formatHolder = new FormatHolder();
> +    protected boolean DEBUG;

nit: please add comments to explain why `DEBUG` and `LOGTAG` are not static and where they will be set. Also consider change the name because they're not `final` either.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:63
(Diff revision 4)
> +    protected boolean isQueuedEnoughData() {
> +        if (demuxedInputSamples.isEmpty()) {
> +            return false;
> +        }
> +        int size = demuxedInputSamples.size();
> +        GeckoHlsSample[] queuedSamples =

making a copy of queue seems unnecessary, how about keeping track of earliest and latest timestamp instead?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:95
(Diff revision 4)
> +            samples.offer(sample);
> +        }
> +        if (samples.isEmpty()) {
> +            if (DEBUG) Log.d(LOGTAG, "getQueuedSamples isEmpty, waitingForData = true !");
> +            waitingForData = true;
> +        } else if (firstSampleStartTime == 0) {

`0` is a valid value for PTS. Suggest to use `-1` or `Long.MIN_VALUE`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:157
(Diff revision 4)
> +        if (format == null) {
> +            readFormat();
> +        }
> +
> +        maybeInitRenderer();
> +        if (initialized) {

This is also checked in `feedInputBufferQueue()`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:325
(Diff revision 4)
> +         */
> +        if (inputSample != null) {
> +            demuxedNoDurationSamples.offer(inputSample);
> +        }
> +        int sizeOfNoDura = demuxedNoDurationSamples.size();
> +        // A calculation window we've ever found suitable for both HLS TS & FMP4.

IIUC, P-, B-frames won't refer to frames outside GOP. Perhaps we can use I-frames to determine the calculation window

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:331
(Diff revision 4)
> +        int range = sizeOfNoDura >= 10 ? 10 : sizeOfNoDura;
> +        GeckoHlsSample[] inputArray =
> +            demuxedNoDurationSamples.toArray(new GeckoHlsSample[sizeOfNoDura]);
> +        if (range >= 10 && !inputStreamEnded) {
> +            // Calculate the first 'range' elements.
> +            for (int i = 0; i < range; i++) {

L331~342 looks quite similar to L353~361. Please try extract to a private method.

Comment 18

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128510/#review141504
Attachment #8856550 - Flags: review?(jolin) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review141506
Attachment #8856551 - Flags: review?(jolin) → review+
(Assignee)

Comment 20

a year ago
mozreview-review
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review141516

Fixed part of issues.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8856551 - Attachment is obsolete: true
(Assignee)

Comment 24

a year ago
Weird ... where's the Part 1 ...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8856551 - Attachment is obsolete: false
(Assignee)

Updated

a year ago
Attachment #8867605 - Attachment is obsolete: true
(Assignee)

Comment 27

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128508/#review142646

Sorry for bothering ... I accidentily messed up the MozReviewId for Part2 ... 
Though all issues (https://reviewboard.mozilla.org/r/128512/) here have been fixed up, but still need to get reviewed-plus agian. Thanks for the help
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

a year ago
mozreview-review
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review142920

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:89
(Diff revision 2)
> +        return RendererCapabilities.ADAPTIVE_NOT_SEAMLESS | formatSupport;
> +    }
> +
> +    @Override
> +    protected final void createInputBuffer() {
> +        mInputBuffer = ByteBuffer.wrap(new byte[22048]);

Add a comment to explain why `22048`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:49
(Diff revision 2)
> +    private static final DefaultBandwidthMeter BANDWIDTH_METER = new DefaultBandwidthMeter();
> +    private static final int MAX_TIMELINE_ITEM_LINES = 3;
> +    private static boolean DEBUG = false;
> +
> +    private DataSource.Factory mMediaDataSourceFactory;
> +    private Timeline.Period mPeriod;

nit: looks like a local variable in `onTimelineChanged()`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:50
(Diff revision 2)
> +    private static final int MAX_TIMELINE_ITEM_LINES = 3;
> +    private static boolean DEBUG = false;
> +
> +    private DataSource.Factory mMediaDataSourceFactory;
> +    private Timeline.Period mPeriod;
> +    private Timeline.Window mWindow;

ditto

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:73
(Diff revision 2)
> +        private final boolean mEnableA;
> +        RendererController(boolean enableVideoRenderer, boolean enableAudioRenderer) {
> +            this.mEnableV = enableVideoRenderer;
> +            this.mEnableA = enableAudioRenderer;
> +        }
> +        boolean VideoRendererEnabled() { return mEnableV; }

nit: `hasVideo()` or `isVideoRendererEnabled()`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:115
(Diff revision 2)
> +        AUDIO,
> +        VIDEO,
> +        TEXT,
> +    }
> +
> +    public enum RESOURCE_ERROR {

nit:`ResourceError`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:122
(Diff revision 2)
> +        UNKNOWN(-101),
> +        PLAYER(-102),
> +        UNSUPPORTED(-103);
> +
> +        private int mNumVal;
> +        RESOURCE_ERROR(int numVal) {

nit: `private`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:123
(Diff revision 2)
> +        PLAYER(-102),
> +        UNSUPPORTED(-103);
> +
> +        private int mNumVal;
> +        RESOURCE_ERROR(int numVal) {
> +            this.mNumVal = numVal;

nit: s/this.//

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:221
(Diff revision 2)
> +                                         ? uri.getLastPathSegment()
> +                                         : "." + overrideExtension);
> +        switch (type) {
> +            case C.TYPE_HLS:
> +                return new HlsMediaSource(uri, mMediaDataSourceFactory, mMainHandler, null);
> +            default: {

nit: no need for `{}`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:223
(Diff revision 2)
> +        switch (type) {
> +            case C.TYPE_HLS:
> +                return new HlsMediaSource(uri, mMediaDataSourceFactory, mMainHandler, null);
> +            default: {
> +                mResourceCallbacks.onError(RESOURCE_ERROR.UNSUPPORTED.code());
> +                throw new IllegalStateException("Unsupported type: " + type);

`IllegalArgumentException`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:245
(Diff revision 2)
> +    }
> +
> +    synchronized void init(String url) {
> +        if (DEBUG) { Log.d(LOGTAG, " init"); }
> +        assertTrue(mResourceCallbacks != null);
> +        if (mIsPlayerInitDone == true) {

nit: s/ == true//

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:275
(Diff revision 2)
> +        if (DEBUG) {
> +            mEventLogger = new EventLogger(mTrackSelector);
> +            mPlayer.addListener(mEventLogger);
> +        }
> +
> +        Uri[] uris = new Uri[]{Uri.parse(url)};

nit: `[]` is not needed when there is only one item.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:403
(Diff revision 2)
> +    }
> +
> +    public long getDuration() {
> +        assertTrue(mPlayer != null);
> +        // Value returned by getDuration() is in milliseconds.
> +        long duratoin = mPlayer.getDuration() * 1000;

s/duratoin/duration/

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:453
(Diff revision 2)
> +        try {
> +            // TODO : Gather Timeline Period / Window information to develop
> +            //        complete timeline, and seekTime should be inside the duration.
> +            Long startTime = Long.MAX_VALUE;
> +            for (GeckoHlsRendererBase r : mRenderers) {
> +                if (r == mVRenderer  && mRendererController.VideoRendererEnabled() ||

nit: one space before `&&`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:463
(Diff revision 2)
> +            }
> +            if (DEBUG) {
> +                Log.d(LOGTAG, "seeking  : " + positionUs / 1000 +
> +                              " (ms); startTime : " + startTime / 1000 + " (ms)");
> +            }
> +            assertTrue(startTime == Long.MAX_VALUE);

Why this assertion if line 456 could change the value of `startTime`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:36
(Diff revision 2)
> +    protected GeckoHlsPlayer.ComponentListener mPlayerListener;
> +
> +    protected ConcurrentLinkedQueue<GeckoHlsSample> mDemuxedInputSamples = new ConcurrentLinkedQueue<>();
> +
> +    protected ByteBuffer mInputBuffer = null;
> +    protected Format mFormat = null;

This seems redundant. Last item in`mFormats` is `mFormat`, right?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsRendererBase.java:95
(Diff revision 2)
> +
> +    public synchronized ConcurrentLinkedQueue<GeckoHlsSample> getQueuedSamples(int number) {
> +        ConcurrentLinkedQueue<GeckoHlsSample> samples =
> +            new ConcurrentLinkedQueue<GeckoHlsSample>();
> +
> +        int queuedSize = mDemuxedInputSamples.size();

This looks racy if `clearInputSamplesQueue()` will be called in another thread.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:92
(Diff revision 2)
> +         * ADAPTIVE_NOT_SEAMLESS : The Renderer can adapt between formats,
> +         *                         but may suffer a brief discontinuity (~50-100ms)
> +         *                         when adaptation occurs.
> +         * ADAPTIVE_SEAMLESS : The Renderer can seamlessly adapt between formats.
> +         */
> +        final String mimeType = mFormat.sampleMimeType;

Shouldn't this be `format`instead?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:286
(Diff revision 2)
> +        int startPos = 0;
> +        mCSDInfo = new byte[size];
> +        for (int i = 0; i < format.initializationData.size(); i++) {
> +            byte[] data = format.initializationData.get(i);
> +            System.arraycopy(data, 0, mCSDInfo, startPos, data.length);
> +            startPos = data.length;

`+=`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsVideoRenderer.java:337
(Diff revision 2)
> +            mDemuxedNoDurationSamples.offer(inputSample);
> +        }
> +        int sizeOfNoDura = mDemuxedNoDurationSamples.size();
> +        // A calculation window we've ever found suitable for both HLS TS & FMP4.
> +        int range = sizeOfNoDura >= 17 ? 17 : sizeOfNoDura;
> +        GeckoHlsSample[] inputArray =

Will `clearInputSamplesQueue()` be called in another thread? If so, this could be racy.
Attachment #8867605 - Flags: review?(jolin) → review+
(Assignee)

Comment 31

a year ago
mozreview-review-reply
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review142920

> Add a comment to explain why `22048`?

Originally, we're not able to know the size of audio sample becase we could not obtain it from the format.
Therefore we set a size which should be way large enough for all kind of audio samples.

But I've had a test by setting the flag DecoderInputBuffer.BufferReplacementMode to BUFFER_REPLACEMENT_MODE_NORMAL or BUFFER_REPLACEMENT_MODE_DIRECT while createing the buffer for dynamically ensuring the space for sample holder.  It works !

I'll implement that solution for both audio/video.

> Why this assertion if line 456 could change the value of `startTime`?

Uh ... my fault, the assertion should be '!='. I'd like to make sure seek should only happend after renderer is enabled, created, and there's sampe demuxed.
Thanks !

> This looks racy if `clearInputSamplesQueue()` will be called in another thread.

See comments at the bottom.

> Shouldn't this be `format`instead?

Uh ... that's careless mistake.
Thanks

> `+=`?

Sure !  Though from [1], I just thought '=' is quite enough.
With '+=', it is more logically correct.

[1] https://developer.android.com/reference/android/media/MediaCodec.html#CSD

> Will `clearInputSamplesQueue()` be called in another thread? If so, this could be racy.

clearInputSamplesQueue() is called inside GeckoHlsRendererBase.onPositionReset() which is exectued on ExoPlayer's internal playback thread.
And this internal playback thread is also the place on where GeckoHlsRendererBase.render() is exectued.

Now we've put _synchronized_ to GeckoHlsRendererBase.feedInputBuffersQueue(called by ExoPlayer's internal thread) / GeckoHlsRendererBase.getQueuedSamples(called by MFR taskqueue), I'll put _synchronized_ to onPositionReset() too.

That should be enough.

Comment 32

a year ago
mozreview-review
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review143052

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsAudioRenderer.java:89
(Diff revision 2)
> +        return RendererCapabilities.ADAPTIVE_NOT_SEAMLESS | formatSupport;
> +    }
> +
> +    @Override
> +    protected final void createInputBuffer() {
> +        mInputBuffer = ByteBuffer.wrap(new byte[22048]);

This is a very dangerous assumption, there's no reason why an AAC or MP3 blob would be less than 22kB
Attachment #8867605 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 33

a year ago
mozreview-review-reply
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review142920
(Assignee)

Comment 34

a year ago
mozreview-review-reply
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review143052

> This is a very dangerous assumption, there's no reason why an AAC or MP3 blob would be less than 22kB

Yes, this is addressed via the dynamic allocation mechanism provided in DecoderInputBuffer.
Comment hidden (mozreview-request)
(Assignee)

Comment 36

a year ago
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

Discarded because I screwed up the mozreview id.
Cannot track the diff history.
Attachment #8856551 - Attachment is obsolete: true
(Assignee)

Comment 37

a year ago
JYA, John Lin, thanks for the review.  You provided many helpful suggestions and critical insights !
Now, I'm working on Bug 1341990 to make ExoPlayer(r2.4.0) landed in nightly.

Once it's done, I'll come back this bug, and adapt it with very few modifications (Override 1 callback and add 1 parameter while reading source).

Great thanks !
(Assignee)

Comment 38

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128508/#review144164

A slight modification is needed and made due to the upgrade of ExoPlayer from r2.2.0 to r2.4.0 in Bug 1341990.
Now we attempt to stick the version to r2.4.0.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

a year ago
mozreview-review
Comment on attachment 8867605 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/139164/#review144424

Remove the file "EventLogger.java" because we won't use it in debug/non-debug build anymore.
So move some debug trace from EventLogger to GeckoHlsPlayer.java. It's simple.
Comment hidden (mozreview-request)
(Assignee)

Comment 43

a year ago
Hi Nick,

I encountered a problem when push try based on Bug 1341990, 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e433736c917431ae71ad709eb49dd548b2402417

I tried to configure my local repo through "./mach configure --with-gradle" and build it.  Then similar failure happened.
I don't have any clue why it went wrong.  Do I miss something ?
Flags: needinfo?(nalexander)
Hi Nick,

I have the questions related to Kilik's comment43,

I've try to fixed the errors that Kilik encountered,

(1) I've amended the "mobile/android/base/generate_build_config.py"

like 
https://hg.mozilla.org/try/rev/75e0807574dcf09f622fc527f1a5b5e74e4323b9
which we suspected the root cause with the build failure.

But still got failure
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c0fddf8852ff2eb366085ab3785dd38b2f8c69e&selectedJob=100322356



(2) I've removed the suspicious code 

if (!mozconfig.substs.MOZ_ANDROID_HLS_SUPPORT) {
         exclude 'com/google/android/exoplayer2/**'
}
which we added in "mobile/android/thirdparty/build.gradle"
on Bug 1341990 - Part 3: Include ExoPlayer in Firefox for Android builds.

https://hg.mozilla.org/try/rev/e6d099a65b77236946d36adec525443b8b5898d2

The try result still failed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7e65d6b1410f1bc007497328231df2401763c4&selectedJob=100321247


Could you please give us some suggestion?

In Addition, 

May I take it as 
Android 4.0 API15+ opt    (Ant build with gradle check like Lint or CheckStyle...)
Android API15+ Gradle opt (gradle build without checking)?

and there are some problem I met
If I use the try syntax with "android-api-15-gradle-dependencies" from TryChooser,
I will got 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca9407fb1512826c27cc27dcfdf2019e7f9035da
Is it a known issue?
Hence, I can only use "all" to run the gradle build.


Thank you so much.
The issue is that we made ExoPlayer part of the "thirdparty" Gradle module, which correspond to Fennec thirdparty sources and as such are not included in GeckoView at all.  In the moz.build system, we include the ExoPlayer sources in the GeckoView JAR.  I had forgotten that there was a mobile/android/geckoview/src/thirdparty directory into which the sources should go.

This mismatch was simply an oversight on my part.  To address it, we should fold the two patches I just pushed to Bug 1341990 into that ticket before landing.  I've flagged maliu to review those small changes, and NIed him here for the context.  If you rebase on top of my changes, your patches should succeed in try: see

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d3b2b1b9fedd22363f097f36d7a99c1f0d6dcbb

Max: sorry to drop you into the middle of this, but this a good early review to learn about the moz.build and Gradle build systems.
Flags: needinfo?(nalexander)
Flags: needinfo?(max)
(Assignee)

Comment 47

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128508/#review145364

Addressed the naming issue in https://reviewboard.mozilla.org/r/136432/#comment183490.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

a year ago
(In reply to Nick Alexander :nalexander from comment #46)
> Max: sorry to drop you into the middle of this, but this a good early review
> to learn about the moz.build and Gradle build systems.
That is totally fine and really appreciate your invite to this patch. Just talked to Kilik and I learn a lot. Thanks! ;)
Flags: needinfo?(max)
(Assignee)

Comment 52

a year ago
mozreview-review-reply
Comment on attachment 8856551 [details]
Bug 1350241 -Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples.

https://reviewboard.mozilla.org/r/128512/#review140496

> It seems this is only used for `inputFormatChanged()`, which calls `AudioRendererEventListener.onAudioInputFormatChanged()`. Why not just call `playerListener.onAudioInputFormatChanged()` directly?

According to ExoPlayer internal implementation, the handler of tracks information change should be called before renderers startup, that is, we should know the track information before we're aware of format change.

I'll revert this issue.
(Assignee)

Comment 53

a year ago
mozreview-review
Comment on attachment 8856550 [details]
Bug 1350241 -Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec.

https://reviewboard.mozilla.org/r/128510/#review146424

Update according to Bug 1350241 Comment 52. 
Also, per discussion with jolin & jacheng offline, revert the use of ByteBuffer to byte[], because the support of java primative types in Gecko's JNI code-generator is richer.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

a year ago
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd51e245a622
Part1: Provide data structure of {Audio,Video}Info and demuxed sample for the use of HLS on Fennec. r=jolin,jya
https://hg.mozilla.org/integration/autoland/rev/1c15a4b5f778
Part2: Create a customized player based on ExoPlayer and inject customzied {Audio,Video}Renderers as the source of HLS demuxed samples. r=jolin,jya
https://hg.mozilla.org/mozilla-central/rev/bd51e245a622
https://hg.mozilla.org/mozilla-central/rev/1c15a4b5f778
Status: NEW → RESOLVED
Last Resolved: a year 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.