Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1385893002:
Remove default receive channel from WVoE; baby step 3. (Closed)
|
Created:
5 years, 2 months ago by the sun Modified:
5 years, 2 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove default receive channel from WVoE; baby step 3.
Get rid of default receive channel.
BUG=webrtc:4690
Committed: https://crrev.com/1ac561447e3e1d81a1e390f95a385b5ed8fe0932
Cr-Commit-Position: refs/heads/master@{#10262}
Patch Set 1 #Patch Set 2 : set upstream #Patch Set 3 : test readding ResetRecvCodecs #Patch Set 4 : remove ResetRecvCodecs again #
Total comments: 17
Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : test re-add the few lines that cause libjingle_peerconnection_unittest not to fail. #Patch Set 8 : type error #Patch Set 9 : remove using_default_recv_channel_ #Patch Set 10 : doh #Patch Set 11 : remove implementation of SetRemoteRenderer #
Total comments: 8
Patch Set 12 : rebase #Patch Set 13 : Output volume settable before creating default stream #Patch Set 14 : comments #Patch Set 15 : rebase #Patch Set 16 : Don't blow up if unable to set playout on added recv stream #
Total comments: 4
Patch Set 17 : rebase #
Depends on Patchset: Messages
Total messages: 17 (3 generated)
solenberg@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2271: } Isn't default_recv_ssrc_ != 0 enough to know that it's using_default_revc_channel_? Why do we need the extra boolean? https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); I found out today that we are calling SetRemoteRenderer() with SSRC=0 to mean "render the audio of the default stream". Obviously this will fail, since it won't be in recevie_channels_. I can only assume that this use case still works and audio is played out because the default channel has been treated specially so that it plays out even without an AudioRenderer. But maybe we should fix that now, with something like: if (ssrc == 0 && using_default_recv_channel_) { ssrc = default_recv_ssrc_; } But, again, we'll need to record it for when it's called *before* the default_recv_ssrc_ is set. It may be that we'll need to have the receive_channels_ be receive_streams_ where stream == (channel, renderer, playout_level), like how it's done in WebRtcVideoEngine2. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2502: int ch_id = GetReceiveChannelId(ssrc); Upon further investigation, it turns out I was wrong about SSRC=0 never being passed in here. SSRC=0 still gets passed in here when the remote side never signals SSRCs and only sends one audio stream. In that case, we want SSRC=0 passed in to SetOutputScaling to mean "scale the output of the default recv channel". So, we need something like: if (ssrc == 0 && using_default_recv_channel_) { ssrc = default_receive_ssrc_; } But there is a problem. We may have SetOutputScaling called *before* default_receive_ssrc_ is called. So we need to record the values passed in and then apply them when default_receive_ssrc_ is applied. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2606: default_recv_ssrc_ = ssrc; Here is where you will need to apply any parameters that were called with SSRC=0.
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2271: } On 2015/10/06 18:58:10, pthatcher1 wrote: > Isn't default_recv_ssrc_ != 0 enough to know that it's > using_default_revc_channel_? Why do we need the extra boolean? I probably don't know all the details about SSRCs. I was under the impression that they were allocated as random 32-bit numbers, and therefore 0 would be a valid SSRC. If this isn't the case, can you point me at the RFC I should read? https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); On 2015/10/06 18:58:10, pthatcher1 wrote: > I found out today that we are calling SetRemoteRenderer() with SSRC=0 to mean > "render the audio of the default stream". Obviously this will fail, since it > won't be in recevie_channels_. I can only assume that this use case still works > and audio is played out because the default channel has been treated specially > so that it plays out even without an AudioRenderer. But maybe we should fix > that now, with something like: > > if (ssrc == 0 && using_default_recv_channel_) { > ssrc = default_recv_ssrc_; > } Could it be that things are "working" because there is no test to cover that case? > > But, again, we'll need to record it for when it's called *before* the > default_recv_ssrc_ is set. It may be that we'll need to have the > receive_channels_ be receive_streams_ where stream == (channel, renderer, > playout_level), like how it's done in WebRtcVideoEngine2. Yes, perhaps. How did you find out we call this function with SSRC=0? Might be useful for me to know where to look... https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2502: int ch_id = GetReceiveChannelId(ssrc); On 2015/10/06 18:58:10, pthatcher1 wrote: > Upon further investigation, it turns out I was wrong about SSRC=0 never being > passed in here. SSRC=0 still gets passed in here when the remote side never > signals SSRCs and only sends one audio stream. In that case, we want SSRC=0 > passed in to SetOutputScaling to mean "scale the output of the default recv > channel". > > So, we need something like: > > if (ssrc == 0 && using_default_recv_channel_) { > ssrc = default_receive_ssrc_; > } > > But there is a problem. We may have SetOutputScaling called *before* > default_receive_ssrc_ is called. So we need to record the values passed in and > then apply them when default_receive_ssrc_ is applied. Maybe it is better to store the values and always apply output scaling in AddRecvStream?
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2271: } On 2015/10/06 20:49:01, the sun wrote: > On 2015/10/06 18:58:10, pthatcher1 wrote: > > Isn't default_recv_ssrc_ != 0 enough to know that it's > > using_default_revc_channel_? Why do we need the extra boolean? > > I probably don't know all the details about SSRCs. I was under the impression > that they were allocated as random 32-bit numbers, and therefore 0 would be a > valid SSRC. If this isn't the case, can you point me at the RFC I should read? You are correct: https://www.ietf.org/rfc/rfc3550.txt Realistically, no one every sends with SSRC=0 because that would cause all kinds of problems (like the fact that AddRecvStream in this file fails with SSRC=0). However, if you want to be strict to the standard, I'm supportive of that. But in that case, let's do one of two things: A. Rename using_default_recv_channel_ to default_recv_ssrc_set_ B. Make default_recv_ssrc_ an int64_t and use -1 to be unset. Since it's only set by an incoming packet, it's guaranteed to be in the 32-bit range. C. Use Settable<uint32_t>. I think I'd prefer C > B > A > current-state. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); On 2015/10/06 20:49:01, the sun wrote: > On 2015/10/06 18:58:10, pthatcher1 wrote: > > I found out today that we are calling SetRemoteRenderer() with SSRC=0 to mean > > "render the audio of the default stream". Obviously this will fail, since it > > won't be in recevie_channels_. I can only assume that this use case still > works > > and audio is played out because the default channel has been treated specially > > so that it plays out even without an AudioRenderer. But maybe we should fix > > that now, with something like: > > > > if (ssrc == 0 && using_default_recv_channel_) { > > ssrc = default_recv_ssrc_; > > } > > Could it be that things are "working" because there is no test to cover that > case? It must be more than that. We have to support the case that when a remote description comes in with no signalled SSRCs, we still play out the audio. So there must be some audio playing out even with a renderer set, at least for the default stream. > > > > > But, again, we'll need to record it for when it's called *before* the > > default_recv_ssrc_ is set. It may be that we'll need to have the > > receive_channels_ be receive_streams_ where stream == (channel, renderer, > > playout_level), like how it's done in WebRtcVideoEngine2. > > Yes, perhaps. > > How did you find out we call this function with SSRC=0? Might be useful for me > to know where to look... AudioRtpReceiver::Reconfigure calls it here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... And uses the ssrc it gets here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... And here's how the video engine handles it: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2502: int ch_id = GetReceiveChannelId(ssrc); On 2015/10/06 20:49:01, the sun wrote: > On 2015/10/06 18:58:10, pthatcher1 wrote: > > Upon further investigation, it turns out I was wrong about SSRC=0 never being > > passed in here. SSRC=0 still gets passed in here when the remote side never > > signals SSRCs and only sends one audio stream. In that case, we want SSRC=0 > > passed in to SetOutputScaling to mean "scale the output of the default recv > > channel". > > > > So, we need something like: > > > > if (ssrc == 0 && using_default_recv_channel_) { > > ssrc = default_receive_ssrc_; > > } > > > > But there is a problem. We may have SetOutputScaling called *before* > > default_receive_ssrc_ is called. So we need to record the values passed in > and > > then apply them when default_receive_ssrc_ is applied. > > Maybe it is better to store the values and always apply output scaling in > AddRecvStream? That sounds reasonable.
Still WIP, but some things addressed. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2271: } On 2015/10/07 16:21:35, pthatcher1 wrote: > On 2015/10/06 20:49:01, the sun wrote: > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > Isn't default_recv_ssrc_ != 0 enough to know that it's > > > using_default_revc_channel_? Why do we need the extra boolean? > > > > I probably don't know all the details about SSRCs. I was under the impression > > that they were allocated as random 32-bit numbers, and therefore 0 would be a > > valid SSRC. If this isn't the case, can you point me at the RFC I should read? > > You are correct: https://www.ietf.org/rfc/rfc3550.txt > > Realistically, no one every sends with SSRC=0 because that would cause all kinds > of problems (like the fact that AddRecvStream in this file fails with SSRC=0). > > However, if you want to be strict to the standard, I'm supportive of that. But > in that case, let's do one of two things: > > A. Rename using_default_recv_channel_ to default_recv_ssrc_set_ > B. Make default_recv_ssrc_ an int64_t and use -1 to be unset. Since it's only > set by an incoming packet, it's guaranteed to be in the 32-bit range. > C. Use Settable<uint32_t>. > > I think I'd prefer C > B > A > current-state. I'm using an int64_t because the code is more concise that way. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); On 2015/10/07 16:21:35, pthatcher1 wrote: > On 2015/10/06 20:49:01, the sun wrote: > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > I found out today that we are calling SetRemoteRenderer() with SSRC=0 to > mean > > > "render the audio of the default stream". Obviously this will fail, since > it > > > won't be in recevie_channels_. I can only assume that this use case still > > works > > > and audio is played out because the default channel has been treated > specially > > > so that it plays out even without an AudioRenderer. But maybe we should fix > > > that now, with something like: > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > ssrc = default_recv_ssrc_; > > > } > > > > Could it be that things are "working" because there is no test to cover that > > case? > > It must be more than that. We have to support the case that when a remote > description comes in with no signalled SSRCs, we still play out the audio. So > there must be some audio playing out even with a renderer set, at least for the > default stream. > > > > > > > > > But, again, we'll need to record it for when it's called *before* the > > > default_recv_ssrc_ is set. It may be that we'll need to have the > > > receive_channels_ be receive_streams_ where stream == (channel, renderer, > > > playout_level), like how it's done in WebRtcVideoEngine2. > > > > Yes, perhaps. > > > > How did you find out we call this function with SSRC=0? Might be useful for me > > to know where to look... > > AudioRtpReceiver::Reconfigure calls it here: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > And uses the ssrc it gets here: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > And here's how the video engine handles it: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... It is indeed more than that. VoE doesn't support individual output of audio streams yet. Received audio is mixed and output via the AudioDeviceModule. AFAICT the AudioRenderer passed in here is always NULL, so I'm removing the implementation until we figure out how to properly hook this up.
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); On 2015/10/08 18:16:04, the sun wrote: > On 2015/10/07 16:21:35, pthatcher1 wrote: > > On 2015/10/06 20:49:01, the sun wrote: > > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > > I found out today that we are calling SetRemoteRenderer() with SSRC=0 to > > mean > > > > "render the audio of the default stream". Obviously this will fail, since > > it > > > > won't be in recevie_channels_. I can only assume that this use case still > > > works > > > > and audio is played out because the default channel has been treated > > specially > > > > so that it plays out even without an AudioRenderer. But maybe we should > fix > > > > that now, with something like: > > > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > > ssrc = default_recv_ssrc_; > > > > } > > > > > > Could it be that things are "working" because there is no test to cover that > > > case? > > > > It must be more than that. We have to support the case that when a remote > > description comes in with no signalled SSRCs, we still play out the audio. So > > there must be some audio playing out even with a renderer set, at least for > the > > default stream. > > > > > > > > > > > > > But, again, we'll need to record it for when it's called *before* the > > > > default_recv_ssrc_ is set. It may be that we'll need to have the > > > > receive_channels_ be receive_streams_ where stream == (channel, renderer, > > > > playout_level), like how it's done in WebRtcVideoEngine2. > > > > > > Yes, perhaps. > > > > > > How did you find out we call this function with SSRC=0? Might be useful for > me > > > to know where to look... > > > > AudioRtpReceiver::Reconfigure calls it here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > And uses the ssrc it gets here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > And here's how the video engine handles it: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > It is indeed more than that. VoE doesn't support individual output of audio > streams yet. Received audio is mixed and output via the AudioDeviceModule. > AFAICT the AudioRenderer passed in here is always NULL, so I'm removing the > implementation until we figure out how to properly hook this up. Wow, you're right. The AudioRenderer here always comes from an AudioTrack from AudioRtpReceiver::track_: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... And that always comes from an AudioTrack class: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... And that implementation is: cricket::AudioRenderer* GetRenderer() override { return NULL; } Further, there is this comment in AudioTrackInterface::GetRenderer: // TODO(xians): Remove the following interface after Chrome switches to AddSink() and RemoveSink() interfaces. The only place in Chrome using GetRenderer is WebRtcLocalAudioTrackAdapter::GetRenderer(), which just returns NULL. I think we should just remove WebRtcVoiceMediaChannel::SetRemoteRenderer and VoiceChannel::SetRemoteRenderer and the calls to it from RtpReceiver. Whether you want to do that in this CL or a separate one is up to you. But I don't want to have a method laying around that does nothing and can only be called with null. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2606: default_recv_ssrc_ = ssrc; On 2015/10/06 18:58:10, pthatcher1 wrote: > Here is where you will need to apply any parameters that were called with > SSRC=0. Are you still going to address the "volume set before packet received on default channel" use case? And can you add a unit test for it? https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1962: // TODO(solenberg): Remove!!!!!! Do you plan to do that in this CL or a future one? https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2211: if (default_recv_ssrc_ == static_cast<int64_t>(ssrc)) { A simple IsDefaultRecvStream(ssrc) might be nice, since this logic is in two places. https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2427: // TODO(solenberg): !!!!!! I assume this means you'll do the logic for "if ssrc == 0, save the volume for when we get a packet on the default stream"? https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.h:323: int64_t default_recv_ssrc_; Can you comment that it's really int32_t with an "unset" value? Can you also comment on what set vs unset means?
https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = receive_channels_.find(ssrc); On 2015/10/08 18:55:40, pthatcher1 wrote: > On 2015/10/08 18:16:04, the sun wrote: > > On 2015/10/07 16:21:35, pthatcher1 wrote: > > > On 2015/10/06 20:49:01, the sun wrote: > > > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > > > I found out today that we are calling SetRemoteRenderer() with SSRC=0 to > > > mean > > > > > "render the audio of the default stream". Obviously this will fail, > since > > > it > > > > > won't be in recevie_channels_. I can only assume that this use case > still > > > > works > > > > > and audio is played out because the default channel has been treated > > > specially > > > > > so that it plays out even without an AudioRenderer. But maybe we should > > fix > > > > > that now, with something like: > > > > > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > > > ssrc = default_recv_ssrc_; > > > > > } > > > > > > > > Could it be that things are "working" because there is no test to cover > that > > > > case? > > > > > > It must be more than that. We have to support the case that when a remote > > > description comes in with no signalled SSRCs, we still play out the audio. > So > > > there must be some audio playing out even with a renderer set, at least for > > the > > > default stream. > > > > > > > > > > > > > > > > > But, again, we'll need to record it for when it's called *before* the > > > > > default_recv_ssrc_ is set. It may be that we'll need to have the > > > > > receive_channels_ be receive_streams_ where stream == (channel, > renderer, > > > > > playout_level), like how it's done in WebRtcVideoEngine2. > > > > > > > > Yes, perhaps. > > > > > > > > How did you find out we call this function with SSRC=0? Might be useful > for > > me > > > > to know where to look... > > > > > > AudioRtpReceiver::Reconfigure calls it here: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > > > And uses the ssrc it gets here: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > > > And here's how the video engine handles it: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > It is indeed more than that. VoE doesn't support individual output of audio > > streams yet. Received audio is mixed and output via the AudioDeviceModule. > > AFAICT the AudioRenderer passed in here is always NULL, so I'm removing the > > implementation until we figure out how to properly hook this up. > > Wow, you're right. The AudioRenderer here always comes from an AudioTrack from > AudioRtpReceiver::track_: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > And that always comes from an AudioTrack class: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > And that implementation is: > > cricket::AudioRenderer* GetRenderer() override { return NULL; } > > > Further, there is this comment in AudioTrackInterface::GetRenderer: > > // TODO(xians): Remove the following interface after Chrome switches to > AddSink() and RemoveSink() interfaces. > > The only place in Chrome using GetRenderer is > WebRtcLocalAudioTrackAdapter::GetRenderer(), which just returns NULL. > > > I think we should just remove WebRtcVoiceMediaChannel::SetRemoteRenderer and > VoiceChannel::SetRemoteRenderer and the calls to it from RtpReceiver. > Whether you want to do that in this CL or a separate one is up to you. But I > don't want to have a method laying around that does nothing and can only be > called with null. Done. https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2502: int ch_id = GetReceiveChannelId(ssrc); On 2015/10/07 16:22:30, pthatcher1 wrote: > On 2015/10/06 20:49:01, the sun wrote: > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > Upon further investigation, it turns out I was wrong about SSRC=0 never > being > > > passed in here. SSRC=0 still gets passed in here when the remote side never > > > signals SSRCs and only sends one audio stream. In that case, we want SSRC=0 > > > passed in to SetOutputScaling to mean "scale the output of the default recv > > > channel". > > > > > > So, we need something like: > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > ssrc = default_receive_ssrc_; > > > } > > > > > > But there is a problem. We may have SetOutputScaling called *before* > > > default_receive_ssrc_ is called. So we need to record the values passed in > > and > > > then apply them when default_receive_ssrc_ is applied. > > > > Maybe it is better to store the values and always apply output scaling in > > AddRecvStream? > > That sounds reasonable. Decided to just keep the volume around for the default receive stream. There's a subtlety with the new impl that I want to check with you: SSRC=0 will work to set the volume of the default recv stream UNTIL it is signalled. In other words, if a stream is added with AddRecvStream (via UpdateRemoteStreams) that has the same SSRC as the (already present) default recv stream, then the signaled SSRC must henceforth be used to set the volume. Appears sensible to me, but will that cause any problems? https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2606: default_recv_ssrc_ = ssrc; On 2015/10/08 18:55:40, pthatcher1 wrote: > On 2015/10/06 18:58:10, pthatcher1 wrote: > > Here is where you will need to apply any parameters that were called with > > SSRC=0. > > Are you still going to address the "volume set before packet received on default > channel" use case? And can you add a unit test for it? Done. https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1962: // TODO(solenberg): Remove!!!!!! On 2015/10/08 18:55:40, pthatcher1 wrote: > Do you plan to do that in this CL or a future one? Done. https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2211: if (default_recv_ssrc_ == static_cast<int64_t>(ssrc)) { On 2015/10/08 18:55:40, pthatcher1 wrote: > A simple IsDefaultRecvStream(ssrc) might be nice, since this logic is in two > places. Done. https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2427: // TODO(solenberg): !!!!!! On 2015/10/08 18:55:40, pthatcher1 wrote: > I assume this means you'll do the logic for "if ssrc == 0, save the volume for > when we get a packet on the default stream"? Done. https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.h:323: int64_t default_recv_ssrc_; On 2015/10/08 18:55:40, pthatcher1 wrote: > Can you comment that it's really int32_t with an "unset" value? Can you also > comment on what set vs unset means? Done.
On 2015/10/12 15:03:01, the sun wrote: > https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... > File talk/media/webrtc/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvoiceengine.cc:2410: ChannelMap::iterator it = > receive_channels_.find(ssrc); > On 2015/10/08 18:55:40, pthatcher1 wrote: > > On 2015/10/08 18:16:04, the sun wrote: > > > On 2015/10/07 16:21:35, pthatcher1 wrote: > > > > On 2015/10/06 20:49:01, the sun wrote: > > > > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > > > > I found out today that we are calling SetRemoteRenderer() with SSRC=0 > to > > > > mean > > > > > > "render the audio of the default stream". Obviously this will fail, > > since > > > > it > > > > > > won't be in recevie_channels_. I can only assume that this use case > > still > > > > > works > > > > > > and audio is played out because the default channel has been treated > > > > specially > > > > > > so that it plays out even without an AudioRenderer. But maybe we > should > > > fix > > > > > > that now, with something like: > > > > > > > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > > > > ssrc = default_recv_ssrc_; > > > > > > } > > > > > > > > > > Could it be that things are "working" because there is no test to cover > > that > > > > > case? > > > > > > > > It must be more than that. We have to support the case that when a remote > > > > description comes in with no signalled SSRCs, we still play out the audio. > > > So > > > > there must be some audio playing out even with a renderer set, at least > for > > > the > > > > default stream. > > > > > > > > > > > > > > > > > > > > > But, again, we'll need to record it for when it's called *before* the > > > > > > default_recv_ssrc_ is set. It may be that we'll need to have the > > > > > > receive_channels_ be receive_streams_ where stream == (channel, > > renderer, > > > > > > playout_level), like how it's done in WebRtcVideoEngine2. > > > > > > > > > > Yes, perhaps. > > > > > > > > > > How did you find out we call this function with SSRC=0? Might be useful > > for > > > me > > > > > to know where to look... > > > > > > > > AudioRtpReceiver::Reconfigure calls it here: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > > > > > And uses the ssrc it gets here: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > > > > > And here's how the video engine handles it: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > > > It is indeed more than that. VoE doesn't support individual output of audio > > > streams yet. Received audio is mixed and output via the AudioDeviceModule. > > > AFAICT the AudioRenderer passed in here is always NULL, so I'm removing the > > > implementation until we figure out how to properly hook this up. > > > > Wow, you're right. The AudioRenderer here always comes from an AudioTrack > from > > AudioRtpReceiver::track_: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > And that always comes from an AudioTrack class: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > > > And that implementation is: > > > > cricket::AudioRenderer* GetRenderer() override { return NULL; } > > > > > > Further, there is this comment in AudioTrackInterface::GetRenderer: > > > > // TODO(xians): Remove the following interface after Chrome switches to > > AddSink() and RemoveSink() interfaces. > > > > The only place in Chrome using GetRenderer is > > WebRtcLocalAudioTrackAdapter::GetRenderer(), which just returns NULL. > > > > > > I think we should just remove WebRtcVoiceMediaChannel::SetRemoteRenderer and > > VoiceChannel::SetRemoteRenderer and the calls to it from RtpReceiver. > > Whether you want to do that in this CL or a separate one is up to you. But > I > > don't want to have a method laying around that does nothing and can only be > > called with null. > > Done. > > https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvoiceengine.cc:2502: int ch_id = > GetReceiveChannelId(ssrc); > On 2015/10/07 16:22:30, pthatcher1 wrote: > > On 2015/10/06 20:49:01, the sun wrote: > > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > > Upon further investigation, it turns out I was wrong about SSRC=0 never > > being > > > > passed in here. SSRC=0 still gets passed in here when the remote side > never > > > > signals SSRCs and only sends one audio stream. In that case, we want > SSRC=0 > > > > passed in to SetOutputScaling to mean "scale the output of the default > recv > > > > channel". > > > > > > > > So, we need something like: > > > > > > > > if (ssrc == 0 && using_default_recv_channel_) { > > > > ssrc = default_receive_ssrc_; > > > > } > > > > > > > > But there is a problem. We may have SetOutputScaling called *before* > > > > default_receive_ssrc_ is called. So we need to record the values passed > in > > > and > > > > then apply them when default_receive_ssrc_ is applied. > > > > > > Maybe it is better to store the values and always apply output scaling in > > > AddRecvStream? > > > > That sounds reasonable. > > Decided to just keep the volume around for the default receive stream. > > There's a subtlety with the new impl that I want to check with you: SSRC=0 will > work to set the volume of the default recv stream UNTIL it is signalled. In > other words, if a stream is added with AddRecvStream (via UpdateRemoteStreams) > that has the same SSRC as the (already present) default recv stream, then the > signaled SSRC must henceforth be used to set the volume. Appears sensible to me, > but will that cause any problems? > > https://codereview.webrtc.org/1385893002/diff/60001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvoiceengine.cc:2606: default_recv_ssrc_ = ssrc; > On 2015/10/08 18:55:40, pthatcher1 wrote: > > On 2015/10/06 18:58:10, pthatcher1 wrote: > > > Here is where you will need to apply any parameters that were called with > > > SSRC=0. > > > > Are you still going to address the "volume set before packet received on > default > > channel" use case? And can you add a unit test for it? > > Done. > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > File talk/media/webrtc/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > talk/media/webrtc/webrtcvoiceengine.cc:1962: // TODO(solenberg): Remove!!!!!! > On 2015/10/08 18:55:40, pthatcher1 wrote: > > Do you plan to do that in this CL or a future one? > > Done. > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > talk/media/webrtc/webrtcvoiceengine.cc:2211: if (default_recv_ssrc_ == > static_cast<int64_t>(ssrc)) { > On 2015/10/08 18:55:40, pthatcher1 wrote: > > A simple IsDefaultRecvStream(ssrc) might be nice, since this logic is in two > > places. > > Done. > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > talk/media/webrtc/webrtcvoiceengine.cc:2427: // TODO(solenberg): !!!!!! > On 2015/10/08 18:55:40, pthatcher1 wrote: > > I assume this means you'll do the logic for "if ssrc == 0, save the volume for > > when we get a packet on the default stream"? > > Done. > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > File talk/media/webrtc/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/1385893002/diff/200001/talk/media/webrtc/webrtc... > talk/media/webrtc/webrtcvoiceengine.h:323: int64_t default_recv_ssrc_; > On 2015/10/08 18:55:40, pthatcher1 wrote: > > Can you comment that it's really int32_t with an "unset" value? Can you also > > comment on what set vs unset means? > > Done. Not WIP anymore, PTAL.
lgtm, with nits https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2407: } Might be slightly more clear as: if (ssrc == 0) { default_recv_volume_ = volume; if (default_recv_ssrc_ == -1) { return true; } ssrc = static_cast<uint32_t>(default_recv_ssrc_); } https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.h:327: double default_recv_volume_; Can you leave a comment about when this is set and when it is used? And could you use the new C++ initializers? int64_t default_recv_ssrc_ = -1; double default_recv_volume_ = 1.0;
https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:2407: } On 2015/10/13 05:38:46, pthatcher1 wrote: > Might be slightly more clear as: > > if (ssrc == 0) { > default_recv_volume_ = volume; > if (default_recv_ssrc_ == -1) { > return true; > } > ssrc = static_cast<uint32_t>(default_recv_ssrc_); > } Done. https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1385893002/diff/300001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.h:327: double default_recv_volume_; On 2015/10/13 05:38:46, pthatcher1 wrote: > Can you leave a comment about when this is set and when it is used? > > And could you use the new C++ initializers? > > int64_t default_recv_ssrc_ = -1; > double default_recv_volume_ = 1.0; > Done.
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1385893002/#ps320001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1385893002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1385893002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/1ac561447e3e1d81a1e390f95a385b5ed8fe0932 Cr-Commit-Position: refs/heads/master@{#10262} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
