Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2539213003:
Support external audio mixer. (Closed)
DescriptionSupport external audio mixer in webrtc.
An external audio mixer will be passed from PeerConnectionFactory to
AudioTransportProxy.
BUG=webrtc:6457
Committed: https://crrev.com/f6bcac59e88c3be5ffd73bbb1098f2d82ee316a1
Cr-Commit-Position: refs/heads/master@{#15556}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Review response #
Total comments: 14
Patch Set 3 : Review response 2 #Patch Set 4 : remove GetRemoteAudioTrackSsrcs #
Total comments: 3
Patch Set 5 : try to fix OSX build issue #Messages
Total messages: 40 (20 generated)
Description was changed from ========== Support external audio mixer. An external audio mixer will passed from Peerconnection factory to AudioTransportProxy. BUG= ========== to ========== Support external audio mixer in webrtc. An external audio mixer will be passed from PeerConnectionFactory to AudioTransportProxy. BUG=webrtc:6457 ==========
Patchset #1 (id:1) has been deleted
gyzhou@chromium.org changed reviewers: + solenberg@webrtc.org
Hi Solenberg, Would you please take a look of this CL? Thanks, https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:27: rtc_static_library("call_api") { Based on this bug https://buganizer.corp.google.com/u/0/issues/31847415 rtc_source-set has to change to rtc_static_library to avoid the link issue of the application that uses webrtc.
solenberg@webrtc.org changed reviewers: + aleloi@webrtc.org
Hi GeorgeZ@! This CL looks very clean and short. I've only noticed minor things. Could you also add the dependency webrtc/api:mixer_api to the targets in BUILD.gn files in which you include the mixer header? I think it's mainly webrtc/api:libjingle_peerconnection, but there could be more. One can check by running "gn refs out/Debug path/to/file_to_which_mixer_include_was_added.h". When it's done, 'gn check out/Debug "*" | wc' should return approximately the same as it did before the changes. I also think that a test for constructing a PC with a custom mixer should be added. It should be possible to reuse the test framework in webrtc/api/peerconnection_unittests.cc. The P2PTestConductor.LocalP2PTest* tests set up a PC and make sure that audio/video is received at the other end. If the PC is created with a mock mixer, a test could check that audio passes through it. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:27: rtc_static_library("call_api") { On 2016/12/01 17:36:32, GeorgeZ wrote: > Based on this bug > https://buganizer.corp.google.com/u/0/issues/31847415 > > rtc_source-set has to change to rtc_static_library to avoid the link issue of > the application that uses webrtc. Please don't post internal links here. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> PeerConnection::GetRemoteAudioTrackSsrcs() { See comment at declaration. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.h:148: std::vector<uint32_t> GetRemoteAudioTrackSsrcs() override; Is there problems with implementing this in client-code through a PeerConnectionObserver instead? https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:109: // Call Initialize synchronously but make sure its executed on its -> it's https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.h:139: rtc::scoped_refptr<AudioMixer> external_audio_mixer_; Please add comment describing what happens if this is nullptr. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:787: // |decoder_factory| are transferred to the returned factory. The comment should mention the new argument |audio_mixed|. For the other arguments, I don't think the same thing has to be repeated as in the first c-tor. (So only document the difference and the new parameter). Also, I think this should be moved above the |worker_and_network_thread|-constructor, because it's more similar to the first c-tor https://codereview.webrtc.org/2539213003/diff/20001/webrtc/audio/audio_receiv... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/audio/audio_receiv... webrtc/audio/audio_receive_stream.cc:315: return config_.rtp.remote_ssrc; Thanks, I think it is correct now. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: if (voe_wrapper->AudioMixer()) Please use brackets for one-line blocks consistently with the rest of this file.
Alex, Thanks for your review. Here is the response of your comments. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:27: rtc_static_library("call_api") { Thanks for reminding. On 2016/12/05 14:03:08, aleloi wrote: > On 2016/12/01 17:36:32, GeorgeZ wrote: > > Based on this bug > > https://buganizer.corp.google.com/u/0/issues/31847415 > > > > rtc_source-set has to change to rtc_static_library to avoid the link issue of > > the application that uses webrtc. > > Please don't post internal links here. Done. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.h:148: std::vector<uint32_t> GetRemoteAudioTrackSsrcs() override; On 2016/12/05 14:03:08, aleloi wrote: > Is there problems with implementing this in client-code through a > PeerConnectionObserver instead? Functions in PeerConnectionObserver are event/callbacks functions to response the state changes of Peerconnection. GetRemoteAudioTrackSsrcs() is more a state inquire/get function and not fit in the event/callback function category. Since PeerconnectionInterface has many get functions, I guess it may be a better place to put GetRemoteAudioTrackSsrcs() into. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:109: // Call Initialize synchronously but make sure its executed on On 2016/12/05 14:03:08, aleloi wrote: > its -> it's Done. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.h:139: rtc::scoped_refptr<AudioMixer> external_audio_mixer_; On 2016/12/05 14:03:08, aleloi wrote: > Please add comment describing what happens if this is nullptr. Done. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:787: // |decoder_factory| are transferred to the returned factory. On 2016/12/05 14:03:08, aleloi wrote: > The comment should mention the new argument |audio_mixed|. For the other > arguments, I don't think the same thing has to be repeated as in the first > c-tor. (So only document the difference and the new parameter). Also, I think > this should be moved above the |worker_and_network_thread|-constructor, because > it's more similar to the first c-tor Done. https://codereview.webrtc.org/2539213003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2539213003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: if (voe_wrapper->AudioMixer()) On 2016/12/05 14:03:08, aleloi wrote: > Please use brackets for one-line blocks consistently with the rest of this file. Done.
Hi Alex, Please see my response below On 2016/12/05 14:03:09, aleloi wrote: > Hi GeorgeZ@! > > This CL looks very clean and short. I've only noticed minor things. > > Could you also add the dependency webrtc/api:mixer_api to the targets in > BUILD.gn files in which you include the mixer header? I think it's mainly > webrtc/api:libjingle_peerconnection, but there could be more. One can check by > running > "gn refs out/Debug path/to/file_to_which_mixer_include_was_added.h". When it's > done, 'gn check out/Debug "*" | wc' should return approximately the same as it > did before the changes. I do not really get your point here. Do you suggest me add webrtc/api:mixer_api as dependency for any source_set or static library that has #include "audio_mixer.h" in its source? what is 'gn check out/Debug "*" | wc'? For .h file without any implementation. Sometime 'include' is enough and library dependency may not definitely necessary. > > I also think that a test for constructing a PC with a custom mixer should be > added. It should be possible to reuse the test framework in > webrtc/api/peerconnection_unittests.cc. The P2PTestConductor.LocalP2PTest* tests > set up a PC and make sure that audio/video is received at the other end. If the > PC is created with a mock mixer, a test could check that audio passes through > it. > I agree that a Test is needed. I will take a look. If it is simple, I will add the test into this CL. Otherwise, the next CL will take care of it. Anyway this CL has been tested by some application on the top of webrtc.
Yes, please add webrtc/api:mixer_api to //webrtc/api:libjingle_peerconnection, //webrtc/media:rtc_media and //webrtc/media:rtc_media_base if it's not already there. There are two reasons for this: it's a compilation dependency (if you change audio_mixer.h, then the targets that include it should be recompiled), and our build analysis tools require that included headers should be added as dependencies. Or, rather, our presubmit tools do not require it right now, because then no one would be able to check anything in to webrtc, because there are too many existing errors. But there is an issue for it and work actively going on to make we use the build system correctly. Another alternative is to forward-declare (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) the mixer. The GN command 'gn check out/Debug "*"' outputs several things, including all violations of the 'depend on what we #include' rule. We should make sure its output does not increase with each CL.
Patchset #2 (id:40001) has been deleted
Hi Fredrik. Would you please take a look of this CL. Thanks,
https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> PeerConnection::GetRemoteAudioTrackSsrcs() { Is this utility really needed? Can you use PC::remote_streams() and iterate over streams and call GetAudioTracks() for each stream, fetching the ssrc by calling id() on each track? https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:60: rtc::scoped_refptr<PeerConnectionFactory> pc_factory( Call CreatePeerConnectionFactoryWithAudioMixer() to avoid duplicating code. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:794: remove space https://codereview.webrtc.org/2539213003/diff/60001/webrtc/audio/audio_receiv... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/audio/audio_receiv... webrtc/audio/audio_receive_stream.cc:315: return config_.rtp.remote_ssrc; good catch! https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcmediaengine.cc:83: adm, webrtc::CreateBuiltinAudioDecoderFactory(), video_encoder_factory, nit: git cl format, or manually stick to one way of indenting please https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoe.h (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoe.h:75: VoEWrapper(rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer) AudioMixer has no business here. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: if (voe_wrapper->AudioMixer()) { Send the mixer as an argument to this function instead of going via the voe_wrapper
Patchset #3 (id:80001) has been deleted
Hi Fredrik, A patch has uploaded to address your comments. Thanks https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> PeerConnection::GetRemoteAudioTrackSsrcs() { On 2016/12/07 15:56:08, the sun wrote: > Is this utility really needed? Can you use PC::remote_streams() and iterate over > streams and call GetAudioTracks() for each stream, fetching the ssrc by calling > id() on each track? PC::remote_streams() provides rtc::scoped_refptr<StreamCollectionInterface>. Iterating StreamCollectionInterface can get MediaStreamInterface. MediaStreamInterface has label() which is not ssrc. AudioTrackInterface can get from MediaStreamInterface. However AudioTrackInterface doesn't have ID or Label. Therefore, I am sure I can get ssrc from PC. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:60: rtc::scoped_refptr<PeerConnectionFactory> pc_factory( On 2016/12/07 15:56:08, the sun wrote: > Call CreatePeerConnectionFactoryWithAudioMixer() to avoid duplicating code. Good catch. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:794: On 2016/12/07 15:56:08, the sun wrote: > remove space Done. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcmediaengine.cc:83: adm, webrtc::CreateBuiltinAudioDecoderFactory(), video_encoder_factory, On 2016/12/07 15:56:08, the sun wrote: > nit: git cl format, or manually stick to one way of indenting please I verified that this was adjusted by git cl format. Maybe git cl format doesn't behave well for this case. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoe.h (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoe.h:75: VoEWrapper(rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer) On 2016/12/07 15:56:08, the sun wrote: > AudioMixer has no business here. Done. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: if (voe_wrapper->AudioMixer()) { On 2016/12/07 15:56:08, the sun wrote: > Send the mixer as an argument to this function instead of going via the > voe_wrapper Good point
LG - just let's sort out whether remote audio ssrcs can be fetched using existing PC methods or not. https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> PeerConnection::GetRemoteAudioTrackSsrcs() { On 2016/12/07 18:28:17, GeorgeZ wrote: > On 2016/12/07 15:56:08, the sun wrote: > > Is this utility really needed? Can you use PC::remote_streams() and iterate > over > > streams and call GetAudioTracks() for each stream, fetching the ssrc by > calling > > id() on each track? > > PC::remote_streams() provides rtc::scoped_refptr<StreamCollectionInterface>. > Iterating StreamCollectionInterface can get MediaStreamInterface. > MediaStreamInterface has label() which is not ssrc. > AudioTrackInterface can get from MediaStreamInterface. However > AudioTrackInterface doesn't have ID or Label. > > Therefore, I am sure I can get ssrc from PC. AudioTrackInterface inherits from MediaStreamTrackInterface, which has an id() method. However, I must confess that it's not clear to me whether this is actually the SSRC or something else. Could you please check if it can be used? I'd like to avoid adding another method to PeerConnectionInterface if we can avoid it.
Hi Fredrik, id() for MediaStreamTrackInterface is a string (ssrc is uint32_t) and is set by PeerConnectionFactoryInterface::CreateAudioTrack(const std::string& label,...) The label is the id(); https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionint... struct StreamParams has ssrcs but the ssrcs don't pass to MediaStream in MediaStream::Create( const std::string& label). I agree with you that I should avoid to add extra PC function. Therefore if you know some other way to get ssrc from existing PC interface, it will be great. Thanks, On 2016/12/08 07:52:29, the sun wrote: > LG - just let's sort out whether remote audio ssrcs can be fetched using > existing PC methods or not. > > https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc > File webrtc/api/peerconnection.cc (right): > > https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... > webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> > PeerConnection::GetRemoteAudioTrackSsrcs() { > On 2016/12/07 18:28:17, GeorgeZ wrote: > > On 2016/12/07 15:56:08, the sun wrote: > > > Is this utility really needed? Can you use PC::remote_streams() and iterate > > over > > > streams and call GetAudioTracks() for each stream, fetching the ssrc by > > calling > > > id() on each track? > > > > PC::remote_streams() provides rtc::scoped_refptr<StreamCollectionInterface>. > > Iterating StreamCollectionInterface can get MediaStreamInterface. > > MediaStreamInterface has label() which is not ssrc. > > AudioTrackInterface can get from MediaStreamInterface. However > > AudioTrackInterface doesn't have ID or Label. > > > > Therefore, I am sure I can get ssrc from PC. > > AudioTrackInterface inherits from MediaStreamTrackInterface, which has an id() > method. However, I must confess that it's not clear to me whether this is > actually the SSRC or something else. Could you please check if it can be used? > I'd like to avoid adding another method to PeerConnectionInterface if we can > avoid it.
Ah yes, I thought I remembered that the label/id was actually populated with the ssrc, but now that I look at the code I must have read things wrong. Another way is to iterate over PC::GetReceivers() and call the GetParameters() for those with media_type() audio. The RtpParameters struct contains a vector of RtpEncodingParameters (of which there should be just one for audio streams), which has an SSRC. On 2016/12/08 16:45:18, GeorgeZ wrote: > Hi Fredrik, > > id() for MediaStreamTrackInterface is a string (ssrc is uint32_t) and is set by > PeerConnectionFactoryInterface::CreateAudioTrack(const std::string& label,...) > The label is the id(); > https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionint... > > struct StreamParams has ssrcs but the ssrcs don't pass to MediaStream in > MediaStream::Create( const std::string& label). > > I agree with you that I should avoid to add extra PC function. Therefore if you > know some other way to get ssrc from existing PC interface, it will be great. > > Thanks, > > On 2016/12/08 07:52:29, the sun wrote: > > LG - just let's sort out whether remote audio ssrcs can be fetched using > > existing PC methods or not. > > > > > https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc > > File webrtc/api/peerconnection.cc (right): > > > > > https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection... > > webrtc/api/peerconnection.cc:1356: std::vector<uint32_t> > > PeerConnection::GetRemoteAudioTrackSsrcs() { > > On 2016/12/07 18:28:17, GeorgeZ wrote: > > > On 2016/12/07 15:56:08, the sun wrote: > > > > Is this utility really needed? Can you use PC::remote_streams() and > iterate > > > over > > > > streams and call GetAudioTracks() for each stream, fetching the ssrc by > > > calling > > > > id() on each track? > > > > > > PC::remote_streams() provides rtc::scoped_refptr<StreamCollectionInterface>. > > > Iterating StreamCollectionInterface can get MediaStreamInterface. > > > MediaStreamInterface has label() which is not ssrc. > > > AudioTrackInterface can get from MediaStreamInterface. However > > > AudioTrackInterface doesn't have ID or Label. > > > > > > Therefore, I am sure I can get ssrc from PC. > > > > AudioTrackInterface inherits from MediaStreamTrackInterface, which has an id() > > method. However, I must confess that it's not clear to me whether this is > > actually the SSRC or something else. Could you please check if it can be used? > > I'd like to avoid adding another method to PeerConnectionInterface if we can > > avoid it.
solenberg@webrtc.org changed reviewers: + deadbeef@webrtc.org
Looping in Taylor for advice on how to get SSRC of remote audio streams. I'd
like to avoid adding more functions to PeerConnectionInterface. Taylor, do you
know if getting the SSRCs is possible?
From an email:
"I just coded and tested the alternative approach and found that
struct RtpEncodingParameters {
rtc::Optional<uint32_t> ssrc;
https://cs.chromium.org/chromium/src/third_party/webrtc/api/rtpparameters.h?s...
The ssrc may be empty since it is rtc::optional.
Therefore, it is not a reliable way to get ssrc."
George, it is not clear to me whether you tried and noticed that the ssrc field
would sometimes not be populated, or if you drew the conclusion from the field
being an rtc::Optional?
On 2016/12/08 20:16:35, the sun wrote:
> Ah yes, I thought I remembered that the label/id was actually populated with
the
> ssrc, but now that I look at the code I must have read things wrong.
>
> Another way is to iterate over PC::GetReceivers() and call the GetParameters()
> for those with media_type() audio. The RtpParameters struct contains a vector
of
> RtpEncodingParameters (of which there should be just one for audio streams),
> which has an SSRC.
>
> On 2016/12/08 16:45:18, GeorgeZ wrote:
> > Hi Fredrik,
> >
> > id() for MediaStreamTrackInterface is a string (ssrc is uint32_t) and is set
> by
> > PeerConnectionFactoryInterface::CreateAudioTrack(const std::string&
label,...)
> > The label is the id();
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionint...
> >
> > struct StreamParams has ssrcs but the ssrcs don't pass to MediaStream in
> > MediaStream::Create( const std::string& label).
> >
> > I agree with you that I should avoid to add extra PC function. Therefore if
> you
> > know some other way to get ssrc from existing PC interface, it will be
great.
> >
> > Thanks,
> >
> > On 2016/12/08 07:52:29, the sun wrote:
> > > LG - just let's sort out whether remote audio ssrcs can be fetched using
> > > existing PC methods or not.
> > >
> > >
> >
>
https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection.cc
> > > File webrtc/api/peerconnection.cc (right):
> > >
> > >
> >
>
https://codereview.webrtc.org/2539213003/diff/60001/webrtc/api/peerconnection...
> > > webrtc/api/peerconnection.cc:1356: std::vector<uint32_t>
> > > PeerConnection::GetRemoteAudioTrackSsrcs() {
> > > On 2016/12/07 18:28:17, GeorgeZ wrote:
> > > > On 2016/12/07 15:56:08, the sun wrote:
> > > > > Is this utility really needed? Can you use PC::remote_streams() and
> > iterate
> > > > over
> > > > > streams and call GetAudioTracks() for each stream, fetching the ssrc
by
> > > > calling
> > > > > id() on each track?
> > > >
> > > > PC::remote_streams() provides
> rtc::scoped_refptr<StreamCollectionInterface>.
> > > > Iterating StreamCollectionInterface can get MediaStreamInterface.
> > > > MediaStreamInterface has label() which is not ssrc.
> > > > AudioTrackInterface can get from MediaStreamInterface. However
> > > > AudioTrackInterface doesn't have ID or Label.
> > > >
> > > > Therefore, I am sure I can get ssrc from PC.
> > >
> > > AudioTrackInterface inherits from MediaStreamTrackInterface, which has an
> id()
> > > method. However, I must confess that it's not clear to me whether this is
> > > actually the SSRC or something else. Could you please check if it can be
> used?
> > > I'd like to avoid adding another method to PeerConnectionInterface if we
can
> > > avoid it.
Hi Fredrik, I removed the GetRemoteAudioTrackSSrcs() from PeerConnrction and tested with Taylor's CL https://codereview.webrtc.org/2568553002/. I uploaded a patch associated with this change. Thanks,
LGTM! Did you look into adding a test? I agree with you that it's not very important, since your application already uses the new interface, and it probably covers all use cases. But I still think that a test would be nice unless it's to hard to write.
LGTM Great work, thanks for you patience! I have left one comment for you to consider; if I'm not mistaken it will save you a little time. https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcmediaengine.cc:81: rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer) { Again, breaking compatibility in this function is probably going to make life harder for you, so only change the one called from PeerConnectionFactory. https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcmediaengine.h (right): https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcmediaengine.h:41: rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer); is this version of the function actually used with the AudioMixer argument? if not, revert the change to it, or you'll probably break downstream.
The CQ bit was checked by gyzhou@chromium.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: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/17064)
The CQ bit was checked by gyzhou@chromium.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.
Fredrik, Thanks for review. https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcmediaengine.h (right): https://codereview.webrtc.org/2539213003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcmediaengine.h:41: rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer); On 2016/12/12 20:50:05, the sun wrote: > is this version of the function actually used with the AudioMixer argument? if > not, revert the change to it, or you'll probably break downstream. Thanks for point this out.
The CQ bit was checked by gyzhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2539213003/#ps140001 (title: "try to fix OSX build issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481588375960970,
"parent_rev": "ab9e034ae5c1435dc09e7743ddc4a96399df6eaf", "commit_rev":
"13f65ce8aac63efd7107bb18e73b93a13b1b8511"}
Message was sent while issue was closed.
Description was changed from ========== Support external audio mixer in webrtc. An external audio mixer will be passed from PeerConnectionFactory to AudioTransportProxy. BUG=webrtc:6457 ========== to ========== Support external audio mixer in webrtc. An external audio mixer will be passed from PeerConnectionFactory to AudioTransportProxy. BUG=webrtc:6457 Review-Url: https://codereview.webrtc.org/2539213003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Support external audio mixer in webrtc. An external audio mixer will be passed from PeerConnectionFactory to AudioTransportProxy. BUG=webrtc:6457 Review-Url: https://codereview.webrtc.org/2539213003 ========== to ========== Support external audio mixer in webrtc. An external audio mixer will be passed from PeerConnectionFactory to AudioTransportProxy. BUG=webrtc:6457 Committed: https://crrev.com/f6bcac59e88c3be5ffd73bbb1098f2d82ee316a1 Cr-Commit-Position: refs/heads/master@{#15556} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f6bcac59e88c3be5ffd73bbb1098f2d82ee316a1 Cr-Commit-Position: refs/heads/master@{#15556}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:140001) has been created in https://codereview.webrtc.org/2562333003/ by gyzhou@chromium.org. The reason for reverting is: A interface change broke some downstream code in google3.. |
