Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753453002:
Adding AudioDeviceDataObserver interface (Closed)
|
Created:
3 years, 9 months ago by lliuu Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, sdk-team_agora.io, peah-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding AudioDeviceDataObserver interface
Added new callback class that enables routing both the locally recorded
and playout audio data to external module.
A factory method that creates an AudioDeviceModule instance that also
registers the AudioTransportCapture with the device is also added.
BUG=webrtc:7337
R=solenberg@webrtc.org
Review-Url: https://codereview.webrtc.org/2753453002 .
Cr-Commit-Position: refs/heads/master@{#17570}
Committed: https://chromium.googlesource.com/external/webrtc/+/4b62001e4ffcda2033e9b1ca20b396c9f662e364
Patch Set 1 #Patch Set 2 : Fix gn typo #
Total comments: 24
Patch Set 3 : Small changes that address comments; format #Patch Set 4 : Remove some unnecessary comments #
Total comments: 27
Patch Set 5 : review comments from fredrik #
Messages
Total messages: 38 (15 generated)
The CQ bit was checked by lliuu@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/14858)
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Adding AudioTransportCapture class Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. Also, changed the sequence of things in VoEBaseImpl::Init so that AudioTransport is registered after audio processing is created. This prevents crashing in ProcessRecordedDataWithAPM, which does not check whether there is a valid instance for audio processing. BUG= ========== to ========== Adding AudioTransportCapture class Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. Also, changed the sequence of things in VoEBaseImpl::Init so that AudioTransport is registered after audio processing is created. This prevents crashing in ProcessRecordedDataWithAPM, which does not check whether there is a valid instance for audio processing. BUG=webrtc:7337 ==========
The CQ bit was checked by lliuu@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: This issue passed the CQ dry run.
lliuu@webrtc.org changed reviewers: + solenberg@webrtc.org
On 2017/03/13 23:08:06, lliuu wrote: > mailto:lliuu@webrtc.org changed reviewers: > + mailto:solenberg@webrtc.org Hi Fredrik, Please kindly help review this CL. Some finer points: - I didn't expose the wrapper on the API level (as you suggested) but instead created a new factory method that creates the wrapper with internal impl. and register the capture callback automatically. - The changes I made in voe_base_impl.cc might be better fixed by checking for nullptr in ProcessRecordedDataWithAPM, let me know what you think. - The AudioTransportCapture only allows intercepting the PCM data without the ability to modify it on the fly; the reason is that I don't know whether if external customers actually need that ability, also that I'm a bit worried that buffering the data that is recorded locally could potentially causing memory thrashing
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:25: AudioTransportCapture* capture_cb) No need for this extra interface. https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:37: bool IsValid() { When is this useful? https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:59: capture_callback_->CaptureRecordedData(audioSamples, nSamples, No need for this new interface - just pass the call on like you do below. A user would subclass the ADMWrapper: class MyInterceptingADM : public ADMWrapper { public: ... int32_t RecordedDataIsAvailable(...) { // Do what I please with the data // Then call base class. return ADMWrapper::RecordedDataIsAvailable(...); } https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:128: audio_transport_sink_ = audioCallback; audio_transport_sink_ will need to be protected by a lock https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:418: CreateAudioDeviceWithTransportCapture( No need for this create function if we can agree to use sub classing and avoid the AudioTransportCapture interface. https://codereview.webrtc.org/2753453002/diff/20001/webrtc/voice_engine/voe_b... File webrtc/voice_engine/voe_base_impl.cc (left): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/voice_engine/voe_b... webrtc/voice_engine/voe_base_impl.cc:231: // Register the AudioTransport implementation You shouldn't need to move this. Setting the audio callback on an ADM which is already delivering media is not supported by the default ADM implementation. See: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... and https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... (sadly, the interface does not mention this limitation)
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:59: capture_callback_->CaptureRecordedData(audioSamples, nSamples, On 2017/03/21 21:42:50, the sun wrote: > No need for this new interface - just pass the call on like you do below. > > A user would subclass the ADMWrapper: > > class MyInterceptingADM : public ADMWrapper { > public: > ... > int32_t RecordedDataIsAvailable(...) { > // Do what I please with the data > > // Then call base class. > return ADMWrapper::RecordedDataIsAvailable(...); > } > Fredrik, what do you think of adding pure virtual CaptureRecordedData to AdmWrapper, and the user can just provide their own implementation without needing to call base class? In this way we don't have to depend on the user to do the right thing https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:128: audio_transport_sink_ = audioCallback; On 2017/03/21 21:42:50, the sun wrote: > audio_transport_sink_ will need to be protected by a lock Acknowledged. https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:418: CreateAudioDeviceWithTransportCapture( On 2017/03/21 21:42:50, the sun wrote: > No need for this create function if we can agree to use sub classing and avoid > the AudioTransportCapture interface. AudioDeviceModule inherits from RefCountedModule, and I think user might actually want to have a create function that returns a ref counted pointer? I could move it to a static member function AdmWrapper::Create, does that sound like a good solution? https://codereview.webrtc.org/2753453002/diff/20001/webrtc/voice_engine/voe_b... File webrtc/voice_engine/voe_base_impl.cc (left): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/voice_engine/voe_b... webrtc/voice_engine/voe_base_impl.cc:231: // Register the AudioTransport implementation On 2017/03/21 21:42:50, the sun wrote: > You shouldn't need to move this. Setting the audio callback on an ADM which is > already delivering media is not supported by the default ADM implementation. > See: > > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... > and > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... > > (sadly, the interface does not mention this limitation) Oh I see, then the crashing bug should be fixed on client side. Thanks for the clarification!!
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:59: capture_callback_->CaptureRecordedData(audioSamples, nSamples, On 2017/03/22 18:50:30, lliuu wrote: > On 2017/03/21 21:42:50, the sun wrote: > > No need for this new interface - just pass the call on like you do below. > > > > A user would subclass the ADMWrapper: > > > > class MyInterceptingADM : public ADMWrapper { > > public: > > ... > > int32_t RecordedDataIsAvailable(...) { > > // Do what I please with the data > > > > // Then call base class. > > return ADMWrapper::RecordedDataIsAvailable(...); > > } > > > > Fredrik, what do you think of adding pure virtual CaptureRecordedData to > AdmWrapper, and the user can just provide their own implementation without > needing to call base class? In this way we don't have to depend on the user to > do the right thing It also occurred to me that if we're letting the user to override RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure to call the base functions in different orders in those 2 override functions, which might not be intuitive
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:37: bool IsValid() { On 2017/03/21 21:42:50, the sun wrote: > When is this useful? I don't want to end up with the situation where the internal impl_ fails to initialize in AudioDeviceModule::Create and we still return a valid ADMWrapper pointer to the user. This also is relevant regarding whether we want to provide a static Create function below https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:59: capture_callback_->CaptureRecordedData(audioSamples, nSamples, On 2017/03/22 20:34:37, lliuu wrote: > On 2017/03/22 18:50:30, lliuu wrote: > > On 2017/03/21 21:42:50, the sun wrote: > > > No need for this new interface - just pass the call on like you do below. > > > > > > A user would subclass the ADMWrapper: > > > > > > class MyInterceptingADM : public ADMWrapper { > > > public: > > > ... > > > int32_t RecordedDataIsAvailable(...) { > > > // Do what I please with the data > > > > > > // Then call base class. > > > return ADMWrapper::RecordedDataIsAvailable(...); > > > } > > > > > > > Fredrik, what do you think of adding pure virtual CaptureRecordedData to > > AdmWrapper, and the user can just provide their own implementation without > > needing to call base class? In this way we don't have to depend on the user to > > do the right thing > > It also occurred to me that if we're letting the user to override > RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure to > call the base functions in different orders in those 2 override functions, which > might not be intuitive I'd like to clarify the last comment: I meant that if user overrides NeedMorePlayData below, they need to call the base function *before* their custom logic, which might not be obvious to the user https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:128: audio_transport_sink_ = audioCallback; On 2017/03/22 18:50:30, lliuu wrote: > On 2017/03/21 21:42:50, the sun wrote: > > audio_transport_sink_ will need to be protected by a lock > > Acknowledged. Actually could you clarify what is the race condition that needs to be protected by a lock? The audio transport callbacks don't seem to be protected in the default ADM/VOE implementations, and I don't feel there's the need to lock between RegisterAudioCallback and the callback functions?
henrika@webrtc.org changed reviewers: + henrika@webrtc.org
Great work. Any plans to add some sort of unit test? I understand that it can be complex to verify actual/real audio data on all platforms and I don't want to make things too complex (given that the interface is kind of best-effort). https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/include/audio_transport_capture.h (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:23: const void* audioSamples, const size_t nSamples, Can't we use new notation in this interface? audio_samples, samples, bytes_per_sec etc.
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:41: // RefCountedObject methods overrides. How are these related to RefCountedObject? https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:82: // Request data from sink. How can we request data *from* a sink?
On 2017/03/23 09:02:42, henrika_webrtc wrote: > Great work. Any plans to add some sort of unit test? I understand that it can be > complex to verify actual/real audio data on all platforms and I don't want to > make things too complex (given that the interface is kind of best-effort). > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/include/audio_transport_capture.h (right): > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:23: const void* > audioSamples, const size_t nSamples, > Can't we use new notation in this interface? > audio_samples, samples, bytes_per_sec etc. Thanks Henrik, let me look into the unit test part
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:41: // RefCountedObject methods overrides. On 2017/03/23 09:08:52, henrika_webrtc wrote: > How are these related to RefCountedObject? Hi Henrik, AudioDeviceModule inherits from RefCountedObject. I've found this kind of a headache that creating subclasses of ADM also means having overriding RefCountedObject, is there some way we can avoid doing it? https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:82: // Request data from sink. On 2017/03/23 09:08:52, henrika_webrtc wrote: > How can we request data *from* a sink? Good point. Here "sink" means the actual sink for audio transport callback functions, but I can see this could easily lead to confusions. Will rename it https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/include/audio_transport_capture.h (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:23: const void* audioSamples, const size_t nSamples, On 2017/03/23 09:02:42, henrika_webrtc wrote: > Can't we use new notation in this interface? > audio_samples, samples, bytes_per_sec etc. Acknowledged.
This looks good. Initially, I thought the full ADMWrapper should instead be exposed, since that would allow the client more flexibility to handle unforeseen use cases. Also, my thinking was that adding a separate interface for side channeling the audio wouldn't be good. Now, I've mostly changed my mind. This implementation allows only tapping audio, not interfering with any of the other APIs on the class, meaning there is more room to clean those up. I guess an alternative would be to use the AudioTransport interface instead of AudioTransportCapture? I also agree that the name is too long. WDYT? https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:59: capture_callback_->CaptureRecordedData(audioSamples, nSamples, On 2017/03/22 21:26:14, lliuu wrote: > On 2017/03/22 20:34:37, lliuu wrote: > > On 2017/03/22 18:50:30, lliuu wrote: > > > On 2017/03/21 21:42:50, the sun wrote: > > > > No need for this new interface - just pass the call on like you do below. > > > > > > > > A user would subclass the ADMWrapper: > > > > > > > > class MyInterceptingADM : public ADMWrapper { > > > > public: > > > > ... > > > > int32_t RecordedDataIsAvailable(...) { > > > > // Do what I please with the data > > > > > > > > // Then call base class. > > > > return ADMWrapper::RecordedDataIsAvailable(...); > > > > } > > > > > > > > > > Fredrik, what do you think of adding pure virtual CaptureRecordedData to > > > AdmWrapper, and the user can just provide their own implementation without > > > needing to call base class? In this way we don't have to depend on the user > to > > > do the right thing > > > > It also occurred to me that if we're letting the user to override > > RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure to > > call the base functions in different orders in those 2 override functions, > which > > might not be intuitive > > I'd like to clarify the last comment: I meant that if user overrides > NeedMorePlayData below, they need to call the base function *before* their > custom logic, which might not be obvious to the user I see, those are fair points. https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:128: audio_transport_sink_ = audioCallback; On 2017/03/22 21:26:14, lliuu wrote: > On 2017/03/22 18:50:30, lliuu wrote: > > On 2017/03/21 21:42:50, the sun wrote: > > > audio_transport_sink_ will need to be protected by a lock > > > > Acknowledged. > > Actually could you clarify what is the race condition that needs to be protected > by a lock? The audio transport callbacks don't seem to be protected in the > default ADM/VOE implementations, and I don't feel there's the need to lock > between RegisterAudioCallback and the callback functions? Right you are. That's the same implicit contract that I was referring to in another comment. :)
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:41: // RefCountedObject methods overrides. Don't think you can at this stage but I would like to get rid of that dependency as well. My comment was based on that it looks like the methods you override are from "webrtc/modules/include/module.h" and not from RefCount.
https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:41: // RefCountedObject methods overrides. On 2017/03/29 07:42:14, henrika_webrtc wrote: > Don't think you can at this stage but I would like to get rid of that dependency > as well. > > My comment was based on that it looks like the methods you override are from > "webrtc/modules/include/module.h" and not from RefCount. Correct - btw lliuu - AudioDeviceModule inherits from RefCountedModule not RefCountedObject. Although the names are similar, those classes are very different. The former is a pure interface, the latter is a template implementation to satisfy an implementation of RefCountInterface. RefCountInterface has more in common with RefCountedModule, but they're still different classes, so it's important to not confuse them.
On 2017/03/29 08:44:33, tommi (webrtc) wrote: > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:41: // RefCountedObject > methods overrides. > On 2017/03/29 07:42:14, henrika_webrtc wrote: > > Don't think you can at this stage but I would like to get rid of that > dependency > > as well. > > > > My comment was based on that it looks like the methods you override are from > > "webrtc/modules/include/module.h" and not from RefCount. > > Correct - btw lliuu - AudioDeviceModule inherits from RefCountedModule not > RefCountedObject. Although the names are similar, those classes are very > different. The former is a pure interface, the latter is a template > implementation to satisfy an implementation of RefCountInterface. > RefCountInterface has more in common with RefCountedModule, but they're still > different classes, so it's important to not confuse them. Thanks for the clarification, I did realize offline that I confused the two as well :) Thanks for the comments everyone! I have not had bandwidth to work on this CL but I promise there will be a new patch this week
On 2017/03/28 23:33:45, the sun wrote: > This looks good. > > Initially, I thought the full ADMWrapper should instead be exposed, since that > would allow the client more flexibility to handle unforeseen use cases. Also, my > thinking was that adding a separate interface for side channeling the audio > wouldn't be good. > > Now, I've mostly changed my mind. This implementation allows only tapping audio, > not interfering with any of the other APIs on the class, meaning there is more > room to clean those up. > > I guess an alternative would be to use the AudioTransport interface instead of > AudioTransportCapture? I also agree that the name is too long. WDYT? > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:59: > capture_callback_->CaptureRecordedData(audioSamples, nSamples, > On 2017/03/22 21:26:14, lliuu wrote: > > On 2017/03/22 20:34:37, lliuu wrote: > > > On 2017/03/22 18:50:30, lliuu wrote: > > > > On 2017/03/21 21:42:50, the sun wrote: > > > > > No need for this new interface - just pass the call on like you do > below. > > > > > > > > > > A user would subclass the ADMWrapper: > > > > > > > > > > class MyInterceptingADM : public ADMWrapper { > > > > > public: > > > > > ... > > > > > int32_t RecordedDataIsAvailable(...) { > > > > > // Do what I please with the data > > > > > > > > > > // Then call base class. > > > > > return ADMWrapper::RecordedDataIsAvailable(...); > > > > > } > > > > > > > > > > > > > Fredrik, what do you think of adding pure virtual CaptureRecordedData to > > > > AdmWrapper, and the user can just provide their own implementation without > > > > needing to call base class? In this way we don't have to depend on the > user > > to > > > > do the right thing > > > > > > It also occurred to me that if we're letting the user to override > > > RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure > to > > > call the base functions in different orders in those 2 override functions, > > which > > > might not be intuitive > > > > I'd like to clarify the last comment: I meant that if user overrides > > NeedMorePlayData below, they need to call the base function *before* their > > custom logic, which might not be obvious to the user > > I see, those are fair points. > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:128: > audio_transport_sink_ = audioCallback; > On 2017/03/22 21:26:14, lliuu wrote: > > On 2017/03/22 18:50:30, lliuu wrote: > > > On 2017/03/21 21:42:50, the sun wrote: > > > > audio_transport_sink_ will need to be protected by a lock > > > > > > Acknowledged. > > > > Actually could you clarify what is the race condition that needs to be > protected > > by a lock? The audio transport callbacks don't seem to be protected in the > > default ADM/VOE implementations, and I don't feel there's the need to lock > > between RegisterAudioCallback and the callback functions? > > Right you are. That's the same implicit contract that I was referring to in > another comment. :) Hi Fredrik, I've also thought about directly using AudioTransport instead of creating new interfaces, but gave up on it due to following reasons: - It is not entirely clear to me what PushCaptureData and PullRenderData interfaces are for, and they are not hooked up in VOE either; - NeedMorePlayData passes the audio data in a non-const pointer, while at least for the scope of pure capture I'd rather we're passing a const pointer Which name do you think is too long, is it AudioTransportCapture or the static create function? Cheers, Lu
Description was changed from ========== Adding AudioTransportCapture class Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. Also, changed the sequence of things in VoEBaseImpl::Init so that AudioTransport is registered after audio processing is created. This prevents crashing in ProcessRecordedDataWithAPM, which does not check whether there is a valid instance for audio processing. BUG=webrtc:7337 ========== to ========== Adding AudioTransportCapture class Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. BUG=webrtc:7337 ==========
On 2017/03/31 22:29:43, lliuu wrote: > On 2017/03/28 23:33:45, the sun wrote: > > This looks good. > > > > Initially, I thought the full ADMWrapper should instead be exposed, since that > > would allow the client more flexibility to handle unforeseen use cases. Also, > my > > thinking was that adding a separate interface for side channeling the audio > > wouldn't be good. > > > > Now, I've mostly changed my mind. This implementation allows only tapping > audio, > > not interfering with any of the other APIs on the class, meaning there is more > > room to clean those up. > > > > I guess an alternative would be to use the AudioTransport interface instead of > > AudioTransportCapture? I also agree that the name is too long. WDYT? > > > > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > > > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > > webrtc/modules/audio_device/audio_transport_capture.cc:59: > > capture_callback_->CaptureRecordedData(audioSamples, nSamples, > > On 2017/03/22 21:26:14, lliuu wrote: > > > On 2017/03/22 20:34:37, lliuu wrote: > > > > On 2017/03/22 18:50:30, lliuu wrote: > > > > > On 2017/03/21 21:42:50, the sun wrote: > > > > > > No need for this new interface - just pass the call on like you do > > below. > > > > > > > > > > > > A user would subclass the ADMWrapper: > > > > > > > > > > > > class MyInterceptingADM : public ADMWrapper { > > > > > > public: > > > > > > ... > > > > > > int32_t RecordedDataIsAvailable(...) { > > > > > > // Do what I please with the data > > > > > > > > > > > > // Then call base class. > > > > > > return ADMWrapper::RecordedDataIsAvailable(...); > > > > > > } > > > > > > > > > > > > > > > > Fredrik, what do you think of adding pure virtual CaptureRecordedData to > > > > > AdmWrapper, and the user can just provide their own implementation > without > > > > > needing to call base class? In this way we don't have to depend on the > > user > > > to > > > > > do the right thing > > > > > > > > It also occurred to me that if we're letting the user to override > > > > RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure > > to > > > > call the base functions in different orders in those 2 override functions, > > > which > > > > might not be intuitive > > > > > > I'd like to clarify the last comment: I meant that if user overrides > > > NeedMorePlayData below, they need to call the base function *before* their > > > custom logic, which might not be obvious to the user > > > > I see, those are fair points. > > > > > https://codereview.webrtc.org/2753453002/diff/20001/webrtc/modules/audio_devi... > > webrtc/modules/audio_device/audio_transport_capture.cc:128: > > audio_transport_sink_ = audioCallback; > > On 2017/03/22 21:26:14, lliuu wrote: > > > On 2017/03/22 18:50:30, lliuu wrote: > > > > On 2017/03/21 21:42:50, the sun wrote: > > > > > audio_transport_sink_ will need to be protected by a lock > > > > > > > > Acknowledged. > > > > > > Actually could you clarify what is the race condition that needs to be > > protected > > > by a lock? The audio transport callbacks don't seem to be protected in the > > > default ADM/VOE implementations, and I don't feel there's the need to lock > > > between RegisterAudioCallback and the callback functions? > > > > Right you are. That's the same implicit contract that I was referring to in > > another comment. :) > > Hi Fredrik, > > I've also thought about directly using AudioTransport instead of creating new > interfaces, but gave up on it due to following reasons: > - It is not entirely clear to me what PushCaptureData and PullRenderData > interfaces are for, and they are not hooked up in VOE either; > - NeedMorePlayData passes the audio data in a non-const pointer, while at least > for the scope of pure capture I'd rather we're passing a const pointer True. AudioTransport is rather hard to understand for this reason. > Which name do you think is too long, is it AudioTransportCapture or the static > create function? I don't remember anymore. :/
Mostly naming changes and nits. This should be quick - the design is good. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : public AudioDeviceModule, public AudioTransport { nit: final class https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:21: explicit ADMWrapper(const int32_t id, nit: explicit not needed with 3 args https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:102: if (audio_transport_) { Won't be called from the default ADM implementation. Add RTC_NOTREACHED() instead of forwarding call. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:116: if (audio_transport_) { Won't be called from the default ADM implementation. Add RTC_NOTREACHED() instead of forwarding call. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:125: int32_t RegisterAudioCallback(AudioTransport* audioCallback) override { nit: no need to copy obsolete casing; use audio_callback, etc https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/include/audio_transport_capture.h (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:11: #ifndef WEBRTC_MODULES_AUDIO_DEVICE_INCLUDE_AUDIO_TRANSPORT_CAPTURE_H_ Rename file "audio_device_observer.h" https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:14: #include "webrtc/base/refcountedobject.h" Include scoped_ref_ptr.h instead. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:16: #include "webrtc/modules/include/module.h" shouldn't be needed here https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:20: class AudioTransportCapture { Ah, the name is a bit confusing since "capture" is often used when talking about the recorded audio (the played out audio being referred to as "render"). How about calling this interface AudioDeviceObserver instead? https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:22: virtual void CaptureRecordedData(const void* audio_samples, Rename to "OnCapture", "OnAudioCapture" or "OnCaptureData" https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:28: virtual void CapturePlayoutData(const void* audio_samples, Rename to "OnRender", "OnAudioRender" or "OnRenderData" https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:39: rtc::scoped_refptr<AudioDeviceModule> CreateAudioDeviceWithTransportCapture( CreateAudioDeviceWithObserver
On 2017/04/05 15:49:39, the sun wrote: > Mostly naming changes and nits. This should be quick - the design is good. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : > public AudioDeviceModule, public AudioTransport { > nit: final class > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:21: explicit > ADMWrapper(const int32_t id, > nit: explicit not needed with 3 args > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:102: if > (audio_transport_) { > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > instead of forwarding call. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:116: if > (audio_transport_) { > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > instead of forwarding call. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:125: int32_t > RegisterAudioCallback(AudioTransport* audioCallback) override { > nit: no need to copy obsolete casing; use audio_callback, etc > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/include/audio_transport_capture.h (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:11: #ifndef > WEBRTC_MODULES_AUDIO_DEVICE_INCLUDE_AUDIO_TRANSPORT_CAPTURE_H_ > Rename file "audio_device_observer.h" > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:14: #include > "webrtc/base/refcountedobject.h" > Include scoped_ref_ptr.h instead. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:16: #include > "webrtc/modules/include/module.h" > shouldn't be needed here > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:20: class > AudioTransportCapture { > Ah, the name is a bit confusing since "capture" is often used when talking about > the recorded audio (the played out audio being referred to as "render"). How > about calling this interface AudioDeviceObserver instead? > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:22: virtual void > CaptureRecordedData(const void* audio_samples, > Rename to "OnCapture", "OnAudioCapture" or "OnCaptureData" > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:28: virtual void > CapturePlayoutData(const void* audio_samples, > Rename to "OnRender", "OnAudioRender" or "OnRenderData" > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:39: > rtc::scoped_refptr<AudioDeviceModule> CreateAudioDeviceWithTransportCapture( > CreateAudioDeviceWithObserver
https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : public AudioDeviceModule, public AudioTransport { On 2017/04/05 15:49:39, the sun wrote: > nit: final class Unfortunately ADMWrapper cannot be final due to RefCountedModule methods that are not implemented in this class https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:21: explicit ADMWrapper(const int32_t id, On 2017/04/05 15:49:39, the sun wrote: > nit: explicit not needed with 3 args Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:102: if (audio_transport_) { On 2017/04/05 15:49:39, the sun wrote: > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > instead of forwarding call. Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:116: if (audio_transport_) { On 2017/04/05 15:49:39, the sun wrote: > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > instead of forwarding call. Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:116: if (audio_transport_) { On 2017/04/05 15:49:39, the sun wrote: > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > instead of forwarding call. Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:125: int32_t RegisterAudioCallback(AudioTransport* audioCallback) override { On 2017/04/05 15:49:39, the sun wrote: > nit: no need to copy obsolete casing; use audio_callback, etc Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/include/audio_transport_capture.h (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:11: #ifndef WEBRTC_MODULES_AUDIO_DEVICE_INCLUDE_AUDIO_TRANSPORT_CAPTURE_H_ On 2017/04/05 15:49:39, the sun wrote: > Rename file "audio_device_observer.h" Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:14: #include "webrtc/base/refcountedobject.h" On 2017/04/05 15:49:39, the sun wrote: > Include scoped_ref_ptr.h instead. Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:16: #include "webrtc/modules/include/module.h" On 2017/04/05 15:49:39, the sun wrote: > shouldn't be needed here Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:20: class AudioTransportCapture { On 2017/04/05 15:49:39, the sun wrote: > Ah, the name is a bit confusing since "capture" is often used when talking about > the recorded audio (the played out audio being referred to as "render"). How > about calling this interface AudioDeviceObserver instead? Unfortunately AudioDeviceObserver is already taken, so I named it AudioDeviceDataObserver instead. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:22: virtual void CaptureRecordedData(const void* audio_samples, On 2017/04/05 15:49:39, the sun wrote: > Rename to "OnCapture", "OnAudioCapture" or "OnCaptureData" Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:28: virtual void CapturePlayoutData(const void* audio_samples, On 2017/04/05 15:49:39, the sun wrote: > Rename to "OnRender", "OnAudioRender" or "OnRenderData" Done. https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:39: rtc::scoped_refptr<AudioDeviceModule> CreateAudioDeviceWithTransportCapture( On 2017/04/05 15:49:39, the sun wrote: > CreateAudioDeviceWithObserver Done.
Description was changed from ========== Adding AudioTransportCapture class Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. BUG=webrtc:7337 ========== to ========== Adding AudioDeviceDataObserver interface Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. BUG=webrtc:7337 ==========
On 2017/04/05 19:45:47, lliuu wrote: > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : > public AudioDeviceModule, public AudioTransport { > On 2017/04/05 15:49:39, the sun wrote: > > nit: final class > > Unfortunately ADMWrapper cannot be final due to RefCountedModule methods that > are not implemented in this class > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:21: explicit > ADMWrapper(const int32_t id, > On 2017/04/05 15:49:39, the sun wrote: > > nit: explicit not needed with 3 args > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:102: if > (audio_transport_) { > On 2017/04/05 15:49:39, the sun wrote: > > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > > instead of forwarding call. > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:116: if > (audio_transport_) { > On 2017/04/05 15:49:39, the sun wrote: > > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > > instead of forwarding call. > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:116: if > (audio_transport_) { > On 2017/04/05 15:49:39, the sun wrote: > > Won't be called from the default ADM implementation. Add RTC_NOTREACHED() > > instead of forwarding call. > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:125: int32_t > RegisterAudioCallback(AudioTransport* audioCallback) override { > On 2017/04/05 15:49:39, the sun wrote: > > nit: no need to copy obsolete casing; use audio_callback, etc > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/include/audio_transport_capture.h (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:11: #ifndef > WEBRTC_MODULES_AUDIO_DEVICE_INCLUDE_AUDIO_TRANSPORT_CAPTURE_H_ > On 2017/04/05 15:49:39, the sun wrote: > > Rename file "audio_device_observer.h" > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:14: #include > "webrtc/base/refcountedobject.h" > On 2017/04/05 15:49:39, the sun wrote: > > Include scoped_ref_ptr.h instead. > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:16: #include > "webrtc/modules/include/module.h" > On 2017/04/05 15:49:39, the sun wrote: > > shouldn't be needed here > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:20: class > AudioTransportCapture { > On 2017/04/05 15:49:39, the sun wrote: > > Ah, the name is a bit confusing since "capture" is often used when talking > about > > the recorded audio (the played out audio being referred to as "render"). How > > about calling this interface AudioDeviceObserver instead? > > Unfortunately AudioDeviceObserver is already taken, so I named it > AudioDeviceDataObserver instead. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:22: virtual void > CaptureRecordedData(const void* audio_samples, > On 2017/04/05 15:49:39, the sun wrote: > > Rename to "OnCapture", "OnAudioCapture" or "OnCaptureData" > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:28: virtual void > CapturePlayoutData(const void* audio_samples, > On 2017/04/05 15:49:39, the sun wrote: > > Rename to "OnRender", "OnAudioRender" or "OnRenderData" > > Done. > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:39: > rtc::scoped_refptr<AudioDeviceModule> CreateAudioDeviceWithTransportCapture( > On 2017/04/05 15:49:39, the sun wrote: > > CreateAudioDeviceWithObserver > > Done. Fredrik, PTAL Thanks!!
LGTM! https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_transport_capture.cc (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : public AudioDeviceModule, public AudioTransport { On 2017/04/05 19:45:46, lliuu wrote: > On 2017/04/05 15:49:39, the sun wrote: > > nit: final class > > Unfortunately ADMWrapper cannot be final due to RefCountedModule methods that > are not implemented in this class Ah! https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/include/audio_transport_capture.h (right): https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/include/audio_transport_capture.h:20: class AudioTransportCapture { On 2017/04/05 19:45:46, lliuu wrote: > On 2017/04/05 15:49:39, the sun wrote: > > Ah, the name is a bit confusing since "capture" is often used when talking > about > > the recorded audio (the played out audio being referred to as "render"). How > > about calling this interface AudioDeviceObserver instead? > > Unfortunately AudioDeviceObserver is already taken, so I named it > AudioDeviceDataObserver instead. Doh! <face-palm>
RS lgtm On 2017/04/06 06:58:38, the sun wrote: > LGTM! > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_transport_capture.cc (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_transport_capture.cc:19: class ADMWrapper : > public AudioDeviceModule, public AudioTransport { > On 2017/04/05 19:45:46, lliuu wrote: > > On 2017/04/05 15:49:39, the sun wrote: > > > nit: final class > > > > Unfortunately ADMWrapper cannot be final due to RefCountedModule methods that > > are not implemented in this class > > Ah! > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/include/audio_transport_capture.h (right): > > https://codereview.webrtc.org/2753453002/diff/80001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/include/audio_transport_capture.h:20: class > AudioTransportCapture { > On 2017/04/05 19:45:46, lliuu wrote: > > On 2017/04/05 15:49:39, the sun wrote: > > > Ah, the name is a bit confusing since "capture" is often used when talking > > about > > > the recorded audio (the played out audio being referred to as "render"). How > > > about calling this interface AudioDeviceObserver instead? > > > > Unfortunately AudioDeviceObserver is already taken, so I named it > > AudioDeviceDataObserver instead. > > Doh! <face-palm>
Description was changed from ========== Adding AudioDeviceDataObserver interface Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. BUG=webrtc:7337 ========== to ========== Adding AudioDeviceDataObserver interface Added new callback class that enables routing both the locally recorded and playout audio data to external module. A factory method that creates an AudioDeviceModule instance that also registers the AudioTransportCapture with the device is also added. BUG=webrtc:7337 R=solenberg@webrtc.org Review-Url: https://codereview.webrtc.org/2753453002 . Cr-Commit-Position: refs/heads/master@{#17570} Committed: https://chromium.googlesource.com/external/webrtc/+/4b62001e4ffcda2033e9b1ca2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 4b62001e4ffcda2033e9b1ca20b396c9f662e364 (presubmit successful).
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
