Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1483433003:
Second layer of mixing for audio elements. (Closed)
|
Created:
5 years ago by o1ka Modified:
4 years, 8 months ago CC:
vanellope-cl_google.com, chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSecond layer of mixing for audio elements.
Now each mixer will accept all the inputs with the same
(renderer frame, channel layout, device id, security origin),
regardless input sample rate, and will produce the output at native
sample rate (on all the systems except CrOS and Android).
BUG=557789
Committed: https://crrev.com/f1688cc20b1ebc03b23e26ac5ea2fb6b84f70ea6
Cr-Commit-Position: refs/heads/master@{#363434}
Patch Set 1 #Patch Set 2 : fixing merge issue #Patch Set 3 : GN build fix #
Total comments: 42
Patch Set 4 : Review comments implementation. #
Total comments: 1
Patch Set 5 : some more nits fixed #
Total comments: 42
Patch Set 6 : more review comment implementation #Patch Set 7 : Fixing similarity #Patch Set 8 : similarity 20% #Patch Set 9 : Similarity back to 10% #
Total comments: 2
Patch Set 10 : Nit fixes #
Total comments: 1
Patch Set 11 : That fix #Messages
Total messages: 29 (9 generated)
Description was changed from ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS). BUG=557789 ========== to ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 ==========
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, grunell@chromium.org
olka@chromium.org changed required reviewers: + dalecurtis@chromium.org, grunell@chromium.org
Could you take a look at mixer changes? Thanks! Olga
Looks good. Only code style and such comments. I defer mixer unit test reviewing to Dale - he's far more familiar than me with that code. https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): when mixing in other audio choose latency depending on the I think we'll only mix low latency audio. WebAudio uses that, right? https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager_unittest.cc:161: media::AudioParameters params3(AudioParameters::AUDIO_PCM_LINEAR, I'm not sure why this should be changed to LINEAR? https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:49: if (IsMasterSampleRate(input_sample_rate)) Use {} here because used in else block. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:56: converters_.insert(std::make_pair( Nit: Can also use converters_[input_sample_rate] = ... syntax. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:64: // Add newly-created resampler as an input to the master mixer. Empty line before comment. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:77: AudioConverter master_mixer_; Maybe name it |master_converter_|? https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:78: // Each of converters handle mixing inputs with a given sample rate Nit: Empty line before comment. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:63: input_parameters_.reserve(sample_rates.size()); No need to reserve. Remove. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:68: // Create output parameters based on test parameters. Empty line before comment. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:101: // |count| here is the number of inputs per input sample rate. Rename the parameter to |inputs_per_sample_rate| and skip the comment. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Same here. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Remove (c). 2015. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:13: // moved from media/audio/virtual_audio_input_stream.cc No need to have this. Remove. https://codereview.chromium.org/1483433003/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1483433003/diff/40001/media/media.gyp#newcode306 media/media.gyp:306: 'base/loopback_audio_converter.cc', Fix indent.
Thank you, Henrik! I added a couple of questions, could you take a look? Thanks, Olga https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): when mixing in other audio choose latency depending on the On 2015/11/30 14:45:02, Henrik Grunell wrote: > I think we'll only mix low latency audio. WebAudio uses that, right? We can either mix low latency, or mix both (not together, but per latency property). In the former case, we should have a check which won't allow to pass high latency streams into the mixer, and in the latter we should add latency property to the mixers map key. The purpose of the comment not to have this silently hard-coded when we add other types of audio into the mix. I'll rephrase. https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager_unittest.cc:161: media::AudioParameters params3(AudioParameters::AUDIO_PCM_LINEAR, On 2015/11/30 14:45:02, Henrik Grunell wrote: > I'm not sure why this should be changed to LINEAR? It should not! :) https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:56: converters_.insert(std::make_pair( On 2015/11/30 14:45:02, Henrik Grunell wrote: > Nit: Can also use converters_[input_sample_rate] = ... syntax. It's done this way to get the iterator to the inserted element right away. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:77: AudioConverter master_mixer_; On 2015/11/30 14:45:02, Henrik Grunell wrote: > Maybe name it |master_converter_|? It does not do any conversion, just mixing, that's why I called it this way. What do you think? https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:63: input_parameters_.reserve(sample_rates.size()); On 2015/11/30 14:45:02, Henrik Grunell wrote: > No need to reserve. Remove. I think that *theoretically* it improves performance, since it eliminates the need of on-the-flight vector reallocation. So if you know the size in advance, why not to reserve it?
https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): when mixing in other audio choose latency depending on the On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > I think we'll only mix low latency audio. WebAudio uses that, right? > We can either mix low latency, or mix both (not together, but per latency > property). In the former case, we should have a check which won't allow to pass > high latency streams into the mixer, and in the latter we should add latency > property to the mixers map key. The purpose of the comment not to have this > silently hard-coded when we add other types of audio into the mix. I'll > rephrase. Acknowledged. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:56: converters_.insert(std::make_pair( On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > Nit: Can also use converters_[input_sample_rate] = ... syntax. > It's done this way to get the iterator to the inserted element right away. Acknowledged. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:77: AudioConverter master_mixer_; On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > Maybe name it |master_converter_|? > It does not do any conversion, just mixing, that's why I called it this way. > What do you think? Yeah, that sounds OK then I think. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:63: input_parameters_.reserve(sample_rates.size()); On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > No need to reserve. Remove. > I think that *theoretically* it improves performance, since it eliminates the > need of on-the-flight vector reallocation. So if you know the size in advance, > why not to reserve it? Yes it will. But since it's a test, I think the reduced lines and slight increase of readability outweighs the performance gain. I'm Ok with keeping it too.
Seems like this will add a couple layers of indirection for CrOS and Android as well as when only one stream is present. Have you measured any changes in the power tests? I think AudioConverter should ensure no copies or unnecessary mixing of the data are made in this case, but I might be missing something. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:59: inline bool IsMasterSampleRate(int sample_rate) { no inline keyword, rename to hacker_style() if you want to keep the definition inline. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:23: LoopbackAudioConverter(const AudioParameters& input_params, Probably you'll get compile errors about no inline constructors and will need to move the methods of this into a .cc file.
It adds one layer of indirection in case resampling is required (i.e. input sample rate is not equal to output sample rate), and no indirection if there is no resampling (streams those do not require resampling go directly to the master audio converter, which is the same behavior as before). For Android and CrOS the output sample rate is equal to the input sample rate (this has not changed), for these platforms we always have only master audio converters, i.e. it works just as it worked before. If not (Android or CrOS) and only one stream is present, the resampling audio converter does the job, and the master audio converter is no-op (according to the source code it just calls ProvideInput which renders into the destination AudioBus). So again, the behavior is unchanged: one resampling, no extra copying. All in all, for both cases you mentioned nothing has changed comparing to the current implementation. We have not run power tests for this yet (preparing for that), but for this particular change there should be no power consumption impact for Android/CrOS. On 2015/11/30 19:09:25, DaleCurtis wrote: > Seems like this will add a couple layers of indirection for CrOS and Android as > well as when only one stream is present. Have you measured any changes in the > power tests? > > I think AudioConverter should ensure no copies or unnecessary mixing of the data > are made in this case, but I might be missing something. > > https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... > File media/base/audio_renderer_mixer.h (right): > > https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... > media/base/audio_renderer_mixer.h:59: inline bool IsMasterSampleRate(int > sample_rate) { > no inline keyword, rename to hacker_style() if you want to keep the definition > inline. > > https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... > File media/base/loopback_audio_converter.h (right): > > https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... > media/base/loopback_audio_converter.h:23: LoopbackAudioConverter(const > AudioParameters& input_params, > Probably you'll get compile errors about no inline constructors and will need to > move the methods of this into a .cc file.
Comments are implemented! https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): when mixing in other audio choose latency depending on the On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > I think we'll only mix low latency audio. WebAudio uses that, right? > We can either mix low latency, or mix both (not together, but per latency > property). In the former case, we should have a check which won't allow to pass > high latency streams into the mixer, and in the latter we should add latency > property to the mixers map key. The purpose of the comment not to have this > silently hard-coded when we add other types of audio into the mix. I'll > rephrase. Done. https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager_unittest.cc:161: media::AudioParameters params3(AudioParameters::AUDIO_PCM_LINEAR, On 2015/11/30 15:20:15, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > I'm not sure why this should be changed to LINEAR? > It should not! :) Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:49: if (IsMasterSampleRate(input_sample_rate)) On 2015/11/30 14:45:02, Henrik Grunell wrote: > Use {} here because used in else block. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:56: converters_.insert(std::make_pair( On 2015/11/30 15:37:41, Henrik Grunell wrote: > On 2015/11/30 15:20:15, olka1 wrote: > > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > > Nit: Can also use converters_[input_sample_rate] = ... syntax. > > It's done this way to get the iterator to the inserted element right away. > > Acknowledged. Acknowledged. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.cc:64: // Add newly-created resampler as an input to the master mixer. On 2015/11/30 14:45:02, Henrik Grunell wrote: > Empty line before comment. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:59: inline bool IsMasterSampleRate(int sample_rate) { On 2015/11/30 19:09:25, DaleCurtis wrote: > no inline keyword, rename to hacker_style() if you want to keep the definition > inline. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:77: AudioConverter master_mixer_; On 2015/11/30 15:37:41, Henrik Grunell wrote: > On 2015/11/30 15:20:15, olka1 wrote: > > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > > Maybe name it |master_converter_|? > > It does not do any conversion, just mixing, that's why I called it this way. > > What do you think? > > Yeah, that sounds OK then I think. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer.h:78: // Each of converters handle mixing inputs with a given sample rate On 2015/11/30 14:45:02, Henrik Grunell wrote: > Nit: Empty line before comment. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:63: input_parameters_.reserve(sample_rates.size()); On 2015/11/30 15:37:41, Henrik Grunell wrote: > On 2015/11/30 15:20:15, olka1 wrote: > > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > > No need to reserve. Remove. > > I think that *theoretically* it improves performance, since it eliminates the > > need of on-the-flight vector reallocation. So if you know the size in advance, > > why not to reserve it? > > Yes it will. But since it's a test, I think the reduced lines and slight > increase of readability outweighs the performance gain. I'm Ok with keeping it > too. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:68: // Create output parameters based on test parameters. On 2015/11/30 14:45:02, Henrik Grunell wrote: > Empty line before comment. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:101: // |count| here is the number of inputs per input sample rate. On 2015/11/30 14:45:02, Henrik Grunell wrote: > Rename the parameter to |inputs_per_sample_rate| and skip the comment. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.cc (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/11/30 14:45:02, Henrik Grunell wrote: > Same here. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/11/30 14:45:02, Henrik Grunell wrote: > Remove (c). > 2015. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:13: // moved from media/audio/virtual_audio_input_stream.cc On 2015/11/30 14:45:02, Henrik Grunell wrote: > No need to have this. Remove. Done. https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:23: LoopbackAudioConverter(const AudioParameters& input_params, On 2015/11/30 19:09:25, DaleCurtis wrote: > Probably you'll get compile errors about no inline constructors and will need to > move the methods of this into a .cc file. Done. https://codereview.chromium.org/1483433003/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1483433003/diff/40001/media/media.gyp#newcode306 media/media.gyp:306: 'base/loopback_audio_converter.cc', On 2015/11/30 14:45:03, Henrik Grunell wrote: > Fix indent. Done.
You also hadn't changed one of the licence headers. But since we decided to keep the original year in the moved file, I think the whole headers should be unchanged. (I.e. revert the change in the other file.) https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... File media/base/loopback_audio_converter.h (right): https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... media/base/loopback_audio_converter.h:13: // moved from media/audio/virtual_audio_input_stream.cc On 2015/12/01 13:17:51, olka1 wrote: > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > No need to have this. Remove. > > Done. Forgot to change this or upload? https://codereview.chromium.org/1483433003/diff/60001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/60001/media/base/audio_render... media/base/audio_renderer_mixer.h:76: // Master mixer which mixes all the signals at output sample rate. Clarify in the comment that this converter will only mix and nothing else.
On 2015/12/01 13:44:50, Henrik Grunell wrote: > You also hadn't changed one of the licence headers. But since we decided to keep > the original year in the moved file, I think the whole headers should be > unchanged. (I.e. revert the change in the other file.) > > https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... > File media/base/loopback_audio_converter.h (right): > > https://codereview.chromium.org/1483433003/diff/40001/media/base/loopback_aud... > media/base/loopback_audio_converter.h:13: // moved from > media/audio/virtual_audio_input_stream.cc > On 2015/12/01 13:17:51, olka1 wrote: > > On 2015/11/30 14:45:02, Henrik Grunell wrote: > > > No need to have this. Remove. > > > > Done. > > Forgot to change this or upload? > > https://codereview.chromium.org/1483433003/diff/60001/media/base/audio_render... > File media/base/audio_renderer_mixer.h (right): > > https://codereview.chromium.org/1483433003/diff/60001/media/base/audio_render... > media/base/audio_renderer_mixer.h:76: // Master mixer which mixes all the > signals at output sample rate. > Clarify in the comment that this converter will only mix and nothing else. Done.
lgtm generally, defer to grunell@ for the fine details. I think the power should be unchanged since this will at worst add a new mixing step. The biggest issue will be when multiple streams are mixed together within a page, this will make it more likely that we exceed the [-1,1] range for values which will cause clipping and other issues. It's possible we need to figure out a better mixing strategy if the number of streams per mixer gets too high. https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): Make sure low latency buffer is not silently hard-coded when You should be able to just add a DCHECK :)
https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): Make sure low latency buffer is not silently hard-coded when On 2015/12/01 18:24:06, DaleCurtis wrote: > You should be able to just add a DCHECK :) Yep, just do that and remove the todo. If we add support for other formats we'll update this then. https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:96: // TODO(olka) add [params.format] to the key when starting to mix other Thinking more of this, I think this todo should be removed. It's a natural part of an future extension and doesn't need to be documented. https://codereview.chromium.org/1483433003/diff/80001/media/audio/virtual_aud... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.cc:80: output_params, params_, false))); Nit: Indentation looks weird. Is this git cl format's work? https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:58: // TODO(olka): we may need to enable FIFO I also think this todo can be skipped, since if a fifo is required when adding WebRTC then just add it then. No need to document it here. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:78: if (is_master_sample_rate(input_sample_rate)) Use {}. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:53: // Maps input sample frequency to the dedicated converter. s/frequency/rate (to be consistent) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:59: bool is_master_sample_rate(int sample_rate) { Empty line before. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:76: // Master converter which mixes all the signals at output sample rate, Nit: Put this after |converters_| and add that it takes all outputs from those, plus mixer inputs that are in the output sample rate. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:80: // Each of converters mixes inputs with a given sample rate Nit: More words fit in this line. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:46: std::vector<int> kTestInput3(kSampleRates, Hmm. kTestInput3. There is no 1 and 2, right? (Those are named differently.) Maybe name it some other way? https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:138: if ((error > epsilon * scale) && (error > epsilon)) { This could be false incorrectly if scale < 1. Was this condition wrong before? Same for EXPECT_NEAR() below? https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:314: // Stop() all even numbered mixer inputs and Play() all odd numbered inputs Why not only Start() + Play() every other input? Is it better to Start() + Stop() the rest, instead of not touching them? (Add comment that explains this.) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:321: // Stop the last input in case the number of inputs is odd You'd get rid of this if not Start+Stop. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:325: ASSERT_TRUE(RenderAndValidateAudioData( This will unfortunately not tell us which test failed since it only prints out this function. I see it's done like this elsewhere here, but it's better (though it clutters the code) to return a bool (success/failure) in this function and ASSERT() when calling it in the test. Then change this ASSERT_TRUE to EXPECT_TRUE and return false if RenderAndValidateAudioData() fails. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:326: std::max(static_cast<size_t>(mixer_inputs_.size() / 2), This seems unnecessarily complicated. !mixer_inputs_.empty() ? mixer_inputs_.size() : 1 should work, right?
Some more fixes, couple of open questions left. https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): Make sure low latency buffer is not silently hard-coded when On 2015/12/02 13:22:38, Henrik Grunell wrote: > On 2015/12/01 18:24:06, DaleCurtis wrote: > > You should be able to just add a DCHECK :) > > Yep, just do that and remove the todo. If we add support for other formats we'll > update this then. Are we sure that now all the inputs are always passed as LOW_LATENCY into the mixer? I'm not sure I can follow all the cases for media elements through the source code. I suspect that for media elements it might be ok to switch to LOW_LATENCY even if input format is LINEAR. I'm reluctant to add DCHECK at this point. Actually, after digging more into the code I think of removing this comment since the problem is not that binary as it is described there. https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.h:96: // TODO(olka) add [params.format] to the key when starting to mix other On 2015/12/02 13:22:38, Henrik Grunell wrote: > Thinking more of this, I think this todo should be removed. It's a natural part > of an future extension and doesn't need to be documented. Done. https://codereview.chromium.org/1483433003/diff/80001/media/audio/virtual_aud... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.cc:80: output_params, params_, false))); On 2015/12/02 13:22:38, Henrik Grunell wrote: > Nit: Indentation looks weird. Is this git cl format's work? Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:58: // TODO(olka): we may need to enable FIFO On 2015/12/02 13:22:38, Henrik Grunell wrote: > I also think this todo can be skipped, since if a fifo is required when adding > WebRTC then just add it then. No need to document it here. Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.cc:78: if (is_master_sample_rate(input_sample_rate)) On 2015/12/02 13:22:38, Henrik Grunell wrote: > Use {}. Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:53: // Maps input sample frequency to the dedicated converter. On 2015/12/02 13:22:38, Henrik Grunell wrote: > s/frequency/rate (to be consistent) Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:59: bool is_master_sample_rate(int sample_rate) { On 2015/12/02 13:22:38, Henrik Grunell wrote: > Empty line before. Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:76: // Master converter which mixes all the signals at output sample rate, On 2015/12/02 13:22:38, Henrik Grunell wrote: > Nit: Put this after |converters_| and add that it takes all outputs from those, > plus mixer inputs that are in the output sample rate. Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer.h:80: // Each of converters mixes inputs with a given sample rate On 2015/12/02 13:22:38, Henrik Grunell wrote: > Nit: More words fit in this line. Acknowledged. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:46: std::vector<int> kTestInput3(kSampleRates, On 2015/12/02 13:22:38, Henrik Grunell wrote: > Hmm. kTestInput3. There is no 1 and 2, right? (Those are named differently.) > Maybe name it some other way? Done. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:138: if ((error > epsilon * scale) && (error > epsilon)) { On 2015/12/02 13:22:38, Henrik Grunell wrote: > This could be false incorrectly if scale < 1. > > Was this condition wrong before? Same for EXPECT_NEAR() below? I'm not quite sure if I get this comment,could you elaborate? (Side by side comparison will show how it was before) (Just for the record, scale is either 0 or >=1 for the tests, since it's basically the number of inputs. Which does not mean that the function is allowed to work incorrectly on scales < 1) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:314: // Stop() all even numbered mixer inputs and Play() all odd numbered inputs On 2015/12/02 13:22:38, Henrik Grunell wrote: > Why not only Start() + Play() every other input? Is it better to Start() + > Stop() the rest, instead of not touching them? (Add comment that explains this.) The top comment of the function explains it :) (This is the original test by dalecurtis@, I just moved it here to reuse) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:321: // Stop the last input in case the number of inputs is odd On 2015/12/02 13:22:38, Henrik Grunell wrote: > You'd get rid of this if not Start+Stop. The test is for post-stop and post-play. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:325: ASSERT_TRUE(RenderAndValidateAudioData( On 2015/12/02 13:22:38, Henrik Grunell wrote: > This will unfortunately not tell us which test failed since it only prints out > this function. I see it's done like this elsewhere here, but it's better (though > it clutters the code) to return a bool (success/failure) in this function and > ASSERT() when calling it in the test. Then change this ASSERT_TRUE to > EXPECT_TRUE and return false if RenderAndValidateAudioData() fails. There is no problem here since EXPECT_NEAR in ValidateAudioData tell us everything about a failed test. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:326: std::max(static_cast<size_t>(mixer_inputs_.size() / 2), On 2015/12/02 13:22:38, Henrik Grunell wrote: > This seems unnecessarily complicated. > > !mixer_inputs_.empty() ? mixer_inputs_.size() : 1 > > should work, right? No, it's different, you missed "/2". We need it to be 1 if there is 1 element and floor(size/2) otherwise.
miu@chromium.org changed reviewers: + miu@chromium.org
virtual_audio_input_stream.cc and loopback* lgtm. However, please upload again with a lower similarity setting (10% might work well) so that code history (i.e., where LoopbackAudioConverter came from) is better preserved. See 'git cl help upload'.
On 2015/12/02 19:32:28, miu wrote: > virtual_audio_input_stream.cc and loopback* lgtm. > > However, please upload again with a lower similarity setting (10% might work > well) so that code history (i.e., where LoopbackAudioConverter came from) is > better preserved. See 'git cl help upload'. Thanks for the hint, done.
https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): Make sure low latency buffer is not silently hard-coded when On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > On 2015/12/01 18:24:06, DaleCurtis wrote: > > > You should be able to just add a DCHECK :) > > > > Yep, just do that and remove the todo. If we add support for other formats > we'll > > update this then. > Are we sure that now all the inputs are always passed as LOW_LATENCY into the > mixer? I'm not sure I can follow all the cases for media elements through the > source code. I suspect that for media elements it might be ok to switch to > LOW_LATENCY even if input format is LINEAR. I'm reluctant to add DCHECK at this > point. Actually, after digging more into the code I think of removing this > comment since the problem is not that binary as it is described there. Isn't LOW_LATENCY always used for MediaElements? Otherwise this is a bug here. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:138: if ((error > epsilon * scale) && (error > epsilon)) { On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > This could be false incorrectly if scale < 1. > > > > Was this condition wrong before? Same for EXPECT_NEAR() below? > > I'm not quite sure if I get this comment,could you elaborate? > (Side by side comparison will show how it was before) > (Just for the record, scale is either 0 or >=1 for the tests, since it's > basically the number of inputs. Which does not mean that the function is allowed > to work incorrectly on scales < 1) OK, then it's fine. Add a DCHECK for this. It will also explain this question for the reader. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:314: // Stop() all even numbered mixer inputs and Play() all odd numbered inputs On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > Why not only Start() + Play() every other input? Is it better to Start() + > > Stop() the rest, instead of not touching them? (Add comment that explains > this.) > > The top comment of the function explains it :) (This is the original test by > dalecurtis@, I just moved it here to reuse) Right, you just moved this. It's fine. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:321: // Stop the last input in case the number of inputs is odd On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > You'd get rid of this if not Start+Stop. > > The test is for post-stop and post-play. Acknowledged. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:325: ASSERT_TRUE(RenderAndValidateAudioData( On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > This will unfortunately not tell us which test failed since it only prints out > > this function. I see it's done like this elsewhere here, but it's better > (though > > it clutters the code) to return a bool (success/failure) in this function and > > ASSERT() when calling it in the test. Then change this ASSERT_TRUE to > > EXPECT_TRUE and return false if RenderAndValidateAudioData() fails. > > There is no problem here since EXPECT_NEAR in ValidateAudioData tell us > everything about a failed test. You won't know if ManyInputMixedStopPlay or ManyInputMixedStopPlayOdd failed. (Try it :)) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:326: std::max(static_cast<size_t>(mixer_inputs_.size() / 2), On 2015/12/02 19:05:41, o1ka wrote: > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > This seems unnecessarily complicated. > > > > !mixer_inputs_.empty() ? mixer_inputs_.size() : 1 > > > > should work, right? > > No, it's different, you missed "/2". We need it to be 1 if there is 1 element > and floor(size/2) otherwise. Right, I was too fast. https://codereview.chromium.org/1483433003/diff/160001/media/base/audio_rende... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/160001/media/base/audio_rende... media/base/audio_renderer_mixer.h:78: // resamples them to the output sample rate. Inputs those do not reqiure s/those/that https://codereview.chromium.org/1483433003/diff/160001/media/base/audio_rende... media/base/audio_renderer_mixer.h:83: // mixer inputs those are initially at the output sample rate. Nit: A bit unclear. Maybe "mixer inputs in the output sample rate."?
lgtm with nit fixed. https://codereview.chromium.org/1483433003/diff/180001/media/base/audio_rende... File media/base/audio_renderer_mixer.h (right): https://codereview.chromium.org/1483433003/diff/180001/media/base/audio_rende... media/base/audio_renderer_mixer.h:83: // mixer inputs those are in the output sample rate. Nit: s/those/that
https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1483433003/diff/80001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:97: // TODO(olka): Make sure low latency buffer is not silently hard-coded when On 2015/12/04 10:11:56, Henrik Grunell wrote: > On 2015/12/02 19:05:41, o1ka wrote: > > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > > On 2015/12/01 18:24:06, DaleCurtis wrote: > > > > You should be able to just add a DCHECK :) > > > > > > Yep, just do that and remove the todo. If we add support for other formats > > we'll > > > update this then. > > Are we sure that now all the inputs are always passed as LOW_LATENCY into the > > mixer? I'm not sure I can follow all the cases for media elements through the > > source code. I suspect that for media elements it might be ok to switch to > > LOW_LATENCY even if input format is LINEAR. I'm reluctant to add DCHECK at > this > > point. Actually, after digging more into the code I think of removing this > > comment since the problem is not that binary as it is described there. > > Isn't LOW_LATENCY always used for MediaElements? Otherwise this is a bug here. As we decided in our conversation, there will be a separate CL to add a DCHECK, so we can revert it if it creates any problems https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_unittest.cc (right): https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:138: if ((error > epsilon * scale) && (error > epsilon)) { On 2015/12/04 10:11:56, Henrik Grunell wrote: > On 2015/12/02 19:05:41, o1ka wrote: > > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > > This could be false incorrectly if scale < 1. > > > > > > Was this condition wrong before? Same for EXPECT_NEAR() below? > > > > I'm not quite sure if I get this comment,could you elaborate? > > (Side by side comparison will show how it was before) > > (Just for the record, scale is either 0 or >=1 for the tests, since it's > > basically the number of inputs. Which does not mean that the function is > allowed > > to work incorrectly on scales < 1) > > OK, then it's fine. Add a DCHECK for this. It will also explain this question > for the reader. Acknowledged. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:314: // Stop() all even numbered mixer inputs and Play() all odd numbered inputs On 2015/12/04 10:11:56, Henrik Grunell wrote: > On 2015/12/02 19:05:41, o1ka wrote: > > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > > Why not only Start() + Play() every other input? Is it better to Start() + > > > Stop() the rest, instead of not touching them? (Add comment that explains > > this.) > > > > The top comment of the function explains it :) (This is the original test by > > dalecurtis@, I just moved it here to reuse) > > Right, you just moved this. It's fine. Acknowledged. https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:325: ASSERT_TRUE(RenderAndValidateAudioData( On 2015/12/04 10:11:56, Henrik Grunell wrote: > On 2015/12/02 19:05:41, o1ka wrote: > > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > > This will unfortunately not tell us which test failed since it only prints > out > > > this function. I see it's done like this elsewhere here, but it's better > > (though > > > it clutters the code) to return a bool (success/failure) in this function > and > > > ASSERT() when calling it in the test. Then change this ASSERT_TRUE to > > > EXPECT_TRUE and return false if RenderAndValidateAudioData() fails. > > > > There is no problem here since EXPECT_NEAR in ValidateAudioData tell us > > everything about a failed test. > > You won't know if ManyInputMixedStopPlay or ManyInputMixedStopPlayOdd failed. > (Try it :)) It works :) https://codereview.chromium.org/1483433003/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_unittest.cc:326: std::max(static_cast<size_t>(mixer_inputs_.size() / 2), On 2015/12/04 10:11:56, Henrik Grunell wrote: > On 2015/12/02 19:05:41, o1ka wrote: > > On 2015/12/02 13:22:38, Henrik Grunell wrote: > > > This seems unnecessarily complicated. > > > > > > !mixer_inputs_.empty() ? mixer_inputs_.size() : 1 > > > > > > should work, right? > > > > No, it's different, you missed "/2". We need it to be 1 if there is 1 element > > and floor(size/2) otherwise. > > Right, I was too fast. Acknowledged.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, miu@chromium.org, grunell@chromium.org Link to the patchset: https://codereview.chromium.org/1483433003/#ps200001 (title: "That fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1483433003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1483433003/200001
Message was sent while issue was closed.
Description was changed from ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 ========== to ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 ========== to ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 Committed: https://crrev.com/f1688cc20b1ebc03b23e26ac5ea2fb6b84f70ea6 Cr-Commit-Position: refs/heads/master@{#363434} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f1688cc20b1ebc03b23e26ac5ea2fb6b84f70ea6 Cr-Commit-Position: refs/heads/master@{#363434}
Message was sent while issue was closed.
Description was changed from ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 Committed: https://crrev.com/f1688cc20b1ebc03b23e26ac5ea2fb6b84f70ea6 Cr-Commit-Position: refs/heads/master@{#363434} ========== to ========== Second layer of mixing for audio elements. Now each mixer will accept all the inputs with the same (renderer frame, channel layout, device id, security origin), regardless input sample rate, and will produce the output at native sample rate (on all the systems except CrOS and Android). BUG=557789 Committed: https://crrev.com/f1688cc20b1ebc03b23e26ac5ea2fb6b84f70ea6 Cr-Commit-Position: refs/heads/master@{#363434} ========== |
