Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1296633002:
NetEq/ACM: Refactor how packet waiting times are calculated (Closed)
|
Created:
5 years, 4 months ago by hlundin-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNetEq/ACM: Refactor how packet waiting times are calculated
With this change, the aggregates for packet waiting times are
calculated in NetEq's StatisticsCalculator insead of in
AcmReceiver. This simplifies things somewhat, and avoids having to
copy the raw data on polling.
R=ivoc@webrtc.org, minyue@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/1bb8cf846d9a0bfe74fceae34ebef60f56d12fa4
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Change from std::list to std::deque #Patch Set 3 : rebase #
Total comments: 3
Patch Set 4 : Simplify median calculation #Patch Set 5 : Rebasing #
Total comments: 2
Messages
Total messages: 39 (15 generated)
Patchset #1 (id:1) has been deleted
henrik.lundin@webrtc.org changed reviewers: + ivoc@webrtc.org, minyue@webrtc.org
henrik.lundin@webrtc.org changed required reviewers: + minyue@webrtc.org
Minyue, Ivo, Please, take a look at this CL. It's a refactoring preparing for some more metrics to be implemented. Thanks!
On 2015/08/14 15:01:51, hlundin-webrtc wrote: > Minyue, Ivo, > > Please, take a look at this CL. It's a refactoring preparing for some more > metrics to be implemented. > > Thanks! Excellent work! LGTM Only one comment about efficiency, and leave it for you to decide.
https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:160: waiting_times_.sort(); std::vector may be faster than std::list in sorting small data types. I prefer to use std::lower_bound to find a position when we add new value, so that it is already sorted here, to make the computation de-centralized. https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:175: sum += time; can also sum when adding. https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right): https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.h:99: std::list<int> waiting_times_; why to change from vector to list?
https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:160: waiting_times_.sort(); On 2015/08/18 12:55:34, minyue-webrtc wrote: > std::vector may be faster than std::list in sorting small data types. > > I prefer to use std::lower_bound to find a position when we add new value, so > that it is already sorted here, to make the computation de-centralized. That won't work. The list is sorted in time order, so that old data can be deleted when new is inserted. It can only be sorted when the stats are averaged, since the list is cleared after that. https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:175: sum += time; On 2015/08/18 12:55:34, minyue-webrtc wrote: > can also sum when adding. I'm worried about numerical stability. We would have to add when new values go in, and subtract again as they become too old and are purged. Doing this in floating-point is likely going to cause drift. https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right): https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.h:99: std::list<int> waiting_times_; On 2015/08/18 12:55:34, minyue-webrtc wrote: > why to change from vector to list? The reasoning behind my choice is that waiting_times_ might in many cases be acting as a FIFO buffer. In particular, if the list is full (waiting_times_.size() >= kLenWaitingTimes), every new value being pushed to the end will result in the oldest value being popped from the beginning of the list. That is not very efficient to do with vectors, which is why I went with std:list. Your input on this is more than welcome.
https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:102: waiting_times_.pop_front(); Ok, I see the deletion according to age here. https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:160: waiting_times_.sort(); On 2015/08/19 08:28:39, hlundin-webrtc wrote: > On 2015/08/18 12:55:34, minyue-webrtc wrote: > > std::vector may be faster than std::list in sorting small data types. > > > > I prefer to use std::lower_bound to find a position when we add new value, so > > that it is already sorted here, to make the computation de-centralized. > > That won't work. The list is sorted in time order, so that old data can be > deleted when new is inserted. It can only be sorted when the stats are averaged, > since the list is cleared after that. Acknowledged. https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:175: sum += time; On 2015/08/19 08:28:39, hlundin-webrtc wrote: > On 2015/08/18 12:55:34, minyue-webrtc wrote: > > can also sum when adding. > > I'm worried about numerical stability. We would have to add when new values go > in, and subtract again as they become too old and are purged. Doing this in > floating-point is likely going to cause drift. Acknowledged. https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right): https://codereview.chromium.org/1296633002/diff/20001/webrtc/modules/audio_co... webrtc/modules/audio_coding/neteq/statistics_calculator.h:99: std::list<int> waiting_times_; On 2015/08/19 08:28:39, hlundin-webrtc wrote: > On 2015/08/18 12:55:34, minyue-webrtc wrote: > > why to change from vector to list? > > The reasoning behind my choice is that waiting_times_ might in many cases be > acting as a FIFO buffer. In particular, if the list is full > (waiting_times_.size() >= kLenWaitingTimes), every new value being pushed to the > end will result in the oldest value being popped from the beginning of the list. > That is not very efficient to do with vectors, which is why I went with > std:list. Your input on this is more than welcome. maybe use std::deque
Change from std::list to std::deque
https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.h (right): https://codereview.webrtc.org/1296633002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.h:99: std::list<int> waiting_times_; On 2015/08/19 11:21:52, minyue-webrtc wrote: > On 2015/08/19 08:28:39, hlundin-webrtc wrote: > > On 2015/08/18 12:55:34, minyue-webrtc wrote: > > > why to change from vector to list? > > > > The reasoning behind my choice is that waiting_times_ might in many cases be > > acting as a FIFO buffer. In particular, if the list is full > > (waiting_times_.size() >= kLenWaitingTimes), every new value being pushed to > the > > end will result in the oldest value being popped from the beginning of the > list. > > That is not very efficient to do with vectors, which is why I went with > > std:list. Your input on this is more than welcome. > > maybe use std::deque Yes, much better. Thanks!
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1296633002/#ps40001 (title: "Change from std::list to std::deque")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1296633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1296633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/5209) linux on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/9180) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/8921)
rebase
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1296633002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1296633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1296633002/60001
The CQ bit was unchecked by henrik.lundin@webrtc.org
LGTM, here's a suggestion for a simplification. https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:263: std::advance(middle_left, (waiting_times_.size() - 1) / 2); I think this can be simplified to: int middle_left = waiting_times_[(waiting_times_.size() - 1) / 2]; and: int middle_right = waiting_times_[waiting_times_.size() / 2];
https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:263: std::advance(middle_left, (waiting_times_.size() - 1) / 2); On 2015/08/24 14:22:17, ivoc wrote: > I think this can be simplified to: > int middle_left = waiting_times_[(waiting_times_.size() - 1) / 2]; > and: > int middle_right = waiting_times_[waiting_times_.size() / 2]; > > +1
Simplify median calculation
https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:263: std::advance(middle_left, (waiting_times_.size() - 1) / 2); On 2015/08/24 15:02:54, minyue-webrtc wrote: > On 2015/08/24 14:22:17, ivoc wrote: > > I think this can be simplified to: > > int middle_left = waiting_times_[(waiting_times_.size() - 1) / 2]; > > and: > > int middle_right = waiting_times_[waiting_times_.size() / 2]; > > > > > > +1 Thanks! I had to use std::advance when I used std::list, but now that I use std::deque, the [] is available.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1296633002/#ps80001 (title: "Simplify median calculation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1296633002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1296633002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Rebasing
The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1296633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1296633002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 1bb8cf846d9a0bfe74fceae34ebef60f56d12fa4 (presubmit successful).
Message was sent while issue was closed.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1296633002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:198: while (waiting_times_.size() >= kLenWaitingTimes) { Drive-by nit: This would violate the Chromium style guide, because you're partially handling a DCHECK failure, which contradicts the point of the DCHECK. You should probably change this to "if (waiting_times_.size() == kLenWaitingTimes) {".
Message was sent while issue was closed.
Thanks for reviewing! https://codereview.webrtc.org/1296633002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/1296633002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/statistics_calculator.cc:198: while (waiting_times_.size() >= kLenWaitingTimes) { On 2015/08/25 22:00:02, Peter Kasting wrote: > Drive-by nit: This would violate the Chromium style guide, because you're > partially handling a DCHECK failure, which contradicts the point of the DCHECK. > You should probably change this to "if (waiting_times_.size() == > kLenWaitingTimes) {". Fixed in https://codereview.webrtc.org/1319953002/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
