After bug 1309516, the MFR now could decide the start time with going through the MediaDecoderReaderWrapper.

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95648 ::: dom/media/MediaDecoderReader.h:374 (Diff revision 1) > > // Notify if this media is not seekable. > MediaEventProducer<void> mOnMediaNotSeekable; > > + // A flag indicating if the start time is known or not. > + bool mHasStartTime; I prefer in-class initialization: bool mHasStartTime = false; ::: dom/media/MediaDecoderReader.cpp:198 (Diff revision 1) > > media::TimeIntervals > MediaDecoderReader::GetBuffered() > { > MOZ_ASSERT(OnTaskQueue()); > - if (!HaveStartTime()) { > + if (!mHasStartTime) { GetEstimatedBufferedTimeRanges() cares about only mDuration. I wonder why checking if start time is set or not here. ::: dom/media/MediaFormatReader.cpp:791 (Diff revision 1) > std::min(mAudio.mFirstDemuxedSampleTime.refOr(TimeUnit::FromInfinity()), > mVideo.mFirstDemuxedSampleTime.refOr(TimeUnit::FromInfinity())); > > if (!startTime.IsInfinite()) { > mInfo.mStartTime = startTime; // mInfo.mStartTime is initialized to 0. > + mHasStartTime = true; If |startTime| is infinite, we will assume zero start time. Either way, mHasStartTime should always be set and UpdateBuffered() should always be called before resolving the promise. ::: dom/media/MediaFormatReader.cpp:2182 (Diff revision 1) > MOZ_ASSERT(OnTaskQueue()); > media::TimeIntervals videoti; > media::TimeIntervals audioti; > media::TimeIntervals intervals; > > if (!mInitDone) { Might be as simple as: if (!mInitDone || !mHasStartTime) { return intervals; } Don't bother checking ForceZeroStartTime() since we want GetBuffered() to be called at least once before resolving the metadata promise. ::: dom/media/MediaFormatReader.cpp:2218 (Diff revision 1) > if (!intervals.Length() || > intervals.GetStart() == media::TimeUnit::FromMicroseconds(0)) { > // IntervalSet already starts at 0 or is empty, nothing to shift. > return intervals; > } > return intervals.Shift(media::TimeUnit::FromMicroseconds(-startTime)); intervals.Shift(media::TimeUnit() - mInfo.mStartTime); So you don't need the local variale |startTime|.

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95648 > GetEstimatedBufferedTimeRanges() cares about only mDuration. I wonder why checking if start time is set or not here. This was first introduced in bug 1183888 which intends to return empty set if the start time is unknown. Without this check, the GetEstimatedBufferedTimeRanges() always produces some valid intervals. Will update a patch without this check and push to try server.

(In reply to Tzuhao Kuo [:kaku] (PTO 11/7) from comment #4) > Will update a patch without this check and push to try server. Try with the check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e895c82bdb26695cf946ae4136b4936264e0ca82 https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeaa3515a64121b4061911a6a5ff3d1afe8d9c12 Try without the check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbcf389a00e41d7d112c4290a679b497106b3eb2 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f327dfc7a5bd3386a0a672f8b1cf3271dfa7166

(In reply to Tzuhao Kuo [:kaku] (PTO 11/7) from comment #5) > (In reply to Tzuhao Kuo [:kaku] (PTO 11/7) from comment #4) > > Will update a patch without this check and push to try server. > Try with the check: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e895c82bdb26695cf946ae4136b4936264e0ca82 > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=aeaa3515a64121b4061911a6a5ff3d1afe8d9c12 > > Try without the check: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=cbcf389a00e41d7d112c4290a679b497106b3eb2 > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=7f327dfc7a5bd3386a0a672f8b1cf3271dfa7166 The try result is very good, lets get rid of the check!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17cc8bbd66ace7cd04cccab00e710395b340c81a https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ce7f23561b1e83eea6777987e21f15b70c702d7

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95796

Comment on attachment 8814332 [details] Bug 1313635 part 2 - remove DispatchSetStartTime(), HaveStartTime() and StartTime(); https://reviewboard.mozilla.org/r/95602/#review95798

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95824 ::: dom/media/MediaDecoderReader.h:293 (Diff revision 2) > > protected: > virtual ~MediaDecoderReader(); > > + // Recomputes mBuffered. > + virtual void UpdateBuffered(); You don't need to move this funtion from private to protected.

Comment on attachment 8814332 [details] Bug 1313635 part 2 - remove DispatchSetStartTime(), HaveStartTime() and StartTime(); https://reviewboard.mozilla.org/r/95602/#review95826

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95824 > You don't need to move this funtion from private to protected. I think it need to be protected. UpdateBuffered() is used in both MediaDecoderReader and MFR.

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95824 > I think it need to be protected. UpdateBuffered() is used in both MediaDecoderReader and MFR. MFR calls its private override (MFR::UpdateBuffered) so MediaDecoderReader::UpdateBuffered doesn't need to be visible to MFR.

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review95824 > MFR calls its private override (MFR::UpdateBuffered) so MediaDecoderReader::UpdateBuffered doesn't need to be visible to MFR. MFR doesn't override the MediaDecoderReader::UpdateBuffered(). MediaDecoderReader::UpdateBuffered() is just a template at MediaDecoderReader, what is really overrided is MediaDecoderReader::GetBuffered().

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review96280 ::: dom/media/MediaDecoderReader.h:293 (Diff revision 2) > > protected: > virtual ~MediaDecoderReader(); > > + // Recomputes mBuffered. > + virtual void UpdateBuffered(); I see. Then MediaDecoderReader::UpdateBuffered doesn't have to be virtual at all.

Comment on attachment 8814331 [details] Bug 1313635 part 1 - don't go through MediaDecoderReaderWrapper to set start time; https://reviewboard.mozilla.org/r/95600/#review96280 > I see. Then MediaDecoderReader::UpdateBuffered doesn't have to be virtual at all. I need it virtual in bug 1319992. Just so that it can do nothing...

Rebase and try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeaadbcc47654777c46e1c872413107fd1d9afea https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d6c2d7fef70165307f4d2f75e464e9cbcc9bdb4

Try looks pretty good, thanks for reviews!

Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0459bc465abc part 1 - don't go through MediaDecoderReaderWrapper to set start time; r=jwwang,jya https://hg.mozilla.org/integration/autoland/rev/965fded52ad6 part 2 - remove DispatchSetStartTime(), HaveStartTime() and StartTime(); r=jwwang,jya

https://hg.mozilla.org/mozilla-central/rev/0459bc465abc https://hg.mozilla.org/mozilla-central/rev/965fded52ad6