Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2633923003:
Add rtcp::TransportFeedback::GetReceivedPackets() (Closed)
DescriptionAdd rtcp::TransportFeedback::GetReceivedPackets()
It combines and simplify use of GetStatusVector and GetReceiveDeltas accesors.
Replace use of all GetStatusVector/GetReceiveDeltasUs
BUG=None
Review-Url: https://codereview.webrtc.org/2633923003
Cr-Commit-Position: refs/heads/master@{#16139}
Committed: https://chromium.googlesource.com/external/webrtc/+/4a0c76402eb2257e2d3b62e2350394aa5d61c4ab
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add explicit copy constructor/operator #Patch Set 3 : Fix for android compilers #
Total comments: 30
Patch Set 4 : packet -> feedback_packet to match .cc #Patch Set 5 : nit #Patch Set 6 : . #
Total comments: 2
Messages
Total messages: 27 (16 generated)
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
Nice! lgtm https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:36: int16_t delta_ticks_; Looks like these can be const as well?
Description was changed from ========== Add rtcp::TransportFeedback::GetReceivedPackets() It combines and simplify use of GetStatusVector and GetReceiveDeltas accesors. Replace use of all GetStatusVector/GetReceiveDeltasUs BUG=None ========== to ========== Add rtcp::TransportFeedback::GetReceivedPackets() It combines and simplify use of GetStatusVector and GetReceiveDeltas accesors. Replace use of all GetStatusVector/GetReceiveDeltasUs BUG=None ==========
danilchap@webrtc.org changed reviewers: + minyue@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:36: int16_t delta_ticks_; On 2017/01/17 09:22:02, språng wrote: > Looks like these can be const as well? Let me try... no, they can't: ReceivedPacket is inside std::vector which require ReceivedPacket to have assign operator (used during reallocations). const members prevent that.
https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:36: int16_t delta_ticks_; On 2017/01/17 09:36:30, danilchap wrote: > On 2017/01/17 09:22:02, språng wrote: > > Looks like these can be const as well? > > Let me try... no, they can't: > ReceivedPacket is inside std::vector which require ReceivedPacket to have assign > operator (used during reallocations). const members prevent that. I see. Don't remember what the style guide says, maybe we should explicitly declare the assignment operator and (I presume) copy constructor then?
The CQ bit was checked by danilchap@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/...
https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:36: int16_t delta_ticks_; On 2017/01/17 09:56:21, språng wrote: > On 2017/01/17 09:36:30, danilchap wrote: > > On 2017/01/17 09:22:02, språng wrote: > > > Looks like these can be const as well? > > > > Let me try... no, they can't: > > ReceivedPacket is inside std::vector which require ReceivedPacket to have > assign > > operator (used during reallocations). const members prevent that. > > I see. Don't remember what the style guide says, maybe we should explicitly > declare the assignment operator and (I presume) copy constructor then? You right, explicit it better, even if copy is trivial. https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
Nice! lgtm % nits below https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc (right): https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:26: std::vector<uint16_t> SequenceNumbers(const rtcp::TransportFeedback& packet) { It is unclear whether packet refers to the RTCP feedback or RTP media packet. Maybe rename |packet| to |feedback_packet|? That way it will match the name in remote_estimator_proxy.cc https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:33: feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:45: MOCK_METHOD1(SendFeedback, bool(rtcp::TransportFeedback* packet)); feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:83: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:100: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:123: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:141: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:164: .WillOnce(Invoke([kTooLargeDelta, this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:172: .WillOnce(Invoke([kTooLargeDelta, this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:185: TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) { Based on this test, it looks like "gracefully handling reordered packets and wrapping seq. numbers" means "not sending any feedback for some packets". Is this correct? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:192: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:208: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:224: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:245: .WillOnce(Invoke([kTimeoutTimeMs, this](rtcp::TransportFeedback* packet) { feedback_packet? https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:272: .WillOnce(Invoke([kTimeoutTimeMs, this](rtcp::TransportFeedback* packet) { feedback_packet?
Patchset #4 (id:60001) has been deleted
https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc (right): https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:26: std::vector<uint16_t> SequenceNumbers(const rtcp::TransportFeedback& packet) { On 2017/01/17 16:32:23, terelius wrote: > It is unclear whether packet refers to the RTCP feedback or RTP media packet. > Maybe rename |packet| to |feedback_packet|? That way it will match the name in > remote_estimator_proxy.cc Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:33: On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:45: MOCK_METHOD1(SendFeedback, bool(rtcp::TransportFeedback* packet)); On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:83: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. To avoid growing these lines too much noticed that moving constants outside fixture eliminate need to capture this. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:100: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:123: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:141: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:164: .WillOnce(Invoke([kTooLargeDelta, this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:172: .WillOnce(Invoke([kTooLargeDelta, this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:185: TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) { On 2017/01/17 16:32:23, terelius wrote: > Based on this test, it looks like "gracefully handling reordered packets and > wrapping seq. numbers" means "not sending any feedback for some packets". Is > this correct? Yes, currently 'gracefully' mean 'ignore but do not disturb handling of other packets'. (RemoteEstimatorProxy::OnPacketArrival is responsible for that). https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:192: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:208: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:224: .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:245: .WillOnce(Invoke([kTimeoutTimeMs, this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done. https://codereview.webrtc.org/2633923003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:272: .WillOnce(Invoke([kTimeoutTimeMs, this](rtcp::TransportFeedback* packet) { On 2017/01/17 16:32:23, terelius wrote: > feedback_packet? Done.
lgtm % nit https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:38: uint16_t sequence_number_; these two vars are const too?
https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:38: uint16_t sequence_number_; On 2017/01/18 08:25:27, minyue-webrtc(OOOtoJan17) wrote: > these two vars are const too? can't declare them const because of assign operator (needed for std::vector<ReceivedPacket>). Except for that, they are const.
The CQ bit was checked by danilchap@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/...
On 2017/01/18 09:20:12, danilchap wrote: > https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): > > https://codereview.webrtc.org/2633923003/diff/120001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:38: uint16_t > sequence_number_; > On 2017/01/18 08:25:27, minyue-webrtc(OOOtoJan17) wrote: > > these two vars are const too? > > can't declare them const because of assign operator (needed for > std::vector<ReceivedPacket>). Except for that, they are const. ok, yes
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 danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2633923003/#ps120001 (title: ".")
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": 120001, "attempt_start_ts": 1484735907267520,
"parent_rev": "e0e3bdfbbfedda7a8547a13106da17734366a9e7", "commit_rev":
"4a0c76402eb2257e2d3b62e2350394aa5d61c4ab"}
Message was sent while issue was closed.
Description was changed from ========== Add rtcp::TransportFeedback::GetReceivedPackets() It combines and simplify use of GetStatusVector and GetReceiveDeltas accesors. Replace use of all GetStatusVector/GetReceiveDeltasUs BUG=None ========== to ========== Add rtcp::TransportFeedback::GetReceivedPackets() It combines and simplify use of GetStatusVector and GetReceiveDeltas accesors. Replace use of all GetStatusVector/GetReceiveDeltasUs BUG=None Review-Url: https://codereview.webrtc.org/2633923003 Cr-Commit-Position: refs/heads/master@{#16139} Committed: https://chromium.googlesource.com/external/webrtc/+/4a0c76402eb2257e2d3b62e23... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/4a0c76402eb2257e2d3b62e23... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
