Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221443002:
Changed mixing api and removed resampler (Closed)
DescriptionChanged mixing api and moved resampler.
Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular,
this is one of the steps required to have a single mixing component
instead of two doing the same thing.
The next planned change (which is not part of this CL) is to plug in the
new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule.
NOTRY=True
Committed: https://crrev.com/4496809b28d8f10506b1b622a6cfd9d930c60617
Cr-Commit-Position: refs/heads/master@{#13674}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Removed TODO. #
Total comments: 5
Patch Set 3 : Added public headers to BUILD.gn #
Total comments: 1
Patch Set 4 : Changed back Mix comment. #Patch Set 5 : Removed minimal frequency from the mixer since it doesn't resample. #Patch Set 6 : Should fix 'mixing_frequency' uninitialized error. #
Total comments: 6
Patch Set 7 : Described Mix args in comments. #Messages
Total messages: 30 (17 generated)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
A step towards merging the two audio mixers. The visible API will be mixer::Mix(rate, channels, void* data). Please comment on function names and signatures! They are very simple to change currently. The next planned CL (a dependent one to this one) will be one that plugs in this mixer. I will try to put it side-by-side with the old one. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.cc (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.cc:374: frame->num_channels_ = num_channels; Moved to NewACmix https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:51: int32_t MixActiveChannels(); This file is the former 'OutputMixer'. Parts of its contents are slowly being merged into the former 'AudioConferenceMixer' to create a single module responsible for mixing. It is not deleted yet because there are several tests that would have to be removed, and because not all content is yet in 'NewAudioConferenceMixer'. In this CL, the resampling is moved to the old mixer. As a consequence, some APIs have to change. This function asked the conference mixer to mix, but it cannot do that anymore, because it doesn't have the sample rate and channel count. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:103: // Converts mixed audio to the audio device output rate. Moved to NewAudioConferenceMixer. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:53: virtual void Mix(AudioFrame* audio_frame_for_mixing) = 0; We are moving away from the AudioFrame. In the future, we want to communicate audio with simpler structures. The 'void*' is a step in that direction. We also may want to mix floating point audio. This will we be main mixer function for clients of the mixer. The mixer will do necessary resampling. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:197: reinterpret_cast<AudioFrame*>(audio_data); It's still an AudioFrame on the inside yet... https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:285: Resampling copied from OutputMixer. It would be nice to know how often the code enters the if statement. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:292: voe::RemixAndResample(*audio_frame_for_mixing, &resampler_, I *think* it's OK for RemixAndResample to put data in the same buffer. I will double check that some test covers it, and upload a new test in another patch set if it's not. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:162: // Converts mixed audio to the audio device output rate. Moved here from the former 'OutputMixer'. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/te... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/te... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:343: audio_mixer_->MixActiveChannels(); This is the test that touches OutputMixer. Now GetMixedAudio also causes the mixer to mix one iteration. It's a bad name for the function, but I think it's OK, as AudioMixer (formerly OutputMixer) will be removed anyway. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/te... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/te... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:109: new_mixer_->Mix(kSampleRateHz, Adepted tests to match new mixer API.
...and I noticed I'm more wordy than usual this time. It won't become a habit :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:46: int RegisterExternalMediaProcessing(VoEMediaProcess& // NOLINT Do you plan to clean out all this gunk before hooking the new mixer into AudioState? https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:53: virtual void Mix(AudioFrame* audio_frame_for_mixing) = 0; On 2016/08/05 09:52:21, aleloi wrote: > We are moving away from the AudioFrame. In the future, we want to communicate > audio with simpler structures. The 'void*' is a step in that direction. We also > may want to mix floating point audio. Then why not float* instead of void*? > This will we be main mixer function for clients of the mixer. The mixer will do > necessary resampling. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:162: // Converts mixed audio to the audio device output rate. On 2016/08/05 09:52:22, aleloi wrote: > Moved here from the former 'OutputMixer'. I thought the idea was that ACM/NetEq could deliver data to us already at the output rate, so the mixer wouldn't need to do resampling?
https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:46: int RegisterExternalMediaProcessing(VoEMediaProcess& // NOLINT On 2016/08/05 12:26:00, the sun wrote: > Do you plan to clean out all this gunk before hooking the new mixer into > AudioState? Yes, I want to remove this file completely. The one called 'new_audio_conference_mixer' contains things that are going to be used. I'm not sure if that's the best way. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (left): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:53: virtual void Mix(AudioFrame* audio_frame_for_mixing) = 0; On 2016/08/05 12:26:00, the sun wrote: > On 2016/08/05 09:52:21, aleloi wrote: > > We are moving away from the AudioFrame. In the future, we want to communicate > > audio with simpler structures. The 'void*' is a step in that direction. We > also > > may want to mix floating point audio. > > Then why not float* instead of void*? > > > This will we be main mixer function for clients of the mixer. The mixer will > do > > necessary resampling. > To do one change at a time, probably. Much of the mixer and surrounding code needs to be changed to mix in floats, I think. In what order do you think it's simpler to do the changes: rewrite mixer and surroundings to mix in floats, or plug in the new mixer? Curious. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:162: // Converts mixed audio to the audio device output rate. On 2016/08/05 12:26:00, the sun wrote: > On 2016/08/05 09:52:22, aleloi wrote: > > Moved here from the former 'OutputMixer'. > > I thought the idea was that ACM/NetEq could deliver data to us already at the > output rate, so the mixer wouldn't need to do resampling? Thank you. I just got reminded of that when working on connecting the mixer to the channels. You are right. Resampling happens ACMReceiver::GetAudio. I will remove in next patch set.
aleloi@google.com changed reviewers: + aleloi@google.com, minyue@webrtc.org - solenberg@webrtc.org
Minyue, could you PTAL? Fredrik went back on vacation before we were finished. https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/1/webrtc/modules/audio_mixer/so... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:244: } Fingers have been itching to rewrite this for months now :) https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/include/audio_mixer_defines.h (left): https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:52: I removed NeededFrequency, since the mixer asks participants for data at a specific rate anyway. This would fix a TODO(andrew) in AudioMixer::GetMixedAudio. The mixer now mixes at the rate asked for by the audio device module. Channel::NeededFrequency was computed from the playout and receiving frequency, so there shouldn't be much (or any) performance differences, just simplifications. https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:56: AudioFrame* audio_frame_for_mixing) = 0; I decided to leave the AudioFrame in the mixer for this CL. Replacing it with int16_t* would require to reimplement the functions in AudioFrame and AudioFrameOperations (which I don't have a concrete plan for yet). It's shifting (>>=), adding, remixing (MonoToStereo, StereoToMono), audio_processing::ProcessReverseStream, AudioLevel::ComputeLevel, and probably a few more. The receiver side is simpler, since data is mostly just copied to the audio device buffer in VoE::GetPlayoutData. https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:62: // Remixes a frame between stereo and mono. I couldn't find any function that does this in WebRTC. There was common_audio/audio_converter, but that only did floats. voe::RemixAndResample was not suitable either. https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:226: for (FrameAndMuteInfo& frame_and_mute : mixList) { Previously, all frames were up-mixed if a single one was stereo, and then remixed to what ADM wanted (probably stereo?) before going in to audio device module. Now we just remix them to what ADM asks for before doing anything else. https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2221443002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:372: Two new tests to check that the mixer asks for the expected sample rate and frequency, and that the result has that rate & frequncy.
https://codereview.webrtc.org/2221443002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2221443002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:58: 'public' is a GN feature. Source files from targets that depend on this target are not allowed to import other headers than those marked 'public'. gn check verifies that there are no forbidden includes. If nothing is marked 'public', all headers are public by default.
Description was changed from ========== Changed mixing api and moved resampler from the 'second' new mixer to the 'first'. This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. ========== to ========== Changed mixing api and moved resampler from the 'second' new mixer to the 'first'. This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. ==========
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Nice! Please consider the nits I found. Also please put the CL title as the first sentence in the description (a good practice). BTW, Is removing NeededFrequency() in channel also planned. https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:51: // mixed result is placed in the provided AudioFrame. Can only be add comments on the new arguments https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:63: void RemixFrame(AudioFrame* frame, size_t number_of_channels) { why is this call Remix? https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:77: // TODO(andrew): consider not modifying |frame| here. TODO andrew is not proper any longer. change it (step-by-step) to you if appropriate
Thanks! Eventually, removing NeededFrequency in channel is also planned. It mostly just returns the audio device rate anyway. Now I want to mix at the rate the audio device asks for, without asking channels. https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:63: void RemixFrame(AudioFrame* frame, size_t number_of_channels) { On 2016/08/08 15:05:44, minyue-webrtc wrote: > why is this call Remix? IIRC, remixing means changing between mono and stereo in the context of WebRTC. E.g. voe::RemixAndResample. I think hlundin@ told me that once. https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:77: // TODO(andrew): consider not modifying |frame| here. On 2016/08/08 15:05:44, minyue-webrtc wrote: > TODO andrew is not proper any longer. change it (step-by-step) to you if > appropriate He's gone, right? I don't really understand the reason for not modifying |frame|, or how to go about removing it. Does it means to apply a limiter? We first shift down by 1, then apply a limiter to -7dB, and then multiply by 2. Do you understand what is meant here?
> Also please put the CL title as the first sentence in the description (a good practice) Right, since that's what goes in to the commit message.
Description was changed from ========== Changed mixing api and moved resampler from the 'second' new mixer to the 'first'. This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. ========== to ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. ==========
lgtm https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2221443002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:77: // TODO(andrew): consider not modifying |frame| here. On 2016/08/08 15:14:36, aleloi wrote: > On 2016/08/08 15:05:44, minyue-webrtc wrote: > > TODO andrew is not proper any longer. change it (step-by-step) to you if > > appropriate > > He's gone, right? > > I don't really understand the reason for not modifying |frame|, or how to go > about removing it. Does it means to apply a limiter? We first shift down by 1, > then apply a limiter to -7dB, and then multiply by 2. Do you understand what is > meant here? I think it means const AudioFrame& frame and *mixed_frame += frame >> 1 But you can give a second thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. ========== to ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. NOTRY=True ==========
The CQ bit was checked by aleloi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2221443002/#ps120001 (title: "Described Mix args in comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. NOTRY=True ========== to ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. NOTRY=True ========== to ========== Changed mixing api and moved resampler. Removed resampler from NewAudioConferenceMixer and AudioMixer (which started as a copy of former OutputMixer). This is part of the mixer rewrite project. In particular, this is one of the steps required to have a single mixing component instead of two doing the same thing. The next planned change (which is not part of this CL) is to plug in the new mixer (NewAudioConferenceMixer) into AudioState and AudioDeviceModule. NOTRY=True Committed: https://crrev.com/4496809b28d8f10506b1b622a6cfd9d930c60617 Cr-Commit-Position: refs/heads/master@{#13674} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4496809b28d8f10506b1b622a6cfd9d930c60617 Cr-Commit-Position: refs/heads/master@{#13674} |
