Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978403004:
Add UMA histograms for WebAudio (Closed)
|
Created:
4 years, 7 months ago by Raymond Toy Modified:
4 years, 6 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, haraken, hongchan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA histograms for WebAudio.
Histograms are added to keep track of the following properties:
AudioBuffer.numberOfChannels
AudioBuffer.length
AudioBuffer.sampleRate
AudioBuffer.sampleRateRatio (buffer rate divided by context rate)
AudioContext.maxChannelCount
AudioContext.sampleRate
BiquadFilter.type
Panner.panningModel
OfflineAudioContext.channelCount
OfflineAudioContext.length
OfflineAudioContext.sampleRate
BUG=607711, 621292
TEST=manually test using chrome://histograms
Committed: https://crrev.com/8ceed89c8064f5c835b51c0ea07f259bceaca1d4
Cr-Commit-Position: refs/heads/master@{#399212}
Patch Set 1 #Patch Set 2 : Add more histograms #Patch Set 3 : Add histogram for createBuffer() #
Total comments: 5
Patch Set 4 : #
Total comments: 35
Patch Set 5 : Address comments #Patch Set 6 : Add units; fix some summaries #Patch Set 7 : Rename MaxChannelCount to MaxChannelsAvailable #Patch Set 8 : Rebase and add more descriptions #
Total comments: 5
Patch Set 9 : Limit size of histogram #Patch Set 10 : Address more review comments. #
Total comments: 2
Patch Set 11 : Use CustomCountHistogram #
Total comments: 2
Messages
Total messages: 47 (8 generated)
Description was changed from ========== Add UMA histogram for PannerNode.panningModel Keep track of the usage of the (two) different panning models available for the PannerNode. BUG=621292 TEST=manually test using chrome://histograms ========== to ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioContext.maxChannelCount AudioContext.sampleRate Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ==========
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
lgtm
Description was changed from ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioContext.maxChannelCount AudioContext.sampleRate Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ========== to ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioBuffer.numberOfChannels AudioBuffer.length AudioBuffer.sampleRate AudioBuffer.sampleRateRatio (buffer rate divided by context rate) AudioContext.maxChannelCount AudioContext.sampleRate BiquadFilter.type Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ==========
PTAL a second look; createBuffer() records some stats too.
https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:218: DEFINE_STATIC_LOCAL(SparseHistogram, audioBufferSampleRateHistogram, I think we can move anything related to the sample rate to the inside of if() clause below.
https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:218: DEFINE_STATIC_LOCAL(SparseHistogram, audioBufferSampleRateHistogram, On 2016/05/18 22:07:01, hoch wrote: > I think we can move anything related to the sample rate to the inside of if() > clause below. The sample rate for the buffer is an argument to createBuffer. So you only want to record the sample rate only for contexts that aren't closed? I think we would get more (indirect) information if we always record the sample rate but only record the ratio when the context is not closed. Then the difference between the number of samples taken between the two tells us how often people create buffers on closed contexts. But sample rate ratio histogram can, of course, go inside the if block below.
https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:218: DEFINE_STATIC_LOCAL(SparseHistogram, audioBufferSampleRateHistogram, On 2016/05/18 22:25:30, Raymond Toy wrote: > On 2016/05/18 22:07:01, hoch wrote: > > I think we can move anything related to the sample rate to the inside of if() > > clause below. > > The sample rate for the buffer is an argument to createBuffer. So you only want > to record the sample rate only for contexts that aren't closed? I think we > would get more (indirect) information if we always record the sample rate but > only record the ratio when the context is not closed. Then the difference > between the number of samples taken between the two tells us how often people > create buffers on closed contexts. > > But sample rate ratio histogram can, of course, go inside the if block below. Acknowledged. https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:220: DEFINE_STATIC_LOCAL(SparseHistogram, audioBufferSampleRateRatioHistogram, Yes, I think this needs to be inside of the if clause below.
https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:220: DEFINE_STATIC_LOCAL(SparseHistogram, audioBufferSampleRateRatioHistogram, On 2016/05/19 21:43:53, hoch wrote: > Yes, I think this needs to be inside of the if clause below. Done.
Confirmed the new change on createBuffer. lgtm.
rtoy@chromium.org changed reviewers: + mpearson@chromium.org
mpearson: PTAL
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59547: +<histogram name="WebAudio.AudioBuffer.Length"> please add units= annotations to all these histograms.
https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:94: DEFINE_STATIC_LOCAL(SparseHistogram, offlineContextChannelCountHistogram, SparseHistograms are slow because they acquire a lock each time you emit to them. Are you sure you want to use the type of histogram here? I would think for most of these counts an appropriate COUNT or ENUMERATED histogram would be better. ditto with all your other histogram usages in other files in this changelist. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> Consider "Available" instead of "Count". Count seems to be more like the number requested, not a hard cap on the number that could be used. Also consider putting hardware in the histogram name as well. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59593: +<histogram name="WebAudio.AudioContext.SampleRate"> ditto about putting hardware in the title, as I can easily imagine having a software audio sample rate too (e.g., audio is resampled at a lower rate for uploading). https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59644: +<histogram name="WebAudio.BiquadFilter.Type"> Please use an enum= and then you won't need the enum descriptions in the histogram description. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. Can you give an approximate description of how often this occurs? https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59664: +<histogram name="WebAudio.OfflineAudioContext.ChannelCount"> Do all the AudioBuffer and other histograms you're updating above apply both to online and offline content? https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59691: +<histogram name="WebAudio.PannerNode.PanningModel"> Please use an enum. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59696: + PannerNode. Recorded every time the panning model is changed. Please describe approximate when this happens.
https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:94: DEFINE_STATIC_LOCAL(SparseHistogram, offlineContextChannelCountHistogram, On 2016/05/23 18:10:24, Mark P wrote: > SparseHistograms are slow because they acquire a lock each time you emit to > them. We don't expect many offline contexts, so speed isn't important. (Plus we're thread-limited on the number of simultaneous contexts.) > Are you sure you want to use the type of histogram here? > I would think for most of these counts an appropriate COUNT or ENUMERATED > histogram would be better. > ditto with all your other histogram usages in other files in this changelist. Yes, after your initial comment (about adding units), I see there are lots of other types that we should consider. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> On 2016/05/23 18:10:24, Mark P wrote: > Consider "Available" instead of "Count". Count seems to be more like the number > requested, not a hard cap on the number that could be used. > Also consider putting hardware in the histogram name as well. I'm ok with changing the name, but maxChannelCount is the name of the attribute in the audio context. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59593: +<histogram name="WebAudio.AudioContext.SampleRate"> On 2016/05/23 18:10:25, Mark P wrote: > ditto about putting hardware in the title, as I can easily imagine having a > software audio sample rate too (e.g., audio is resampled at a lower rate for > uploading). There is a spec change where an audio context is supposed to allow an constructor option to specify the sample rate. Not sure how to handle this, but perhaps HardWareSampleRate is best in this case. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59644: +<histogram name="WebAudio.BiquadFilter.Type"> On 2016/05/23 18:10:24, Mark P wrote: > Please use an enum= and then you won't need the enum descriptions in the > histogram description. Acknowledged. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/23 18:10:25, Mark P wrote: > Can you give an approximate description of how often this occurs? I suspect this only happens once per filter node that is created. The default is lowpass and people probably only change the type once. I find it odd to be changing the filter type back and forth, unless they want to do weird things. hongchan: WDYT? https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59664: +<histogram name="WebAudio.OfflineAudioContext.ChannelCount"> On 2016/05/23 18:10:25, Mark P wrote: > Do all the AudioBuffer and other histograms you're updating above apply both to > online and offline content? Yes. We don't much care in in the other histograms whether you're using an offline or online context. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59691: +<histogram name="WebAudio.PannerNode.PanningModel"> On 2016/05/23 18:10:24, Mark P wrote: > Please use an enum. Acknowledged. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59696: + PannerNode. Recorded every time the panning model is changed. On 2016/05/23 18:10:24, Mark P wrote: > Please describe approximate when this happens. Like the filter type, we expect the panner model to be fixed. Panner nodes default to equalpower on creation and I suspect it only gets changed to HRTF and remains that way forever.
You replied to my comments. Did you mean to upload a new patchset? --mark
On 2016/05/23 21:19:48, Mark P wrote: > You replied to my comments. Did you mean to upload a new patchset? > > --mark No, I don't have a set ready yet. Just wanted to respond to the obvious questions you had on the frequency of calls and some of the name changes.
Okay, I will wait for another patchset before looking again. --mark https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:94: DEFINE_STATIC_LOCAL(SparseHistogram, offlineContextChannelCountHistogram, On 2016/05/23 20:36:03, Raymond Toy wrote: > On 2016/05/23 18:10:24, Mark P wrote: > > SparseHistograms are slow because they acquire a lock each time you emit to > > them. > > We don't expect many offline contexts, so speed isn't important. (Plus we're > thread-limited on the number of simultaneous contexts.) > > > Are you sure you want to use the type of histogram here? > > I would think for most of these counts an appropriate COUNT or ENUMERATED > > histogram would be better. > > ditto with all your other histogram usages in other files in this changelist. > > Yes, after your initial comment (about adding units), I see there are lots of > other types that we should consider. Yes, I would encourage you to use other types when it feels appropriate. Even if speed isn't an issue, one thing to keep in mind is that sparse histograms can grow in memory unbounded if you keep emitting distinct values to them. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> On 2016/05/23 20:36:03, Raymond Toy wrote: > On 2016/05/23 18:10:24, Mark P wrote: > > Consider "Available" instead of "Count". Count seems to be more like the > number > > requested, not a hard cap on the number that could be used. > > Also consider putting hardware in the histogram name as well. > > I'm ok with changing the name, but maxChannelCount is the name of the attribute > in the audio context. I'd encourage clarity in the histograms because it might not be just team members looking at them. (Yes, even at the expense of differing naming in the code. People who care about the details can always look up the call. But if the name is misleading, they might think they know what's going on but me wrong.) (Also, you could always revise the code.) https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59593: +<histogram name="WebAudio.AudioContext.SampleRate"> On 2016/05/23 20:36:03, Raymond Toy wrote: > On 2016/05/23 18:10:25, Mark P wrote: > > ditto about putting hardware in the title, as I can easily imagine having a > > software audio sample rate too (e.g., audio is resampled at a lower rate for > > uploading). > > There is a spec change where an audio context is supposed to allow an > constructor option to specify the sample rate. Not sure how to handle this, but > perhaps HardWareSampleRate is best in this case. Ah. BTW, "Hardware" is the normal way to capitalize the word.
https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:94: DEFINE_STATIC_LOCAL(SparseHistogram, offlineContextChannelCountHistogram, On 2016/05/23 21:30:24, Mark P wrote: > On 2016/05/23 20:36:03, Raymond Toy wrote: > > On 2016/05/23 18:10:24, Mark P wrote: > > > SparseHistograms are slow because they acquire a lock each time you emit to > > > them. > > > > We don't expect many offline contexts, so speed isn't important. (Plus we're > > thread-limited on the number of simultaneous contexts.) > > > > > Are you sure you want to use the type of histogram here? > > > I would think for most of these counts an appropriate COUNT or ENUMERATED > > > histogram would be better. > > > ditto with all your other histogram usages in other files in this > changelist. > > > > Yes, after your initial comment (about adding units), I see there are lots of > > other types that we should consider. > > Yes, I would encourage you to use other types when it feels appropriate. Even > if speed isn't an issue, one thing to keep in mind is that sparse histograms can > grow in memory unbounded if you keep emitting distinct values to them. Unboundedness might be problem? Not really sure, but I suspect the sample rates are probably the typical audio hardware sample rates (44.1 kHz, 48 kHz, mostly and maybe 192). The possible rates allowed by WebAudio are 3000-192000 Hz. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59547: +<histogram name="WebAudio.AudioBuffer.Length"> On 2016/05/20 23:03:30, Mark P wrote: > please add units= annotations to all these histograms. Any constraints on what units might be? https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59593: +<histogram name="WebAudio.AudioContext.SampleRate"> On 2016/05/23 21:30:24, Mark P wrote: > On 2016/05/23 20:36:03, Raymond Toy wrote: > > On 2016/05/23 18:10:25, Mark P wrote: > > > ditto about putting hardware in the title, as I can easily imagine having a > > > software audio sample rate too (e.g., audio is resampled at a lower rate for > > > uploading). > > > > There is a spec change where an audio context is supposed to allow an > > constructor option to specify the sample rate. Not sure how to handle this, > but > > perhaps HardWareSampleRate is best in this case. > > Ah. > > BTW, "Hardware" is the normal way to capitalize the word. Yeah, typo on my part. I've changed it to HardwareSampleRate now. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59644: +<histogram name="WebAudio.BiquadFilter.Type"> On 2016/05/23 18:10:24, Mark P wrote: > Please use an enum= and then you won't need the enum descriptions in the > histogram description. Done. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/23 18:10:25, Mark P wrote: > Can you give an approximate description of how often this occurs? Did you mean in the summary, or just for your infomation? https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59691: +<histogram name="WebAudio.PannerNode.PanningModel"> On 2016/05/23 18:10:24, Mark P wrote: > Please use an enum. Done.
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59547: +<histogram name="WebAudio.AudioBuffer.Length"> On 2016/05/23 23:12:01, Raymond Toy wrote: > On 2016/05/20 23:03:30, Mark P wrote: > > please add units= annotations to all these histograms. > > Any constraints on what units might be? No. It's just a string displayed in the dashboard column. If you use enum=, then it has to be something in the enum list at the bottom of the file. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/23 23:12:00, Raymond Toy wrote: > On 2016/05/23 18:10:25, Mark P wrote: > > Can you give an approximate description of how often this occurs? > > Did you mean in the summary, or just for your infomation? Both times I asked for descriptions of when they're emitted, I meant to add it to the description in the file.
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/23 23:41:54, Mark P wrote: > On 2016/05/23 23:12:00, Raymond Toy wrote: > > On 2016/05/23 18:10:25, Mark P wrote: > > > Can you give an approximate description of how often this occurs? > > > > Did you mean in the summary, or just for your infomation? > > Both times I asked for descriptions of when they're emitted, I meant to add it > to the description in the file. Ok. The summary currently says it's recorded each time the type is set. Do you want more than this?
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> On 2016/05/23 21:30:24, Mark P wrote: > On 2016/05/23 20:36:03, Raymond Toy wrote: > > On 2016/05/23 18:10:24, Mark P wrote: > > > Consider "Available" instead of "Count". Count seems to be more like the > > number > > > requested, not a hard cap on the number that could be used. > > > Also consider putting hardware in the histogram name as well. > > > > I'm ok with changing the name, but maxChannelCount is the name of the > attribute > > in the audio context. > > I'd encourage clarity in the histograms because it might not be just team > members looking at them. (Yes, even at the expense of differing naming in the > code. People who care about the details can always look up the call. But if > the name is misleading, they might think they know what's going on but me > wrong.) (Also, you could always revise the code.) I'm ok with changing the name, but maxChannelCount is the actual AudioContext idl attribute name so I thought that would be clearer. WDYT?
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> On 2016/05/24 17:04:12, Raymond Toy wrote: > On 2016/05/23 21:30:24, Mark P wrote: > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > Consider "Available" instead of "Count". Count seems to be more like the > > > number > > > > requested, not a hard cap on the number that could be used. > > > > Also consider putting hardware in the histogram name as well. > > > > > > I'm ok with changing the name, but maxChannelCount is the name of the > > attribute > > > in the audio context. > > > > I'd encourage clarity in the histograms because it might not be just team > > members looking at them. (Yes, even at the expense of differing naming in the > > code. People who care about the details can always look up the call. But if > > the name is misleading, they might think they know what's going on but me > > wrong.) (Also, you could always revise the code.) > > I'm ok with changing the name, but maxChannelCount is the actual AudioContext > idl attribute name so I thought that would be clearer. > > WDYT? Again, I'd still prefer the name changed. I'd read this histogram name the maximum number of channels used is a webaudio stream, not the maximum available. People don't always read the histogram descriptions; let's not mislead them based on the name. I don't see any downside to this name--even if users are familiar with the spec, they'll be able to tell what the new name means. If you want to change the code too for consistency and clarity, that's fine, but that's more of a discussion with the code owners. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/24 16:54:22, Raymond Toy wrote: > On 2016/05/23 23:41:54, Mark P wrote: > > On 2016/05/23 23:12:00, Raymond Toy wrote: > > > On 2016/05/23 18:10:25, Mark P wrote: > > > > Can you give an approximate description of how often this occurs? > > > > > > Did you mean in the summary, or just for your infomation? > > > > Both times I asked for descriptions of when they're emitted, I meant to add it > > to the description in the file. > > Ok. The summary currently says it's recorded each time the type is set. Do you > want more than this? Yes. It's not obvious how often this is set. It is set open every web audio load? Only web audio loads that have different values than the previous one? Set every time a web audio is rebuffered? Set every ten seconds? I don't need precision but having a good idea of the type of things that cause this to be emitted is good.
https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59584: +<histogram name="WebAudio.AudioContext.MaxChannelCount"> On 2016/05/24 17:35:18, Mark P wrote: > On 2016/05/24 17:04:12, Raymond Toy wrote: > > On 2016/05/23 21:30:24, Mark P wrote: > > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > > Consider "Available" instead of "Count". Count seems to be more like > the > > > > number > > > > > requested, not a hard cap on the number that could be used. > > > > > Also consider putting hardware in the histogram name as well. > > > > > > > > I'm ok with changing the name, but maxChannelCount is the name of the > > > attribute > > > > in the audio context. > > > > > > I'd encourage clarity in the histograms because it might not be just team > > > members looking at them. (Yes, even at the expense of differing naming in > the > > > code. People who care about the details can always look up the call. But > if > > > the name is misleading, they might think they know what's going on but me > > > wrong.) (Also, you could always revise the code.) > > > > I'm ok with changing the name, but maxChannelCount is the actual AudioContext > > idl attribute name so I thought that would be clearer. > > > > WDYT? > > Again, I'd still prefer the name changed. I'd read this histogram name the > maximum number of channels used is a webaudio stream, not the maximum available. > People don't always read the histogram descriptions; let's not mislead them > based on the name. I don't see any downside to this name--even if users are > familiar with the spec, they'll be able to tell what the new name means. If you > want to change the code too for consistency and clarity, that's fine, but that's > more of a discussion with the code owners. No problem; I'll change it. https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:59650: + the type is set. On 2016/05/24 17:35:18, Mark P wrote: > On 2016/05/24 16:54:22, Raymond Toy wrote: > > On 2016/05/23 23:41:54, Mark P wrote: > > > On 2016/05/23 23:12:00, Raymond Toy wrote: > > > > On 2016/05/23 18:10:25, Mark P wrote: > > > > > Can you give an approximate description of how often this occurs? > > > > > > > > Did you mean in the summary, or just for your infomation? > > > > > > Both times I asked for descriptions of when they're emitted, I meant to add > it > > > to the description in the file. > > > > Ok. The summary currently says it's recorded each time the type is set. Do > you > > want more than this? > > Yes. It's not obvious how often this is set. It is set open every web audio > load? Only web audio loads that have different values than the previous one? > Set every time a web audio is rebuffered? Set every ten seconds? I don't need > precision but having a good idea of the type of things that cause this to be > emitted is good. I don't have a good feel for this. It's certainly set once (to the default lowpass) whenever a biquad filter is created. I assume it gets set once more if the user wanted a different filter. I'm assuming after that it never gets set again. But a particular page could generate hundreds or thousands of these (but probably only on user interaction, so not more than a few tens of filters per second?). I'm not sure how to phrase this in the summary. For the other entries for AudioContext or OfflineAudioContext, I think there's probably only one AudioContext per page. OfflineAudioContexts can have more but typically none (?), but I have seen pages that want to create 1000+ (which doesn't work because we run out of threads). For the AudioBuffer, there are probably quite a few per page. Up to hundreds, or maybe thousands, since these hold the sounds to be played. I don't have a feel for how often these are created. I suspect these are created at application start. hongchan: Do you have any feel for these?
On 2016/05/24 17:46:40, Raymond Toy wrote: > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:59584: +<histogram > name="WebAudio.AudioContext.MaxChannelCount"> > On 2016/05/24 17:35:18, Mark P wrote: > > On 2016/05/24 17:04:12, Raymond Toy wrote: > > > On 2016/05/23 21:30:24, Mark P wrote: > > > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > > > Consider "Available" instead of "Count". Count seems to be more like > > the > > > > > number > > > > > > requested, not a hard cap on the number that could be used. > > > > > > Also consider putting hardware in the histogram name as well. > > > > > > > > > > I'm ok with changing the name, but maxChannelCount is the name of the > > > > attribute > > > > > in the audio context. > > > > > > > > I'd encourage clarity in the histograms because it might not be just team > > > > members looking at them. (Yes, even at the expense of differing naming in > > the > > > > code. People who care about the details can always look up the call. But > > if > > > > the name is misleading, they might think they know what's going on but me > > > > wrong.) (Also, you could always revise the code.) > > > > > > I'm ok with changing the name, but maxChannelCount is the actual > AudioContext > > > idl attribute name so I thought that would be clearer. > > > > > > WDYT? > > > > Again, I'd still prefer the name changed. I'd read this histogram name the > > maximum number of channels used is a webaudio stream, not the maximum > available. > > People don't always read the histogram descriptions; let's not mislead them > > based on the name. I don't see any downside to this name--even if users are > > familiar with the spec, they'll be able to tell what the new name means. If > you > > want to change the code too for consistency and clarity, that's fine, but > that's > > more of a discussion with the code owners. > > No problem; I'll change it. > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:59650: + the type is set. > On 2016/05/24 17:35:18, Mark P wrote: > > On 2016/05/24 16:54:22, Raymond Toy wrote: > > > On 2016/05/23 23:41:54, Mark P wrote: > > > > On 2016/05/23 23:12:00, Raymond Toy wrote: > > > > > On 2016/05/23 18:10:25, Mark P wrote: > > > > > > Can you give an approximate description of how often this occurs? > > > > > > > > > > Did you mean in the summary, or just for your infomation? > > > > > > > > Both times I asked for descriptions of when they're emitted, I meant to > add > > it > > > > to the description in the file. > > > > > > Ok. The summary currently says it's recorded each time the type is set. Do > > you > > > want more than this? > > > > Yes. It's not obvious how often this is set. It is set open every web audio > > load? Only web audio loads that have different values than the previous one? > > Set every time a web audio is rebuffered? Set every ten seconds? I don't > need > > precision but having a good idea of the type of things that cause this to be > > emitted is good. > > I don't have a good feel for this. It's certainly set once (to the default > lowpass) whenever a biquad filter is created. I assume it gets set once more if > the user wanted a different filter. I'm assuming after that it never gets set > again. Mark P: can you give some guidance on what you want to see here? > > But a particular page could generate hundreds or thousands of these (but > probably only on user interaction, so not more than a few tens of filters per > second?). > > I'm not sure how to phrase this in the summary. > > For the other entries for AudioContext or OfflineAudioContext, I think there's > probably only one AudioContext per page. OfflineAudioContexts can have more but > typically none (?), but I have seen pages that want to create 1000+ (which > doesn't work because we run out of threads). > > For the AudioBuffer, there are probably quite a few per page. Up to hundreds, or > maybe thousands, since these hold the sounds to be played. I don't have a feel > for how often these are created. I suspect these are created at application > start. > > hongchan: Do you have any feel for these?
On Tue, May 31, 2016 at 2:36 PM, rtoy@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2016/05/24 17:46:40, Raymond Toy wrote: > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:59584: +<histogram > > name="WebAudio.AudioContext.MaxChannelCount"> > > On 2016/05/24 17:35:18, Mark P wrote: > > > On 2016/05/24 17:04:12, Raymond Toy wrote: > > > > On 2016/05/23 21:30:24, Mark P wrote: > > > > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > > > > Consider "Available" instead of "Count". Count seems to be more > like > > > the > > > > > > number > > > > > > > requested, not a hard cap on the number that could be used. > > > > > > > Also consider putting hardware in the histogram name as well. > > > > > > > > > > > > I'm ok with changing the name, but maxChannelCount is the name > of the > > > > > attribute > > > > > > in the audio context. > > > > > > > > > > I'd encourage clarity in the histograms because it might not be > just > team > > > > > members looking at them. (Yes, even at the expense of differing > naming > in > > > the > > > > > code. People who care about the details can always look up the > call. > But > > > if > > > > > the name is misleading, they might think they know what's going on > but > me > > > > > wrong.) (Also, you could always revise the code.) > > > > > > > > I'm ok with changing the name, but maxChannelCount is the actual > > AudioContext > > > > idl attribute name so I thought that would be clearer. > > > > > > > > WDYT? > > > > > > Again, I'd still prefer the name changed. I'd read this histogram name > the > > > maximum number of channels used is a webaudio stream, not the maximum > > available. > > > People don't always read the histogram descriptions; let's not mislead > them > > > based on the name. I don't see any downside to this name--even if > users are > > > familiar with the spec, they'll be able to tell what the new name > means. If > > you > > > want to change the code too for consistency and clarity, that's fine, > but > > that's > > > more of a discussion with the code owners. > > > > No problem; I'll change it. > > > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:59650: + the type is set. > > On 2016/05/24 17:35:18, Mark P wrote: > > > On 2016/05/24 16:54:22, Raymond Toy wrote: > > > > On 2016/05/23 23:41:54, Mark P wrote: > > > > > On 2016/05/23 23:12:00, Raymond Toy wrote: > > > > > > On 2016/05/23 18:10:25, Mark P wrote: > > > > > > > Can you give an approximate description of how often this > occurs? > > > > > > > > > > > > Did you mean in the summary, or just for your infomation? > > > > > > > > > > Both times I asked for descriptions of when they're emitted, I > meant to > > add > > > it > > > > > to the description in the file. > > > > > > > > Ok. The summary currently says it's recorded each time the type is > set. > Do > > > you > > > > want more than this? > > > > > > Yes. It's not obvious how often this is set. It is set open every web > audio > > > load? Only web audio loads that have different values than the previous > one? > > > Set every time a web audio is rebuffered? Set every ten seconds? I > don't > > need > > > precision but having a good idea of the type of things that cause this > to be > > > emitted is good. > > > > I don't have a good feel for this. It's certainly set once (to the > default > > lowpass) whenever a biquad filter is created. I assume it gets set once > more > if > > the user wanted a different filter. I'm assuming after that it never > gets set > > again. > > Mark P: can you give some guidance on what you want to see here? > I'm hoping for an additional explanation, something like: Set once (to the default lowpass) whenever a biquad filter is created. Later changes could happen due to user action or programmatically, though it's hard to imagine why it would be re-set multiple additional times. > > > But a particular page could generate hundreds or thousands of these (but > > probably only on user interaction, so not more than a few tens of > filters per > > second?). > > > > I'm not sure how to phrase this in the summary. > > > > For the other entries for AudioContext or OfflineAudioContext, I think > there's > > probably only one AudioContext per page. OfflineAudioContexts can have > more > but > > typically none (?), but I have seen pages that want to create 1000+ > (which > > doesn't work because we run out of threads). > > > > For the AudioBuffer, there are probably quite a few per page. Up to > hundreds, > or > > maybe thousands, since these hold the sounds to be played. I don't have a > feel > > for how often these are created. I suspect these are created at > application > > start. > > > > hongchan: Do you have any feel for these? > > > > https://codereview.chromium.org/1978403004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Tue, May 31, 2016 at 2:36 PM, rtoy@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2016/05/24 17:46:40, Raymond Toy wrote: > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:59584: +<histogram > > name="WebAudio.AudioContext.MaxChannelCount"> > > On 2016/05/24 17:35:18, Mark P wrote: > > > On 2016/05/24 17:04:12, Raymond Toy wrote: > > > > On 2016/05/23 21:30:24, Mark P wrote: > > > > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > > > > Consider "Available" instead of "Count". Count seems to be more > like > > > the > > > > > > number > > > > > > > requested, not a hard cap on the number that could be used. > > > > > > > Also consider putting hardware in the histogram name as well. > > > > > > > > > > > > I'm ok with changing the name, but maxChannelCount is the name > of the > > > > > attribute > > > > > > in the audio context. > > > > > > > > > > I'd encourage clarity in the histograms because it might not be > just > team > > > > > members looking at them. (Yes, even at the expense of differing > naming > in > > > the > > > > > code. People who care about the details can always look up the > call. > But > > > if > > > > > the name is misleading, they might think they know what's going on > but > me > > > > > wrong.) (Also, you could always revise the code.) > > > > > > > > I'm ok with changing the name, but maxChannelCount is the actual > > AudioContext > > > > idl attribute name so I thought that would be clearer. > > > > > > > > WDYT? > > > > > > Again, I'd still prefer the name changed. I'd read this histogram name > the > > > maximum number of channels used is a webaudio stream, not the maximum > > available. > > > People don't always read the histogram descriptions; let's not mislead > them > > > based on the name. I don't see any downside to this name--even if > users are > > > familiar with the spec, they'll be able to tell what the new name > means. If > > you > > > want to change the code too for consistency and clarity, that's fine, > but > > that's > > > more of a discussion with the code owners. > > > > No problem; I'll change it. > > > > > > https://codereview.chromium.org/1978403004/diff/60001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:59650: + the type is set. > > On 2016/05/24 17:35:18, Mark P wrote: > > > On 2016/05/24 16:54:22, Raymond Toy wrote: > > > > On 2016/05/23 23:41:54, Mark P wrote: > > > > > On 2016/05/23 23:12:00, Raymond Toy wrote: > > > > > > On 2016/05/23 18:10:25, Mark P wrote: > > > > > > > Can you give an approximate description of how often this > occurs? > > > > > > > > > > > > Did you mean in the summary, or just for your infomation? > > > > > > > > > > Both times I asked for descriptions of when they're emitted, I > meant to > > add > > > it > > > > > to the description in the file. > > > > > > > > Ok. The summary currently says it's recorded each time the type is > set. > Do > > > you > > > > want more than this? > > > > > > Yes. It's not obvious how often this is set. It is set open every web > audio > > > load? Only web audio loads that have different values than the previous > one? > > > Set every time a web audio is rebuffered? Set every ten seconds? I > don't > > need > > > precision but having a good idea of the type of things that cause this > to be > > > emitted is good. > > > > I don't have a good feel for this. It's certainly set once (to the > default > > lowpass) whenever a biquad filter is created. I assume it gets set once > more > if > > the user wanted a different filter. I'm assuming after that it never > gets set > > again. > > Mark P: can you give some guidance on what you want to see here? > I'm hoping for an additional explanation, something like: Set once (to the default lowpass) whenever a biquad filter is created. Later changes could happen due to user action or programmatically, though it's hard to imagine why it would be re-set multiple additional times. > > > But a particular page could generate hundreds or thousands of these (but > > probably only on user interaction, so not more than a few tens of > filters per > > second?). > > > > I'm not sure how to phrase this in the summary. > > > > For the other entries for AudioContext or OfflineAudioContext, I think > there's > > probably only one AudioContext per page. OfflineAudioContexts can have > more > but > > typically none (?), but I have seen pages that want to create 1000+ > (which > > doesn't work because we run out of threads). > > > > For the AudioBuffer, there are probably quite a few per page. Up to > hundreds, > or > > maybe thousands, since these hold the sounds to be played. I don't have a > feel > > for how often these are created. I suspect these are created at > application > > start. > > > > hongchan: Do you have any feel for these? > > > > https://codereview.chromium.org/1978403004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. I've added more descriptions; let me know how if I can make them better.
On 2016/05/31 22:50:35, Raymond Toy wrote: > PTAL. I've added more descriptions; let me know how if I can make them better. I'm sorry for the delay. I promise I'll do this tomorrow. --mark
histograms.xml is fine, but I still have some other comments on this changelist --mark https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:94: DEFINE_STATIC_LOCAL(SparseHistogram, offlineContextChannelCountHistogram, On 2016/05/23 23:12:00, Raymond Toy wrote: > On 2016/05/23 21:30:24, Mark P wrote: > > On 2016/05/23 20:36:03, Raymond Toy wrote: > > > On 2016/05/23 18:10:24, Mark P wrote: > > > > SparseHistograms are slow because they acquire a lock each time you emit > to > > > > them. > > > > > > We don't expect many offline contexts, so speed isn't important. (Plus we're > > > thread-limited on the number of simultaneous contexts.) > > > > > > > Are you sure you want to use the type of histogram here? > > > > I would think for most of these counts an appropriate COUNT or ENUMERATED > > > > histogram would be better. > > > > ditto with all your other histogram usages in other files in this > > changelist. > > > > > > Yes, after your initial comment (about adding units), I see there are lots > of > > > other types that we should consider. > > > > Yes, I would encourage you to use other types when it feels appropriate. Even > > if speed isn't an issue, one thing to keep in mind is that sparse histograms > can > > grow in memory unbounded if you keep emitting distinct values to them. > > Unboundedness might be problem? Not really sure, but I suspect the sample rates > are probably the typical audio hardware sample rates (44.1 kHz, 48 kHz, mostly > and maybe 192). The possible rates allowed by WebAudio are 3000-192000 Hz. I'm still nervous about these being sparse histograms. I can easily imagine a web page showing off fun features, say, do something like for i = 0 to 10000 { create and play audio context of length i } or // Listen how sample rates affect things for i = 3k to 192k { create and play audio context with sample rate of length i } and the browser's memory will expand substantially (O(megabytes)), which will not be released until a reboot. Are you comfortable with this penalty when webaudio pages do strange things? (Or are you sure no one is going to do strange things.) And if the length goes from, say, 0 to 10m instead, we're talking hundreds of megabytes. ... Actually, I just noticed you use sparse histograms in many places in this changelist. I'll emphasize again that such histograms keep every distinct value emitted to in memory for as long as chrome is running. Please audit these to make sure the distinct values in each histogram is going to be from a small set. https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp (right): https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp:141: ("WebAudio.BiquadFilter.Type", BiquadProcessor::Allpass + 1)); nit: extra space https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp:141: ("WebAudio.BiquadFilter.Type", BiquadProcessor::Allpass + 1)); Please put something by the enum where this is declared that the enum entries shouldn't ever be renumbered or deleted. https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/PannerNode.cpp:293: panningModelHistogram.count(model); Please put a comment in third_party/WebKit/Source/platform/audio/Panner.h by the definition that values are written to logs and should not be changed.
Thanks for your comments. As you rightly point out, the sparse histograms could have been basically unbounded. I've added code to limit the size of the histograms to 60 entries at most. The text in the descriptions have been updated to show the formula used so that someone can map the recorded value to the actual data. https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp (right): https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp:141: ("WebAudio.BiquadFilter.Type", BiquadProcessor::Allpass + 1)); On 2016/06/03 19:52:37, Mark P wrote: > nit: extra space Done. https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/1978403004/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/PannerNode.cpp:293: panningModelHistogram.count(model); On 2016/06/03 19:52:37, Mark P wrote: > Please put a comment in > third_party/WebKit/Source/platform/audio/Panner.h > by the definition that values are written to logs and should not be changed. Done.
https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:254: audioBufferLengthHistogram.sample(clampTo(static_cast<int>(0.5 + histogramValue), 0, 60)); If you're doing a histogram with min and max and exponentially spaced buckets between them, you might want to use a CustomCountHistogram. It does all this log math for you. (Admittedly this is no longer sparse, but it'll yield cleaner code, be faster, and handle edge cases correctly.) ditto everywhere below, and elsewhere in this changelist
On 2016/06/09 20:18:24, Mark P wrote: > https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp > (right): > > https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:254: > audioBufferLengthHistogram.sample(clampTo(static_cast<int>(0.5 + > histogramValue), 0, 60)); > If you're doing a histogram with min and max and exponentially spaced buckets > between them, you might want to use a CustomCountHistogram. It does all this > log math for you. > (Admittedly this is no longer sparse, but it'll yield cleaner code, be faster, > and handle edge cases correctly.) > > ditto everywhere below, and elsewhere in this changelist Thanks for the hint. I'll upload this change soon. However, I'm curious how to interpret the results. I set the offline context sample rate to a custom histogram with min 1, max 60, buckets = 60. I ran a test where all sample rates were 16000. This histogram (from chrome://histograms) produces a value of 60. How is the 60 mapped to 16000?
On 2016/06/09 21:17:55, Raymond Toy wrote: > On 2016/06/09 20:18:24, Mark P wrote: > > > https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp > > (right): > > > > > https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:254: > > audioBufferLengthHistogram.sample(clampTo(static_cast<int>(0.5 + > > histogramValue), 0, 60)); > > If you're doing a histogram with min and max and exponentially spaced buckets > > between them, you might want to use a CustomCountHistogram. It does all this > > log math for you. > > (Admittedly this is no longer sparse, but it'll yield cleaner code, be faster, > > and handle edge cases correctly.) > > > > ditto everywhere below, and elsewhere in this changelist > > Thanks for the hint. I'll upload this change soon. > > However, I'm curious how to interpret the results. I set the offline context > sample rate to a custom histogram with min 1, max 60, buckets = 60. I ran a > test where all sample rates were 16000. This histogram (from > chrome://histograms) produces a value of 60. How is the 60 mapped to 16000? Oops. I had the wrong max values specified for the histogram.
https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:254: audioBufferLengthHistogram.sample(clampTo(static_cast<int>(0.5 + histogramValue), 0, 60)); On 2016/06/09 20:18:23, Mark P wrote: > If you're doing a histogram with min and max and exponentially spaced buckets > between them, you might want to use a CustomCountHistogram. It does all this > log math for you. > (Admittedly this is no longer sparse, but it'll yield cleaner code, be faster, > and handle edge cases correctly.) > > ditto everywhere below, and elsewhere in this changelist Done.
metrics lgtm, with one additional comment below --mark https://codereview.chromium.org/1978403004/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:73: ("WebAudio.AudioContext.HardwareSampleRate")); Is this generally fixed or should this be a custom histogram too?
https://codereview.chromium.org/1978403004/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1978403004/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:73: ("WebAudio.AudioContext.HardwareSampleRate")); On 2016/06/10 04:02:53, Mark P wrote: > Is this generally fixed or should this be a custom histogram too? We expect the possible values for the hardware sample rate to be from a fairly small set. Our expectations are that 44100, 48000, 88200, 96000, and 192000 represent basically all of the possible values. We have some suspicions that there are some other values, though and we'd like to capture the actual value.
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/1978403004/#ps190001 (title: "Use CustomCountHistogram")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978403004/190001
Message was sent while issue was closed.
Description was changed from ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioBuffer.numberOfChannels AudioBuffer.length AudioBuffer.sampleRate AudioBuffer.sampleRateRatio (buffer rate divided by context rate) AudioContext.maxChannelCount AudioContext.sampleRate BiquadFilter.type Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ========== to ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioBuffer.numberOfChannels AudioBuffer.length AudioBuffer.sampleRate AudioBuffer.sampleRateRatio (buffer rate divided by context rate) AudioContext.maxChannelCount AudioContext.sampleRate BiquadFilter.type Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioBuffer.numberOfChannels AudioBuffer.length AudioBuffer.sampleRate AudioBuffer.sampleRateRatio (buffer rate divided by context rate) AudioContext.maxChannelCount AudioContext.sampleRate BiquadFilter.type Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms ========== to ========== Add UMA histograms for WebAudio. Histograms are added to keep track of the following properties: AudioBuffer.numberOfChannels AudioBuffer.length AudioBuffer.sampleRate AudioBuffer.sampleRateRatio (buffer rate divided by context rate) AudioContext.maxChannelCount AudioContext.sampleRate BiquadFilter.type Panner.panningModel OfflineAudioContext.channelCount OfflineAudioContext.length OfflineAudioContext.sampleRate BUG=607711, 621292 TEST=manually test using chrome://histograms Committed: https://crrev.com/8ceed89c8064f5c835b51c0ea07f259bceaca1d4 Cr-Commit-Position: refs/heads/master@{#399212} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8ceed89c8064f5c835b51c0ea07f259bceaca1d4 Cr-Commit-Position: refs/heads/master@{#399212} |
