Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649913002:
Count FlexFEC packets in |fec_bitrate_| in RTPSenderVideo. (Closed)
DescriptionCount FlexFEC packets in |fec_bitrate_| in RTPSenderVideo.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2649913002
Cr-Commit-Position: refs/heads/master@{#16238}
Committed: https://chromium.googlesource.com/external/webrtc/+/81eab61172a1f509f775ee369184973d721fe0e1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Unit test. #
Total comments: 5
Patch Set 3 : danilchap comments 2. #
Messages
Total messages: 18 (10 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org
Old TODO that I have missed to fix.
The CQ bit was checked by brandtr@webrtc.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: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10080) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15438) win_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5499) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21902)
https://codereview.webrtc.org/2649913002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2649913002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:207: fec_bitrate_.Update(packet_length, clock_->TimeInMilliseconds()); add/extend a unittest for this.
https://codereview.webrtc.org/2649913002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2649913002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:207: fec_bitrate_.Update(packet_length, clock_->TimeInMilliseconds()); On 2017/01/23 12:42:39, danilchap wrote: > add/extend a unittest for this. Done.
lgtm https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:922: .WillRepeatedly(testing::Return()); if it doesn't matter how many times InsertPacket will run, you can try to use ON_CALL instead of EXPECT_CALL. but may be instead of WillRepeatedly write Times(20) since you sure how many packets will be sent. or, if you want to remove comments: const size_t kMediaPackets = 10; const size_t kFecPackets = kMediaPackets; .Times(kMediaPackets + kFecPackets); https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:933: EXPECT_EQ(static_cast<uint32_t>(10 * 8 * kPacketLength / 0.101f), may be EXPECT_NEAR to avoid cast and magic constant 0.101f
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:922: .WillRepeatedly(testing::Return()); On 2017/01/24 09:56:27, danilchap wrote: > if it doesn't matter how many times InsertPacket will run, you can try to use > ON_CALL instead of EXPECT_CALL. > but may be instead of WillRepeatedly write Times(20) since you sure how many > packets will be sent. > or, if you want to remove comments: > const size_t kMediaPackets = 10; > const size_t kFecPackets = kMediaPackets; > .Times(kMediaPackets + kFecPackets); Done. https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:933: EXPECT_EQ(static_cast<uint32_t>(10 * 8 * kPacketLength / 0.101f), On 2017/01/24 09:56:27, danilchap wrote: > may be EXPECT_NEAR to avoid cast and magic constant 0.101f Done. WDYT?
https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2649913002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:933: EXPECT_EQ(static_cast<uint32_t>(10 * 8 * kPacketLength / 0.101f), On 2017/01/24 10:28:13, brandtr wrote: > On 2017/01/24 09:56:27, danilchap wrote: > > may be EXPECT_NEAR to avoid cast and magic constant 0.101f > > Done. WDYT? Nice!
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2649913002/#ps60001 (title: "danilchap comments 2.")
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": 1485257198957680,
"parent_rev": "0608ffdcb40f5cadaf843d312b82662e54910b90", "commit_rev":
"81eab61172a1f509f775ee369184973d721fe0e1"}
Message was sent while issue was closed.
Description was changed from ========== Count FlexFEC packets in |fec_bitrate_| in RTPSenderVideo. BUG=webrtc:5654 ========== to ========== Count FlexFEC packets in |fec_bitrate_| in RTPSenderVideo. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2649913002 Cr-Commit-Position: refs/heads/master@{#16238} Committed: https://chromium.googlesource.com/external/webrtc/+/81eab61172a1f509f775ee369... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/81eab61172a1f509f775ee369... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
