Closed Bug 1299019 Opened 3 years ago Closed 3 years ago

VideoSink::mVideoFrameEndTime could become 0 when the queue is empty

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

(Keywords: regression)

Attachments

(1 file)

https://hg.mozilla.org/mozilla-central/file/tip/dom/media/mediasink/VideoSink.cpp#l418

lastDisplayedFrameEndTime (which is initialized to 0) is assigned to mVideoFrameEndTime (which will become 0) when the queue is empty.

We should take the maximum to ensure mVideoFrameEndTime is mono-increasing as playback advances.
A regression from bug 1258870.
Assignee: nobody → jwwang
Blocks: 1258870
Keywords: regression
Priority: -- → P3
Attachment #8786177 - Flags: review?(kikuo)
Comment on attachment 8786177 [details]
Bug 1299019 - ensure mVideoFrameEndTime is mono-increasing.

https://reviewboard.mozilla.org/r/75136/#review73030

::: dom/media/mediasink/VideoSink.cpp:400
(Diff revision 1)
>    TimeStamp nowTime;
>    const int64_t clockTime = mAudioSink->GetPosition(&nowTime);
>    NS_ASSERTION(clockTime >= 0, "Should have positive clock time.");
>  
>    // Skip frames up to the playback position.
>    int64_t lastDisplayedFrameEndTime = 0;

I think the modification is equivalent to set |int64_t lastDisplayedFrameEndTime = mVideoFrameEndTime;| in advance.

But I'm ok with the modification you've done.
Comment on attachment 8786177 [details]
Bug 1299019 - ensure mVideoFrameEndTime is mono-increasing.

https://reviewboard.mozilla.org/r/75136/#review73032
Attachment #8786177 - Flags: review?(kikuo) → review+
(In reply to Kilik Kuo [:kikuo] from comment #3)
> I think the modification is equivalent to set |int64_t
> lastDisplayedFrameEndTime = mVideoFrameEndTime;| in advance.
> 
> But I'm ok with the modification you've done.

That should work. But the merit of using std::max is to ensure mono-increasing mVideoFrameEndTime in spite of disordered timestamps.

https://hg.mozilla.org/mozilla-central/file/acfb2c3ac6ae0a704e2756184815296ac1314f89/dom/media/mediasink/VideoSink.cpp#l367
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0efb2a2aff27
ensure mVideoFrameEndTime is mono-increasing. r=kikuo
https://hg.mozilla.org/mozilla-central/rev/0efb2a2aff27
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1300056
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.