Closed Bug 1306551 Opened 4 years ago Closed 4 years ago

Intermittent dom/media/test/test_background_video_suspend.html | Assertion count 115 is greater than expected range 0-0 assertions from "ASSERTION: Clock should go forwards"

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Assignee: nobody → jwwang
Component: Audio/Video → Audio/Video: Playback
Attachment #8800486 - Flags: review?(kaku)
Blocks: 1276556
Comment on attachment 8800486 [details]
Bug 1306551 - don't update playback position for video-only seek when seek is completed.

https://reviewboard.mozilla.org/r/85398/#review84028

::: dom/media/MediaDecoderStateMachine.cpp:967
(Diff revision 1)
> +      // Don't update playback position for video-only seek.
> +      // Otherwise we might have |newCurrentTime > mMediaSink->GetPosition()|
> +      // and fail the assertion in GetClock() since we didn't stop MediaSink.

Does this happen due to fast seek which might seek to somewhere after the seek target?
Comment on attachment 8800486 [details]
Bug 1306551 - don't update playback position for video-only seek when seek is completed.

https://reviewboard.mozilla.org/r/85398/#review84028

> Does this happen due to fast seek which might seek to somewhere after the seek target?

Since we pass GetMediaTime() to MediaSink::Start(), the clock time returned by the media sink is always >= GetMediaTime(). However, we don't restart the media sink for the video-only seek, the invariant doesn't necessarily hold. We could have newCurrentTime > mMediaSink->GetPosition() when seek is completed.
Comment on attachment 8800486 [details]
Bug 1306551 - don't update playback position for video-only seek when seek is completed.

https://reviewboard.mozilla.org/r/85398/#review84028

> Since we pass GetMediaTime() to MediaSink::Start(), the clock time returned by the media sink is always >= GetMediaTime(). However, we don't restart the media sink for the video-only seek, the invariant doesn't necessarily hold. We could have newCurrentTime > mMediaSink->GetPosition() when seek is completed.

But, you see, if we trigger the video-only-seek at media time _m_, clock time _c_ ( _m_ <= _c_) and the media seek keeps going. 
After time _t_, the seek is resolved and the media time becomes _m_ + _t_ and the clock time becomes _c_ + _t_. 
The _newCurrentTime_ could be set to:
(1) the original seek-target, which is smaller than the current clock.
(2) the start time of the 1st audio sample in the audio queue, which follows the current clock since we do not do audio seek.
(3) the start time of the 1st video sample in the video queue, which might be larger than the current clock if fast seek was applied.

Not sure if the above analysis is right or not...

Whatever, I think it's just right to not update the media time here if we did not stop the playback.
Comment on attachment 8800486 [details]
Bug 1306551 - don't update playback position for video-only seek when seek is completed.

https://reviewboard.mozilla.org/r/85398/#review84074
Attachment #8800486 - Flags: review?(kaku) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e2c6be60379
don't update playback position for video-only seek when seek is completed. r=kaku
https://hg.mozilla.org/mozilla-central/rev/4e2c6be60379
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.