Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1576913002:
Remove cast to LocalAudioSource from AudioRtpSender. (Closed)
DescriptionRemove cast to LocalAudioSource from AudioRtpSender.
We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either.
BUG=5423
R=solenberg@webrtc.org
Committed: https://crrev.com/3c16978c8489e77d49932ab4b5af720fe427e544
Cr-Commit-Position: refs/heads/master@{#11341}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Revert change for everyone but chromium #Messages
Total messages: 20 (5 generated)
Description was changed from ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG= ========== to ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG=5423 ==========
tommi@webrtc.org changed reviewers: + solenberg@webrtc.org
Please take a look. A more conservative approach is also possible, where we #ifdef out the cast for Chrome only (for now) if we need this in there for the time being.
https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc File talk/app/webrtc/rtpsender.cc (right): https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:192: renderer); Are you sure these options are always empty? Is there a test you can rely on? If this holds water, you should be able to drop the AudioOptions from the SetAudioSend() calls all the way down the stack. (and upwards, since we're now disregarding any constraints passed to LocalAudioSource::Create()). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... Options are passed down to WVoMC::SetSendParameters() too, from WebRtcSession::UpdateSessionState(), but contraints don't appear to be part of the mix there, so I guess there options are passed in both places. Some here, some there.
https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc File talk/app/webrtc/rtpsender.cc (right): https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:192: renderer); On 2016/01/11 14:49:31, the sun wrote: > Are you sure these options are always empty? Is there a test you can rely on? > > If this holds water, you should be able to drop the AudioOptions from the > SetAudioSend() calls all the way down the stack. (and upwards, since we're now > disregarding any constraints passed to LocalAudioSource::Create()). > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > Options are passed down to WVoMC::SetSendParameters() too, from > WebRtcSession::UpdateSessionState(), but contraints don't appear to be part of > the mix there, so I guess there options are passed in both places. Some here, > some there. In the case of Chrome, these options usually contain a set of default values but can contain values set by callers via getUserMedia. However, Chrome handles those values itself and applies it to its instance of the APM. In this case, these options are being applied on a different instance if I understand correctly and in theory, constaints passed to getUserMedia could be applied to an unrelated remote audio track. Dropping the AudioOptions from SetAudioSend sgtm if this is the only call site.
On 2016/01/11 14:49:32, the sun wrote: > https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc > File talk/app/webrtc/rtpsender.cc (right): > > https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc#... > talk/app/webrtc/rtpsender.cc:192: renderer); > Are you sure these options are always empty? Is there a test you can rely on? > > If this holds water, you should be able to drop the AudioOptions from the > SetAudioSend() calls all the way down the stack. (and upwards, since we're now > disregarding any constraints passed to LocalAudioSource::Create()). > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > Options are passed down to WVoMC::SetSendParameters() too, from > WebRtcSession::UpdateSessionState(), but contraints don't appear to be part of > the mix there, so I guess there options are passed in both places. Some here, > some there. btw, I'm not following where we're disregarding the constraints. They're being passed to Initialize() and converted to options, right? (I have a feeling I'm missing something important)
https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc File talk/app/webrtc/rtpsender.cc (right): https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:192: renderer); On 2016/01/14 16:24:11, tommi-webrtc wrote: > On 2016/01/11 14:49:31, the sun wrote: > > Are you sure these options are always empty? Is there a test you can rely on? > > > > If this holds water, you should be able to drop the AudioOptions from the > > SetAudioSend() calls all the way down the stack. (and upwards, since we're now > > disregarding any constraints passed to LocalAudioSource::Create()). > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > Options are passed down to WVoMC::SetSendParameters() too, from > > WebRtcSession::UpdateSessionState(), but contraints don't appear to be part of > > the mix there, so I guess there options are passed in both places. Some here, > > some there. > > In the case of Chrome, these options usually contain a set of default values but > can contain values set by callers via getUserMedia. However, Chrome handles > those values itself and applies it to its instance of the APM. In this case, > these options are being applied on a different instance if I understand > correctly and in theory, constaints passed to getUserMedia could be applied to > an unrelated remote audio track. > > Dropping the AudioOptions from SetAudioSend sgtm if this is the only call site. Non-Chrome clients use the VoE APM instance, e.g. AppRTC (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...) and push constraints down through LocalAudioSource.
On 2016/01/14 16:28:24, tommi-webrtc wrote: > On 2016/01/11 14:49:32, the sun wrote: > > https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc > > File talk/app/webrtc/rtpsender.cc (right): > > > > > https://codereview.webrtc.org/1576913002/diff/1/talk/app/webrtc/rtpsender.cc#... > > talk/app/webrtc/rtpsender.cc:192: renderer); > > Are you sure these options are always empty? Is there a test you can rely on? > > > > If this holds water, you should be able to drop the AudioOptions from the > > SetAudioSend() calls all the way down the stack. (and upwards, since we're now > > disregarding any constraints passed to LocalAudioSource::Create()). > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > Options are passed down to WVoMC::SetSendParameters() too, from > > WebRtcSession::UpdateSessionState(), but contraints don't appear to be part of > > the mix there, so I guess there options are passed in both places. Some here, > > some there. > > btw, I'm not following where we're disregarding the constraints. They're being > passed to Initialize() and converted to options, right? (I have a feeling I'm > missing something important) I meant in LocalAudioSource::Initialize() (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...). The spot in AudioRtpSender you're changing here appears to be the only place LocalAudioSource::options() is called. So if we can disregard what it returns, it means that the conversion of constraints->options in LocalAudioSource can be removed and LocalAudioSource become a no-op class. However, as per my other comment, I don't think that is the case.
Revert change for everyone but chromium
Thanks for pointing that out. I'm putting the behavior back in for everyone except chromium.
On 2016/01/18 18:58:12, tommi-webrtc wrote: > Thanks for pointing that out. I'm putting the behavior back in for everyone > except chromium. ping
On 2016/01/21 08:58:05, tommi-webrtc wrote: > On 2016/01/18 18:58:12, tommi-webrtc wrote: > > Thanks for pointing that out. I'm putting the behavior back in for everyone > > except chromium. > > ping Sorry for the slow response, I've been off sick for 2 days. The only option that can be set through LocalSource, which isn't affecting APM/ADM, is AudioOptions::stereo_swapping, which causes a flag to be set on TransmitMixer: (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin..., https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...). If we don't need this constraint, the change should be ok.
On 2016/01/21 10:13:02, the sun wrote: > On 2016/01/21 08:58:05, tommi-webrtc wrote: > > On 2016/01/18 18:58:12, tommi-webrtc wrote: > > > Thanks for pointing that out. I'm putting the behavior back in for everyone > > > except chromium. > > > > ping > > Sorry for the slow response, I've been off sick for 2 days. Ah, sorry, should have known. > The only option that can be set through LocalSource, which isn't affecting > APM/ADM, is AudioOptions::stereo_swapping, which causes a flag to be set on > TransmitMixer: > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin..., > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...). > > If we don't need this constraint, the change should be ok. Chrome is handling this already for local streams: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Does the transmit mixer need to be aware of this? How does it know for which track this applies and sets it on the right 'processor'? (looks to me like this is done on a global VoEAudioProcessing instance inside of webrtc?)
On 2016/01/21 11:02:42, tommi-webrtc wrote: > On 2016/01/21 10:13:02, the sun wrote: > > On 2016/01/21 08:58:05, tommi-webrtc wrote: > > > On 2016/01/18 18:58:12, tommi-webrtc wrote: > > > > Thanks for pointing that out. I'm putting the behavior back in for > everyone > > > > except chromium. > > > > > > ping > > > > Sorry for the slow response, I've been off sick for 2 days. > > Ah, sorry, should have known. > > > The only option that can be set through LocalSource, which isn't affecting > > APM/ADM, is AudioOptions::stereo_swapping, which causes a flag to be set on > > TransmitMixer: > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin..., > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...). > > > > If we don't need this constraint, the change should be ok. > > Chrome is handling this already for local streams: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > Does the transmit mixer need to be aware of this? How does it know for which > track this applies and sets it on the right 'processor'? (looks to me like this > is done on a global VoEAudioProcessing instance inside of webrtc?) The TransmitMixer is global for a VoE instance and thus shared between all sending channels. The name is confusing; it duplicates the recorded audio to multiple sending channels, which in some parts of the code is referred to as "upmixing". But nevertheless, the TransmitMixer will not be invoked in the audio path Chrome uses VoEBaseImpl::OnData(), so it looks like this change should be good after all. LGTM
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576913002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576913002/20001
Description was changed from ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG=5423 ========== to ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG=5423 R=solenberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3c16978c8489e77d49932ab4b... ==========
Message was sent while issue was closed.
Description was changed from ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG=5423 R=solenberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3c16978c8489e77d49932ab4b... ========== to ========== Remove cast to LocalAudioSource from AudioRtpSender. We can't assume that the audio source implementation will be our own internal one and we shouldn't apply local audio options to a remote audio track this way either. BUG=5423 R=solenberg@webrtc.org Committed: https://crrev.com/3c16978c8489e77d49932ab4b5af720fe427e544 Cr-Commit-Position: refs/heads/master@{#11341} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3c16978c8489e77d49932ab4b5af720fe427e544 Cr-Commit-Position: refs/heads/master@{#11341}
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 3c16978c8489e77d49932ab4b5af720fe427e544 (presubmit successful). |
