Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827263002:
Early initialize recording on the ADM from WebRtcVoiceMediaChannel. (Closed)
|
Created:
4 years, 9 months ago by the sun Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, pbos-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionEarly initialize recording on the ADM from WebRtcVoiceMediaChannel.
BUG=webrtc:4690
Committed: https://crrev.com/d53a3f9758c6c9d7fb71a88a33cdbd8557a70b55
Cr-Commit-Position: refs/heads/master@{#12369}
Patch Set 1 #Patch Set 2 : set upstream to 1830213002 #
Total comments: 10
Patch Set 3 : addressed comments #
Total comments: 4
Patch Set 4 : Simplify the solution #
Total comments: 2
Patch Set 5 : addressed comment #Patch Set 6 : Tests work #Patch Set 7 : comment #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : breaking test #
Messages
Total messages: 39 (15 generated)
Description was changed from ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 ========== to ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 ==========
solenberg@webrtc.org changed reviewers: + deadbeef@webrtc.org
https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2538: void WebRtcVoiceMediaChannel::SetupRecording() { Maybe add a comment explaining that this is done as a performance optimization, in case people wonder later why InitRecording is called in two places. https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2544: if (engine()->adm()->StartRecording() != 0) { It's really only "InitRecording" that was slow on Android. I'm not sure about the side-effects of calling "StartRecording" early on various platforms, so maybe we should leave that out for now? https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.cc:164: RTC_DCHECK_EQ(0, error); Is there a reason to switch from logging an error to using RTC_DCHECK_EQ?
https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2538: void WebRtcVoiceMediaChannel::SetupRecording() { On 2016/03/29 01:05:37, Taylor Brandstetter wrote: > Maybe add a comment explaining that this is done as a performance optimization, > in case people wonder later why InitRecording is called in two places. Done. https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2544: if (engine()->adm()->StartRecording() != 0) { On 2016/03/29 01:05:37, Taylor Brandstetter wrote: > It's really only "InitRecording" that was slow on Android. I'm not sure about > the side-effects of calling "StartRecording" early on various platforms, so > maybe we should leave that out for now? Note that StartRecording() is only called once we have a stream to send && sending is enabled. Added a comment. https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.cc:164: RTC_DCHECK_EQ(0, error); On 2016/03/29 01:05:38, Taylor Brandstetter wrote: > Is there a reason to switch from logging an error to using RTC_DCHECK_EQ? Yes, there is a reason. AFAICT StartSend() can never fail, so this isn't something we need to log. Rather, it is a programming logic error, i.e. either I read the code wrong just now, or someone changes the code in the below layers so the assumption doesn't hold anymore. In both cases a logic error, therefore an assertion is appropriate.
https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2544: if (engine()->adm()->StartRecording() != 0) { On 2016/03/31 10:49:47, the sun wrote: > On 2016/03/29 01:05:37, Taylor Brandstetter wrote: > > It's really only "InitRecording" that was slow on Android. I'm not sure about > > the side-effects of calling "StartRecording" early on various platforms, so > > maybe we should leave that out for now? > > Note that StartRecording() is only called once we have a stream to send && > sending is enabled. Added a comment. But after my recent change, a stream won't call "Start" until it's enabled for sending *and* has a source (look in "UpdateSendState"). If an application does an initial offer/answer just to get ICE connected/etc., sending may be enabled here even when the application isn't ready to start sending media. https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.cc:164: RTC_DCHECK_EQ(0, error); On 2016/03/31 10:49:47, the sun wrote: > On 2016/03/29 01:05:38, Taylor Brandstetter wrote: > > Is there a reason to switch from logging an error to using RTC_DCHECK_EQ? > > Yes, there is a reason. AFAICT StartSend() can never fail, so this isn't > something we need to log. Rather, it is a programming logic error, i.e. either I > read the code wrong just now, or someone changes the code in the below layers so > the assumption doesn't hold anymore. In both cases a logic error, therefore an > assertion is appropriate. If it should never fail, it seems like it would make sense to remove the return code in order to make that requirement explicit. But that's a separate matter from this CL, so this is good for now.
juberti@chromium.org changed reviewers: + juberti@chromium.org
https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1815: SetupRecording(); Won't this happen anyway as a result of SetSend? https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2514: LOG(LS_WARNING) << "Failed to initialize recording"; Shouldn't we propagate this error upwards instead of silently failing?
https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2544: if (engine()->adm()->StartRecording() != 0) { On 2016/03/31 18:11:25, Taylor Brandstetter wrote: > On 2016/03/31 10:49:47, the sun wrote: > > On 2016/03/29 01:05:37, Taylor Brandstetter wrote: > > > It's really only "InitRecording" that was slow on Android. I'm not sure > about > > > the side-effects of calling "StartRecording" early on various platforms, so > > > maybe we should leave that out for now? > > > > Note that StartRecording() is only called once we have a stream to send && > > sending is enabled. Added a comment. > > But after my recent change, a stream won't call "Start" until it's enabled for > sending *and* has a source (look in "UpdateSendState"). If an application does > an initial offer/answer just to get ICE connected/etc., sending may be enabled > here even when the application isn't ready to start sending media. Ah, thanks for explaining. I decided to leave the old call to VoEBase::StartSend() in AudioSendStream and only make the speculative call to InitRecording() from WVoMC::SetSend(). https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1827263002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.cc:164: RTC_DCHECK_EQ(0, error); On 2016/03/31 18:11:26, Taylor Brandstetter wrote: > On 2016/03/31 10:49:47, the sun wrote: > > On 2016/03/29 01:05:38, Taylor Brandstetter wrote: > > > Is there a reason to switch from logging an error to using RTC_DCHECK_EQ? > > > > Yes, there is a reason. AFAICT StartSend() can never fail, so this isn't > > something we need to log. Rather, it is a programming logic error, i.e. either > I > > read the code wrong just now, or someone changes the code in the below layers > so > > the assumption doesn't hold anymore. In both cases a logic error, therefore an > > assertion is appropriate. > > If it should never fail, it seems like it would make sense to remove the return > code in order to make that requirement explicit. But that's a separate matter > from this CL, so this is good for now. voe::Channel is still accessed via the legacy VoE interfaces (VoEBase, etc), which are being deprecated. I don't see much point in changing code in them. Instead, I've introduced the interim ChannelProxy class which provides a cleaned-up "view" of voe::Channel to the new AudioReceive/SendStream classes, as well as allowing to mock voe::Channel functionality so the stream classes can be tested. Once the legacy APIs are removed, voe::Channel can more easily be changed, and its functionality assimilated by the stream classes. https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1815: SetupRecording(); On 2016/03/31 18:57:38, juberti2 wrote: > Won't this happen anyway as a result of SetSend? Well, it used to, but in the patch set you comment on here, AudioSendStream had been changed to call directly onto voe::Channel (instead of going through VoEBase), meaning only the channel state would be altered and the ADM untouched, hence the explicit call to SetupRecording() here. https://codereview.webrtc.org/1827263002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2514: LOG(LS_WARNING) << "Failed to initialize recording"; On 2016/03/31 18:57:38, juberti2 wrote: > Shouldn't we propagate this error upwards instead of silently failing? We used to propagate it up, but eventually, in pc/channel.cc, the return value would be ignored. See: https://codereview.webrtc.org/1741933002/diff/280001/webrtc/pc/channel.cc (line 1457) The decision was made to simplify the logic and maintain current semantics.
https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1815: if (!engine()->adm()->Recording()) { Shouldn't this call "RecordingIsInitialized"?
https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1815: if (!engine()->adm()->Recording()) { On 2016/04/01 18:41:02, Taylor Brandstetter wrote: > Shouldn't this call "RecordingIsInitialized"? Actually, it looks like both must be checked. Looking at the various ADM implementations of InitRecording(), most will return -1 if we're already recording, but 0 if we're already initialized (see e.g. audio_device_alsa_linux.cc, audio_device_mac.cc, audio_device_core_win.cc). But looking closer at the Android and iOS implementations (audio_record_jni.cc, WebRtcAudioRecord.java, audio_device_ios.mm), those will instead DCHECK that the device is neither recording nor initialized for recording.
On 2016/04/01 21:35:40, the sun wrote: > https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.cc:1815: if > (!engine()->adm()->Recording()) { > On 2016/04/01 18:41:02, Taylor Brandstetter wrote: > > Shouldn't this call "RecordingIsInitialized"? > > Actually, it looks like both must be checked. Looking at the various ADM > implementations of InitRecording(), most will return -1 if we're already > recording, but 0 if we're already initialized (see e.g. > audio_device_alsa_linux.cc, audio_device_mac.cc, audio_device_core_win.cc). But > looking closer at the Android and iOS implementations (audio_record_jni.cc, > WebRtcAudioRecord.java, audio_device_ios.mm), those will instead DCHECK that the > device is neither recording nor initialized for recording. Based on bot failures, looks like `RecordingIsInitialized` needs to be checked in VoEBaseImpl before `InitRecording` is called.
On 2016/04/05 01:10:56, Taylor Brandstetter wrote: > On 2016/04/01 21:35:40, the sun wrote: > > > https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... > > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > > > > https://codereview.webrtc.org/1827263002/diff/60001/webrtc/media/engine/webrt... > > webrtc/media/engine/webrtcvoiceengine.cc:1815: if > > (!engine()->adm()->Recording()) { > > On 2016/04/01 18:41:02, Taylor Brandstetter wrote: > > > Shouldn't this call "RecordingIsInitialized"? > > > > Actually, it looks like both must be checked. Looking at the various ADM > > implementations of InitRecording(), most will return -1 if we're already > > recording, but 0 if we're already initialized (see e.g. > > audio_device_alsa_linux.cc, audio_device_mac.cc, audio_device_core_win.cc). > But > > looking closer at the Android and iOS implementations (audio_record_jni.cc, > > WebRtcAudioRecord.java, audio_device_ios.mm), those will instead DCHECK that > the > > device is neither recording nor initialized for recording. > > Based on bot failures, looks like `RecordingIsInitialized` needs to be checked > in VoEBaseImpl before `InitRecording` is called. Quite right (although the failing tests were due to other issues).
lgtm
The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827263002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827263002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/06 21:29:01, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Justin, did you have any more comments?
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/1827263002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827263002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827263002/160001
On 2016/04/07 08:02:27, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1827263002/160001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1827263002/160001 Justin, I'll need your approval for the change to webrtc/api/test/fakeaudiocapturemodule.cc Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4662)
solenberg@webrtc.org changed reviewers: + pthatcher@webrtc.org
Adding pthatcher@ for review of webrtc/api/test/, in case Justin is running low on cycles...
The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827263002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827263002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/14244) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14266)
lgtm
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1827263002/#ps220001 (title: "breaking test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827263002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827263002/220001
Message was sent while issue was closed.
Description was changed from ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 ========== to ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 ========== to ========== Early initialize recording on the ADM from WebRtcVoiceMediaChannel. BUG=webrtc:4690 Committed: https://crrev.com/d53a3f9758c6c9d7fb71a88a33cdbd8557a70b55 Cr-Commit-Position: refs/heads/master@{#12369} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d53a3f9758c6c9d7fb71a88a33cdbd8557a70b55 Cr-Commit-Position: refs/heads/master@{#12369} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
