Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776813002:
Bug Fix: WebRTC Receiver Timestamp Jump Detection (Closed)
DescriptionBug Fix: WebRTC Receiver Timestamp Jump Detection
RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame).
Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream.
This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time.
BUG=chromium:705679
Review-Url: https://codereview.webrtc.org/2776813002
Cr-Commit-Position: refs/heads/master@{#17770}
Committed: https://chromium.googlesource.com/external/webrtc/+/067121ab3f97f949a1480502e8a57f11b63a7b35
Patch Set 1 : Style #
Total comments: 3
Patch Set 2 : Timestamp Jump #Messages
Total messages: 27 (15 generated)
Description was changed from ========== Timestamp Jump BUG= ========== to ========== Timestamp Jump BUG= ==========
qiangchen@chromium.org changed reviewers: + pbos@webrtc.org
Can you apply this patch and see whether the problem resolves?
Description was changed from ========== Timestamp Jump BUG= ========== to ========== Bug Fix: WebRTC Receiver Timestamp Jump Detection RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame). Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream. This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time. BUG=705679 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Can you take a look?
pbos@webrtc.org changed reviewers: + nisse@webrtc.org, stefan@webrtc.org
stefan@/nisse@ can either you or someone else take a look at this one? I think I'm a worse pick for time-code stuff.
On 2017/03/29 16:19:44, pbos-webrtc wrote: > stefan@/nisse@ can either you or someone else take a look at this one? I think > I'm a worse pick for time-code stuff. This will address receiving simulcast from RTCVideoEncoder pre-M59 but also any case where the sender gets reset (tab reloads), but the app reuses the SSRC.
I'm not really familiar with this code. But I think it generally makes sense to detect when the sender misbehaves. I'd like to have some confirmation that it solves the problem before the change is landed. And can we have some unit test as well? https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/timestamp_extrapolator.cc (right): https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/timestamp_extrapolator.cc:87: 90 * 1000 * 10) This limit deserves a symbolic name. And I think it makes sense to log a message when reset happens, since it should be rare and indicates a problem somewhere in the system.
Have you looked at this code which also tries to handle something similar? https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/timestamp_extrapolator.cc (right): https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/timestamp_extrapolator.cc:87: 90 * 1000 * 10) On 2017/03/30 06:37:07, nisse-webrtc wrote: > This limit deserves a symbolic name. > > And I think it makes sense to log a message when reset happens, since it should > be rare and indicates a problem somewhere in the system. I think this will trigger cases where we may not want to reset. For instance if there was some network interruption for 10 seconds and then it resumed fine. But maybe it's still fine to reset in that case anyway? Maybe we should use the existing DelayChangeDetection() method instead and reset after looking for a consistent offset in timestamps?
Description was changed from ========== Bug Fix: WebRTC Receiver Timestamp Jump Detection RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame). Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream. This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time. BUG=705679 ========== to ========== Bug Fix: WebRTC Receiver Timestamp Jump Detection RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame). Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream. This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time. BUG=chromium:705679 ==========
stefan@webrtc.org changed reviewers: + philipel@webrtc.org
Re stefan: Yes, I've noticed the timestamp check in VCMReceiver::FrameForDecoding, however, that function is not on the code path now. But it is replaced by FrameBuffer::NextFrame, which does not prohibit timestamp jumping back. pbos@ told me it is related to the new Jitter buffer development. For just within the extrapolator, I just replied your idea. It seems there is a simpler way to fix. https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/timestamp_extrapolator.cc (right): https://codereview.webrtc.org/2776813002/diff/40001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/timestamp_extrapolator.cc:87: 90 * 1000 * 10) On 2017/03/30 06:55:14, stefan-webrtc wrote: > On 2017/03/30 06:37:07, nisse-webrtc wrote: > > This limit deserves a symbolic name. > > > > And I think it makes sense to log a message when reset happens, since it > should > > be rare and indicates a problem somewhere in the system. > > I think this will trigger cases where we may not want to reset. For instance if > there was some network interruption for 10 seconds and then it resumed fine. But > maybe it's still fine to reset in that case anyway? > > Maybe we should use the existing DelayChangeDetection() method instead and reset > after looking for a consistent offset in timestamps? That's a good idea, and I do see the following code (line 109) does it. In that case, _pP[1][1] is set to a large number, which essentially means we throw away the historical sample data, and in turn w will quickly be adjusted to proper value by the following several updates. But line 89 is before this check, which means that if we have a large timestamp jump back, line 109 can never be triggered for long time, further the extrapolater stops updating at all, which in turn results in wrong clock time conversion for a long time. Do you think it is proper to 1. remove my current resetting fix, and 2. put the block (ln88~94) after the block (ln106~116)?
just add a patch utilizing DelayChange function.
lgtm
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492620922615990,
"parent_rev": "6737045af14f3df3537be4c9400936c226601119", "commit_rev":
"067121ab3f97f949a1480502e8a57f11b63a7b35"}
Message was sent while issue was closed.
Description was changed from ========== Bug Fix: WebRTC Receiver Timestamp Jump Detection RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame). Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream. This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time. BUG=chromium:705679 ========== to ========== Bug Fix: WebRTC Receiver Timestamp Jump Detection RTCVideoEncoder does not propagate RTP timestamps properly for encoded video frames, and as such whenever switching between simulcast layers there's a large timestamp gap that causes the incoming stream to freeze (timestamps look like they're either too far ahead or too far behind the previous frame). Ideally RTCVideoEncoder would propagate these timestamps, but even so, when there's a large timestamp gap it would seem reasonable that the receiver resets quickly and consider this to be a new stream. This CL detects the large jump for timestamps, if that happens, we reset the time extrapolator, which is the class for convertion from RTP timestamp to clock time. BUG=chromium:705679 Review-Url: https://codereview.webrtc.org/2776813002 Cr-Commit-Position: refs/heads/master@{#17770} Committed: https://chromium.googlesource.com/external/webrtc/+/067121ab3f97f949a1480502e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/067121ab3f97f949a1480502e... |
