Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268253002:
UMA stats for browser/renderer audio rendering buffer size mismatch. (Closed)
|
Created:
4 years, 4 months ago by o1ka Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA stats for browser/renderer audio rendering buffer size mismatch.
In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based).
It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration.
For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time.
It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation.
Addind a UMA stat to reflect the affected popluation of audio rendering sessions.
BUG=538152
Committed: https://crrev.com/5cdf58442f79d5e681ada5b9e0aaa7564b1574f1
Cr-Commit-Position: refs/heads/master@{#415418}
Patch Set 1 #Patch Set 2 : passing latency info to browser #
Total comments: 8
Patch Set 3 : cleanup, updated histograms.xml #Patch Set 4 : build fix #
Total comments: 11
Patch Set 5 : UMA fixes #
Total comments: 3
Patch Set 6 : fixing tests and nits #Patch Set 7 : new version of UMA histogram #
Total comments: 14
Patch Set 8 : comments addressed #Patch Set 9 : merge conflict resolved #Patch Set 10 : histogram rename #Messages
Total messages: 62 (36 generated)
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, grunell@chromium.org
PTAL: PS1 adds the metric which is logged for all the audio streams. The problem with that is that we configure audio output differently for different latencies, and the metric does not allow to distinguish those cases - and we are particularly interested in RTC case. PS2 is a proposal to pass latency tag as a part of AudioParameters. This way we can distinguish audio with different latencies on the browser side and collect stats separately for them. It also enables collecting other stats basing on the latency (basically, it addresses https://bugs.chromium.org/p/chromium/issues/detail?id=538152). The problem with this approach that I don't really want to make "latency" a real property of AudioParameters so that it, for example, defines frame buffer duration - I think it would complicate the code and make it less readable. So we pass "latency tag" as an optional (in a way that it's ok to not specify it) information field, which looks a bit hacky - not sure if it's too hacky or acceptable. (histograms.xml is not updated for PS2 - will do it depending on the comments)
https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:147: std::max(input_buffer_size, output_buffer_size); I believe you had it the other way around (i.e. min(i, o)) when you presented the idea to me. Maybe you wanted to switch which case should be negative? https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:167: UMA_HISTOGRAM_SPARSE_SLOWLY( This should never happen, right? Maybe NOTREACHED() and no histogram? https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... File media/base/audio_parameters.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... media/base/audio_parameters.cc:20: : latency_tag_(AudioLatency::LATENCY_COUNT) { I don't think an invalid value should be used to say it's unspecified. What about making it a base::Optional?
https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:147: std::max(input_buffer_size, output_buffer_size); On 2016/08/23 15:53:19, Henrik Grunell wrote: > I believe you had it the other way around (i.e. min(i, o)) when you presented > the idea to me. Maybe you wanted to switch which case should be negative? Right, it's a misprint, thanks for catching! https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:167: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2016/08/23 15:53:19, Henrik Grunell wrote: > This should never happen, right? Maybe NOTREACHED() and no histogram? Why not? It can be the case in the future, and it's ok to have (you just proposed optional in another comment). https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... File media/base/audio_parameters.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... media/base/audio_parameters.cc:20: : latency_tag_(AudioLatency::LATENCY_COUNT) { On 2016/08/23 15:53:19, Henrik Grunell wrote: > I don't think an invalid value should be used to say it's unspecified. What > about making it a base::Optional? It would be a pain for IPC
olka@chromium.org changed reviewers: + isherman@chromium.org
PTAL at the updated version. dalecurtis@ - friendly ping; isherman@ - PTAL at UMA.
The CQ bit was checked by olka@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:167: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2016/08/23 16:20:56, o1ka wrote: > On 2016/08/23 15:53:19, Henrik Grunell wrote: > > This should never happen, right? Maybe NOTREACHED() and no histogram? > > Why not? It can be the case in the future, and it's ok to have (you just > proposed optional in another comment). It could be missed when adding a new type. Better to state all possible cases explicitly and force adding new ones here. https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... File media/base/audio_parameters.cc (right): https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... media/base/audio_parameters.cc:20: : latency_tag_(AudioLatency::LATENCY_COUNT) { On 2016/08/23 16:20:56, o1ka wrote: > On 2016/08/23 15:53:19, Henrik Grunell wrote: > > I don't think an invalid value should be used to say it's unspecified. What > > about making it a base::Optional? > > It would be a pain for IPC OK. It's still hacky I think to use the count to indicate unspecified. I'd prefer an explicit "UNSPECIFIED" value. Let's see what the other reviewers say about the general idea before digging in more.
On 2016/08/24 12:47:15, Henrik Grunell wrote: > https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... > File media/audio/audio_output_resampler.cc (right): > > https://codereview.chromium.org/2268253002/diff/20001/media/audio/audio_outpu... > media/audio/audio_output_resampler.cc:167: UMA_HISTOGRAM_SPARSE_SLOWLY( > On 2016/08/23 16:20:56, o1ka wrote: > > On 2016/08/23 15:53:19, Henrik Grunell wrote: > > > This should never happen, right? Maybe NOTREACHED() and no histogram? > > > > Why not? It can be the case in the future, and it's ok to have (you just > > proposed optional in another comment). > > It could be missed when adding a new type. Better to state all possible cases > explicitly and force adding new ones here. > > https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... > File media/base/audio_parameters.cc (right): > > https://codereview.chromium.org/2268253002/diff/20001/media/base/audio_parame... > media/base/audio_parameters.cc:20: : latency_tag_(AudioLatency::LATENCY_COUNT) { > On 2016/08/23 16:20:56, o1ka wrote: > > On 2016/08/23 15:53:19, Henrik Grunell wrote: > > > I don't think an invalid value should be used to say it's unspecified. What > > > about making it a base::Optional? > > > > It would be a pain for IPC > > OK. It's still hacky I think to use the count to indicate unspecified. I'd > prefer an explicit "UNSPECIFIED" value. Let's see what the other reviewers say > about the general idea before digging in more. AudioLatency::LATENCY_COUNT is used as a container size, so I would prefer to not add non-meaningful values to the enum.
The CQ bit was checked by olka@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
From reading the CL description, it seems like you could simply record a tristate histogram: (1) buffer sizes exactly match; (2) buffer A is larger than buffer B; (3) buffer A is smaller than buffer B. Or, maybe it would help to split one or both of (2) and (3) along the dimension of "is the larger buffer's size a multiple of the smaller buffer's size?". But, it's not clear to me what you would do with any additional data beyond those concepts. If you only need that small handful of concepts, please record precisely what you need, using a simple UMA_HISTOGRAM_ENUMERATION. https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:143: const int& output_buffer_size = output_params.frames_per_buffer(); nit: An int is generally going to be smaller or equally sized to a pointer, and have one less level of indirection than a reference. I'll defer to owners of this code, but IMO it's generally preferable to copy ints than to have const references to them ;-) https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:147: std::min(input_buffer_size, output_buffer_size); What is the possible range for these values? It's not appropriate to use a sparse histogram to record a value that might take on many different for a single client. https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:166: default: Please explicitly list all possible cases. AFAICT there are no other valid cases, so there is no reason to have an additional histogram. https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21882: + call to renderer, which can cause problems for RTC. How can you identify values that are not multiples of 1000 when they are scaled down by a factor of 1000? https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:97320: + <suffix name="LatencyOther"/> Please provide a label for each suffix.
On 2016/08/24 22:20:23, Ilya Sherman wrote: > From reading the CL description, it seems like you could simply record a > tristate histogram: (1) buffer sizes exactly match; (2) buffer A is larger than > buffer B; (3) buffer A is smaller than buffer B. Or, maybe it would help to > split one or both of (2) and (3) along the dimension of "is the larger buffer's > size a multiple of the smaller buffer's size?". But, it's not clear to me what > you would do with any additional data beyond those concepts. If you only need > that small handful of concepts, please record precisely what you need, using a > simple UMA_HISTOGRAM_ENUMERATION. I updated histogram description a bit to clarify this. Ideally, we'd like to have a 2-d histogram with input/output buffer sizes on the axes. Since we can't have it(?), we are trying to come up with something which gives us more than just a flag. The ratio gives us some hint of negative effect periodicy, and we can also guess the exact buffer size values (since the set of those is pretty limited). > > https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... > File media/audio/audio_output_resampler.cc (right): > > https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... > media/audio/audio_output_resampler.cc:143: const int& output_buffer_size = > output_params.frames_per_buffer(); > nit: An int is generally going to be smaller or equally sized to a pointer, and > have one less level of indirection than a reference. I'll defer to owners of > this code, but IMO it's generally preferable to copy ints than to have const > references to them ;-) > > https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... > media/audio/audio_output_resampler.cc:147: std::min(input_buffer_size, > output_buffer_size); > What is the possible range for these values? It's not appropriate to use a > sparse histogram to record a value that might take on many different for a > single client. > > https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... > media/audio/audio_output_resampler.cc:166: default: > Please explicitly list all possible cases. AFAICT there are no other valid > cases, so there is no reason to have an additional histogram. > > https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:21882: + call to renderer, which can > cause problems for RTC. > How can you identify values that are not multiples of 1000 when they are scaled > down by a factor of 1000? > > https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:97320: + <suffix name="LatencyOther"/> > Please provide a label for each suffix.
isherman@ - thanks for the comments, please see the answers + the updated PS. https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:143: const int& output_buffer_size = output_params.frames_per_buffer(); On 2016/08/24 22:20:23, Ilya Sherman wrote: > nit: An int is generally going to be smaller or equally sized to a pointer, and > have one less level of indirection than a reference. I'll defer to owners of > this code, but IMO it's generally preferable to copy ints than to have const > references to them ;-) It's extending lifetime of a temporary rather than having a reference (aka pointer), so it should not require any additional memory, AFAIK. On the other hand, "const int" should be optimized by copy elision => does not cause any copy. I'll leave out references, just to make the code clearer. https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:147: std::min(input_buffer_size, output_buffer_size); On 2016/08/24 22:20:23, Ilya Sherman wrote: > What is the possible range for these values? It's not appropriate to use a > sparse histogram to record a value that might take on many different for a > single client. For both buffer sizes the range is between 128 and 4096. A buffer size is usually: (a) a power of 2, or (b) a sample rate (https://en.wikipedia.org/wiki/Sampling_(signal_processing)#Sampling_rate) divided by 100 (10 ms buffers) or 50 (20 ms buffers). So, the range of the values we want to record is [-31, 31] (before scaling), but we'll see only a subset of integers in that range, and for some of those integers a very limited subset of non-round values. I'm not sure what is considered to be a client in this context. We expect that each (audio hardware + drivers) combination will produce 1 value per histogram (might be 2-3 in some cases if code paths for the specific audio latency diverge on renderer side) https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:166: default: On 2016/08/24 22:20:23, Ilya Sherman wrote: > Please explicitly list all possible cases. AFAICT there are no other valid > cases, so there is no reason to have an additional histogram. Agree. Done https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21882: + call to renderer, which can cause problems for RTC. On 2016/08/24 22:20:23, Ilya Sherman wrote: > How can you identify values that are not multiples of 1000 when they are scaled > down by a factor of 1000? They are scaled up, not down. I changed the wording. Also, I changed the factor to be 100 (though it does not really affect the number of histogram buckets). https://codereview.chromium.org/2268253002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:97320: + <suffix name="LatencyOther"/> On 2016/08/24 22:20:23, Ilya Sherman wrote: > Please provide a label for each suffix. Done.
The CQ bit was checked by olka@chromium.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.chromium.or...
olka@chromium.org changed reviewers: + kenrb@chromium.org
kenrb@ - PTAL at IPC change (adding a enum value to AudioParameters)
dalecurtis@ - just to double-check: does passing latency tag to browser as a member of AudioParameters really LGTY?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm https://codereview.chromium.org/2268253002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:144: DCHECK(input_buffer_size); Nit: Consider DCHECK_NE(x, 0). Slightly more readable. https://codereview.chromium.org/2268253002/diff/80001/media/base/audio_parame... File media/base/audio_parameters.cc (right): https://codereview.chromium.org/2268253002/diff/80001/media/base/audio_parame... media/base/audio_parameters.cc:20: : latency_tag_(AudioLatency::LATENCY_COUNT) { Nit: Consider a comment on the usage of LATENCY_COUNT. I think that can be good. https://codereview.chromium.org/2268253002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2268253002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21880: + per one request to the renderer (multiplied by 100). Any values those are Nit: We had this discussion before :) I believe s/those/that - it's a conjunction and not a pronoun afaict. Other reviewers can pitch in on this.
The CQ bit was checked by olka@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_resampler.cc:147: std::min(input_buffer_size, output_buffer_size); On 2016/08/25 11:48:34, o1ka wrote: > On 2016/08/24 22:20:23, Ilya Sherman wrote: > > What is the possible range for these values? It's not appropriate to use a > > sparse histogram to record a value that might take on many different for a > > single client. > > For both buffer sizes the range is between 128 and 4096. > A buffer size is usually: (a) a power of 2, or (b) a sample rate > (https://en.wikipedia.org/wiki/Sampling_(signal_processing)#Sampling_rate) > divided by 100 (10 ms buffers) or 50 (20 ms buffers). > > So, the range of the values we want to record is [-31, 31] (before scaling), but > we'll see only a subset of integers in that range, and for some of those > integers a very limited subset of non-round values. > > I'm not sure what is considered to be a client in this context. We expect that > each (audio hardware + drivers) combination will produce 1 value per histogram > (might be 2-3 in some cases if code paths for the specific audio latency diverge > on renderer side) I think you are trying to pack too much into a single histogram. It would be great if we could record 2D histograms -- and, actually, if you know the precise set of inputs, you could just flatten the cross-product. OTOH, if you're not sure that you know the exact set of inputs, then I'd recommend recording two histograms: One indicating whether the sizes are compatible, and another indicating the precise periodicity if they do not match (or, in all cases, if you prefer). That would both (a) give you clearer data, and (b) make me more confident that I can correctly reason about the structure of the histogram.
ipc lgtm
> OTOH, if you're not sure that you know the exact set of inputs, then I'd > recommend recording two histograms: One indicating whether the sizes are > compatible, and another indicating the precise periodicity if they do not match > (or, in all cases, if you prefer). That would both (a) give you clearer data, > and (b) make me more confident that I can correctly reason about the structure > of the histogram. The more I think of it, the more I realize that we can't really get any benefit out of knowing the exact periodicy of artifacts. Here is the new approach: (1) 0 if the render calls come evenly as expected; (2) -1 if we periodically skip a call; (3) if more than one adjacent call is possible, we want to know, how many more and if this number varies. I'm not sure I know the exact set of inputs. Theoretically expected range of values is [-1, (4096/128-1)*2+1] => [-1, 63]. PTAL
I continue to think that this might be easier to interpret as two separate histograms. However, if you and other folks working on the audio rendering pipeline really prefer squashing this into a single histogram, I'm okay with the latest layout, modulo the nits below: https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:147: int value = 0; I think it would be best to document the value here. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:149: // 0 if input size is a multiply of output size; otherwise -1. nit: s/multiply/multiple https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:157: } The range [-1, 63] sounds like an acceptable range. Could you please add code that will explicitly cap the histogram to, say, 64? https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21876: + renderer buffer size is a multiply of system buffer size, and render calls nit: s/multiply/multiple (throughout) https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21878: + buffer sizes is larger than system buffer size, but is not an exact multiply nit: s/sizes/size
lgtm Fix nits is optional. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:153: if (output_buffer_size % input_buffer_size) { Nit: (x % y != 0) is more readable. Same above. You could also split == and > to two different if blocks above (clearer imo). https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:177: DLOG(WARNING) << "Latency tag is not set"; Nit: It seems to me DVLOG that is more appropriate. Or is it expected that the latency tag is set?
>>I continue to think that this might be easier to interpret as two separate histograms. I agree that the meaning of two histograms is easier to interpret, but they would give us less information. There is a difference between 1(+ one extra periodically) and 2(+ one extra periodically) callbacks, and knowing the base number of callbacks + their regularity gives us more hints to what to look at. PTAL at neat fixes. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:147: int value = 0; On 2016/08/27 06:38:45, Ilya Sherman wrote: > I think it would be best to document the value here. I'm not sure if you mean copying the description from the histogram. Added a reference to it. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:149: // 0 if input size is a multiply of output size; otherwise -1. On 2016/08/27 06:38:45, Ilya Sherman wrote: > nit: s/multiply/multiple Done. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:153: if (output_buffer_size % input_buffer_size) { On 2016/08/29 15:26:15, Henrik Grunell wrote: > Nit: (x % y != 0) is more readable. Same above. You could also split == and > to > two different if blocks above (clearer imo). Acknowledged. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:157: } On 2016/08/27 06:38:45, Ilya Sherman wrote: > The range [-1, 63] sounds like an acceptable range. Could you please add code > that will explicitly cap the histogram to, say, 64? Done. https://codereview.chromium.org/2268253002/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:177: DLOG(WARNING) << "Latency tag is not set"; On 2016/08/29 15:26:15, Henrik Grunell wrote: > Nit: It seems to me DVLOG that is more appropriate. Or is it expected that the > latency tag is set? Done. https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21876: + renderer buffer size is a multiply of system buffer size, and render calls On 2016/08/27 06:38:45, Ilya Sherman wrote: > nit: s/multiply/multiple (throughout) Done. https://codereview.chromium.org/2268253002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21878: + buffer sizes is larger than system buffer size, but is not an exact multiply On 2016/08/27 06:38:45, Ilya Sherman wrote: > nit: s/sizes/size Done.
The CQ bit was checked by olka@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by olka@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/08/29 15:52:17, o1ka wrote: > >>I continue to think that this might be easier to interpret as two separate > histograms. > > I agree that the meaning of two histograms is easier to interpret, but they > would give us less information. There is a difference between 1(+ one extra > periodically) and 2(+ one extra periodically) callbacks, and knowing the base > number of callbacks + their regularity gives us more hints to what to look at. Sorry, I think we're misunderstanding each other somehow. Maybe it would make sense to have a brief IM conversation or video conference? I believe you're 9 timezones ahead of me, which I realize makes this a bit tricky :/ In the meantime, let me try to clarify my comment a bit more: The current histogram setup creates a single histogram that tries to capture a few unrelated pieces of information, and is therefore hard to understand. Is there a way to split it up into two (or more) histograms, each of which is measuring a single thing? Perhaps the specific split that I suggested doesn't quite make sense, because I'm not fully understanding something about what you're trying to measure... but is there another split that you can design that would make sense?
> Sorry, I think we're misunderstanding each other somehow. Maybe it would make > sense to have a brief IM conversation or video conference? Sure! Could you ping me today between 9:30-11 your time? > > In the meantime, let me try to clarify my comment a bit more: The current > histogram setup creates a single histogram that tries to capture a few unrelated > pieces of information, and is therefore hard to understand. > Is there a way to > split it up into two (or more) histograms, each of which is measuring a single > thing? Perhaps the specific split that I suggested doesn't quite make sense, > because I'm not fully understanding something about what you're trying to > measure... but is there another split that you can design that would make sense? I would argue the word "unrelated". We've got the following situations: 1) regular callback from the browser (=0) - this is the only "normal" case; 2) periodically skipped callbacks (=-1) 3) several adjacent callbacks from browser - in this case we need to know, how many. And this "how many" may be a constant N; or it may be N, but sometimes N+1. It does not make any sense to me to split it to histograms like "constant/varying number of callbacks" + "usual number of callbacks" - the meaning of these is easier to understand, but it does not really contain the information we need. It does not make sense to me to split it to "skipped/normal/extra" histogram and "how many extra calls" histogrameither - it would be just the same histogram we have now, just stretched between those two. I think the histogram as it is now captures exactly what we want :) I renamed it - probably this way it makes more sense to you? PTAL
The CQ bit was checked by olka@chromium.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.chromium.or...
Description was changed from ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. ========== to ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. BUG=538152 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/30 10:20:36, o1ka wrote: > > Sorry, I think we're misunderstanding each other somehow. Maybe it would make > > sense to have a brief IM conversation or video conference? > > Sure! Could you ping me today between 9:30-11 your time? Whoops, sorry for missing this! I ended up having a pretty full morning today. Hope you weren't too tethered to a work device waiting for me... :/ > > In the meantime, let me try to clarify my comment a bit more: The current > > histogram setup creates a single histogram that tries to capture a few > unrelated > > pieces of information, and is therefore hard to understand. > > Is there a way to > > split it up into two (or more) histograms, each of which is measuring a single > > thing? Perhaps the specific split that I suggested doesn't quite make sense, > > because I'm not fully understanding something about what you're trying to > > measure... but is there another split that you can design that would make > sense? > > > I would argue the word "unrelated". We've got the following situations: > 1) regular callback from the browser (=0) - this is the only "normal" case; > 2) periodically skipped callbacks (=-1) > 3) several adjacent callbacks from browser - in this case we need to know, how > many. > And this "how many" may be a constant N; or it may be N, but sometimes N+1. > > It does not make any sense to me to split it to histograms like > "constant/varying number of callbacks" + "usual number of callbacks" - > the meaning of these is easier to understand, but it does not really contain the > information we need. > > It does not make sense to me to split it to "skipped/normal/extra" histogram and > "how many extra calls" histogrameither - > it would be just the same histogram we have now, just stretched between those > two. > > I think the histogram as it is now captures exactly what we want :) > I renamed it - probably this way it makes more sense to you? > > PTAL Okay, it sounds like you've given this plenty of thought and prefer this histogram layout, so I'll go ahead and approve: LGTM. I guess if this metric proves to be too confusing to make use of, you can always come back and reformulate it in the future =)
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, kenrb@chromium.org, grunell@chromium.org Link to the patchset: https://codereview.chromium.org/2268253002/#ps180001 (title: "histogram rename")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. BUG=538152 ========== to ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. BUG=538152 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. BUG=538152 ========== to ========== UMA stats for browser/renderer audio rendering buffer size mismatch. In case AudioOutputDevice buffer size is not the same as hardware buffer size, AudioOutputResampler on browser side does rebuffering (fifo-based). It means that AudioOuputDevice::Render() calles are NOT issued with a constant interval based on output buffer size duration. For example, for WebRTC it means Voice Engine can receive 2+ calls one by one without 10 ms interval, or a call can be skipped from time to time. It may result in a missed deadline on Browser/Renderer edge, which causes audio glitch. And it may create problems for echo cancellation. Addind a UMA stat to reflect the affected popluation of audio rendering sessions. BUG=538152 Committed: https://crrev.com/5cdf58442f79d5e681ada5b9e0aaa7564b1574f1 Cr-Commit-Position: refs/heads/master@{#415418} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5cdf58442f79d5e681ada5b9e0aaa7564b1574f1 Cr-Commit-Position: refs/heads/master@{#415418}
Message was sent while issue was closed.
On 2016/08/30 20:34:45, Ilya Sherman wrote: > On 2016/08/30 10:20:36, o1ka wrote: > > > Sorry, I think we're misunderstanding each other somehow. Maybe it would > make > > > sense to have a brief IM conversation or video conference? > > > > Sure! Could you ping me today between 9:30-11 your time? > > Whoops, sorry for missing this! I ended up having a pretty full morning today. > Hope you weren't too tethered to a work device waiting for me... :/ No problem at all :) > Okay, it sounds like you've given this plenty of thought and prefer this > histogram layout, so I'll go ahead and approve: LGTM. I guess if this metric > proves to be too confusing to make use of, you can always come back and > reformulate it in the future =) :) |
