Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754373002:
TransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire fe… (Closed)
|
Created:
3 years, 9 months ago by elad.alon Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire feedback
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2754373002
Cr-Commit-Position: refs/heads/master@{#17322}
Committed: https://chromium.googlesource.com/external/webrtc/+/92e448d5294b6efe6edb610090d28e07531801d8
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR response #Patch Set 3 : . #
Total comments: 4
Patch Set 4 : Renaming #Patch Set 5 : . #
Depends on Patchset: Messages
Total messages: 40 (21 generated)
Description was changed from ========== TransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire feedback BUG=webrtc:7058 ========== to ========== TransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire feedback BUG=webrtc:7058 ==========
elad.alon@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
Ready for review.
https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:104: const auto& it = packet_status_window_.find(packet.sequence_number); Can this be more efficient if we avoid start searching from the beginning each time? https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: void OnNewTransportFeedbacks( How about OnPacketFeedback()?
https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:104: const auto& it = packet_status_window_.find(packet.sequence_number); On 2017/03/20 18:14:44, stefan-webrtc wrote: > Can this be more efficient if we avoid start searching from the beginning each > time? Maybe, although I am not sure if the complexity of the code (*) would be worth the savings in search-time. How about we do that in a separate CL? (This was not introduced in this CL, so no degradation here.) --- (*) First lookup, wrap-around of sequence number, sequence numbers in the vector which aren't in the map, etc. https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: void OnNewTransportFeedbacks( On 2017/03/20 18:14:44, stefan-webrtc wrote: > How about OnPacketFeedback()? OnPacketFeedbacks, plural, maybe? Wdyt?
https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:104: const auto& it = packet_status_window_.find(packet.sequence_number); On 2017/03/20 18:21:51, elad.alon wrote: > On 2017/03/20 18:14:44, stefan-webrtc wrote: > > Can this be more efficient if we avoid start searching from the beginning each > > time? > > Maybe, although I am not sure if the complexity of the code (*) would be worth > the savings in search-time. How about we do that in a separate CL? (This was not > introduced in this CL, so no degradation here.) > > --- > (*) First lookup, wrap-around of sequence number, sequence numbers in the vector > which aren't in the map, etc. (By the way, an earlier more efficient deque-based implementation was replaced with this map-based implementation for readability and maintainability reasons.)
https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:104: const auto& it = packet_status_window_.find(packet.sequence_number); On 2017/03/20 18:21:51, elad.alon wrote: > On 2017/03/20 18:14:44, stefan-webrtc wrote: > > Can this be more efficient if we avoid start searching from the beginning each > > time? > > Maybe, although I am not sure if the complexity of the code (*) would be worth > the savings in search-time. How about we do that in a separate CL? (This was not > introduced in this CL, so no degradation here.) > > --- > (*) First lookup, wrap-around of sequence number, sequence numbers in the vector > which aren't in the map, etc. I guess it depends on the size of the window in packets. Let's leave it for a future discussion, as you suggest. https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: void OnNewTransportFeedbacks( On 2017/03/20 18:21:51, elad.alon wrote: > On 2017/03/20 18:14:44, stefan-webrtc wrote: > > How about OnPacketFeedback()? > > OnPacketFeedbacks, plural, maybe? Wdyt? Sounds good.
https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2754373002/diff/1/webrtc/voice_engine/transport... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: void OnNewTransportFeedbacks( On 2017/03/20 18:36:21, stefan-webrtc wrote: > On 2017/03/20 18:21:51, elad.alon wrote: > > On 2017/03/20 18:14:44, stefan-webrtc wrote: > > > How about OnPacketFeedback()? > > > > OnPacketFeedbacks, plural, maybe? Wdyt? > > Sounds good. Done.
The CQ bit was checked by elad.alon@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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by elad.alon@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: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
lgtm good work! only a small nit https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: for (const PacketFeedback& packet : packet_feedbacks) { nit PacketFeedback -> auto
https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: for (const PacketFeedback& packet : packet_feedbacks) { On 2017/03/20 21:14:35, minyue-webrtc(OOO_until_Mar19) wrote: > nit PacketFeedback -> auto I prefer not using auto if the underlying type isn't particularly long (e.g., iterator). This is more readable to me as I don't have to look up the type while reading the code. I vote for keeping it as is.
lgtm
On 2017/03/21 06:10:56, stefan-webrtc wrote: > https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: for (const > PacketFeedback& packet : packet_feedbacks) { > On 2017/03/20 21:14:35, minyue-webrtc(OOO_until_Mar19) wrote: > > nit PacketFeedback -> auto > > I prefer not using auto if the underlying type isn't particularly long (e.g., > iterator). This is more readable to me as I don't have to look up the type while > reading the code. > > I vote for keeping it as is. lgtm
https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: for (const PacketFeedback& packet : packet_feedbacks) { On 2017/03/21 06:10:56, stefan-webrtc wrote: > On 2017/03/20 21:14:35, minyue-webrtc(OOO_until_Mar19) wrote: > > nit PacketFeedback -> auto > > I prefer not using auto if the underlying type isn't particularly long (e.g., > iterator). This is more readable to me as I don't have to look up the type while > reading the code. > > I vote for keeping it as is. (That was my reasoning.)
The CQ bit was checked by elad.alon@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2751653006 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
elad.alon@webrtc.org changed reviewers: + pbos@webrtc.org
Adding pbos@ for the fuzzer.
elad.alon@webrtc.org changed reviewers: + solenberg@webrtc.org
I think we'll need Fredrick too.
lgtm https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2754373002/diff/40001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: void OnPacketFeedbacks(const std::vector<PacketFeedback>& packet_feedbacks); Drop the plural "s"
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2754373002/#ps60001 (title: "Renaming")
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
Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2754373002/#ps80001 (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": 80001, "attempt_start_ts": 1490104880793590,
"parent_rev": "559af38a15f6590926759440002512b70d01ba58", "commit_rev":
"92e448d5294b6efe6edb610090d28e07531801d8"}
Message was sent while issue was closed.
Description was changed from ========== TransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire feedback BUG=webrtc:7058 ========== to ========== TransportFeedbackPacketLossTracker to receive std::vector<PacketFeedback> in place of the entire feedback BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2754373002 Cr-Commit-Position: refs/heads/master@{#17322} Committed: https://chromium.googlesource.com/external/webrtc/+/92e448d5294b6efe6edb61009... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/92e448d5294b6efe6edb61009... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
