[Fennec][HLS] Make HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kikuo, Assigned: kikuo)

Tracking

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

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
MediaFormatReader may get nothing but a wait-for-data notification from HLSDemxuer when it requests the first demuxed video/audio sample for start time, but the data may not be downloaded/arrived at that time in the underlying java components.

As ExoPlayer always adds a time stamp offset of 60,000,000(Us) for each demuxed sample.

We either
1) Make HLSDemuxer initialized after there's data in underlying components. Handling this in java is much easier.

2) A. We should not assign an infinite value here [1] just because the failure of demuxing first sample right after demuxer initialized . Because we can eventually demux a sample later if the media is correct.
   B. After MFR enters wait-for-data state due to the failure of demuxing the first sample, MFR should demux the first sample for start time AGAIN when it receives data-arrived notification.

[1] http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/dom/media/MediaFormatReader.cpp#3178


I think 2) is kinda a refactor to make MFR general and reliable.
I'm not sure about, for MFR, whether there is an assumption that data should be ready  right after demuxer has been initialized or not.
(Assignee)

Comment 1

a year ago
jya, any suggestion regarding Comment 0 ?
Flags: needinfo?(jyavenard)
(In reply to Kilik Kuo [:kikuo] from comment #0)
> MediaFormatReader may get nothing but a wait-for-data notification from
> HLSDemxuer when it requests the first demuxed video/audio sample for start
> time, but the data may not be downloaded/arrived at that time in the
> underlying java components.
> 
> As ExoPlayer always adds a time stamp offset of 60,000,000(Us) for each
> demuxed sample.
> 
> We either
> 1) Make HLSDemuxer initialized after there's data in underlying components.
> Handling this in java is much easier.
> 
> 2) A. We should not assign an infinite value here [1] just because the
> failure of demuxing first sample right after demuxer initialized . Because
> we can eventually demux a sample later if the media is correct.
>    B. After MFR enters wait-for-data state due to the failure of demuxing
> the first sample, MFR should demux the first sample for start time AGAIN
> when it receives data-arrived notification.

that's exactly what the MFR will do once NotifyDataArrived is called.
Is the HLS java wrapper properly calling MediaDecoder::NotifyDataArrived() once new data is available?

The way to works is thise.
The MDSM called MFR::RequestVideoData() and is given a VideoDataPromise

The MFR will attempt to demux a new sample, if this demux fail with WAITING_FOR_DATA then the MFR will reject the VideoDataPromise with WAITING_FOR_DATA.
The MDSM then calls a MFR::WaitForData() and is returned a WaitForDataPromise.

Then next time NotifyDataArrived() is called, the WaitForDataPromise is resolved and the MDSM will call once again MFR::RequestVideoData and will attempt to demux a new sample again.

Everything relies on NotifyDataArrived being called as requied.

For example, with MSE, once an appendBuffer completes, NotifyDataArrived is called there:
https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBuffer.cpp#451


> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 972b7a5149bbb2eaab48aafa87678c33d5f2f045/dom/media/MediaFormatReader.cpp#3178
> 
> 
> I think 2) is kinda a refactor to make MFR general and reliable.
> I'm not sure about, for MFR, whether there is an assumption that data should
> be ready  right after demuxer has been initialized or not.

There should be nothing to do in the MFR to handle this case. If you do, you're doing it wrong :)
Flags: needinfo?(jyavenard)
(Assignee)

Comment 3

a year ago
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> (In reply to Kilik Kuo [:kikuo] from comment #0)
> > MediaFormatReader may get nothing but a wait-for-data notification from
> > HLSDemxuer when it requests the first demuxed video/audio sample for start
> > time, but the data may not be downloaded/arrived at that time in the
> > underlying java components.
> > 
> > As ExoPlayer always adds a time stamp offset of 60,000,000(Us) for each
> > demuxed sample.
> > 
> > We either
> > 1) Make HLSDemuxer initialized after there's data in underlying components.
> > Handling this in java is much easier.
> > 
> > 2) A. We should not assign an infinite value here [1] just because the
> > failure of demuxing first sample right after demuxer initialized . Because
> > we can eventually demux a sample later if the media is correct.
> >    B. After MFR enters wait-for-data state due to the failure of demuxing
> > the first sample, MFR should demux the first sample for start time AGAIN
> > when it receives data-arrived notification.
> 
> that's exactly what the MFR will do once NotifyDataArrived is called.
> Is the HLS java wrapper properly calling MediaDecoder::NotifyDataArrived()
> once new data is available?
> 
> The way to works is thise.
> The MDSM called MFR::RequestVideoData() and is given a VideoDataPromise
> 
> The MFR will attempt to demux a new sample, if this demux fail with
> WAITING_FOR_DATA then the MFR will reject the VideoDataPromise with
> WAITING_FOR_DATA.
> The MDSM then calls a MFR::WaitForData() and is returned a
> WaitForDataPromise.
> 
> Then next time NotifyDataArrived() is called, the WaitForDataPromise is
> resolved and the MDSM will call once again MFR::RequestVideoData and will
> attempt to demux a new sample again.
> 
> Everything relies on NotifyDataArrived being called as requied.
> 
> For example, with MSE, once an appendBuffer completes, NotifyDataArrived is
> called there:
> https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/
> SourceBuffer.cpp#451
> 
> 

Ah, I might not explain it well.

The "1st" action of demuxing sample is triggered in the resolving callback MFR::OnDemuxerInitDone().
The result of this "1st" action leads to either |MFR::OnFirstDemuxCompleted()| or |MFR::OnFirstDemuxFailed()| where {mAudio.mFirstDemuxedSampleTime,mVideo.mFirstDemuxedSampleTime} is set to SOME_TIMESTAMP or INFINITY accordingly.
Then MFR may be resolving MetadataPromise based on the FirstDemuxedSampleTime.

If we hit MFR::OnFirstDemuxFailed(), the starttime [1] will be infinity and won't do any change to mInfo.mStartTime nor MediaDecoderReaderWrapper::StartTime().   That means, they are still 0.

[1] http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/media/MediaFormatReader.cpp#1490

Even after MFR::NotifyDataArrived is called and MFR completes the first "successful" sample-demuxing action, MediaDecoderReaderWrapper still consider the start time being 0, but in fact it's not the first sample's timestamp.
That's the defect I was talking about.
Does that make sense to you ?

If we want to keep the mechanism (Bug 1309516) to get the start time of first demuxed sample before decoding first frame, I think one way is to update the mStartTime in MediaDecoderReaderWrapper after MFR get "the 1st successfully demuxed" sample, so that we can avoid a playback delay like the case here.

Another way is to notify HLSDemuxer::onInitialized after we received data in underlying java wrapper.
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8875626 [details]
Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

https://reviewboard.mozilla.org/r/147054/#review151180
Attachment #8875626 - Flags: review?(jolin) → review+
Maybe we can assume that the first sample demuxed by the HLS demuxer will always be 0, in which case have the demuxer use the method:

bool ShouldComputeStartTime() const override { return false; }
Flags: needinfo?(jyavenard)
it could be that the code in bug 1309516 and the handling of the first demuxed sample is broken and doesn't handle failure properly (it should simply retry later).

The reason it didn't matter if the failure wasn't handled was that the only other demuxer that can return WAITING_FOR_DATA is the MSE demuxer, and it doesn't care about the time of the first sample demuxed. By spec, the media always start at time 0.

If the media doesn't start at 0, playback won't start until you seek to a buffered position.

Comment 8

a year ago
mozreview-review
Comment on attachment 8875626 [details]
Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

https://reviewboard.mozilla.org/r/147054/#review151196

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:108
(Diff revision 1)
> +            } else if (trackType == C.TRACK_TYPE_AUDIO) {
> +                mAudioDataArrived = true;
> +            }
> +        }
>          public boolean videoReady() {
> -            return hasVideo() ? mVideoInfoUpdated : true;
> +            return hasVideo() ? mVideoInfoUpdated && mVideoDataArrived : true;

return !hasVideo() || (mVideoInfoUpdated && mVideoDataArrived);

same for audioReady
(Assignee)

Comment 9

a year ago
mozreview-review
Comment on attachment 8875626 [details]
Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

https://reviewboard.mozilla.org/r/147052/#review151248

Issue addressed !
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
Comment on attachment 8875626 [details]
Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

https://reviewboard.mozilla.org/r/147054/#review151276

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:108
(Diff revisions 1 - 2)
>              } else if (trackType == C.TRACK_TYPE_AUDIO) {
>                  mAudioDataArrived = true;
>              }
>          }
>          public boolean videoReady() {
> -            return hasVideo() ? mVideoInfoUpdated && mVideoDataArrived : true;
> +            return !hasVideo() || mVideoInfoUpdated && mVideoDataArrived;

you likely want to put the 2nd terms with parenthesis (in C++ this would give a compilation warning)
(Assignee)

Comment 12

a year ago
mozreview-review
Comment on attachment 8875626 [details]
Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data.

https://reviewboard.mozilla.org/r/147054/#review151338

Thanks for reminder, while compiling java, no warning is shown here.  But I still put the parenthese to the 2nd condition.
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d3323e4e071
Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data. r=jolin

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d3323e4e071
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1373329
You need to log in before you can comment on or make changes to this bug.