Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067863003:
Mixing audio with different latency requirements (Closed)
|
Created:
4 years, 6 months ago by o1ka Modified:
4 years, 5 months ago Reviewers:
Henrik Grunell, tommi (sloooow) - chröme, Steven Holte, chcunningham, DaleCurtis, henrika (OOO until Aug 14) CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, vanellope-cl_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMixing audio with different latency requirements.
Follow-up is introducing mixing for audio other than media elements (under finch).
** Helper changes:
1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now).
2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it.
** Mixing changes:
3) AudioRendererMixerPool/AudioRendererMixerManager:
* latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration.
* ARMM adjusts mixer output buffer siza based on input latency and output sample rate.
BUG=560378, 564472
Committed: https://crrev.com/6bbdb8b35b1977bd45e74b882dce8e70416cf190
Cr-Commit-Position: refs/heads/master@{#403120}
Patch Set 1 #Patch Set 2 : mixing inputs of the same latency #
Total comments: 42
Patch Set 3 : revew comments addressed #Patch Set 4 : UMA added, LATENCY_EXACT_MS support removed #Patch Set 5 : Unit tests, cleanup #
Total comments: 27
Patch Set 6 : rebase #
Total comments: 5
Patch Set 7 : UMA fix, unit tests and compile error fixes on some platforms, review comments addressed #
Total comments: 42
Patch Set 8 : fixing bot redness #
Total comments: 10
Patch Set 9 : Review comments addressed #
Total comments: 3
Patch Set 10 : UMA boundary fixes according to recomendations, rebase #Patch Set 11 : android test fix #Messages
Total messages: 60 (14 generated)
Description was changed from ========== Mixing audio with different latency requirements BUG= ========== to ========== Mixing audio with different latency requirements. Pre-requisite for this CL is landing grunell@’s CLs on making AudioOutputDevice restartable. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that inputs with all the latency requirements but LATENCY_EXACT_MS can be mixed together; they will be played out at the lowest latency in the mix. LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same output buffer size duration. 4) AudioRendererMixer::OutputParamDetector interfaces is introduced, which allows to implement different rebuffering strategies for a mixer. ARMM implements two of those: a) output at the exact buffers size in ms (used for LATENCY_EXACT_MS inputs); b) output at the lowers buffer size in the mix (used for all the inputs but LATENCY_EXACT_MS). 5) AudioRendererMixer: Reconfigures and restarts a sink when a change of output bufffer size is detected by AudioRendererMixer::OutputParamDetector when mixer inputs added/removed. * Mixer inputs operate on at least two threads (main and media renderer threads), sink restart operations need to be serialized. |opreation_lock_| is introduced for this purpose; and |converter_lock_| guards converters. This split up is required because a sink cannot be stopped under the lock which is acquired in Render() call (causes deadlock). * Sink pause is now performed immediately as soon as there are no inputs. This can be changed to posting a delayed task on the thread a mixer is created on; I’m not sure of the usecase it addresses and if it’s still valid now when we restart the sink. Sink pausing cannot be done in Render() call any more, because of the locking problem (above). BUG=560378,564472 ==========
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, grunell@chromium.org
Dale, PTAL at the approach. The main points I'm looking your input on are in the description - could you evaluate all of them? Unit tests are not there yet, and manual testing did not work out today, so "beware of bugs in the above code; I have only proved it correct, not tried it" Thanks
I think we'll want to avoid stream restart as much as possible, some platforms have startup times in the hundreds of milliseconds (see AudioOutputController UMA entries) -- is it possible to avoid 5) entirely?
dalecurtis@chromium.org changed reviewers: + chcunningham@chromium.org
+chcunningham who I'd like to help review.
On 2016/06/15 23:16:30, DaleCurtis wrote: > I think we'll want to avoid stream restart as much as possible, some platforms > have startup times in the hundreds of milliseconds (see AudioOutputController > UMA entries) -- is it possible to avoid 5) entirely? This is my major concern as well. We've had another round of discussions here today, and decided to not go for device restart so far, which means we won't be changing output buffer sizes, and we'll have another set of rules for mixing. So please HOLD ON with the review (and I also found some bugs as well). I'll have another patch ready soon.
Description was changed from ========== Mixing audio with different latency requirements. Pre-requisite for this CL is landing grunell@’s CLs on making AudioOutputDevice restartable. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that inputs with all the latency requirements but LATENCY_EXACT_MS can be mixed together; they will be played out at the lowest latency in the mix. LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same output buffer size duration. 4) AudioRendererMixer::OutputParamDetector interfaces is introduced, which allows to implement different rebuffering strategies for a mixer. ARMM implements two of those: a) output at the exact buffers size in ms (used for LATENCY_EXACT_MS inputs); b) output at the lowers buffer size in the mix (used for all the inputs but LATENCY_EXACT_MS). 5) AudioRendererMixer: Reconfigures and restarts a sink when a change of output bufffer size is detected by AudioRendererMixer::OutputParamDetector when mixer inputs added/removed. * Mixer inputs operate on at least two threads (main and media renderer threads), sink restart operations need to be serialized. |opreation_lock_| is introduced for this purpose; and |converter_lock_| guards converters. This split up is required because a sink cannot be stopped under the lock which is acquired in Render() call (causes deadlock). * Sink pause is now performed immediately as soon as there are no inputs. This can be changed to posting a delayed task on the thread a mixer is created on; I’m not sure of the usecase it addresses and if it’s still valid now when we restart the sink. Sink pausing cannot be done in Render() call any more, because of the locking problem (above). BUG=560378,564472 ========== to ========== Mixing audio with different latency requirements. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: * latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration. * ARMM adjusts mixer output buffer siza based on input latency and output sample rate. BUG=560378,564472 ==========
PTAL at the new version. I'm also going to add UMA stat for maximum number of inputs per mixer for each latency type (registered on mixer destruction) as a part of this CL, if you don't mind. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:51: case media::AudioLatency::LATENCY_EXACT_MS: It's not used right now, but we'll need it for http://crbug/564276. I'm not sure if it should be dropped from the CL. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:61: media::AudioLatency::GetInteractiveBufferSize( Also not sure about this check, probably should be dropped. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:104: int AudioLatency::GetInteractiveBufferSize(int hardware_buffer_size) { Not sure if we should have it here for now. I borrowed the logic from AudioDestination. It's very confusing that WebAudio now uses default output device to get output parameters, and I wanted to add an extra check in case it switches someday to allow output to non-default sink - to make sure it uses the right sink to get output parameters.
Probably won't have time to look over this today, so defer to Chris and Henrik to do the initial review.
Apologies for the delay - I took some time to re-learn some of the affected parts of the stack. Here are some easy comments. Still getting to the meat of this. Back later tonight. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_device_factory.h:41: kSourceWebAudioInteractive, Can you add some comments about what makes these types unique? I know we haven't done it for the existing types, but its less obvious to me what these are about. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:129: return CompareLatency(a, b); Can you CompareLatency just once above this if and then keep the logic here and below the same? Maybe I'm missing something subtle https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latency.h File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.h:17: LATENCY_INTERACTIVE, Ditto on documenting comments here. To first time reader its not clear how whether interactive is higher or lower latency than rtc/playback etc. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer_input_unittest.cc:80: EXPECT_CALL(*this, ReturnMixer(mixers_[idx].get())); Can we rename this to ReleaseMixer? Return is a little ambiguous between fetching/releasing.
https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:61: media::AudioLatency::GetInteractiveBufferSize( On 2016/06/21 15:16:41, o1ka wrote: > Also not sure about this check, probably should be dropped. I think this would be wrong for android, where GetInteractiveBufferSize is not just a passthrough return of the provided argument. I think the check should go and be replaced by setting output_buffer_size = GetInteractiveBufferSize. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:70: case media::AudioLatency::LATENCY_PLAYBACK: Could you combine this case with the default? Would this be a more sane default than having output_size = input_size? (if so, I'd also remove that setting on line 46). https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:75: DCHECK(false); If you don't combine with LATENCY_PLAYBACK, this should be NOTREACHED(); https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:113: media::AudioLatency::LatencyType latency) { Has the spec settled on latency coming in through the AudioContext constructor? I'm wondering if we should allow latency to change (like is possible for AudioParameters) after the input is created? If so, my next question would be what are the tradeoffs of having latency actually be a part of AudioParameters. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:170: DVLOG(1) << "GetMixer: mixer " << mixer << " latency " << latency nit: use __FUNCTION__. nit: can you format this log to help readability. something like << " mixer:" << mixer << " latency:" << latency https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... File media/base/audio_hardware_config_unittest.cc (left): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... media/base/audio_hardware_config_unittest.cc:91: TEST(AudioHardwareConfig, HighLatencyBufferSizes) { Are you planning to move this test to a new unit test file for AudioLatency? https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:104: int AudioLatency::GetInteractiveBufferSize(int hardware_buffer_size) { On 2016/06/21 15:16:41, o1ka wrote: > Not sure if we should have it here for now. > I borrowed the logic from AudioDestination. It's very confusing that WebAudio > now uses default output device to get output parameters, and I wanted to add an > extra check in case it switches someday to allow output to non-default sink - to > make sure it uses the right sink to get output parameters. I'm not as familiar with WebAudio, but IIUC that sounds like its actually a bug to choose the default output device in the even that a non-default sink is being used. Is that right? Is it tracked? https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latency.h File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.h:16: LATENCY_EXACT_MS = 0, is EXACT planned to be part of the spec? It seems somewhat counter to the way things settled in https://github.com/WebAudio/web-audio-api/issues/348... ie: the user agent should be the one to decide. What is the usecase for EXACT?
Answered questions, cleaned up the code. (working on metrics and some more unit tests now). https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_device_factory.h:41: kSourceWebAudioInteractive, On 2016/06/22 02:13:56, chcunningham wrote: > Can you add some comments about what makes these types unique? I know we haven't > done it for the existing types, but its less obvious to me what these are about. Tried to explain in the enum comment. I don't think it makes sense to explain individual sources and their specifics here, since they are defined by the factory and may change. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:46: int output_buffer_size = input_params.frames_per_buffer(); Note that here we rely on the current situation when clients adjust input sample rate/buffer size to fulfill their latency requirements. Probably we should DCHECK that? https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:61: media::AudioLatency::GetInteractiveBufferSize( On 2016/06/22 04:34:07, chcunningham wrote: > On 2016/06/21 15:16:41, o1ka wrote: > > Also not sure about this check, probably should be dropped. > > I think this would be wrong for android, where GetInteractiveBufferSize is not > just a passthrough return of the provided argument. I think the check should go > and be replaced by setting output_buffer_size = GetInteractiveBufferSize. Currently WebAudio sets the buffer size in AudioDestination and expects it to be the one it set. If we check it here, we crash early. If we set it here, we crash during Render() call when they check it. Currently WebAudio is going through the default device only, AFAIK. Do you think it's more future-proof to set it here? (All the WebAudio-related part here is pretty subtle and a bit painful because of WebAudio unawareness of non-default devices and because it tries to pass us the values we already know) https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:75: DCHECK(false); On 2016/06/22 04:34:07, chcunningham wrote: > If you don't combine with LATENCY_PLAYBACK, this should be NOTREACHED(); I would prefer to not combine, because it may be the case that a new latency added and the switch needs to be updated. Separate default will notice it. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:113: media::AudioLatency::LatencyType latency) { On 2016/06/22 04:34:07, chcunningham wrote: > Has the spec settled on latency coming in through the AudioContext constructor? > I'm wondering if we should allow latency to change (like is possible for > AudioParameters) after the input is created? If so, my next question would be > what are the tradeoffs of having latency actually be a part of AudioParameters. Yes, it is passed in AudioContextOptions as a constructor parameter (https://webaudio.github.io/web-audio-api/#idl-def-AudioContextOptions). And other sources are not supposed to change the latency as well. Also, latency implies some relation between sampler rate and buffer size, so adding it to AudioParameters complicates things. I don't think we should add it to AudioParameters at this point. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:170: DVLOG(1) << "GetMixer: mixer " << mixer << " latency " << latency On 2016/06/22 04:34:07, chcunningham wrote: > nit: use __FUNCTION__. > nit: can you format this log to help readability. something like << " mixer:" > << mixer << " latency:" << latency Done. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:129: return CompareLatency(a, b); On 2016/06/22 02:13:56, chcunningham wrote: > Can you CompareLatency just once above this if and then keep the logic here and > below the same? Maybe I'm missing something subtle It's me who's missed something :) Done. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... File media/base/audio_hardware_config_unittest.cc (left): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... media/base/audio_hardware_config_unittest.cc:91: TEST(AudioHardwareConfig, HighLatencyBufferSizes) { On 2016/06/22 04:34:07, chcunningham wrote: > Are you planning to move this test to a new unit test file for AudioLatency? I think it makes more sense to add tests for mixer manager those ensure a mixer with correct output parameters is created for an input with given audio parameters and latency. Working on that. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.cc:104: int AudioLatency::GetInteractiveBufferSize(int hardware_buffer_size) { On 2016/06/22 04:34:07, chcunningham wrote: > On 2016/06/21 15:16:41, o1ka wrote: > > Not sure if we should have it here for now. > > I borrowed the logic from AudioDestination. It's very confusing that WebAudio > > now uses default output device to get output parameters, and I wanted to add > an > > extra check in case it switches someday to allow output to non-default sink - > to > > make sure it uses the right sink to get output parameters. > > I'm not as familiar with WebAudio, but IIUC that sounds like its actually a bug > to choose the default output device in the even that a non-default sink is being > used. Is that right? Is it tracked? WebAudio uses only the default device now, the cause is unaware of device selection. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latency.h File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.h:16: LATENCY_EXACT_MS = 0, On 2016/06/22 04:34:07, chcunningham wrote: > is EXACT planned to be part of the spec? It seems somewhat counter to the way > things settled in https://github.com/WebAudio/web-audio-api/issues/348... ie: > the user agent should be the one to decide. What is the usecase for EXACT? (The link you gave does not work). Here https://webaudio.github.io/web-audio-api/#widl-AudioContextOptions-latencyHint latency can be specified as latency category or a double value in seconds. Quoting WebAudio folks: The usecase is DAW scenario. Applications that use this, will be applications that allow users to tweak their settings to get the lowest possible latency. The users themselves will be expected to do some work to minimize the risk of running into glitches (e.g. by not running multiple tabs, close external processes, make sure extensions aren't running etc). https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.h:17: LATENCY_INTERACTIVE, On 2016/06/22 02:13:56, chcunningham wrote: > Ditto on documenting comments here. To first time reader its not clear how > whether interactive is higher or lower latency than rtc/playback etc. Done. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer_input_unittest.cc:80: EXPECT_CALL(*this, ReturnMixer(mixers_[idx].get())); On 2016/06/22 02:13:56, chcunningham wrote: > Can we rename this to ReleaseMixer? Return is a little ambiguous between > fetching/releasing. I remember renaming it once, it used to be RemoveMixer() :) Probably ReleaseMixer() is better; it's a bit out of scope of the change though. I can do it if you insist and Dale supports it as well :)
PTAL: Added UMA stats and some unit tests (unit tests is a work in progress); Removed LATENCY_EXAC_MS calculations, since they require some more attention and platform-specific maths.
https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_device_factory.h:41: kSourceWebAudioInteractive, On 2016/06/23 16:36:15, o1ka wrote: > On 2016/06/22 02:13:56, chcunningham wrote: > > Can you add some comments about what makes these types unique? I know we > haven't > > done it for the existing types, but its less obvious to me what these are > about. > Tried to explain in the enum comment. I don't think it makes sense to explain > individual sources and their specifics here, since they are defined by the > factory and may change. Acknowledged. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:46: int output_buffer_size = input_params.frames_per_buffer(); You bring up an interesting point - now I'm confused ;). Do we trust the clients to choose a good buffer size? My main confusion is I don't see where we're really using the input buffer_size. IIUC, before your change the input buffer size is ignored. Output buffer size is historically computed here in the mixer manager via GetHighLatencyBS(samplerate, ...). In AudioRendererImpl, you can see we choose a buffer size "just for looks" in the case of src=. I don't know why we bother choosing a high latency buffer size in the MSE (expecting_configing_changes_ == true). In short, I'm not sure we can trust the input selection to be reasonable. If possible, I think we should continue ignoring input buffer size and centralize the buffer size decisions here in this one place. Dale, can you weigh in? https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:48: if (output_sample_rate != input_params.sample_rate()) { Why do we only go down this path when in/out samplerates don't match? https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:61: media::AudioLatency::GetInteractiveBufferSize( I think this gets at my comment on line 46 above -- I don't like how the choosing of buffer size is spread around - prefer to centralize here if we can. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:75: DCHECK(false); Sounds good. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:113: media::AudioLatency::LatencyType latency) { On 2016/06/23 16:36:15, o1ka wrote: > On 2016/06/22 04:34:07, chcunningham wrote: > > Has the spec settled on latency coming in through the AudioContext > constructor? > > I'm wondering if we should allow latency to change (like is possible for > > AudioParameters) after the input is created? If so, my next question would be > > what are the tradeoffs of having latency actually be a part of > AudioParameters. > > Yes, it is passed in AudioContextOptions as a constructor parameter > (https://webaudio.github.io/web-audio-api/#idl-def-AudioContextOptions). And > other sources are not supposed to change the latency as well. > Also, latency implies some relation between sampler rate and buffer size, so > adding it to AudioParameters complicates things. I don't think we should add it > to AudioParameters at this point. Acknowledged. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... File media/base/audio_hardware_config_unittest.cc (left): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... media/base/audio_hardware_config_unittest.cc:91: TEST(AudioHardwareConfig, HighLatencyBufferSizes) { Why not both? ¯\_(ツ)_/¯ These tests cover the math/platform nuances of the specific function. It depends on what your plans are for the MixerManager... if you're going to exercise and verify all the branches of this function in those tests then OK, but I think thats probably a bit low level for the AudioMixerManager. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latency.h File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_latenc... media/base/audio_latency.h:16: LATENCY_EXACT_MS = 0, On 2016/06/23 16:36:16, o1ka wrote: > On 2016/06/22 04:34:07, chcunningham wrote: > > is EXACT planned to be part of the spec? It seems somewhat counter to the way > > things settled in https://github.com/WebAudio/web-audio-api/issues/348... ie: > > the user agent should be the one to decide. What is the usecase for EXACT? > > (The link you gave does not work). Here > https://webaudio.github.io/web-audio-api/#widl-AudioContextOptions-latencyHint > latency can be specified as latency category or a double value in seconds. > Quoting WebAudio folks: The usecase is DAW scenario. Applications that use this, > will be applications that allow users to tweak their settings to get the lowest > possible latency. > The users themselves will be expected to do some work to minimize the risk of > running into glitches (e.g. by not running multiple tabs, close external > processes, make sure extensions aren't running etc). Acknowledged. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer_input_unittest.cc:80: EXPECT_CALL(*this, ReturnMixer(mixers_[idx].get())); Understood. I just kept reading ReturnMixer as something like "CreateMixer". We'll see what Dale thinks
https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:46: int output_buffer_size = input_params.frames_per_buffer(); On 2016/06/27 23:12:24, chcunningham wrote: > You bring up an interesting point - now I'm confused ;). Do we trust the clients > to choose a good buffer size? My main confusion is I don't see where we're > really using the input buffer_size. IIUC, before your change the input buffer > size is ignored. Output buffer size is historically computed here in the mixer > manager via GetHighLatencyBS(samplerate, ...). > > In AudioRendererImpl, you can see we choose a buffer size "just for looks" in > the case of src=. I don't know why we bother choosing a high latency buffer size > in the MSE (expecting_configing_changes_ == true). > > In short, I'm not sure we can trust the input selection to be reasonable. > > If possible, I think we should continue ignoring input buffer size and > centralize the buffer size decisions here in this one place. Dale, can you weigh > in? Unfortunately I did not let you know: I changed it later, you are looking at an old PS. See the latest PS, I changed the code to not rely on the client. https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:61: media::AudioLatency::GetInteractiveBufferSize( On 2016/06/27 23:12:25, chcunningham wrote: > I think this gets at my comment on line 46 above -- I don't like how the > choosing of buffer size is spread around - prefer to centralize here if we can. > Agreed. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... File media/base/audio_hardware_config_unittest.cc (left): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_hardwa... media/base/audio_hardware_config_unittest.cc:91: TEST(AudioHardwareConfig, HighLatencyBufferSizes) { On 2016/06/27 23:12:25, chcunningham wrote: > Why not both? ¯\_(ツ)_/¯ > > These tests cover the math/platform nuances of the specific function. It depends > on what your plans are for the MixerManager... if you're going to exercise and > verify all the branches of this function in those tests then OK, but I think > thats probably a bit low level for the AudioMixerManager. :)) Agreed. Done. https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer_input_unittest.cc:80: EXPECT_CALL(*this, ReturnMixer(mixers_[idx].get())); On 2016/06/27 23:12:25, chcunningham wrote: > Understood. I just kept reading ReturnMixer as something like "CreateMixer". > We'll see what Dale thinks Acknowledged.
olka@chromium.org changed reviewers: + henrika@chromium.org
PTAL at the latest PS. henrika@: PTAL at buffer size calculations in audio_latency*, audio_renderer_mixer_manager* (including unit tests).
olka@chromium.org changed reviewers: + tommi@chromium.org
Adding tommi@ as a reviewer to speed up things a bit.
Lot's of new stuff here and hard to see all implications. Regarding AudioLatency, could you perhaps point out which parts are new or are all actual calculations moved from existing code? It is difficult to see that in the existing CL. Has any set of manual tests been done on all platform to ensure that nothing is broken here?
https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:85: // TODO(henrika): Keep tuning this scheme and espcicially for low-latency nit, especially, and perhaps a good time to remove TODO(henrika) given the new structure. Perhaps TODO(olka) ;-) Note that these settings are very old.
https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... File media/base/audio_latency_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... media/base/audio_latency_unittest.cc:17: for (int i = 6400; i <= 204800; i *= 2) I don't know where this set of sample rate comes from. Not used in real life on any platform AFAIK.
https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:26: const media::AudioParameters& hardware_params, are the 'hardware' params for output or also for input? https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:38: // fake, resample to hadrware sample rate. Otherwise, pass the input one and hardware https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:82: const std::string GetMixerUmaHistogramName( The return value type here should be const char*. However, assuming that the return value will be used for histograms, I'm not sure this is a good idea. See the comment here (search for "must be the same string"): https://cs-staging.chromium.org/chromium/src/base/metrics/histogram_macros.h?... https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:112: if (std::size_t subset_value = latency_map_.to_ulong()) { why std::size_t and not size_t? also, reading the code looks like you may have meant to do ==. Assuming you meant to do assignment, I suggest you split up the assignment and comparison check: size_t subset_value = latency_map_.to_ulong(); if (subset_value) { ... } https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:119: if (a.latency != b.latency) { nit: no {} https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:169: std::bitset<media::AudioLatency::LATENCY_LAST + 1> latency_map_; this assumes the first enum value is 0 and the last is equal to the count -1. Would it make sense to have a value called LATENCY_COUNT? (and set it to be LATENCY_LAST + 1 inside the enum declaration?) https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:28: LOCAL_HISTOGRAM_CUSTOM_COUNTS(histogram_name_, max_count_, 1, 100, 100); see other comment about using a variable here https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:32: void Increment() { thread checks? https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:42: const std::string histogram_name_; const char* https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:52: AudioParameters GetOutputParamsForTesting() { return output_params_; }; nit: const& https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_pool.h (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_pool.h:40: virtual void ReturnMixer(const AudioRendererMixer* mixer) = 0; why const? I'm thinking that the "Get" method returns a non-const pointer, this returns a mixer back, which will make it "writable" again, then having the const qualifier might be misleading (assuming this pointer might be returned from GetMixer at some point in the future) https://codereview.chromium.org/2067863003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2067863003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:20924: + Number of simultaneous inputs coming to the AudioRendererMixer rendering I'm having some problems parsing the description. Let me see if my understanding is correct: Number of simultaneous inputs with user specified latency, mixed by an AudioRendererMixer. The captured value will be the highest number of inputs at any point throughout the lifetime of the mixer instance. It is useful for ... is that correct? (I'm not sure I got the part about latency right) https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... content/renderer/media/audio_device_factory.cc:45: NOTREACHED(); does this compile on all platforms?
https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:110: // |mixers_| may leak (i.e., may be non-empty at this time) as well. This means mixer destructor is never called and we need to manually log per-mixer stats here (but see the problem below). https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:113: LOCAL_HISTOGRAM_CUSTOM_COUNTS("Media.Audio.Render.AudioMixing.LatencyMap", The problem is that the destructor for RendererThreadImpl => for AudioRendererMixerManager is almost never called. Have not figured out what can be done here yet.
https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:113: LOCAL_HISTOGRAM_CUSTOM_COUNTS("Media.Audio.Render.AudioMixing.LatencyMap", On 2016/06/28 15:51:28, o1ka wrote: > The problem is that the destructor for RendererThreadImpl => for > AudioRendererMixerManager is almost never called. Have not figured out what can > be done here yet. Looks like we need to send updates to the browser each time the value changes, and log stats on the browser side upon renderer shut down.
https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:113: LOCAL_HISTOGRAM_CUSTOM_COUNTS("Media.Audio.Render.AudioMixing.LatencyMap", On 2016/06/28 16:05:32, o1ka wrote: > On 2016/06/28 15:51:28, o1ka wrote: > > The problem is that the destructor for RendererThreadImpl => for > > AudioRendererMixerManager is almost never called. Have not figured out what > can > > be done here yet. > > Looks like we need to send updates to the browser each time the value changes, > and log stats on the browser side upon renderer shut down. Another option is to log the values each time they change. The histogram will be more ambiguous/harder to interpret (especially "latency subset" one), but it may be a tradeoff to pay.
Change is looking good to me. Lets see what Dale says :) https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:46: int output_buffer_size = input_params.frames_per_buffer(); On 2016/06/28 13:04:57, o1ka wrote: > On 2016/06/27 23:12:24, chcunningham wrote: > > You bring up an interesting point - now I'm confused ;). Do we trust the > clients > > to choose a good buffer size? My main confusion is I don't see where we're > > really using the input buffer_size. IIUC, before your change the input buffer > > size is ignored. Output buffer size is historically computed here in the mixer > > manager via GetHighLatencyBS(samplerate, ...). > > > > In AudioRendererImpl, you can see we choose a buffer size "just for looks" in > > the case of src=. I don't know why we bother choosing a high latency buffer > size > > in the MSE (expecting_configing_changes_ == true). > > > > In short, I'm not sure we can trust the input selection to be reasonable. > > > > If possible, I think we should continue ignoring input buffer size and > > centralize the buffer size decisions here in this one place. Dale, can you > weigh > > in? > > Unfortunately I did not let you know: I changed it later, you are looking at an > old PS. See the latest PS, I changed the code to not rely on the client. Ack - my bad. This method looks good in the new PS.
https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:46: int output_buffer_size = input_params.frames_per_buffer(); On 2016/06/28 at 18:43:49, chcunningham wrote: > On 2016/06/28 13:04:57, o1ka wrote: > > On 2016/06/27 23:12:24, chcunningham wrote: > > > You bring up an interesting point - now I'm confused ;). Do we trust the > > clients > > > to choose a good buffer size? My main confusion is I don't see where we're > > > really using the input buffer_size. IIUC, before your change the input buffer > > > size is ignored. Output buffer size is historically computed here in the mixer > > > manager via GetHighLatencyBS(samplerate, ...). > > > > > > In AudioRendererImpl, you can see we choose a buffer size "just for looks" in > > > the case of src=. I don't know why we bother choosing a high latency buffer > > size > > > in the MSE (expecting_configing_changes_ == true). > > > > > > In short, I'm not sure we can trust the input selection to be reasonable. > > > > > > If possible, I think we should continue ignoring input buffer size and > > > centralize the buffer size decisions here in this one place. Dale, can you > > weigh > > > in? > > > > Unfortunately I did not let you know: I changed it later, you are looking at an > > old PS. See the latest PS, I changed the code to not rely on the client. > > Ack - my bad. This method looks good in the new PS. I don't think it matters either way; the size is "just for looks" for historical reasons and we can either continue ignoring it or update it to make a meaningful choice. So long as the documentation is clear and the audio doesn't glitch or waste power it doesn't really matter :)
https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:26: const media::AudioParameters& hardware_params, On 2016/06/28 14:14:54, tommi-chrömium wrote: > are the 'hardware' params for output or also for input? Tried to clarify the comment. https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:38: // fake, resample to hadrware sample rate. Otherwise, pass the input one and On 2016/06/28 14:14:54, tommi-chrömium wrote: > hardware Done. https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:82: const std::string GetMixerUmaHistogramName( On 2016/06/28 14:14:54, tommi-chrömium wrote: > The return value type here should be const char*. However, assuming that the > return value will be used for histograms, I'm not sure this is a good idea. > See the comment here (search for "must be the same string"): > > https://cs-staging.chromium.org/chromium/src/base/metrics/histogram_macros.h?... Yes, I misunderstood that statement. Fixed. https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:112: if (std::size_t subset_value = latency_map_.to_ulong()) { On 2016/06/28 14:14:54, tommi-chrömium wrote: > why std::size_t and not size_t? > > also, reading the code looks like you may have meant to do ==. > Assuming you meant to do assignment, I suggest you split up the assignment and > comparison check: > > size_t subset_value = latency_map_.to_ulong(); > if (subset_value) { > ... > } |subset_value| is declared there in "if", so hopefully it does not look like I want to compare a just-declared values with something :) Not applicable any more: UMA logging is removed from destructor. https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:119: if (a.latency != b.latency) { On 2016/06/28 14:14:54, tommi-chrömium wrote: > nit: no {} Done. https://codereview.chromium.org/2067863003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:169: std::bitset<media::AudioLatency::LATENCY_LAST + 1> latency_map_; On 2016/06/28 14:14:54, tommi-chrömium wrote: > this assumes the first enum value is 0 and the last is equal to the count -1. > Would it make sense to have a value called LATENCY_COUNT? (and set it to be > LATENCY_LAST + 1 inside the enum declaration?) Yes, this is nice, thanks. I did not like that line. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... media/base/audio_latency.cc:85: // TODO(henrika): Keep tuning this scheme and espcicially for low-latency On 2016/06/28 13:26:25, henrika wrote: > nit, especially, and perhaps a good time to remove TODO(henrika) given the new > structure. Perhaps TODO(olka) ;-) Note that these settings are very old. Modified TODO. I'm not gonna touch those old setting now, I'm afraid they will turn into dust :) https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... File media/base/audio_latency_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_latenc... media/base/audio_latency_unittest.cc:17: for (int i = 6400; i <= 204800; i *= 2) On 2016/06/28 13:33:13, henrika wrote: > I don't know where this set of sample rate comes from. Not used in real life on > any platform AFAIK. This is moved from AudioHardwareConfig unit tests. I don't think it's really important to have all the real values here, it's more for math checking. Agree that the tests can be extended/improved with real-world sample rate usage. Added TODO. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:28: LOCAL_HISTOGRAM_CUSTOM_COUNTS(histogram_name_, max_count_, 1, 100, 100); On 2016/06/28 14:14:54, tommi-chrömium wrote: > see other comment about using a variable here Done. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:32: void Increment() { On 2016/06/28 14:14:54, tommi-chrömium wrote: > thread checks? Updated a comment. It's used under a lock from multiple threads. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:42: const std::string histogram_name_; On 2016/06/28 14:14:54, tommi-chrömium wrote: > const char* Acknowledged. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:52: AudioParameters GetOutputParamsForTesting() { return output_params_; }; On 2016/06/28 14:14:54, tommi-chrömium wrote: > nit: const& Done. https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_pool.h (right): https://codereview.chromium.org/2067863003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_pool.h:40: virtual void ReturnMixer(const AudioRendererMixer* mixer) = 0; On 2016/06/28 14:14:54, tommi-chrömium wrote: > why const? > > I'm thinking that the "Get" method returns a non-const pointer, this returns a > mixer back, which will make it "writable" again, then having the const qualifier > might be misleading (assuming this pointer might be returned from GetMixer at > some point in the future) Done.
https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:20: // the destruciton. NOT thread-safe, make sure it is used under lock. Fix the comment
Some comments. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:32: int preferred_high_latency_output_bufffer_size = 0; fff -> ff https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:51: // WebAudio should provide correct callback size in frames; it does not Confusing. What does WebAudio have to do with this? AudioLatency is asked for the buffer size. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:57: // adjust output buffer size according to the latency requirement. That's what the whole switch block does. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:74: DCHECK(output_buffer_size); Nit: Better (clearer) to DCHECK_NE(x, 0) https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:86: LOCAL_HISTOGRAM_CUSTOM_COUNTS( LOCAL_ -> UMA_ https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:87: "Media.Audio.Render.AudioInputsPerMixer.LatencyExact", value, 1, 100, I think a max of 100 is unnecessarily high. 20 or 30 is probably enough, more than that will clamp to max. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:137: media::AudioLatency::LatencyType latency) { Nit: Higher readability if it's called latency_type everywhere. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:171: LOCAL_HISTOGRAM_CUSTOM_COUNTS("Media.Audio.Render.AudioMixing.LatencyMap", UMA_ https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:105: media::AudioLatency::LatencyType latency; Nit: "latency_type" is a better name. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:122: // TODO(olka) add buffer duration comparison for LATENCY_EXACT_MS when Nit: I think it's better to remove the LATENCY_EXACT_MS value since it's not allowed anyway. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:167: // Map of the output latencies encountered throughout mixer manager lifetime. Nit: Maybe describe why we store it throughout the lifetime? https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:78: // No |hardware_buffer_size| is specified, fail back to 10 ms buffer size. Fail back or fall back? https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency.h:14: // Categoties of expected latencies for input/output audio. Do not change "Categories" (spelling). https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency_unittest.cc:18: #if defined(OS_WIN) Nit: seems like only diffs by 100 vs 32. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:38: void Decrement() { --count_; } DCHECK greater than 0 before decrease? https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:22: void LogUma(int){}; Nit: space before {}. https://codereview.chromium.org/2067863003/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/2067863003/diff/120001/media/media.gyp#newcod... media/media.gyp:266: 'base/audio_latency.h', Indentation. https://codereview.chromium.org/2067863003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2067863003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21063: + a new maximum for the mixer. It is useful for evaluating how mixing audio in I think you could skip the use case example. It's a pretty specific piece of work we're doing now.
qq - instead of the noop LogUma methods, can we create a noop callback object inline? https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:172: latency_map_.to_ulong(), 1, 0xff, 0xff); won't this cause other latencies to be counted multiple times? https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:164: }; nit: remove ; https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:22: void LogUma(int){}; no ; space before { variable name for int argument https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:82: audio_parameters_, sinks_[idx].get(), base::Bind(LogUma))); &LogUma https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_unittest.cc:29: void LogUma(int){}; same here
On 2016/06/29 12:26:52, tommi-chrömium wrote: > qq - instead of the noop LogUma methods, can we create a noop callback object > inline? > > https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... > File content/renderer/media/audio_renderer_mixer_manager.cc (right): > > https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... > content/renderer/media/audio_renderer_mixer_manager.cc:172: > latency_map_.to_ulong(), 1, 0xff, 0xff); > won't this cause other latencies to be counted multiple times? > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > File media/base/audio_renderer_mixer.cc (right): > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > media/base/audio_renderer_mixer.cc:164: }; > nit: remove ; > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > File media/base/audio_renderer_mixer_input_unittest.cc (right): > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > media/base/audio_renderer_mixer_input_unittest.cc:22: void LogUma(int){}; > no ; > space before { > variable name for int argument > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > media/base/audio_renderer_mixer_input_unittest.cc:82: audio_parameters_, > sinks_[idx].get(), base::Bind(LogUma))); > &LogUma > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > File media/base/audio_renderer_mixer_unittest.cc (right): > > https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... > media/base/audio_renderer_mixer_unittest.cc:29: void LogUma(int){}; > same here oh, and lgtm with those fixed (all minor).
grunell@, tommi@, - thanks for the review. Comments addressed. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:32: int preferred_high_latency_output_bufffer_size = 0; On 2016/06/29 12:20:54, Henrik Grunell wrote: > fff -> ff Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:51: // WebAudio should provide correct callback size in frames; it does not On 2016/06/29 12:20:54, Henrik Grunell wrote: > Confusing. What does WebAudio have to do with this? AudioLatency is asked for > the buffer size. Outdated comment, removed. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:57: // adjust output buffer size according to the latency requirement. On 2016/06/29 12:20:54, Henrik Grunell wrote: > That's what the whole switch block does. Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:74: DCHECK(output_buffer_size); On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: Better (clearer) to DCHECK_NE(x, 0) Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:86: LOCAL_HISTOGRAM_CUSTOM_COUNTS( On 2016/06/29 12:20:54, Henrik Grunell wrote: > LOCAL_ -> UMA_ Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:87: "Media.Audio.Render.AudioInputsPerMixer.LatencyExact", value, 1, 100, On 2016/06/29 12:20:54, Henrik Grunell wrote: > I think a max of 100 is unnecessarily high. 20 or 30 is probably enough, more > than that will clamp to max. Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:137: media::AudioLatency::LatencyType latency) { On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: Higher readability if it's called latency_type everywhere. (See above) https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:171: LOCAL_HISTOGRAM_CUSTOM_COUNTS("Media.Audio.Render.AudioMixing.LatencyMap", On 2016/06/29 12:20:54, Henrik Grunell wrote: > UMA_ Done. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:105: media::AudioLatency::LatencyType latency; On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: "latency_type" is a better name. Discussed offline, agreed to keep it is it is now https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:122: // TODO(olka) add buffer duration comparison for LATENCY_EXACT_MS when On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: I think it's better to remove the LATENCY_EXACT_MS value since it's not > allowed anyway. Discussed offline, decided to keep it. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:167: // Map of the output latencies encountered throughout mixer manager lifetime. On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: Maybe describe why we store it throughout the lifetime? Done. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency.cc:78: // No |hardware_buffer_size| is specified, fail back to 10 ms buffer size. On 2016/06/29 12:20:54, Henrik Grunell wrote: > Fail back or fall back? Done. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency.h (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency.h:14: // Categoties of expected latencies for input/output audio. Do not change On 2016/06/29 12:20:54, Henrik Grunell wrote: > "Categories" (spelling). Done. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency_unittest.cc:18: #if defined(OS_WIN) On 2016/06/29 12:20:54, Henrik Grunell wrote: > Nit: seems like only diffs by 100 vs 32. Discussed offline https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:20: // the destruciton. NOT thread-safe, make sure it is used under lock. On 2016/06/29 10:12:08, o1ka wrote: > Fix the comment Done. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:38: void Decrement() { --count_; } On 2016/06/29 12:20:54, Henrik Grunell wrote: > DCHECK greater than 0 before decrease? Done. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:22: void LogUma(int){}; On 2016/06/29 12:20:55, Henrik Grunell wrote: > Nit: space before {}. Done. https://codereview.chromium.org/2067863003/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/2067863003/diff/120001/media/media.gyp#newcod... media/media.gyp:266: 'base/audio_latency.h', On 2016/06/29 12:20:55, Henrik Grunell wrote: > Indentation. Done. https://codereview.chromium.org/2067863003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2067863003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21063: + a new maximum for the mixer. It is useful for evaluating how mixing audio in On 2016/06/29 12:20:55, Henrik Grunell wrote: > I think you could skip the use case example. It's a pretty specific piece of > work we're doing now. Discussed offline https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:172: latency_map_.to_ulong(), 1, 0xff, 0xff); On 2016/06/29 12:26:52, tommi-chrömium wrote: > won't this cause other latencies to be counted multiple times? Yes, unfortunately. But we can't log stats in the destructor (because it's almost never called). So we'll have a sort of exponential scale here, with a smaller subset logged both on its own and as a part of any larger subset in the same renderer. https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:164: }; On 2016/06/29 12:26:52, tommi-chrömium wrote: > nit: remove ; removed the method (not used). https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_input_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:22: void LogUma(int){}; On 2016/06/29 12:26:52, tommi-chrömium wrote: > no ; > space before { > variable name for int argument Done. https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input_unittest.cc:82: audio_parameters_, sinks_[idx].get(), base::Bind(LogUma))); On 2016/06/29 12:26:52, tommi-chrömium wrote: > &LogUma Done. https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_unittest.cc:29: void LogUma(int){}; On 2016/06/29 12:26:52, tommi-chrömium wrote: > same here Done.
On 2016/06/29 12:27:32, tommi-chrömium wrote: > On 2016/06/29 12:26:52, tommi-chrömium wrote: > > qq - instead of the noop LogUma methods, can we create a noop callback object > > inline? > > From what I know, default-constructed base::Callback() is not runnable (crashes) and there is no neat way to test if it is runnable (to check if it is default-constructed I need to compare it with another default-constructed one, which is pretty awkward). And looks like base::Bind does not accept lambdas. So I haven't found a way to create noop inline.
On 2016/06/28 13:26:12, henrika wrote: > Lot's of new stuff here and hard to see all implications. > > Regarding AudioLatency, could you perhaps point out which parts are new or are > all actual calculations moved from existing code? > It is difficult to see that in the existing CL. AudioLatency::GetHighLatencyBufferSize is moved from AudiohardwareConfig AudioLatency::GetRtcBufferSize used to be WebRTCAudioRenderer::GetOptimalBufferSize AudioLatency::GetInteractiveBufferSiz is borrowed from here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/audio... > Has any set of manual tests been done on all platform to ensure that nothing is > broken here? Manually I tested only on Linux so far. Unit tests did catch some problems on other platforms, which are fixed now. Note that the CL does not actually trigger most of the new code paths. Those will be enabled under finch by this CL https://codereview.chromium.org/2108673004/. This reduces risks a tiny bit. Also, I tried to keep platform-specific paths untouched, just moved the code around mainly.
LGTM
olka@chromium.org changed reviewers: + holte@chromium.org
holte@ - PTAL at UMA histograms.
On 2016/06/29 14:57:11, o1ka wrote: > holte@ - PTAL at UMA histograms. Logged in audio_renderer_mixer_manager.cc (it also passes logging callback to audio_renderer_mixer.cc).
FYI: follow-up CL to enable the logic under finch for dev and canary https://codereview.chromium.org/2108673004/
lgtm
lgtm https://codereview.chromium.org/2067863003/diff/160001/media/base/audio_rende... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2067863003/diff/160001/media/base/audio_rende... media/base/audio_renderer_mixer.cc:19: // Tracks the maximum value of a counter and logs it into a UMA histogram upon Feels like there should be some UMA built in for this, but don't have a better suggestion.
lgtm https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:137: media::AudioLatency::LatencyType latency) { On 2016/06/29 13:57:31, o1ka wrote: > On 2016/06/29 12:20:54, Henrik Grunell wrote: > > Nit: Higher readability if it's called latency_type everywhere. > > (See above) Acknowledged. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:105: media::AudioLatency::LatencyType latency; On 2016/06/29 13:57:31, o1ka wrote: > On 2016/06/29 12:20:54, Henrik Grunell wrote: > > Nit: "latency_type" is a better name. > > Discussed offline, agreed to keep it is it is now Acknowledged. https://codereview.chromium.org/2067863003/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:122: // TODO(olka) add buffer duration comparison for LATENCY_EXACT_MS when On 2016/06/29 13:57:31, o1ka wrote: > On 2016/06/29 12:20:54, Henrik Grunell wrote: > > Nit: I think it's better to remove the LATENCY_EXACT_MS value since it's not > > allowed anyway. > > Discussed offline, decided to keep it. Acknowledged. https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... File media/base/audio_latency_unittest.cc (right): https://codereview.chromium.org/2067863003/diff/120001/media/base/audio_laten... media/base/audio_latency_unittest.cc:18: #if defined(OS_WIN) On 2016/06/29 13:57:31, o1ka wrote: > On 2016/06/29 12:20:54, Henrik Grunell wrote: > > Nit: seems like only diffs by 100 vs 32. > > Discussed offline Acknowledged.
lgtm % some bucket bounds issues. https://codereview.chromium.org/2067863003/diff/160001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2067863003/diff/160001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:85: 20); If you want exact numbers below 20, you probably want buckets min=1,max=20,buckets=21 to account for overflow bucket, otherwise you will have an [18,20) bucket. You could also use UMA_HISTOGRAM_ENUMERATION with a boundary value of 20. https://codereview.chromium.org/2067863003/diff/160001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:173: latency_map_.to_ulong(), 1, 0xff, 0xff); Similarly here, though you might consider using UMA_HISTOGRAM_SPARSE_SLOWLY here.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, grunell@chromium.org, holte@chromium.org, chcunningham@chromium.org, dalecurtis@chromium.org, henrika@chromium.org Link to the patchset: https://codereview.chromium.org/2067863003/#ps180001 (title: "UMA boundary fixes according to recomendations, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks everybody for the timely review!
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, tommi@chromium.org, holte@chromium.org, chcunningham@chromium.org, dalecurtis@chromium.org, henrika@chromium.org Link to the patchset: https://codereview.chromium.org/2067863003/#ps200001 (title: "android test fix")
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 ========== Mixing audio with different latency requirements. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: * latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration. * ARMM adjusts mixer output buffer siza based on input latency and output sample rate. BUG=560378,564472 ========== to ========== Mixing audio with different latency requirements. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: * latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration. * ARMM adjusts mixer output buffer siza based on input latency and output sample rate. BUG=560378,564472 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Mixing audio with different latency requirements. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: * latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration. * ARMM adjusts mixer output buffer siza based on input latency and output sample rate. BUG=560378,564472 ========== to ========== Mixing audio with different latency requirements. Follow-up is introducing mixing for audio other than media elements (under finch). ** Helper changes: 1) AudioDeviceFactory: Added separate categories for WebAudio source basing on expected output latency. Not used by WebAudio right now, but they give a sense of what to expect in the nearest future regarding output latency requirements. (May be dropped for now). 2) AudioLatency class is introduced; latency-based output buffer size calculation is moved into it. ** Mixing changes: 3) AudioRendererMixerPool/AudioRendererMixerManager: * latency requirement is introduced as one more ARMInput parameter; now it’s a part of ARM key. The mixing strategy (see ARM key comparison) is that only inputs with the same latency requirements can be mixed together; LATENCY_EXACT_MS inputs are mixed only with the likewise and only if they have the same buffer duration. * ARMM adjusts mixer output buffer siza based on input latency and output sample rate. BUG=560378,564472 Committed: https://crrev.com/6bbdb8b35b1977bd45e74b882dce8e70416cf190 Cr-Commit-Position: refs/heads/master@{#403120} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6bbdb8b35b1977bd45e74b882dce8e70416cf190 Cr-Commit-Position: refs/heads/master@{#403120} |
