Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633843003:
Added validation between RTP and RTCP timestamps (Closed)
|
Created:
4 years, 10 months ago by danilchap Modified:
4 years, 10 months ago Reviewers:
pbos-webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, andresp, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChanged test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports.
Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time.
BUG=webrtc:5433
Committed: https://crrev.com/f4b9c775122b463db7eb2c4101603759a0d00caf
Cr-Commit-Position: refs/heads/master@{#11417}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 2
Patch Set 7 : dropped double-locking #Messages
Total messages: 17 (6 generated)
Description was changed from ========== Added validation gap between RTP and RTCP timestamps is not too big BUG=webrtc:5433 ========== to ========== Changed test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports. Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time. BUG=webrtc:5433 ==========
danilchap@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1633843003/diff/60001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/60001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2955: void ValidateTimestampGap(uint32_t ssrc, uint32_t timestamp) { Can you use TimeDiff or TimeIsBetween in webrtc/base/timeutils.h to not implement another roll-over?
https://codereview.webrtc.org/1633843003/diff/60001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/60001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2955: void ValidateTimestampGap(uint32_t ssrc, uint32_t timestamp) { On 2016/01/28 10:25:06, pbos-webrtc wrote: > Can you use TimeDiff or TimeIsBetween in webrtc/base/timeutils.h to not > implement another roll-over? Thanks, that makes test more clear what is checked.
https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2962: auto find_timestamp = last_observed_timestamp_.find(ssrc); s/find_timestamp/timestamp_it https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2962: auto find_timestamp = last_observed_timestamp_.find(ssrc); Should this map (and function) also be guarded by crit_?
https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2962: auto find_timestamp = last_observed_timestamp_.find(ssrc); On 2016/01/28 12:10:12, pbos-webrtc wrote: > Should this map (and function) also be guarded by crit_? Yes, RTP and RTCP arrive on different threads. Added also a thread_check_ as a documentation why last_observed_sequence_number_ and configured_ssrcs_ do not need protection. https://codereview.webrtc.org/1633843003/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2962: auto find_timestamp = last_observed_timestamp_.find(ssrc); On 2016/01/28 12:10:12, pbos-webrtc wrote: > s/find_timestamp/timestamp_it Done.
lgtm if you drop the double-locking https://codereview.webrtc.org/1633843003/diff/100001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/100001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:2962: void ValidateTimestampGap(uint32_t ssrc, uint32_t timestamp) { EXCLUSIVE_LOCKS_REQUIRED(crit_) before {, then drop the lock inside this function (and call it from under crit_).
https://codereview.webrtc.org/1633843003/diff/100001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/1633843003/diff/100001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:2962: void ValidateTimestampGap(uint32_t ssrc, uint32_t timestamp) { On 2016/01/28 12:50:28, pbos-webrtc wrote: > EXCLUSIVE_LOCKS_REQUIRED(crit_) before {, then drop the lock inside this > function (and call it from under crit_). Done.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1633843003/#ps120001 (title: "dropped double-locking")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633843003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633843003/120001
Message was sent while issue was closed.
Description was changed from ========== Changed test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports. Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time. BUG=webrtc:5433 ========== to ========== Changed test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports. Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time. BUG=webrtc:5433 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Changed test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports. Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time. BUG=webrtc:5433 ========== to ========== Changed test to validate rtp timstamps not just in RTP packets but also in RTCP Sender Reports. Altered it to accept negative value since it is normal for RTCP packet coming before RTP packet to have slightly later time. BUG=webrtc:5433 Committed: https://crrev.com/f4b9c775122b463db7eb2c4101603759a0d00caf Cr-Commit-Position: refs/heads/master@{#11417} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f4b9c775122b463db7eb2c4101603759a0d00caf Cr-Commit-Position: refs/heads/master@{#11417}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.webrtc.org/1652973002/ by danilchap@webrtc.org. The reason for reverting is: May be the reason for mac_asan timeout. |
