Improve handling of frame timestamps
Categories
(Core :: WebRTC: Audio/Video, enhancement, P2)
Tracking
()
People
(Reporter: dminor, Assigned: pehrsons)
References
Details
Attachments
(10 files, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
In Bug 1522238, we use webrtc::Clock::GetRealTimeClock() to set the frame timestamp.
As Andreas pointed out during review, we get the actual capture time above in QueueVideoChunk as a timestamp. We could convert that to a TimeDuration, e.g. by subtracting TimeStamp::GetProcessStart() and use that instead.
The wrinkle is that those timestamps could be in the future, which webrtc.org code explicitly disallows in video_stream_encoder.cc, resetting any future timestamps to the current time.
Beyond that, there's a bit of risk if we're not using the same epoch as the webrtc code for our timestamps, that if they do overwrite it, we'll end up sending out wildly varying timestamps which could cause difficult to track down bugs. Meet relies upon the timestamps being valid, so we should test that site carefully if we make any changes to the timestamps.
One solution that Andreas mentioned for the future timestamp problem is to buffer them locally in MediaPipeline until the future is now. We could also try to fix the upstream code to support this properly.
Reporter | ||
Comment 1•6 years ago
|
||
Here's an example of where the frame timestamp is expected to be directly comparable to the webrtc.org real time clock [1].
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Assignee | ||
Comment 3•3 years ago
|
||
Assignee | ||
Comment 4•3 years ago
|
||
This also fixes what looks like a bug with the timestamp (wrong base) in
WebrtcMediaDataDecoderCodec.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
The new Pacer includes both pacing of incoming frames, and the duplication logic
from VideoFrameConverter. It guarantees that no events happen early, thanks to
MediaTimer.
The largest difference from the old pacing logic is that only one timer is used,
VideoFrameConverter used two -- one for pacing and one for same-frame
duplications. The same-frame timer was positioned later in the pipe, to avoid
convert the same input frame multiple times, something users of the new Pacer
will have to handle. Two timers however uncovered flaws from nsTimer -- namely
that the ordering between two timers is not guaranteed as there is no nsTimer
API using absolute timestamps. Using only one timer avoids this.
Assignee | ||
Comment 6•3 years ago
|
||
Assignee | ||
Comment 7•3 years ago
|
||
Assignee | ||
Comment 8•3 years ago
|
||
Assignee | ||
Comment 9•3 years ago
|
||
Assignee | ||
Comment 10•3 years ago
|
||
Assignee | ||
Comment 11•3 years ago
|
||
Assignee | ||
Comment 12•3 years ago
|
||
Comment 14•3 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d0640f52161d Rewrite the pacing part of VideoFrameConverter as Pacer<T>. r=bwc https://hg.mozilla.org/integration/autoland/rev/ac180bf89fd0 Introduce initHighResolutionWithNamedFuncCallback for nsITimer. r=xpcom-reviewers,nika https://hg.mozilla.org/integration/autoland/rev/142d94304c86 Improve MediaTimer accuracy. r=padenot https://hg.mozilla.org/integration/autoland/rev/8c204db3e91d Propagate gecko timestamps for video into libwebrtc. r=bwc https://hg.mozilla.org/integration/autoland/rev/8a8ee5055c0d Update some more webrtc::VideoFrame ctors. r=bwc,jolin https://hg.mozilla.org/integration/autoland/rev/033cc341ad2e Add gtests for the race between the same-frame-timer and the pacing-timer. r=bwc https://hg.mozilla.org/integration/autoland/rev/bd2122ea3cc3 Don't let time go backwards in VideoFrameConverter. r=bwc https://hg.mozilla.org/integration/autoland/rev/39e2b3143ecd Improve VideoFrameConverter logging and dispatch assertions. r=bwc https://hg.mozilla.org/integration/autoland/rev/3c5d719c54db Make VideoFrameConverter use the new Pacer. r=bwc https://hg.mozilla.org/integration/autoland/rev/e7b7072ac90c Make VideoFrameConverter serve frames though a MediaEventSource. r=bwc
Comment 15•3 years ago
|
||
Backed out for causing bp-hybrid build bustages on TestPacer.
Failure log for OS X Cross Compiled plain
Failure log for Linux x64 plain
Failure log for Windows 2012 x64 debug
Comment 16•3 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d4a050b13db0 Rewrite the pacing part of VideoFrameConverter as Pacer<T>. r=bwc https://hg.mozilla.org/integration/autoland/rev/f5270dce0170 Introduce initHighResolutionWithNamedFuncCallback for nsITimer. r=xpcom-reviewers,nika https://hg.mozilla.org/integration/autoland/rev/d091a7c071ba Improve MediaTimer accuracy. r=padenot https://hg.mozilla.org/integration/autoland/rev/d5aba13d6d90 Propagate gecko timestamps for video into libwebrtc. r=bwc https://hg.mozilla.org/integration/autoland/rev/17fbdf16f20f Update some more webrtc::VideoFrame ctors. r=bwc,jolin https://hg.mozilla.org/integration/autoland/rev/7d48e575c07d Add gtests for the race between the same-frame-timer and the pacing-timer. r=bwc https://hg.mozilla.org/integration/autoland/rev/2e9e4635a3cc Don't let time go backwards in VideoFrameConverter. r=bwc https://hg.mozilla.org/integration/autoland/rev/810bed84bde4 Improve VideoFrameConverter logging and dispatch assertions. r=bwc https://hg.mozilla.org/integration/autoland/rev/4fe3ec529a44 Make VideoFrameConverter use the new Pacer. r=bwc https://hg.mozilla.org/integration/autoland/rev/e3a169560ee8 Make VideoFrameConverter serve frames though a MediaEventSource. r=bwc
Comment 17•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d4a050b13db0
https://hg.mozilla.org/mozilla-central/rev/f5270dce0170
https://hg.mozilla.org/mozilla-central/rev/d091a7c071ba
https://hg.mozilla.org/mozilla-central/rev/d5aba13d6d90
https://hg.mozilla.org/mozilla-central/rev/17fbdf16f20f
https://hg.mozilla.org/mozilla-central/rev/7d48e575c07d
https://hg.mozilla.org/mozilla-central/rev/2e9e4635a3cc
https://hg.mozilla.org/mozilla-central/rev/810bed84bde4
https://hg.mozilla.org/mozilla-central/rev/4fe3ec529a44
https://hg.mozilla.org/mozilla-central/rev/e3a169560ee8
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Description
•