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.

jya, any suggestion regarding Comment 0 ?

(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 :)

(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.

Comment on attachment 8875626 [details] Bug 1369598 - Notify HLSDemuxer initialized after underlying GekcoHlsPlayer is ready with data. https://reviewboard.mozilla.org/r/147054/#review151180

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; }

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

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 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)

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.

try run with mach try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebf99586680d3e5f85d7c11016870b45f3b961b

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