Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190343002:
Adds delta-time logging for audio playout (Closed)
|
Created:
4 years, 4 months ago by henrika_webrtc Modified:
4 years, 4 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds delta-time logging for audio playout
BUG=NONE
R=magjed@webrtc.org
Committed: https://crrev.com/3d7346fd46a266d66e3d0e69f74b1fe5d8c75d13
Cr-Commit-Position: refs/heads/master@{#13576}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Feedback from magjed@ #Patch Set 3 : nit #Patch Set 4 : nit #Patch Set 5 : Now uses and array instead #
Total comments: 1
Patch Set 6 : nit #Patch Set 7 : Fixed compile issues #Patch Set 8 : Fixed div by zero #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== Adds delta-time logging for audio playout BUG= ========== to ========== Adds delta-time logging for audio playout BUG=NONE ==========
henrika@webrtc.org changed reviewers: + magjed@webrtc.org
First round, no cap on max delay yet. Will add later. Could you check anyhow perhaps?
https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:78: while (!playout_diff_time_map_.empty()) { Use a range-based loop instead: for (const auto& keyAndValue : playout_diff_time_map_) { int time_diff = keyAndValue.first; int num_elements = keyAndValue.second; } https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:360: int64_t diff_time = rtc::TimeDiff(now_time, last_playout_time_); Consider using diff_time = std::min(1000, diff_time); to limit the number of elements in the map. Also, you could use a plain array instead of a map if you cap the numbers that way, but I'm not sure how sparse the array will be and if it's an improvement over using a map. https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:180: std::map<int, int> playout_diff_time_map_; You can consider using std::unordered_map instead of map, but I'm not sure it's faster for few elements. Maybe ask tommi@.
Thanks. PTAL.
https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:78: while (!playout_diff_time_map_.empty()) { Thanks. https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:360: int64_t diff_time = rtc::TimeDiff(now_time, last_playout_time_); Will add the cap, thanks! Would like to keep the map since the array would be sparse and we would have to allocate memory for zeros. Hope that is OK. https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:180: std::map<int, int> playout_diff_time_map_; I think it only affects reading and not writing. And we only read it once at destruction. Hence not essential. Agree?
https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:360: int64_t diff_time = rtc::TimeDiff(now_time, last_playout_time_); On 2016/07/29 11:11:00, henrika_webrtc wrote: > Will add the cap, thanks! > > Would like to keep the map since the array would be sparse and we would have to > allocate memory for zeros. Hope that is OK. The map has a big overhead per element and will need to store at least two pointers (left/right child), the key, the value, probably a color (it's a red–black tree), and will contain as many internal nodes as leaf nodes, so probably something like 80 bytes per element in average, compared to an array with just 8 bytes (64 bit int) per element. So if you have more than 50 elements, an array will use less memory. An array will probably be something like 10x faster as well. 100 lookups per second is not completely negligible. https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:180: std::map<int, int> playout_diff_time_map_; On 2016/07/29 11:11:00, henrika_webrtc wrote: > I think it only affects reading and not writing. And we only read it once at > destruction. Hence not essential. Agree? It affects both reading and writing, a map will use an ordered tree (typically red-black tree), and an unordered_map will use a hash map. It also affects memory usage.
Change to an array like this and I'll be happy and stamp this CL:
uint32_t playout_diff_time_map_[kMaxDeltaTimeInMsStoredInMap + 1] = {0};
for (int time_diff = 0; time_diff <= kMaxDeltaTimeInMsStoredInMap; ++time_diff)
{
int num_elements = playout_diff_time_map_[time_diff];
if (num_elements > 0) {
total_diff_time += num_elements * time_diff;
num_measurements += num_elements;
LOG(INFO) << time_diff << " => " << num_elements;
}
}
https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:360: int64_t diff_time = rtc::TimeDiff(now_time, last_playout_time_); Will change https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2190343002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:180: std::map<int, int> playout_diff_time_map_; On 2016/07/29 12:16:07, magjed_webrtc wrote: > On 2016/07/29 11:11:00, henrika_webrtc wrote: > > I think it only affects reading and not writing. And we only read it once at > > destruction. Hence not essential. Agree? > > It affects both reading and writing, a map will use an ordered tree (typically > red-black tree), and an unordered_map will use a hash map. It also affects > memory usage. Acknowledged.
Great comments. Thanks! PTAL
lgtm https://codereview.webrtc.org/2190343002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2190343002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:14: #include <map> remove
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2190343002/#ps100001 (title: "nit")
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_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_dbg/builds/12506)
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2190343002/#ps120001 (title: "Fixed compile issues")
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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
Description was changed from ========== Adds delta-time logging for audio playout BUG=NONE ========== to ========== Adds delta-time logging for audio playout BUG=NONE R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3d7346fd46a266d66e3d0e69f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 3d7346fd46a266d66e3d0e69f74b1fe5d8c75d13 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Adds delta-time logging for audio playout BUG=NONE R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3d7346fd46a266d66e3d0e69f... ========== to ========== Adds delta-time logging for audio playout BUG=NONE R=magjed@webrtc.org Committed: https://crrev.com/3d7346fd46a266d66e3d0e69f74b1fe5d8c75d13 Cr-Commit-Position: refs/heads/master@{#13576} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
