Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402283003:
Cleanup of the mixer interface. (Closed)
DescriptionCleanup of the mixer interface.
This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely
* Removing anonymous mixing.
* Removing the volume meter.
BUG=webrtc:6346
Committed: https://crrev.com/e97974d203b77e8fd13c057efadaba1b1c55fb1e
Cr-Commit-Position: refs/heads/master@{#14609}
Patch Set 1 : Minimized diff #
Total comments: 24
Patch Set 2 : Formulation & upper case. #
Total comments: 1
Patch Set 3 : Rebase. #
Total comments: 8
Patch Set 4 : Blank line #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 25 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Implemented mixer interface change suggestions. Suggestions are by solenberg@ in https://codereview.webrtc.org/2386383003/ * Changed mixability status into AddSource/RemoveSource. * Removed anonymous mixing. * Removed the volume meter. BUG=webrtc:6346 ========== to ========== Cleanup of the mixer interface. This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely * Removing anonymous mixing. * Removing the volume meter. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
Please take a look! This CL is part 1 of the mixer interface cleanup. There is also a part 2, and after that I think we can move the interface to //webrtc/api. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:16: #include "webrtc/modules/include/module.h" Was not used since when the mixer derived from Module. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:31: All frequencies are allowed now. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:55: int sample_rate_hz) = 0; The id is removed everywhere. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:75: bool mixable) = 0; Removed anonymous mixing. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:79: // called from a single thread. The rate and channels arguments The threading comments refer to our particular mixer implementation. A different implementation could use a different threading model. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:99: RTC_DISALLOW_COPY_AND_ASSIGN(AudioMixer); Also removed the audio level functionality (it is unused) and construction/copying details from the interface. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:20: #include "webrtc/system_wrappers/include/trace.h" Trace removed and replaced with 'LOG' where it makes sense. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:160: if (!limiter.get()) { Consistent use of {} for one-line blocks. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:14: #include <map> Unused since the changes in the mixing algorithm. Previously, participants were identified by the 'id_' field of the audio frame, which was a key in a map.
I love this! >300 lines shorter. Nice cleanup! Only one real question - whether the output frame needs clearing. Besides that, lgtm % nits https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:16: #include "webrtc/modules/include/module.h" On 2016/10/10 12:54:39, aleloi wrote: > Was not used since when the mixer derived from Module. Acknowledged. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:31: On 2016/10/10 12:54:39, aleloi wrote: > All frequencies are allowed now. Acknowledged. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:55: int sample_rate_hz) = 0; On 2016/10/10 12:54:39, aleloi wrote: > The id is removed everywhere. Acknowledged. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:75: bool mixable) = 0; On 2016/10/10 12:54:39, aleloi wrote: > Removed anonymous mixing. yay! https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:79: // called from a single thread. The rate and channels arguments On 2016/10/10 12:54:39, aleloi wrote: > The threading comments refer to our particular mixer implementation. A different > implementation could use a different threading model. Replace "Should" with "Will". The implementer needs to know how the interface is used by the framework it plugs into, and that goes for all methods on the interface (including destruction). https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:99: RTC_DISALLOW_COPY_AND_ASSIGN(AudioMixer); On 2016/10/10 12:54:39, aleloi wrote: > Also removed the audio level functionality (it is unused) and > construction/copying details from the interface. Acknowledged. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:20: #include "webrtc/system_wrappers/include/trace.h" On 2016/10/10 12:54:39, aleloi wrote: > Trace removed and replaced with 'LOG' where it makes sense. Acknowledged. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:84: if (audio_frame_list.empty()) { Don't you need to null the output frame? https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:160: if (!limiter.get()) { On 2016/10/10 12:54:39, aleloi wrote: > Consistent use of {} for one-line blocks. That warms my heart. :) https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:298: LOG_F(LS_WARNING) << "failed to GetAudioFrameWithMuted() from source"; total nit: Capital 'F' in failed https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:14: #include <map> On 2016/10/10 12:54:39, aleloi wrote: > Unused since the changes in the mixing algorithm. Previously, participants were > identified by the 'id_' field of the audio frame, which was a key in a map. Acknowledged.
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
aleloi@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:79: // called from a single thread. The rate and channels arguments On 2016/10/10 13:46:06, the sun wrote: > On 2016/10/10 12:54:39, aleloi wrote: > > The threading comments refer to our particular mixer implementation. A > different > > implementation could use a different threading model. > > Replace "Should" with "Will". The implementer needs to know how the interface is > used by the framework it plugs into, and that goes for all methods on the > interface (including destruction). Done. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:84: if (audio_frame_list.empty()) { On 2016/10/10 13:46:06, the sun wrote: > Don't you need to null the output frame? It's done in Mix when the algorithm compares frame->samples_per_channel_ to 0. If it's 0, then nothing was mixed and the frame is muted. https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:298: LOG_F(LS_WARNING) << "failed to GetAudioFrameWithMuted() from source"; On 2016/10/10 13:46:07, the sun wrote: > total nit: Capital 'F' in failed Done.
https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:84: if (audio_frame_list.empty()) { On 2016/10/10 14:10:16, aleloi wrote: > On 2016/10/10 13:46:06, the sun wrote: > > Don't you need to null the output frame? > > It's done in Mix when the algorithm compares frame->samples_per_channel_ to 0. > If it's 0, then nothing was mixed and the frame is muted. Acknowledged. https://codereview.webrtc.org/2402283003/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2402283003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:59: // mixed result is placed in the provided AudioFrame. Will only be With this comment, it seems the thread expectations are missing from the other methods.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:198: Remove (at least) one blank line. https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:91: int GetOutputAudioLevel() override; You are removing the audio level calculations from the mixer (good), but are you replacing it somewhere? https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:71: AudioFrameList GetNonAnonymousAudio() EXCLUSIVE_LOCKS_REQUIRED(crit_); Should this just be called GetAudio now that GetAnonymousAudio is gone?
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Next patch set: removed a blank line :) https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:198: On 2016/10/11 11:37:57, hlundin-webrtc wrote: > Remove (at least) one blank line. Done. https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:91: int GetOutputAudioLevel() override; On 2016/10/11 11:37:57, hlundin-webrtc wrote: > You are removing the audio level calculations from the mixer (good), but are you > replacing it somewhere? There are already audio calculations for every participating stream. These are used for selecting active streams and passing the level values up. Currently, I havn't found any usage of the audio level measurements from the (old) mixer. To obtain it you must call VoeVolumeControl::GetSpeechOutputLevel(channel) with channel=-1, which just doesn't seem to be done anywhere. https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:71: AudioFrameList GetNonAnonymousAudio() EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2016/10/11 11:37:57, hlundin-webrtc wrote: > Should this just be called GetAudio now that GetAnonymousAudio is gone? It's renamed to GetAudioFromSources in the next dependent CL. The two CLs come from a single large one, which I divided into removals and renamings. (Sorry for the confusion!)
lgtm https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:91: int GetOutputAudioLevel() override; On 2016/10/11 12:54:26, aleloi wrote: > On 2016/10/11 11:37:57, hlundin-webrtc wrote: > > You are removing the audio level calculations from the mixer (good), but are > you > > replacing it somewhere? > > There are already audio calculations for every participating stream. These are > used for selecting active streams and passing the level values up. Currently, I > havn't found any usage of the audio level measurements from the (old) mixer. To > obtain it you must call VoeVolumeControl::GetSpeechOutputLevel(channel) with > channel=-1, which just doesn't seem to be done anywhere. Acknowledged. https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2402283003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:71: AudioFrameList GetNonAnonymousAudio() EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2016/10/11 12:54:26, aleloi wrote: > On 2016/10/11 11:37:57, hlundin-webrtc wrote: > > Should this just be called GetAudio now that GetAnonymousAudio is gone? > > It's renamed to GetAudioFromSources in the next dependent CL. The two CLs come > from a single large one, which I divided into removals and renamings. (Sorry for > the confusion!) Acknowledged.
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2402283003/#ps180001 (title: "Blank line ")
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 ========== Cleanup of the mixer interface. This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely * Removing anonymous mixing. * Removing the volume meter. BUG=webrtc:6346 ========== to ========== Cleanup of the mixer interface. This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely * Removing anonymous mixing. * Removing the volume meter. BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup of the mixer interface. This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely * Removing anonymous mixing. * Removing the volume meter. BUG=webrtc:6346 ========== to ========== Cleanup of the mixer interface. This implements some of the suggestions in https://codereview.webrtc.org/2386383003/, namely * Removing anonymous mixing. * Removing the volume meter. BUG=webrtc:6346 Committed: https://crrev.com/e97974d203b77e8fd13c057efadaba1b1c55fb1e Cr-Commit-Position: refs/heads/master@{#14609} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e97974d203b77e8fd13c057efadaba1b1c55fb1e Cr-Commit-Position: refs/heads/master@{#14609} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
