Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2436033002:
Replace AudioConferenceMixer with AudioMixer. (Closed)
|
Created:
4 years, 2 months ago by aleloi Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, henrika_webrtc, stefan-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReplace AudioConferenceMixer with AudioMixer.
This CL re-routes audio through AudioMixer instead of AudioConferenceMixer.
This is done without any modifications to VoiceEngine.
Previously, output audio was polled by an AudioDevice through an AudioTransport
pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the
request for data on to OutputMixer and further to AudioConferenceMixer.
This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice
to have another AudioTransport pointer, which points to an AudioTransportProxy.
The AudioTransportProxy is responsible for feeding mixed data to the
AudioProcessing component for echo cancellation, and to resample the audio data
after AudioProcessing and before it is sent to the AudioDevice.
The set up of the audio path was previously done during VoiceEngine
initialization. Now it is changed in the AudioState constructor.
This list shows where audio-path-related VoiceEngine functionality has been
moved:
OutputMixer --> AudioTransportProxy
VoiceEngineImpl --> AudioState, AudioTransportProxy
SharedData --> AudioState
Channel --> AudioReceiveStream, ChannelProxy, Channel
AudioState owns the new mixer and connects it to AudioTransport and
AudioDevice on initialization.
The audio input source is AudioReceiveStream, which registers itself with the
mixer (which it gets from AudioState) on Start and Stop.
# Since the AudioTransport interface contains non-const references.
NOPRESUBMIT=True
BUG=webrtc:6346
Committed: https://crrev.com/04c0722cf1fc38e6888ef44e413603e0cb461ea4
Cr-Commit-Position: refs/heads/master@{#15193}
Patch Set 1 : Tests pass (except for chromium mac which detects sample rate issue). #Patch Set 2 : Added errors and logs to AudioTransport. #
Total comments: 47
Patch Set 3 : WebRTC tests will fail until the mix sample rate change has landed. #Patch Set 4 : New mixer unit test. #Patch Set 5 : forgot dependency. #
Total comments: 37
Patch Set 6 : Consistent thread checker, errcode handling in RecStream. #
Total comments: 15
Patch Set 7 : New ReceiveStream test and no fixture in AudioPath test. #
Total comments: 2
Patch Set 8 : Declare constants and depend on RemoveSource CL. #
Total comments: 10
Patch Set 9 : Correct struct member names, less code duplication in tests, minor fixes. #
Total comments: 8
Patch Set 10 : Added mock mixer, merged audio state tests. #
Depends on Patchset: Messages
Total messages: 95 (75 generated)
aleloi@google.com changed reviewers: + aleloi@google.com
https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.cc#n... webrtc/audio/audio_state.cc:27: audio_transport_proxy_(nullptr) { audio_transport_proxy_ initializer not needed: the unique ptr default ctor already does this. AudioMixerImpl::Create can fail. Then nullptr is returned, so we should DCHECK or CHECK for a mixer. https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.cc#n... webrtc/audio/audio_state.cc:41: LOG(LS_INFO) << "fail fail fail!"; Something better than 'fail fail fail'? https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.cc#n... webrtc/audio/audio_state.cc:65: AudioMixer* AudioState::mixer() { Return shared_ptr, or put mixer in unique_ptr. https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.h#ne... webrtc/audio/audio_state.h:39: AudioMixer* mixer(); Implicit conversion to/from scoped_refptr. Bad! https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.h#ne... webrtc/audio/audio_state.h:66: rtc::scoped_refptr<AudioMixer> mixer_; It shouldn't be scoped, because no one borrows it. mixer() is used by audio_receive_stream. It shouldn't implicitly convert to Mixer*... TODO: investigate! https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/audio_state.h#ne... webrtc/audio/audio_state.h:69: std::unique_ptr<AudioTransportProxy> audio_transport_proxy_; There is no naked pointer... https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/webrtc_audio.gypi File webrtc/audio/webrtc_audio.gypi (right): https://codereview.webrtc.org/2436033002/diff/1/webrtc/audio/webrtc_audio.gyp... webrtc/audio/webrtc_audio.gypi:14: '<(webrtc_root)/modules:webrtc_mixer_impl', Should be modules.gyp
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Connecting mixer, final upload! TODO(aleloi): add details here. TODO(aleloi): everything in audio_state.cc and transport_proxy is not checked. BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. A small addition to VoiceEngine is made in order for voe::Channel to not register itself with the new mixer. BUG=webrtc:6346 ==========
Patchset #2 (id:140001) has been deleted
https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:164: // to the old mixer. It will go away, once the old mixer is removed. As the comment says, the reason for replacing |base->StartPlayout(channel_id)| with the block below is that VoE::StartPlayout calls Channel::StartPlayout, which registers the channel with the old mixer. That doesn't need to be a problem, because the AudioTransport that polls the old mixer for data is disconnected in AudioState. Another issue is that the channel dtor calls StopPlayout on itself, which causes the channel to de-register with the old mixer. In total: should we rather change VoE::StartPlayout back and have a participant in two mixers (with only one active)? WDYT? https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_sta... File webrtc/audio/audio_state.cc (right): https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_sta... webrtc/audio/audio_state.cc:26: mixer_(AudioMixerImpl::Create()) { Alternative: pass a unique_ptr<AudioMixer> and move creation to webrtcvoiceengine or even higher. That way we could do integration tests with mixer mocks! https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_sta... webrtc/audio/audio_state.cc:42: } A few details, some of which should maybe be made into code comments: All of |audio_transport(), audio_processing(), audio_device()| can be |nullptr| (e.g. in tests). If the transport or APM is NULL, we could still receive and produce audio through the mixer. There are checks in AudioTransportProxy that make sure never to access the APM and transport in that case. If the device is NULL, no audio will be played out, requested or mixed. Then there is no point in constructing an AudioTransportProxy. https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_sta... File webrtc/audio/audio_state.h (right): https://codereview.chromium.org/2436033002/diff/160001/webrtc/audio/audio_sta... webrtc/audio/audio_state.h:65: rtc::scoped_refptr<AudioMixer> mixer_; We don't need shared ownership: the mixer is owned by this module. There is a mixer() accessor that lends it out to AudioReceiveStream::Stop and Start. Do you think we can make it unique_ptr instead? The mixer is a RefCountInterface, so there is a danger that someone else might call Release on it in the future.
Patchset #3 (id:180001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #2 (id:200001) has been deleted
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. A small addition to VoiceEngine is made in order for voe::Channel to not register itself with the new mixer. BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. A small addition to VoiceEngine is made in order for voe::Channel to not register itself with the new mixer. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: - aleloi@google.com
Patchset #1 (id:160001) has been deleted
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. A small addition to VoiceEngine is made in order for voe::Channel to not register itself with the new mixer. BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + ossu@webrtc.org, solenberg@webrtc.org
Hi! Here is a CL that finally connects the mixer and routes audio through it. There is an issue, which are not visible from the code: when AudioReceiveStream::Start is called, it calls VoE::StartPlayout(channel_id), which calls Channel::StartPlayout(), which connects the channel to the old mixer. Similarly, when Stop() is called, the underlying channel attempts to disconnect itself. After an offline discussion with ossu@, I decided that this is not a problem, because 1. I can't make the channel *not* disconnect itself without modifying what happens in ~Channel and how channel_state is set. 2. The old mixer is never called, since we disconnect the AudioTransport that asks it to produce audio. Another issue is a lack of tests. I have 3 tests (or test groups) planned that would check for rather complex behaviors: 1. Check that AudioReceiveStream sends audio to mixer when everything is set up correctly 2. Check that AudioState sends audio from mixer to audio device when given right ctor args. 3. Check that old mixer does not get audio from a channel when channel, receive stream, AudioState are connected together. Yet another issue is a bug in the mixer: it sends data to the limiter (which is an APM instance) without checking if the sample rate is native. Fortunately it isn't on one chrome test. Since this CL doesn't touch the mixer, I can work on this issue at the same time. https://codereview.chromium.org/2436033002/diff/280001/webrtc/audio/audio_sta... File webrtc/audio/audio_state.cc (right): https://codereview.chromium.org/2436033002/diff/280001/webrtc/audio/audio_sta... webrtc/audio/audio_state.cc:26: mixer_(AudioMixerImpl::Create()) { Alternative: pass a unique_ptr<AudioMixer> and move creation to webrtcvoiceengine or even higher. That way we could do integration tests with mixer mocks! https://codereview.chromium.org/2436033002/diff/280001/webrtc/audio/audio_sta... webrtc/audio/audio_state.cc:42: } A few details, some of which should maybe be made into code comments: All of |audio_transport(), audio_processing(), audio_device()| can be |nullptr| (e.g. in tests). I've tried to handle that by adding error logs wherever (here and in audio_transport_proxy) a resource that may be NULL is used. If the transport or APM is NULL, we could still receive and produce audio through the mixer. There are checks in AudioTransportProxy that make sure never to access the APM and transport in that case. If the device is NULL, no audio will be played out, requested or mixed. Then there is no point in constructing an AudioTransportProxy.
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:158: if (playing_) { Is this fine or does it indicate a usage error? I.e. would it be useful to be told that you've messed up your Start()/Stop() calls rather than just swallowing any combination of them? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:172: static_cast<internal::AudioState*>(audio_state_.get()); I wonder about the cast here. I see it's being done in other parts of AudioReceiveStream as well. Do you know why and does that apply to your case? Should the AudioState interface not expose the mixer? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (left): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:122: EXPECT_CALL(voice_engine_, StopPlayout(kChannelId)).WillOnce(Return(0)); Why won't StopPlayout be called after this change? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:26: mixer_(AudioMixerImpl::Create()) { On 2016/10/24 11:59:14, aleloi wrote: > Alternative: pass a unique_ptr<AudioMixer> and move creation to > webrtcvoiceengine or even higher. That way we could do integration tests with > mixer mocks! I do believe integration testing is supposed to be done without mocks, with mocks used to facilitate unit testing. If we don't want pluggable mixers (unsure), it's probably easier to have AudioState create the mixer and instead make AudioState mockable for the cases where we want to use something else. With a mockable AudioState, the AudioState implementation could just have an AudioMixerImpl object, rather than having to call AudioMixerImpl::Create(). If you wanted to hide the implementation of the AudioMixer, shouldn't the Create method be on AudioMixer instead of AudioMixerImpl? i.e. AudioMixer::Create() (which does new AudioMixerImpl). https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:42: } On 2016/10/24 11:59:14, aleloi wrote: > A few details, some of which should maybe be made into code comments: > > All of |audio_transport(), audio_processing(), audio_device()| can be > |nullptr| (e.g. in tests). I've tried to handle that by adding error logs > wherever (here and in audio_transport_proxy) a resource that may be NULL is > used. > > If the transport or APM is NULL, we could still receive and produce > audio through the mixer. There are checks in AudioTransportProxy that > make sure never to access the APM and transport in that case. > > If the device is NULL, no audio will be played out, requested or > mixed. Then there is no point in constructing an AudioTransportProxy. When will this happen? Only in tests? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:63: rtc::scoped_refptr<AudioMixer> AudioState::mixer() const { Is the AudioMixer supposed to be able to outlive AudioState? If not, why is it reference counted?
That's looking pretty good - nice to see this happening! https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:157: RTC_DCHECK_RUN_ON(&thread_checker_); This is nice, but should all thread checking in this class use _RUN_ON instead of checking CalledOnValidThread()? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:158: if (playing_) { On 2016/10/25 14:13:33, ossu wrote: > Is this fine or does it indicate a usage error? > I.e. would it be useful to be told that you've messed up your Start()/Stop() > calls rather than just swallowing any combination of them? Since this is going to be part of the public API I think a defensive approach is preferable. For anything internal, DCHECKs should be used though. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:171: auto* const audio_state = Make a class private method internal::AudioState* audio_state() which does does this casting for you. Use it in all the places we cast. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:172: static_cast<internal::AudioState*>(audio_state_.get()); On 2016/10/25 14:13:33, ossu wrote: > I wonder about the cast here. I see it's being done in other parts of > AudioReceiveStream as well. Do you know why and does that apply to your case? > Should the AudioState interface not expose the mixer? AudioState is intended as (for clients of the Call API) an opaque type which keeps the (audio) state that's shared between multiple Call instances. The solution here is in line with that design choice. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:175: LOG(LS_ERROR) << "Failed to add source to mixer."; Does that mean we're not actually playing and we should have left playing_ == false so that we try again? Or does it mean the result from AddSource() is actually pointless? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:189: if (!audio_state->mixer()->RemoveSource(this)) { Uhm, sorry, I missed there was a return code for RemoveSource() - I should have opposed that... https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (left): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:122: EXPECT_CALL(voice_engine_, StopPlayout(kChannelId)).WillOnce(Return(0)); On 2016/10/25 14:13:33, ossu wrote: > Why won't StopPlayout be called after this change? Because it is called from the dtor, but now the call will only happen if the stream has actually been started. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:26: mixer_(AudioMixerImpl::Create()) { On 2016/10/25 14:13:33, ossu wrote: > On 2016/10/24 11:59:14, aleloi wrote: > > Alternative: pass a unique_ptr<AudioMixer> and move creation to > > webrtcvoiceengine or even higher. That way we could do integration tests with > > mixer mocks! > > I do believe integration testing is supposed to be done without mocks, with > mocks used to facilitate unit testing. If we don't want pluggable mixers > (unsure), it's probably easier to have AudioState create the mixer and instead > make AudioState mockable for the cases where we want to use something else. > > With a mockable AudioState, the AudioState implementation could just have an > AudioMixerImpl object, rather than having to call AudioMixerImpl::Create(). If > you wanted to hide the implementation of the AudioMixer, shouldn't the Create > method be on AudioMixer instead of AudioMixerImpl? i.e. AudioMixer::Create() > (which does new AudioMixerImpl). We want the mixer to be pluggable to allow alternative implementations, so it should be passed in the AudioState::Config instead. Put the creation in webrtcvoiceengine.cc:MakeAudioStateConfig() for now. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:32: RTC_CHECK(mixer_); This should be a DCHECK instead. Program should have terminated already if new failed. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:35: if (device) { DCHECK that instead. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:38: device->RegisterAudioCallback(nullptr); Doesn't look like this is necessary? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:42: } On 2016/10/25 14:13:33, ossu wrote: > On 2016/10/24 11:59:14, aleloi wrote: > > A few details, some of which should maybe be made into code comments: > > > > All of |audio_transport(), audio_processing(), audio_device()| can be > > |nullptr| (e.g. in tests). I've tried to handle that by adding error logs > > wherever (here and in audio_transport_proxy) a resource that may be NULL is > > used. > > > > If the transport or APM is NULL, we could still receive and produce > > audio through the mixer. There are checks in AudioTransportProxy that > > make sure never to access the APM and transport in that case. > > > > If the device is NULL, no audio will be played out, requested or > > mixed. Then there is no point in constructing an AudioTransportProxy. > > When will this happen? Only in tests? If possible to avoid these special cases by updating the tests, that could be a better way. I generally prefer to use empty implementations instead of littering the code with null checks. The null checks add implicit extra states the class may be in, which makes it harder to reason about correctness. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:57: if (config_.audio_device_module) { This field appears unused right now. Just stick to calling voe_base_->audio_device_module(); (and remove this utility method). Also, comment out the field in AudioState::Config. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:63: rtc::scoped_refptr<AudioMixer> AudioState::mixer() const { On 2016/10/25 14:13:33, ossu wrote: > Is the AudioMixer supposed to be able to outlive AudioState? If not, why is it > reference counted? Ultimately, it will possibly be supplied by an API client and therefore ownership is shared. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:18: #include "webrtc/audio/audio_transport_proxy.h" You can get away with forward-declaring the AudioTransportProxy in the .h https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:38: AudioDeviceModule* audio_device(); This is an internal util, make it private. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:65: rtc::scoped_refptr<AudioMixer> mixer_; This should really live in the AudioState::Config, as we ultimately want to make it settable from the client. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:32: virtual ~AudioTransportProxy() {} override https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:64: int64_t* ntp_time_ms) override { RTC_DCHECK_EQ(2u, nBytesPerSample); RTC_DCHECK(audioSamples); etc - Everything you can check, such as the requested nSamples and nChannels not causing us to try reading outside the AudioFrame buffer. (Similarly for all methods here.) https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:73: LOG(LS_ERROR) << "NeedMorePlayData called, but no APM provided."; In what context does this happen? Is the right way to fix the test/mock that gives you a nullptr in audio_processing()? https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:90: size_t number_of_frames) override { Add comment here why this path is unreachable. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:115: AudioFrame frame_for_mixing_; DISALLOW_IMPLICIT... https://codereview.webrtc.org/2436033002/diff/280001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:284: EXPECT_EQ(0, voe_base->StopPlayout(recv_channel_id)); audio_receive_stream->Stop();
Patchset #4 (id:320001) has been deleted
Patchset #3 (id:300001) has been deleted
Patchset #4 (id:360001) has been deleted
Patchset #4 (id:380001) has been deleted
Patchset #3 (id:340001) has been deleted
Patchset #4 (id:420001) has been deleted
Hi! I've finally uploaded the next version. There are major changes: I've divided this CL in four. https://codereview.webrtc.org/2456363002 only adds a mixer field in AudioState::Config. https://codereview.webrtc.org/2454373002 adds an empty TransportProxy https://codereview.webrtc.org/2469743002 makes the mixer field mandatory and creates a mixer to pass to AudioState wherever it is created. What is left of this CL modifies the AudioReceiveStream Start/Stop logic, sends audio through the mixer and APM and adds resampling in AudioTransportProxy. I'll send some of the parts out to you when they are finished. Can you look at what's left of this CL? And sorry for having to re-read it again after two weeks... https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:157: RTC_DCHECK_RUN_ON(&thread_checker_); On 2016/10/27 10:06:45, the sun wrote: > This is nice, but should all thread checking in this class use _RUN_ON instead > of checking CalledOnValidThread()? I added an ACCESS_ON annotation on playout_. Now every method that accesses it needs to acquire (lockable) thread_checker_, which is done by the RTC_DCHECK_RUN_ON macro in debug mode (I think the locking is only there to trick Clang++, and no real mutex locks are acquired at runtime: in thread_checker.h EXCLUSIVE_LOCK_FUNCTION seems to have an empty implementation). tldr, if I change it back, I have to remove the guard annotation from playout_. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:158: if (playing_) { On 2016/10/27 10:06:45, the sun wrote: > On 2016/10/25 14:13:33, ossu wrote: > > Is this fine or does it indicate a usage error? > > I.e. would it be useful to be told that you've messed up your Start()/Stop() > > calls rather than just swallowing any combination of them? > > Since this is going to be part of the public API I think a defensive approach is > preferable. For anything internal, DCHECKs should be used though. webrtcvoiceengine runs Stop on everything in SetPlayout without bothering to check. If I were to add error logging or checks, we'd have to fix it first. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:171: auto* const audio_state = On 2016/10/27 10:06:45, the sun wrote: > Make a class private method internal::AudioState* audio_state() which does does > this casting for you. Use it in all the places we cast. That's a design tip from Scott Meyers! I read that just a few days ago :) Fixed in next patch set. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:175: LOG(LS_ERROR) << "Failed to add source to mixer."; On 2016/10/27 10:06:45, the sun wrote: > Does that mean we're not actually playing and we should have left playing_ == > false so that we try again? > > Or does it mean the result from AddSource() is actually pointless? Thanks! It's an error that is never supposed to happen. If we are actually playing, the the receive stream is in an invalid state. I think a NOTREACHED assertion fits here. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:189: if (!audio_state->mixer()->RemoveSource(this)) { On 2016/10/27 10:06:45, the sun wrote: > Uhm, sorry, I missed there was a return code for RemoveSource() - I should have > opposed that... It's not too late to change! But why? Isn't it the same as with AddSource: we leave a possibility for mixer implementation to report that something is seriously wrong? It's not supposed to happen anyway, so I've added NOTREACHED here as well. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (left): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:122: EXPECT_CALL(voice_engine_, StopPlayout(kChannelId)).WillOnce(Return(0)); On 2016/10/27 10:06:45, the sun wrote: > On 2016/10/25 14:13:33, ossu wrote: > > Why won't StopPlayout be called after this change? > > Because it is called from the dtor, but now the call will only happen if the > stream has actually been started. Thanks, sorry for being late! https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:26: mixer_(AudioMixerImpl::Create()) { On 2016/10/27 10:06:45, the sun wrote: > On 2016/10/25 14:13:33, ossu wrote: > > On 2016/10/24 11:59:14, aleloi wrote: > > > Alternative: pass a unique_ptr<AudioMixer> and move creation to > > > webrtcvoiceengine or even higher. That way we could do integration tests > with > > > mixer mocks! > > > > I do believe integration testing is supposed to be done without mocks, with > > mocks used to facilitate unit testing. If we don't want pluggable mixers > > (unsure), it's probably easier to have AudioState create the mixer and instead > > make AudioState mockable for the cases where we want to use something else. > > > > With a mockable AudioState, the AudioState implementation could just have an > > AudioMixerImpl object, rather than having to call AudioMixerImpl::Create(). If > > you wanted to hide the implementation of the AudioMixer, shouldn't the Create > > method be on AudioMixer instead of AudioMixerImpl? i.e. AudioMixer::Create() > > (which does new AudioMixerImpl). > > We want the mixer to be pluggable to allow alternative implementations, so it > should be passed in the AudioState::Config instead. Put the creation in > webrtcvoiceengine.cc:MakeAudioStateConfig() for now. Changes: A mixer instance is passed in the config struct. There are divided opinions on whether to make it a unique_ptr and remove the refcounting. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:32: RTC_CHECK(mixer_); On 2016/10/27 10:06:46, the sun wrote: > This should be a DCHECK instead. Program should have terminated already if new > failed. Done. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:35: if (device) { On 2016/10/27 10:06:45, the sun wrote: > DCHECK that instead. Done. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:38: device->RegisterAudioCallback(nullptr); On 2016/10/27 10:06:45, the sun wrote: > Doesn't look like this is necessary? I had a vague memory that it broke something. Webrtc tests pass without register(nullptr), but I have some chromium issues now that were not there before. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:38: device->RegisterAudioCallback(nullptr); On 2016/10/27 10:06:45, the sun wrote: > Doesn't look like this is necessary? It is in Chrome. WebRtcAudioDeviceImpl::RegisterAudioCallback contains this line: DCHECK_EQ(audio_transport_callback_ == NULL, audio_callback != NULL); https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:42: } On 2016/10/27 10:06:46, the sun wrote: > On 2016/10/25 14:13:33, ossu wrote: > > On 2016/10/24 11:59:14, aleloi wrote: > > > A few details, some of which should maybe be made into code comments: > > > > > > All of |audio_transport(), audio_processing(), audio_device()| can be > > > |nullptr| (e.g. in tests). I've tried to handle that by adding error logs > > > wherever (here and in audio_transport_proxy) a resource that may be NULL is > > > used. > > > > > > If the transport or APM is NULL, we could still receive and produce > > > audio through the mixer. There are checks in AudioTransportProxy that > > > make sure never to access the APM and transport in that case. > > > > > > If the device is NULL, no audio will be played out, requested or > > > mixed. Then there is no point in constructing an AudioTransportProxy. > > > > When will this happen? Only in tests? > > If possible to avoid these special cases by updating the tests, that could be a > better way. I generally prefer to use empty implementations instead of littering > the code with null checks. The null checks add implicit extra states the class > may be in, which makes it harder to reason about correctness. @ossu, yes only in tests. I've updated all tests to send an audio device in a dependence CL. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:57: if (config_.audio_device_module) { On 2016/10/27 10:06:45, the sun wrote: > This field appears unused right now. Just stick to calling > voe_base_->audio_device_module(); (and remove this utility method). Also, > comment out the field in AudioState::Config. IIRC, that broke some test. I'll take a look at it! https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:18: #include "webrtc/audio/audio_transport_proxy.h" On 2016/10/27 10:06:46, the sun wrote: > You can get away with forward-declaring the AudioTransportProxy in the .h Style guide (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) says to avoid forward declaration where possible. What do we win by forward-declaring (compilation speed?) https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:38: AudioDeviceModule* audio_device(); On 2016/10/27 10:06:46, the sun wrote: > This is an internal util, make it private. Missed that! I used it previously in audio_receive_stream (hence private) to make the work of voe->StartPlayout(channel) without connecting the channel to the old mixer. https://codereview.webrtc.org/2436033002/diff/280001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:65: rtc::scoped_refptr<AudioMixer> mixer_; On 2016/10/27 10:06:46, the sun wrote: > This should really live in the AudioState::Config, as we ultimately want to make > it settable from the client. Moved in dependence CL.
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19877) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18567)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15969) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12874)
Patchset #5 (id:460001) has been deleted
Patchset #5 (id:480001) has been deleted
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12311)
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/...
Patchset #5 (id:500001) has been deleted
Patchset #4 (id:440001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/15928)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12477) ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/2355) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14832) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20060)
Patchset #5 (id:540001) has been deleted
Patchset #5 (id:560001) has been deleted
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/...
Patchset #4 (id:520001) has been deleted
https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:156: RTC_DCHECK_RUN_ON(&thread_checker_); I don't like that some methods in this class use RTC_DCHECK_RUN_ON(&thread_checker_); and some use RTC_DCHECK(thread_checker_.CalledOnValidThread()); Please make it consistent. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:164: RTC_NOTREACHED() << "AudioReceiveStream::Start failed with error: " Why did you change this? We can conceivably end up here at run time, so this should still be logging, not "NOTREACHED" https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:169: playing_ = true; Note that you're violating the AudioMixer promises of never calling RemoveSource() unless the source was successfully added. You need to sort out the logic and and the missing test case. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:188: RTC_NOTREACHED() << "Failed to remove stream from mixer."; If the return code is *supposed* to always be true, what's the point of it. Change the interface. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:314: return static_cast<internal::AudioState*>(audio_state_.get()); Pull into a local variable and DCHECK it's not null before cast+return. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:318: internal::AudioState* the_audio_state = audio_state(); Make that a one-liner instead https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:14: #include "webrtc/base/refcountedobject.h" why? https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:22: class AudioStateAudioPathTest : public testing::Test { Please avoid fixtures. Like in the stream tests, make a little helper class that you put on the stack in the test cases instead. This usually makes the code easier to follow (everything you need to know is right in front of you - no magic initialization happening before the test function runs, etc). https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:18: int Resample(const AudioFrame& frame, I'd find this order more intuitive: source_frame, resampler destination_sample_rate destination Try sticking to either of "target", destination, "dest" or "dst". https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:22: const int frame_sample_rate = frame.sample_rate_hz_; Remove this local https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:30: static_cast<int16_t*>(destination), cast not needed https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:39: RTC_DCHECK(apm); nit: use same order as ctor init list https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:81: mixer_->Mix(static_cast<int>(nChannels), &frame_for_mixing_); Cast should be unnecessary here. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:89: static_cast<int16_t*>(audioSamples)); DCHECK that nSamplesOut is nSamples? https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:111: RTC_DCHECK_EQ(static_cast<size_t>(bits_per_sample), 8 * sizeof(int16_t)); Heh, it's called int16 for a reason. http://en.cppreference.com/w/cpp/types/integer https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:123: Resample(frame_for_mixing_, sample_rate, &resampler_, Check return value? https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:16: #include "webrtc/base/logging.h" Logging is not required here. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:69: AudioMixer* mixer_; This should be a scoped_refptr https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:70: AudioFrame frame_for_mixing_; nit: how about "mixed_frame_"?
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12539) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20106) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18806) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16215)
Responses to comments and new patch set. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:156: RTC_DCHECK_RUN_ON(&thread_checker_); On 2016/11/14 20:03:46, the sun wrote: > I don't like that some methods in this class use > RTC_DCHECK_RUN_ON(&thread_checker_); > and some use > RTC_DCHECK(thread_checker_.CalledOnValidThread()); > > Please make it consistent. Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:164: RTC_NOTREACHED() << "AudioReceiveStream::Start failed with error: " On 2016/11/14 20:03:46, the sun wrote: > Why did you change this? We can conceivably end up here at run time, so this > should still be logging, not "NOTREACHED" Point, that was wrong. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:169: playing_ = true; On 2016/11/14 20:03:46, the sun wrote: > Note that you're violating the AudioMixer promises of never calling > RemoveSource() unless the source was successfully added. You need to sort out > the logic and and the missing test case. Acknowledged. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:188: RTC_NOTREACHED() << "Failed to remove stream from mixer."; On 2016/11/14 20:03:46, the sun wrote: > If the return code is *supposed* to always be true, what's the point of it. > Change the interface. For AudioMixerImpl it's supposed to always be true. What about other implementations? I thought an AddSource() return code of 'false' allowed other impls that way have more complex initialization to signal failure. But then asserting that it will never happen here is wrong... I'll ask the interested parties about changing the interface. We shouldn't land this before we've sorted it out of course. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:314: return static_cast<internal::AudioState*>(audio_state_.get()); On 2016/11/14 20:03:46, the sun wrote: > Pull into a local variable and DCHECK it's not null before cast+return. Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:318: internal::AudioState* the_audio_state = audio_state(); On 2016/11/14 20:03:46, the sun wrote: > Make that a one-liner instead Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:14: #include "webrtc/base/refcountedobject.h" On 2016/11/14 20:03:46, the sun wrote: > why? Not needed; removed! https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:18: int Resample(const AudioFrame& frame, On 2016/11/14 20:03:47, the sun wrote: > I'd find this order more intuitive: > > source_frame, > resampler > destination_sample_rate > destination > > Try sticking to either of "target", destination, "dest" or "dst". I was trying to follow the style guide for parameter ordering. First come the input-only vars, then both input and output and lastly output. The resampler is both input/output because it changes state. On the other hand the destination sample rate and destination are two parameters that both describe the 'output'. But making a whole new struct for that seems unnecessary... https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:22: const int frame_sample_rate = frame.sample_rate_hz_; On 2016/11/14 20:03:46, the sun wrote: > Remove this local Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:30: static_cast<int16_t*>(destination), On 2016/11/14 20:03:47, the sun wrote: > cast not needed Acknowledged. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:39: RTC_DCHECK(apm); On 2016/11/14 20:03:46, the sun wrote: > nit: use same order as ctor init list Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:89: static_cast<int16_t*>(audioSamples)); On 2016/11/14 20:03:46, the sun wrote: > DCHECK that nSamplesOut is nSamples? Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:111: RTC_DCHECK_EQ(static_cast<size_t>(bits_per_sample), 8 * sizeof(int16_t)); On 2016/11/14 20:03:46, the sun wrote: > Heh, it's called int16 for a reason. > http://en.cppreference.com/w/cpp/types/integer I hoped it helped with reading the code. I added comments instead. Hope it looks better! https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:123: Resample(frame_for_mixing_, sample_rate, &resampler_, On 2016/11/14 20:03:47, the sun wrote: > Check return value? Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:16: #include "webrtc/base/logging.h" On 2016/11/14 20:03:47, the sun wrote: > Logging is not required here. Removed. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:69: AudioMixer* mixer_; On 2016/11/14 20:03:47, the sun wrote: > This should be a scoped_refptr Done. https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:70: AudioFrame frame_for_mixing_; On 2016/11/14 20:03:47, the sun wrote: > nit: how about "mixed_frame_"? Done.
https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:18: int Resample(const AudioFrame& frame, On 2016/11/15 16:56:54, aleloi wrote: > On 2016/11/14 20:03:47, the sun wrote: > > I'd find this order more intuitive: > > > > source_frame, > > resampler > > destination_sample_rate > > destination > > > > Try sticking to either of "target", destination, "dest" or "dst". > > I was trying to follow the style guide for parameter ordering. First come the > input-only vars, then both input and output and lastly output. The resampler is > both input/output because it changes state. > > On the other hand the destination sample rate and destination are two parameters > that both describe the 'output'. But making a whole new struct for that seems > unnecessary... Uhm, ok. Thanks for educating me. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (left): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:147: channel_proxy_->DisassociateSendChannel(); Bad merge https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:184: dd https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:190: auto* const the_audio_state = audio_state(); Don't need this local https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:336: auto* voice_engine = audio_state()->voice_engine(); Yes, nicer! :) https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:122: // EXPECT_CALL(*channel_proxy_, DisassociateSendChannel()).Times(1); Uncomment https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:21: class AudioStateAudioPathTest : public testing::Test { Please avoid fixtures. Like in the stream tests, make a little helper class that you put on the stack in the test cases instead. This usually makes the code easier to follow (everything you need to know is right in front of you - no magic initialization happening before the test function runs, etc). In this specific instance, you gain nothing by inheriting from testing::Test, which is a sign that the dependency is not needed. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:71: RTC_DCHECK_EQ(2 * nChannels, nBytesPerSample); Here I think sizeof(int16_t) made the code more readable. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:114: // 8 * 2 = bits per byte * bytes in sample. Sorry for not being clear. I thought you could just DCHECK against "16" instead of calculating the number of bits in an int16_t ... I don't think a comment would be needed then. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:121: // 100 = 1 second / data duration. 1 s / 10 ms ?
On 2016/11/16 20:22:58, the sun wrote: > https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... > File webrtc/audio/audio_transport_proxy.cc (right): > > https://codereview.webrtc.org/2436033002/diff/600001/webrtc/audio/audio_trans... > webrtc/audio/audio_transport_proxy.cc:18: int Resample(const AudioFrame& frame, > On 2016/11/15 16:56:54, aleloi wrote: > > On 2016/11/14 20:03:47, the sun wrote: > > > I'd find this order more intuitive: > > > > > > source_frame, > > > resampler > > > destination_sample_rate > > > destination > > > > > > Try sticking to either of "target", destination, "dest" or "dst". > > > > I was trying to follow the style guide for parameter ordering. First come the > > input-only vars, then both input and output and lastly output. The resampler > is > > both input/output because it changes state. > > > > On the other hand the destination sample rate and destination are two > parameters > > that both describe the 'output'. But making a whole new struct for that seems > > unnecessary... > > Uhm, ok. Thanks for educating me. > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > File webrtc/audio/audio_receive_stream.cc (left): > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > webrtc/audio/audio_receive_stream.cc:147: > channel_proxy_->DisassociateSendChannel(); > Bad merge > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > File webrtc/audio/audio_receive_stream.cc (right): > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > webrtc/audio/audio_receive_stream.cc:184: > dd > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > webrtc/audio/audio_receive_stream.cc:190: auto* const the_audio_state = > audio_state(); > Don't need this local > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > webrtc/audio/audio_receive_stream.cc:336: auto* voice_engine = > audio_state()->voice_engine(); > Yes, nicer! :) > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > File webrtc/audio/audio_receive_stream_unittest.cc (right): > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... > webrtc/audio/audio_receive_stream_unittest.cc:122: // > EXPECT_CALL(*channel_proxy_, DisassociateSendChannel()).Times(1); > Uncomment > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... > File webrtc/audio/audio_state_audio_path_unittest.cc (right): > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... > webrtc/audio/audio_state_audio_path_unittest.cc:21: class > AudioStateAudioPathTest : public testing::Test { > Please avoid fixtures. Like in the stream tests, make a little helper class that > you put on the stack in the test cases instead. This usually makes the code > easier to follow (everything you need to know is right in front of you - no > magic initialization happening before the test function runs, etc). > > In this specific instance, you gain nothing by inheriting from testing::Test, > which is a sign that the dependency is not needed. > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... > File webrtc/audio/audio_transport_proxy.cc (right): > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... > webrtc/audio/audio_transport_proxy.cc:71: RTC_DCHECK_EQ(2 * nChannels, > nBytesPerSample); > Here I think sizeof(int16_t) made the code more readable. > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... > webrtc/audio/audio_transport_proxy.cc:114: // 8 * 2 = bits per byte * bytes in > sample. > Sorry for not being clear. I thought you could just DCHECK against "16" instead > of calculating the number of bits in an int16_t ... I don't think a comment > would be needed then. > > https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... > webrtc/audio/audio_transport_proxy.cc:121: // 100 = 1 second / data duration. > 1 s / 10 ms ? Just sort out the RemoveSource() interface and you're pretty much done!
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10379)
Patchset #7 (id:640001) has been deleted
Patchset #8 (id:680001) has been deleted
New version with fixtures removed and AudioReceiveState unit tests added. A dependent CL for fixing the return value of ::RemoveSource is added. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (left): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:147: channel_proxy_->DisassociateSendChannel(); On 2016/11/16 20:22:57, the sun wrote: > Bad merge Good spot! Thanks! https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:190: auto* const the_audio_state = audio_state(); On 2016/11/16 20:22:57, the sun wrote: > Don't need this local Acknowledged. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:122: // EXPECT_CALL(*channel_proxy_, DisassociateSendChannel()).Times(1); On 2016/11/16 20:22:57, the sun wrote: > Uncomment Done. Still wondering what happened when I uploaded the commented line :) https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:21: class AudioStateAudioPathTest : public testing::Test { On 2016/11/16 20:22:57, the sun wrote: > Please avoid fixtures. Like in the stream tests, make a little helper class that > you put on the stack in the test cases instead. This usually makes the code > easier to follow (everything you need to know is right in front of you - no > magic initialization happening before the test function runs, etc). > > In this specific instance, you gain nothing by inheriting from testing::Test, > which is a sign that the dependency is not needed. I've removed the fixture and moved the code that extracts the transport proxy out to the unit tests. It is a complicated piece of code that seems to be hard to hide behind an abstraction. I think it's (marginally) simpler to understand what the test does this way. https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:114: // 8 * 2 = bits per byte * bytes in sample. On 2016/11/16 20:22:57, the sun wrote: > Sorry for not being clear. I thought you could just DCHECK against "16" instead > of calculating the number of bits in an int16_t ... I don't think a comment > would be needed then. Ah, of course... https://codereview.webrtc.org/2436033002/diff/620001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:121: // 100 = 1 second / data duration. On 2016/11/16 20:22:57, the sun wrote: > 1 s / 10 ms ? Acknowledged. https://codereview.webrtc.org/2436033002/diff/660001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.h (right): https://codereview.webrtc.org/2436033002/diff/660001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.h:69: int SetVoiceEnginePlayout(bool playout); Added method for VoE->StartPlayout() / StopPlayout() to use in Start() and Stop(). https://codereview.webrtc.org/2436033002/diff/660001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/660001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:76: audio_mixer_(AudioMixerImpl::Create()) { Changed the helper method to save a pointer to the mixer and provide a getter to it. It's used in the two new tests below.
A few nits and one question - lgtm if fixed. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:169: if (!the_audio_state->mixer()->AddSource(this)) { nit: make it a one-liner https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:180: dd https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:187: dd https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:50: private: https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:110: EXPECT_CALL(helper.voice_engine(), audio_device_module()).Times(2); Are you sure you couldn't have line 110-123 in the helper's ctor and add a helper to get audio_transport_proxy()?
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/...
New patch set. Issues fixed. Running all tests including chrome and internal projects now. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:169: if (!the_audio_state->mixer()->AddSource(this)) { On 2016/11/18 16:30:17, the sun wrote: > nit: make it a one-liner Done. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:180: On 2016/11/18 16:30:17, the sun wrote: > dd Remove empty line? If so, done. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:187: On 2016/11/18 16:30:17, the sun wrote: > dd Same here. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:50: On 2016/11/18 16:30:17, the sun wrote: > private: Done. https://codereview.webrtc.org/2436033002/diff/700001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:110: EXPECT_CALL(helper.voice_engine(), audio_device_module()).Times(2); On 2016/11/18 16:30:17, the sun wrote: > Are you sure you couldn't have line 110-123 in the helper's ctor and add a > helper to get audio_transport_proxy()? Done. I was initially reluctant to do that: I thought we were hiding what the tests does somewhere else. But I didn't like the code duplication either.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10445)
You're getting there! I only have a couple of minor issues. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:384: EXPECT_EQ(helper.audio_mixer()->GetSourcePresenceStatusForTest(&recv_stream), Eugh! I don't like that there's an extra method on the mixer just for testing, like this. :/ Fortunately it's not on the interface. I think this could be solved as easily with a mock, where AddSource() emits a failure when called in this version. In the "should be called" version, I guess it would set or increase a variable and the value of that variable is then part of an EXPECT check. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:1: /* As we spoke about offline, I don't think you should add a new file for these two unit tests. It's not how other unit tests in WebRTC are laid out, and I think you could easily merge the functionality in the two files. Also, they're pretty small, so there's no mental overhead in having them all in the same file. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:77: // 100 = 1 second / data duration. Maybe: 100 = 1 second / data duration (10 ms) there's one below that should be changed as well. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:119: // 100 = 1 s / 10 ms. Right here!
https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:62: MockVoiceEngine& voice_engine() { return mock_voice_engine; } nit: this doesn't strictly need to be public...
Hi! I've added another patch set. Do you think there is something particular to keep in mind before landing? https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... File webrtc/audio/audio_state_audio_path_unittest.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_state... webrtc/audio/audio_state_audio_path_unittest.cc:1: /* On 2016/11/21 17:07:32, ossu wrote: > As we spoke about offline, I don't think you should add a new file for these two > unit tests. It's not how other unit tests in WebRTC are laid out, and I think > you could easily merge the functionality in the two files. Also, they're pretty > small, so there's no mental overhead in having them all in the same file. Done. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.cc (right): https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:77: // 100 = 1 second / data duration. On 2016/11/21 17:07:32, ossu wrote: > Maybe: > 100 = 1 second / data duration (10 ms) > > there's one below that should be changed as well. Done. https://codereview.webrtc.org/2436033002/diff/720001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.cc:119: // 100 = 1 s / 10 ms. On 2016/11/21 17:07:32, ossu wrote: > Right here! Done.
LGTM. Always look both ways before landing!
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any (large) modifications in VoiceEngine. The AudioTransport that connected the old mixer to the AudioDevice is disconnected from AudioDevice and replaced by another one. AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. AudioReceiveStream registers itself with the mixer (which it gets from AudioState) on Start and Stop. BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. BUG=webrtc:6346 ==========
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. # Since the AudioTransport interface contains non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ==========
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 Link to the patchset: https://codereview.webrtc.org/2436033002/#ps740001 (title: "Added mock mixer, merged audio state tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 740001, "attempt_start_ts": 1479824699089390,
"parent_rev": "62c3865703ae2fcb7abbea430d61fb9d61adde06", "commit_rev":
"e28ef1a874d67bd4fb5fb451f9b4bb5629936841"}
Message was sent while issue was closed.
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. # Since the AudioTransport interface contains non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. # Since the AudioTransport interface contains non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:740001)
Message was sent while issue was closed.
Description was changed from ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. # Since the AudioTransport interface contains non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ========== to ========== Replace AudioConferenceMixer with AudioMixer. This CL re-routes audio through AudioMixer instead of AudioConferenceMixer. This is done without any modifications to VoiceEngine. Previously, output audio was polled by an AudioDevice through an AudioTransport pointer, which was an instance of VoEBaseImpl. VoiceEngineImpl sent the request for data on to OutputMixer and further to AudioConferenceMixer. This CL changes the audio flow to an AudioDevice. We reconfigure the AudioDevice to have another AudioTransport pointer, which points to an AudioTransportProxy. The AudioTransportProxy is responsible for feeding mixed data to the AudioProcessing component for echo cancellation, and to resample the audio data after AudioProcessing and before it is sent to the AudioDevice. The set up of the audio path was previously done during VoiceEngine initialization. Now it is changed in the AudioState constructor. This list shows where audio-path-related VoiceEngine functionality has been moved: OutputMixer --> AudioTransportProxy VoiceEngineImpl --> AudioState, AudioTransportProxy SharedData --> AudioState Channel --> AudioReceiveStream, ChannelProxy, Channel AudioState owns the new mixer and connects it to AudioTransport and AudioDevice on initialization. The audio input source is AudioReceiveStream, which registers itself with the mixer (which it gets from AudioState) on Start and Stop. # Since the AudioTransport interface contains non-const references. NOPRESUBMIT=True BUG=webrtc:6346 Committed: https://crrev.com/04c0722cf1fc38e6888ef44e413603e0cb461ea4 Cr-Commit-Position: refs/heads/master@{#15193} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/04c0722cf1fc38e6888ef44e413603e0cb461ea4 Cr-Commit-Position: refs/heads/master@{#15193} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
