Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1291113004:
Add unit tests for more packet types in rtcp_sender_unittest. (Closed)
|
Created:
5 years, 4 months ago by åsapersson Modified:
5 years, 4 months ago Reviewers:
sprang_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd unit tests for more packet types in rtcp_sender_unittest.
BUG=webrtc:2450
Committed: https://crrev.com/22ff75a1635597d96644084707645b11bb3e6f95
Cr-Commit-Position: refs/heads/master@{#9751}
Patch Set 1 : #Patch Set 2 : #
Total comments: 14
Patch Set 3 : addressed comments #
Total comments: 2
Patch Set 4 : addressed comments #Messages
Total messages: 21 (8 generated)
Patchset #1 (id:1) has been deleted
asapersson@webrtc.org changed reviewers: + sprang@webrtc.org
Looks a lot better now, nice work! LG, just a few minor comments. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:242: RTPHeader header = {}; Do we need {} here? https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:387: EXPECT_EQ(0, strncmp((const char*)kData, static_cast rather than c-cast https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:550: EXPECT_EQ(1, parser()->voip_metric()->LossRate()); Reuse metric.lossRate here? Makes it more readable since the number itself isn't significant as the ones above here are. Same below. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:669: const unsigned int kBitrate = 312000; kBitrateBps? Just since we're using kbps below.. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:676: EXPECT_EQ(312U, parser()->tmmbr_item()->BitrateKbps()); kBitrate / 1000 https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:704: bounding_set.AddEntry(32768, 40, kSourceSsrc); Named constants for 32768 and 40
https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:242: RTPHeader header = {}; On 2015/08/19 12:25:07, språng wrote: > Do we need {} here? Would like to initialize some members for example paddingLength. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:387: EXPECT_EQ(0, strncmp((const char*)kData, On 2015/08/19 12:25:07, språng wrote: > static_cast rather than c-cast Changed this. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:550: EXPECT_EQ(1, parser()->voip_metric()->LossRate()); On 2015/08/19 12:25:07, språng wrote: > Reuse metric.lossRate here? Makes it more readable since the number itself isn't > significant as the ones above here are. > Same below. Done. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:669: const unsigned int kBitrate = 312000; On 2015/08/19 12:25:07, språng wrote: > kBitrateBps? Just since we're using kbps below.. Done. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:676: EXPECT_EQ(312U, parser()->tmmbr_item()->BitrateKbps()); On 2015/08/19 12:25:07, språng wrote: > kBitrate / 1000 Done. https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:704: bounding_set.AddEntry(32768, 40, kSourceSsrc); On 2015/08/19 12:25:07, språng wrote: > Named constants for 32768 and 40 Done.
https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:242: RTPHeader header = {}; On 2015/08/19 14:46:06, åsapersson wrote: > On 2015/08/19 12:25:07, språng wrote: > > Do we need {} here? > > Would like to initialize some members for example paddingLength. This is an empty initializer list, won't that have the same effect as the default constructor? So same as RTPHeader header; Or am I missing something? https://codereview.webrtc.org/1291113004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:586: EXPECT_EQ(0x11111111U, parser()->dlrr_items()->Ssrc(0)); last_xr_rr.sourceSSRC etc
Patchset #4 (id:80001) has been deleted
https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:242: RTPHeader header = {}; On 2015/08/19 15:17:03, språng wrote: > On 2015/08/19 14:46:06, åsapersson wrote: > > On 2015/08/19 12:25:07, språng wrote: > > > Do we need {} here? > > > > Would like to initialize some members for example paddingLength. > > This is an empty initializer list, won't that have the same effect as the > default constructor? So same as > > RTPHeader header; > > Or am I missing something? Done (didn't think it had a constructor). https://codereview.webrtc.org/1291113004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right): https://codereview.webrtc.org/1291113004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:586: EXPECT_EQ(0x11111111U, parser()->dlrr_items()->Ssrc(0)); On 2015/08/19 15:17:03, språng wrote: > last_xr_rr.sourceSSRC > etc Done.
lgtm
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291113004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291113004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291113004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291113004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291113004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291113004/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/22ff75a1635597d96644084707645b11bb3e6f95 Cr-Commit-Position: refs/heads/master@{#9751} |
