Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783743003:
Making RtpSender tests cover BWE with overhead. (Closed)
|
Created:
3 years, 8 months ago by minyue-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMaking RtpSender tests cover BWE with overhead.
BUG=webrtc:7418, webrtc:6315
Review-Url: https://codereview.webrtc.org/2783743003
Cr-Commit-Position: refs/heads/master@{#17499}
Committed: https://chromium.googlesource.com/external/webrtc/+/3a407eee1cbadda19ac49da4559f7592a8be59a8
Patch Set 1 #
Total comments: 5
Dependent Patchsets: Messages
Total messages: 16 (7 generated)
Description was changed from ========== Making RtpSender tests cover BWE with overhead. BUG= ========== to ========== Making RtpSender tests cover BWE with overhead. BUG= ==========
minyue@webrtc.org changed reviewers: + danilchap@webrtc.org, michaelt@webrtc.org, philipel@webrtc.org, stefan@webrtc.org
Hi, As you see that more overhead related changes (https://codereview.webrtc.org/2783743003) are on the way, I would like to add extensive (even may be too extensive) tests to cover overhead-included-BWE. WDYT?
May be you want to add the bug number (7418). lgtm Reviewing tests and having two cases (with/without) only when it matters is a different task https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (left): https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1435: class MockOverheadObserver : public OverheadObserver { you already have this Mock defined above.
https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (left): https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1435: class MockOverheadObserver : public OverheadObserver { On 2017/03/29 10:49:11, danilchap wrote: > you already have this Mock defined above. I think I just moved it above https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:351: TransportFeedbackObserverGetsCorrectByteCount) { Add a reason whey moving this up: I removed the check of packet size in the next test, see EXPECT_CALL(feedback_observer_, AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, _, sizeof(kPayloadData) + kGenericHeaderLength, So I want this test to be placed before it to check the packet size.
https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (left): https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1435: class MockOverheadObserver : public OverheadObserver { On 2017/03/29 12:00:39, minyue-webrtc wrote: > On 2017/03/29 10:49:11, danilchap wrote: > > you already have this Mock defined above. > > I think I just moved it above oops, sorry, got confused between 'old code' and 'new code'.
lgtm
Description was changed from ========== Making RtpSender tests cover BWE with overhead. BUG= ========== to ========== Making RtpSender tests cover BWE with overhead. BUG=webrtc:7418 ==========
Added BUG. I am committing this shortly per approvals from Danil and Michael, if I don't see more comments. https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (left): https://codereview.webrtc.org/2783743003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1435: class MockOverheadObserver : public OverheadObserver { On 2017/03/29 12:40:59, danilchap wrote: > On 2017/03/29 12:00:39, minyue-webrtc wrote: > > On 2017/03/29 10:49:11, danilchap wrote: > > > you already have this Mock defined above. > > > > I think I just moved it above > > oops, sorry, got confused between 'old code' and 'new code'. NP, I sometimes get the exactly same confusion.
lgtm
Description was changed from ========== Making RtpSender tests cover BWE with overhead. BUG=webrtc:7418 ========== to ========== Making RtpSender tests cover BWE with overhead. BUG=webrtc:7418, webrtc:6315 ==========
The CQ bit was checked by minyue@webrtc.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": 1, "attempt_start_ts": 1491205709065260, "parent_rev":
"c5d62e29ca21da19a89c0a287c2d5231afa6465a", "commit_rev":
"3a407eee1cbadda19ac49da4559f7592a8be59a8"}
Message was sent while issue was closed.
Description was changed from ========== Making RtpSender tests cover BWE with overhead. BUG=webrtc:7418, webrtc:6315 ========== to ========== Making RtpSender tests cover BWE with overhead. BUG=webrtc:7418, webrtc:6315 Review-Url: https://codereview.webrtc.org/2783743003 Cr-Commit-Position: refs/heads/master@{#17499} Committed: https://chromium.googlesource.com/external/webrtc/+/3a407eee1cbadda19ac49da45... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/3a407eee1cbadda19ac49da45... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
