Closed Bug 1313635 Opened 3 years ago Closed 3 years ago

Remove MediaDecoder::DispatchSetStartTime()

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(2 files)

After bug 1309516, the MFR now could decide the start time with going through the MediaDecoderReaderWrapper.
Depends on: 1309516, 1313632
Assignee: nobody → kaku
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.
Attachment #8814331 - Flags: review?(jyavenard)
Attachment #8814332 - Flags: review?(jyavenard)
(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!
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
Attachment #8814331 - Flags: review?(jyavenard) → review+
Comment on attachment 8814332 [details]
Bug 1313635 part 2 - remove DispatchSetStartTime(), HaveStartTime() and StartTime();

https://reviewboard.mozilla.org/r/95602/#review95798
Attachment #8814332 - Flags: review?(jyavenard) → review+
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.
Attachment #8814331 - Flags: review?(jwwang) → review+
Comment on attachment 8814332 [details]
Bug 1313635 part 2 - remove DispatchSetStartTime(), HaveStartTime() and StartTime();

https://reviewboard.mozilla.org/r/95602/#review95826
Attachment #8814332 - Flags: review?(jwwang) → review+
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...
Try looks pretty good, thanks for reviews!
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0459bc465abc
https://hg.mozilla.org/mozilla-central/rev/965fded52ad6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.