Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454373002:
Added an empty AudioTransportProxy to AudioState. (Closed)
DescriptionAdded an empty AudioTransportProxy to AudioState.
All audio in calls is now routed through AudioTransportProxy. The
AudioTransport implemented by VoEBaseImpl is disconnected from
AudioDevice and replaced by an empty proxy layer that forwards calls
to the old Transport. This is a refactoring CL in preparation for
landing https://codereview.webrtc.org/2436033002/, which will connect
the new AudioMixer.
In the planned configuration, the currently empty AudioTransportProxy
will query the new mixer for audio instead of polling data from the
old Transport. Mixed audio will be passed to an AudioProcessing
interface. AudioTransportProxy is initialized with an AudioProcessing*,
which is currently unused.
No presubmit since we implement an interface with non-const references.
NOPRESUBMIT=True
BUG=webrtc:6346
Committed: https://crrev.com/dd31071e19bdfe0bedc335f070e1592cfd5d2eb6
Cr-Commit-Position: refs/heads/master@{#15133}
Patch Set 1 #
Total comments: 18
Patch Set 2 : No heap transport, WillOnce, comparison with constants. #
Total comments: 23
Patch Set 3 : Added unit test for recorded data path. #
Total comments: 18
Patch Set 4 : Removed logic non-const, moved all proxy defs to .cc, removed unused mocks. #Patch Set 5 : Rebase. GYP is removed! #
Dependent Patchsets: Messages
Total messages: 62 (42 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #2 (id:160001) has been deleted
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + ossu@webrtc.org, solenberg@webrtc.org
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + henrika@webrtc.org - ossu@webrtc.org, solenberg@webrtc.org
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #1 (id:140001) has been deleted
aleloi@webrtc.org changed reviewers: + ossu@webrtc.org, solenberg@webrtc.org
Hi! Can you please take a look at this CL? @ossu, this is one of the pieces left of https://codereview.webrtc.org/2436033002/ after I broke it up in parts. @henrika, I primarily want you to look at the single line change in webrtc/test/mock_voice_engine.h. If you want to see the context of this change, I've communicated it through other means. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:56: } Caused by including gmock file. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/BUILD.gn#new... webrtc/call/BUILD.gn:25: Small unrelated change. Quote from "gn help public_deps": Generally if you are writing a target B and you include C's headers as part of B's public headers, or targets depending on B should consider B and C to be part of a unit, you should use public_deps instead of deps. B="call", C="call_api".
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org TBR=kwiberg@webrtc.org # for webrtc/test/mock_voice_engine.h BUG=webrtc:6346 ==========
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org TBR=kwiberg@webrtc.org # for webrtc/test/mock_voice_engine.h BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org BUG=webrtc:6346 ==========
https://codereview.webrtc.org/2454373002/diff/220001/webrtc/test/mock_voice_e... File webrtc/test/mock_voice_engine.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/test/mock_voice_e... webrtc/test/mock_voice_engine.h:114: MOCK_METHOD0(audio_device_module, AudioDeviceModule*()); It is not clear to me why this method was added in the main interface. Is it essential to mock it?
https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:82: EXPECT_CALL(voice_engine_, audio_transport()); Don't these require actions, i.e. WillOnce(Return(nullptr)) or something? https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:186: testing::NiceMock<webrtc::test::MockAudioDeviceModule> mock_audio_device_; What's so nice about it!? :) Why not just use an unadorned MockAudioDeviceModule? Are there millions of calls you don't want to implement actions for? https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:37: rtc::scoped_refptr<AudioMixer> mixer() const; I believe this should just return an AudioMixer& (or possibly an AudioMixer*, but can it ever be null?) C.f. the discussion in the previous CL about unique_ptr. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:68: std::unique_ptr<AudioTransportProxy> audio_transport_proxy_; Why does the AudioTransportProxy need to be allocated on the heap? https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:62: RTC_DCHECK_LE(1u, nChannels); I'd prefer if the parameters in the two nChannels checks were in the same order, i.e.: RTC_DCHECK_GE(nChannels, 1u); RTC_DCHECK_LE(nChannels, 2u); I think it's easier to read, though the current formulation is maybe closer to the more mathematical notation of: 1 <= nChannels <= 2. Actually, I'd prefer even more if it were possible to check for both these values in one line, without losing the diagnostic printouts. Can't think of a reasonable way of doing that, though. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:64: RTC_DCHECK_LE( Alright, so maybe a bit nitpicky, but don't we usually put the thing we're comparing on the lhs of expressions? i.e. if (some_variable < 5) rather than (5 > some_variable)? In any case, I think that makes it easier to read and the same applies for these checks. RTC_DCHECK_GE(samplesPerSec, ...) etc.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/bitrate_esti... File webrtc/call/bitrate_estimator_tests.cc (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:114: .WillByDefault(Return(&mock_audio_device_)); Could you try to remove everything audio related here instead? I don't think it uses it any longer.
I've uploaded a new PS and responded to comments. Please take another look! https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:82: EXPECT_CALL(voice_engine_, audio_transport()); On 2016/11/03 14:02:04, ossu wrote: > Don't these require actions, i.e. WillOnce(Return(nullptr)) or something? I think the default action (which is Return(nullptr)) runs if you don't specify anything. I replaced the ON_CALL below with WillOnce for audio_device_module to make it shorter and more consistent. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:186: testing::NiceMock<webrtc::test::MockAudioDeviceModule> mock_audio_device_; On 2016/11/03 14:02:04, ossu wrote: > What's so nice about it!? :) > Why not just use an unadorned MockAudioDeviceModule? Are there millions of calls > you don't want to implement actions for? Probably an ordinary mock make sense here, because of the way |audio_device| is used in AudioState (only one method is called). I read this quote from the mock cook book: Finally, you should be very cautious about when to use naggy or strict mocks, as they tend to make tests more brittle and harder to maintain. When you refactor your code without changing its externally visible behavior, ideally you should't need to update any tests. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:37: rtc::scoped_refptr<AudioMixer> mixer() const; On 2016/11/03 14:02:04, ossu wrote: > I believe this should just return an AudioMixer& (or possibly an AudioMixer*, > but can it ever be null?) C.f. the discussion in the previous CL about > unique_ptr. We decided to leave the mixer refcounted for the time being. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:68: std::unique_ptr<AudioTransportProxy> audio_transport_proxy_; On 2016/11/03 14:02:04, ossu wrote: > Why does the AudioTransportProxy need to be allocated on the heap? You are right, it doesn't! But now initialization of AudioState is a little different and happens in another order. In an earlier iteration of this CL the transport was not always initialized. Now that I changed the tests to always provide a transport it is and we don't need a pointer. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:62: RTC_DCHECK_LE(1u, nChannels); On 2016/11/03 14:02:04, ossu wrote: > I'd prefer if the parameters in the two nChannels checks were in the same order, > i.e.: > RTC_DCHECK_GE(nChannels, 1u); > RTC_DCHECK_LE(nChannels, 2u); > I think it's easier to read, though the current formulation is maybe closer to > the more mathematical notation of: 1 <= nChannels <= 2. > > Actually, I'd prefer even more if it were possible to check for both these > values in one line, without losing the diagnostic printouts. Can't think of a > reasonable way of doing that, though. Done. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:64: RTC_DCHECK_LE( On 2016/11/03 14:02:04, ossu wrote: > Alright, so maybe a bit nitpicky, but don't we usually put the thing we're > comparing on the lhs of expressions? i.e. if (some_variable < 5) rather than (5 > > some_variable)? In any case, I think that makes it easier to read and the same > applies for these checks. RTC_DCHECK_GE(samplesPerSec, ...) etc. I made comparisons to constants consistent in the next PS. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/bitrate_esti... File webrtc/call/bitrate_estimator_tests.cc (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/call/bitrate_esti... webrtc/call/bitrate_estimator_tests.cc:114: .WillByDefault(Return(&mock_audio_device_)); On 2016/11/03 14:33:53, stefan-webrtc (holmer) wrote: > Could you try to remove everything audio related here instead? I don't think it > uses it any longer. I've done it in another CL; I'll rebase this one as soon as https://codereview.webrtc.org/2479383003/has landed. https://codereview.webrtc.org/2454373002/diff/220001/webrtc/test/mock_voice_e... File webrtc/test/mock_voice_engine.h (right): https://codereview.webrtc.org/2454373002/diff/220001/webrtc/test/mock_voice_e... webrtc/test/mock_voice_engine.h:114: MOCK_METHOD0(audio_device_module, AudioDeviceModule*()); On 2016/11/02 10:02:06, henrika_webrtc wrote: > It is not clear to me why this method was added in the main interface. Is it > essential to mock it? I am now accessing the AudioDevice from the constructor of AudioState: one AudioTransportProxy gets replaced with another one. When an AudioState is constructed in a unit test, the default behavior was to pass a mock VoiceEngine with an unmocked audio_device_module(), which returns nullptr. One way to solve it is to have a special case for when |device| is nullptr in AudioState. That would lead to more conditional branches of execution and more complex code, so we decided to change the tests by providing a mock VoE in which audio_device_module returns a mock audio device. I also plan to write more unit tests for AudioState which will test the new functionality by using the audio_device_module.
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. TBR=solenberg@webrtc.org BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. BUG=webrtc:6346 ==========
LGTM on webrtc/test/mock_voice_engine.h
Patchset #2 (id:240001) has been deleted
https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:57: if (is_win) { Where do you get this warning? Is there any way we can work around it without adding clutter to the build configs? https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/DEPS File webrtc/audio/DEPS (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/DEPS#newcode5 webrtc/audio/DEPS:5: "+webrtc/modules/audio_device", Could you clean up the order and redundancy issues here? https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:34: device->RegisterAudioCallback(nullptr); Is this really necessary? In AudioDeviceBuffer::RegisterAudioCallback() it doesn't look like that, but perhaps other platforms implement it in different ways? https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:48: AudioDeviceModule* AudioState::audio_device() { Only used in one place. Please make the call directly in the ctor instead. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:14: #include <memory> not needed? https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:23: #include "webrtc/modules/audio_device/include/audio_device_defines.h" No need. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:49: AudioDeviceModule* audio_device(); Remove this method. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:23: AudioTransportProxy(AudioTransport* voe_audio_transport, Make a .cc file for this - this is too much implementation to keep in the header. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:24: AudioProcessing* apm, So, hooking up APM and mixer will be in a follow-up CL? Could you at least DCHECK the pointers in the ctor here? https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:41: if (voe_audio_transport_) { So this is to not have to set up the transport in unit tests? I have an idea of how we can avoid clutter in the tests and make this code unambiguous - see mock_voice_engine.h. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/test/mock_voice_e... File webrtc/test/mock_voice_engine.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/test/mock_voice_e... webrtc/test/mock_voice_engine.h:38: // will create a NiceMock of a voe::ChannelProxy. How about we do the same for ADM, APM and transport? The would reduce clutter in the unit tests and you could remove the null checks in AudioTransportProxy. ON_CALL(*this, audio_, audio_processing()) .WillByDefault(testing::Return(&mock_audio_processing)); ... testing::NiceMock<webrtc::test::MockAudioProcessing> mock_audio_processing;
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/9992)
https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:57: if (is_win) { On 2016/11/09 09:41:52, the sun wrote: > Where do you get this warning? Is there any way we can work around it without > adding clutter to the build configs? I don't think so, but I'll investigate: the code that results in the warning originates somewhere in gmock. I've commented this out in the meantime to look at the compilation error once again. What we probably could (and should) do is to create a target for the mock dependency and move the flag to it. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/DEPS File webrtc/audio/DEPS (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/DEPS#newcode5 webrtc/audio/DEPS:5: "+webrtc/modules/audio_device", On 2016/11/09 09:41:52, the sun wrote: > Could you clean up the order and redundancy issues here? Done. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:34: device->RegisterAudioCallback(nullptr); On 2016/11/09 09:41:52, the sun wrote: > Is this really necessary? In AudioDeviceBuffer::RegisterAudioCallback() it > doesn't look like that, but perhaps other platforms implement it in different > ways? It's needed in Chrome. WebRtcAudioDeviceImpl::RegisterAudioCallback contains this line: DCHECK_EQ(audio_transport_callback_ == NULL, audio_callback != NULL); https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:48: AudioDeviceModule* AudioState::audio_device() { On 2016/11/09 09:41:52, the sun wrote: > Only used in one place. Please make the call directly in the ctor instead. Done. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:14: #include <memory> On 2016/11/09 09:41:52, the sun wrote: > not needed? Done. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:23: #include "webrtc/modules/audio_device/include/audio_device_defines.h" On 2016/11/09 09:41:52, the sun wrote: > No need. You're right! Removed. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:49: AudioDeviceModule* audio_device(); On 2016/11/09 09:41:52, the sun wrote: > Remove this method. Right, because we removed the device from AudioState::Config and only read it from the voe pointer. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:23: AudioTransportProxy(AudioTransport* voe_audio_transport, On 2016/11/09 09:41:52, the sun wrote: > Make a .cc file for this - this is too much implementation to keep in the > header. I've already done it in the dependent CL https://codereview.webrtc.org/2436033002 that connects the mixer. I moved the change here, because the whole point was to divide that CL into several smaller ones so that turning on the new mixer is as simple as possible. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:24: AudioProcessing* apm, On 2016/11/09 09:41:53, the sun wrote: > So, hooking up APM and mixer will be in a follow-up CL? > > Could you at least DCHECK the pointers in the ctor here? I have a check for the mixer in the next dependent CL https://codereview.webrtc.org/2469743002. I've added a check for apm and transport in this one. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:41: if (voe_audio_transport_) { On 2016/11/09 09:41:53, the sun wrote: > So this is to not have to set up the transport in unit tests? I have an idea of > how we can avoid clutter in the tests and make this code unambiguous - see > mock_voice_engine.h. Acknowledged. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/test/mock_voice_e... File webrtc/test/mock_voice_engine.h (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/test/mock_voice_e... webrtc/test/mock_voice_engine.h:38: // will create a NiceMock of a voe::ChannelProxy. On 2016/11/09 09:41:53, the sun wrote: > How about we do the same for ADM, APM and transport? The would reduce clutter in > the unit tests and you could remove the null checks in AudioTransportProxy. > > ON_CALL(*this, audio_, audio_processing()) > .WillByDefault(testing::Return(&mock_audio_processing)); > > ... > > testing::NiceMock<webrtc::test::MockAudioProcessing> mock_audio_processing; Acknowledged. https://codereview.webrtc.org/2454373002/diff/280001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/280001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:83: .WillOnce(Return(0)); reordered alphabetically.
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
Don't think my review is needed any longer, removing myself. :)
stefan@webrtc.org changed reviewers: - stefan@webrtc.org
aleloi@webrtc.org changed reviewers: - henrika@webrtc.org
Patchset #5 (id:360001) has been deleted
Patchset #4 (id:340001) has been deleted
Hi! Can you take another look? I moved the transport, device and processing stubs into mock_voice_engine. I also removed the windows warning suppression. https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2454373002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:57: if (is_win) { On 2016/11/09 09:41:52, the sun wrote: > Where do you get this warning? Is there any way we can work around it without > adding clutter to the build configs? Solved in https://codereview.webrtc.org/2492713003 ! I added a target |audio_device:mock_audio_device| for the mock dependency and a config |audio_device:mock_audio_device_config| in that target that is applied to everything that depends on the mock audio device.
Patchset #4 (id:380001) has been deleted
Patchset #3 (id:280001) has been deleted
Just nits basically... LG https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:81: EXPECT_CALL(voice_engine_, audio_processing()); Q: Is .Times(1) the default? https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:202: MockAudioDeviceModule mock_audio_device_; Not needed https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state.cc File webrtc/audio/audio_state.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.cc:34: device->RegisterAudioCallback(nullptr); Please add a comment about this being necessary for how Chrome's ADM is implemented. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:14: #include "webrtc/api/audio/audio_mixer.h" This include should be in audio_state.h instead, right? https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:34: rtc::scoped_refptr<AudioMixer> mixer() const; Remove const - you're returning a non-const pointer to an object we own. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... File webrtc/audio/audio_state_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state_unittest.cc:112: RecordedDataIsAvailable(nullptr, 80, 2, 1, 8000, 0, 0, 0, false, Do you need to test the other forwarding calls as well? https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:26: : voe_audio_transport_(voe_audio_transport) { Move to .cc https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:44: return voe_audio_transport_->RecordedDataIsAvailable( Move to .cc https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:66: RTC_NOTREACHED(); Here as well
New PS and responses to comments. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:81: EXPECT_CALL(voice_engine_, audio_processing()); On 2016/11/14 13:50:08, the sun wrote: > Q: Is .Times(1) the default? Yes, the default is 1 when there is no .WillOnce or .WillRepeatedly ref: https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m... https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:202: MockAudioDeviceModule mock_audio_device_; On 2016/11/14 13:50:08, the sun wrote: > Not needed Acknowledged. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:14: #include "webrtc/api/audio/audio_mixer.h" On 2016/11/14 13:50:08, the sun wrote: > This include should be in audio_state.h instead, right? I'm not sure. I think there is a similar relationship between class header and base class' header as between .cc and related .h. Here's what the style guide says: You should include all the headers that define the symbols you rely upon <...> However, any includes present in the related header do not need to be included again in the related cc https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:34: rtc::scoped_refptr<AudioMixer> mixer() const; On 2016/11/14 13:50:08, the sun wrote: > Remove const - you're returning a non-const pointer to an object we own. Makes sense! I think it's called logical constness. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... File webrtc/audio/audio_state_unittest.cc (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state_unittest.cc:112: RecordedDataIsAvailable(nullptr, 80, 2, 1, 8000, 0, 0, 0, false, On 2016/11/14 13:50:08, the sun wrote: > Do you need to test the other forwarding calls as well? I don't think so. The other calls are NeedMorePlayData, PullRenderData and PushCaptureData. The first two are changed in the next CLs and will stop forwarding. PushCaptureData is deprecated and has a 'RTC_NOTREACHED()'. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... File webrtc/audio/audio_transport_proxy.h (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:26: : voe_audio_transport_(voe_audio_transport) { On 2016/11/14 13:50:09, the sun wrote: > Move to .cc For clarity to have all defs in the same place? (Done). https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:44: return voe_audio_transport_->RecordedDataIsAvailable( On 2016/11/14 13:50:09, the sun wrote: > Move to .cc Done. https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_trans... webrtc/audio/audio_transport_proxy.h:66: RTC_NOTREACHED(); On 2016/11/14 13:50:08, the sun wrote: > Here as well Done.
Patchset #5 (id:430001) has been deleted
lgtm https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state.h File webrtc/audio/audio_state.h (right): https://codereview.webrtc.org/2454373002/diff/390001/webrtc/audio/audio_state... webrtc/audio/audio_state.h:14: #include "webrtc/api/audio/audio_mixer.h" On 2016/11/14 14:24:42, aleloi wrote: > On 2016/11/14 13:50:08, the sun wrote: > > This include should be in audio_state.h instead, right? > > I'm not sure. I think there is a similar relationship between class header and > base class' header as between .cc and related .h. Here's what the style guide > says: > > > You should include all the headers that define the symbols you rely upon <...> > However, any includes present in the related header do not need to be included > again in the related cc Right, so by that logic, since the base class' header relies on AudioMixer and pulls in the header, there's no need to do it here.
Patchset #5 (id:450001) has been deleted
Alright. LGTM!
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2454373002/#ps470001 (title: "Rebase. GYP is removed!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Wohoo! Landing time!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10366)
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. No presubmit since we implement an interface with non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ==========
The CQ bit was checked by aleloi@webrtc.org
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 ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. No presubmit since we implement an interface with non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. No presubmit since we implement an interface with non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. No presubmit since we implement an interface with non-const references. NOPRESUBMIT=True BUG=webrtc:6346 ========== to ========== Added an empty AudioTransportProxy to AudioState. All audio in calls is now routed through AudioTransportProxy. The AudioTransport implemented by VoEBaseImpl is disconnected from AudioDevice and replaced by an empty proxy layer that forwards calls to the old Transport. This is a refactoring CL in preparation for landing https://codereview.webrtc.org/2436033002/, which will connect the new AudioMixer. In the planned configuration, the currently empty AudioTransportProxy will query the new mixer for audio instead of polling data from the old Transport. Mixed audio will be passed to an AudioProcessing interface. AudioTransportProxy is initialized with an AudioProcessing*, which is currently unused. No presubmit since we implement an interface with non-const references. NOPRESUBMIT=True BUG=webrtc:6346 Committed: https://crrev.com/dd31071e19bdfe0bedc335f070e1592cfd5d2eb6 Cr-Commit-Position: refs/heads/master@{#15133} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dd31071e19bdfe0bedc335f070e1592cfd5d2eb6 Cr-Commit-Position: refs/heads/master@{#15133} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
