Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513063003:
Add the OnAddTrack callback for Objective-C wrapper.
DescriptionRelanding: Add the OnAddTrack callback for Objective-C wrapper.
Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API
The callback function is called when a track is signaled by remote side and a new RtpReceiver is created.
The application can tell when tracks are added to the streams by listening to this callback.
BUG=webrtc:6112
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR comments. #
Total comments: 2
Patch Set 3 : Remove the stream entry from the dictionary when the stream is removed. #
Total comments: 16
Patch Set 4 : renaming. #
Total comments: 5
Patch Set 5 : Add optional method check. Use the stream->label() as the Key #
Total comments: 1
Patch Set 6 : Try to use NSStringFromStdString #Patch Set 7 : replace the NSStringFromStdString with stringForStdString #
Total comments: 2
Patch Set 8 : Fix the AppRTC. Remove the media stream caching. #
Messages
Total messages: 47 (26 generated)
Description was changed from ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:webrtc:6112 ========== to ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ==========
Patchset #2 (id:20001) has been deleted
zhihuang@webrtc.org changed reviewers: + tkchin@webrtc.org
Hi, I have made a simple change on obj-c api. Please take a look. Thanks,
https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:52: void OnAddTrack( what is the distinction between track and receiver? Strange that we're getting callback that a track is being added, but instead get a receiver. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:62: [NSMutableDictionary dictionary]; Shouldn't this be initialized in ctor? https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:205: RTCRtpReceiver* rtpReceiver = * on right RTCRtpReceiver *rtpReceiver here and elsewhere https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:219: RTCMediaStream* PeerConnectionDelegateAdapter::GetOrCreateObjcStream( I'd suggest placing these streams refs on peerconnection instead so that their lifetime is tied to the peerconnection. You can add private methods for this. Currently, if the delegate outlives the peerconnection, these streams in the dictionary won't get deallocated. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:112: didAddTrack:(RTCRtpReceiver*)rtpReceiver align : space after * (RTCPeerConnection *) NSArray<RTCMediaStream *> *) https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:113: attachedStreams:(NSArray<RTCMediaStream*>*)mediaStreams; Seems different than spec? https://www.w3.org/TR/webrtc/#dom-rtctrackevent Would've expected: - (void)peerConnection:(RTCPeerConnection *)peerConnection didAddTrack:(RTCMediaStreamTrack *)track streams:(NSArray<RTCMediaStream *> *)streams transceiver:(RTCRtpTransceiver *)transceiver;
Patchset #2 (id:40001) has been deleted
Please take another look. Thanks! https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:52: void OnAddTrack( On 2016/11/22 20:41:34, tkchin_webrtc wrote: > what is the distinction between track and receiver? Strange that we're getting > callback that a track is being added, but instead get a receiver. There is one track attached to one RtpReceiver. According to the spec, we need to provide RtpRecceiver, Track, MediaStreams and Transceivers. We can get the Track from RtpReceiver and we haven't implemented the Transceivers in native C++ yet. This is why we have these two parameters in the call back. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:62: [NSMutableDictionary dictionary]; On 2016/11/22 20:41:34, tkchin_webrtc wrote: > Shouldn't this be initialized in ctor? Done. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:205: RTCRtpReceiver* rtpReceiver = On 2016/11/22 20:41:34, tkchin_webrtc wrote: > * on right > RTCRtpReceiver *rtpReceiver > > here and elsewhere Done. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:219: RTCMediaStream* PeerConnectionDelegateAdapter::GetOrCreateObjcStream( On 2016/11/22 20:41:34, tkchin_webrtc wrote: > I'd suggest placing these streams refs on peerconnection instead so that their > lifetime is tied to the peerconnection. You can add private methods for this. > > Currently, if the delegate outlives the peerconnection, these streams in the > dictionary won't get deallocated. Ah, yes. This is a good point. Done. https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:112: didAddTrack:(RTCRtpReceiver*)rtpReceiver On 2016/11/22 20:41:34, tkchin_webrtc wrote: > align : > > space after * > (RTCPeerConnection *) > NSArray<RTCMediaStream *> *) Done. Should I align the ":" in previous functions such as "didChangeIceConnectionState" ? https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:113: attachedStreams:(NSArray<RTCMediaStream*>*)mediaStreams; On 2016/11/22 20:41:34, tkchin_webrtc wrote: > Seems different than spec? > https://www.w3.org/TR/webrtc/#dom-rtctrackevent > > Would've expected: > - (void)peerConnection:(RTCPeerConnection *)peerConnection > didAddTrack:(RTCMediaStreamTrack *)track > streams:(NSArray<RTCMediaStream *> *)streams > transceiver:(RTCRtpTransceiver *)transceiver; According to the spec, the Receiver is required. We can easily get the Track from the Receiver, not the other way around. We don't have transceivers yet. I think it would be fine as long as the native api provide enough info for JS api. I agree that this may seems a little bit confusing. Maybe make it clear with comments?
On 2016/11/22 23:38:42, Zhi Huang wrote: > Please take another look. Thanks! > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h (right): > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:52: void > OnAddTrack( > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > what is the distinction between track and receiver? Strange that we're getting > > callback that a track is being added, but instead get a receiver. > > There is one track attached to one RtpReceiver. > > According to the spec, we need to provide RtpRecceiver, Track, MediaStreams and > Transceivers. > > We can get the Track from RtpReceiver and we haven't implemented the > Transceivers in native C++ yet. This is why we have these two parameters in the > call back. > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Private.h:62: > [NSMutableDictionary dictionary]; > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > Shouldn't this be initialized in ctor? > > Done. > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:205: RTCRtpReceiver* > rtpReceiver = > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > * on right > > RTCRtpReceiver *rtpReceiver > > > > here and elsewhere > > Done. > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:219: RTCMediaStream* > PeerConnectionDelegateAdapter::GetOrCreateObjcStream( > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > I'd suggest placing these streams refs on peerconnection instead so that their > > lifetime is tied to the peerconnection. You can add private methods for this. > > > > Currently, if the delegate outlives the peerconnection, these streams in the > > dictionary won't get deallocated. > > Ah, yes. This is a good point. > > Done. > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:112: > didAddTrack:(RTCRtpReceiver*)rtpReceiver > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > align : > > > > space after * > > (RTCPeerConnection *) > > NSArray<RTCMediaStream *> *) > > Done. > > Should I align the ":" in previous functions such as > "didChangeIceConnectionState" ? > > https://codereview.webrtc.org/2513063003/diff/1/webrtc/sdk/objc/Framework/Hea... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:113: > attachedStreams:(NSArray<RTCMediaStream*>*)mediaStreams; > On 2016/11/22 20:41:34, tkchin_webrtc wrote: > > Seems different than spec? > > https://www.w3.org/TR/webrtc/#dom-rtctrackevent > > > > Would've expected: > > - (void)peerConnection:(RTCPeerConnection *)peerConnection > > didAddTrack:(RTCMediaStreamTrack *)track > > streams:(NSArray<RTCMediaStream *> *)streams > > transceiver:(RTCRtpTransceiver *)transceiver; > > According to the spec, the Receiver is required. We can easily get the Track > from the Receiver, not the other way around. > We don't have transceivers yet. > > I think it would be fine as long as the native api provide enough info for JS > api. > > I agree that this may seems a little bit confusing. Maybe make it clear with > comments? Hi, Any updates on this? Thanks.
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
Pinging Zeke. This looks good to me, aside from the one comment. https://codereview.webrtc.org/2513063003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:142: didRemoveStream:mediaStream]; The stream's entry in _nativeToObjcMediaStream should be removed here, to avoid a hanging reference to the C++ media stream (and by extension, its tracks, sources, etc).
Please take another look. Thanks. https://codereview.webrtc.org/2513063003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:142: didRemoveStream:mediaStream]; On 2017/02/15 21:34:45, Taylor Brandstetter wrote: > The stream's entry in _nativeToObjcMediaStream should be removed here, to avoid > a hanging reference to the C++ media stream (and by extension, its tracks, > sources, etc). Done.
https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: void PeerConnectionDelegateAdapter::OnAddTrack( strange that the method is OnAddTrack but it gives you a RtpReceiver is this temporary or permanent? What's in the w3c spec? https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:206: RTCRtpReceiver *rtpReceiver = use C++ style in C++ methods https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:218: didAddTrack:rtpReceiver ObjC naming usually follows type (so I'd expect peerConnection:didAddRtpReceiver:streams:). But if spec is that the method name is track but the type is rtpreceiver then we'll do that. attachedStreams->streams though https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:231: NSMutableDictionary *_nativeToObjcMediaStream; NSMutableDictionary<NSNumber, RTCMediaStream *> *_mediaStreamsByStreamId; https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:612: - (RTCMediaStream *)getOrCreateObjcStream: "get" prefix is forbidden in ObjC style - (RTCMediaStream *)mediaStreamForNativeStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream { } https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:615: _nativeToObjcMediaStream[[NSNumber numberWithLong:(long)stream.get()]]; Is there anything better than pointer value to use as a key here? use @((long)stream.get()); https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:625: - (void)removeObjcStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream { nit: - (void)removeNativeMediaStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream); https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:109: @optional either all these methods should be optional or none of them unless there's a specific reason this one needs to be optional?
https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: void PeerConnectionDelegateAdapter::OnAddTrack( On 2017/02/15 23:43:48, tkchin_webrtc wrote: > strange that the method is OnAddTrack but it gives you a RtpReceiver > is this temporary or permanent? What's in the w3c spec? That's what's in the spec (it's called "ontrack" there). Yes, OnAddTrack gives you an RtpReceiver, and RemoveTrack takes an RtpSender. Strange world we've come to. :) https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:615: _nativeToObjcMediaStream[[NSNumber numberWithLong:(long)stream.get()]]; On 2017/02/15 23:43:48, tkchin_webrtc wrote: > Is there anything better than pointer value to use as a key here? > > use @((long)stream.get()); They have an ID field which should be unique within a PeerConnection at any given time.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Hi, Please take another look. Thanks! https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:206: RTCRtpReceiver *rtpReceiver = On 2017/02/15 23:43:47, tkchin_webrtc wrote: > use C++ style in C++ methods Done. https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:218: didAddTrack:rtpReceiver On 2017/02/15 23:43:48, tkchin_webrtc wrote: > ObjC naming usually follows type (so I'd expect > peerConnection:didAddRtpReceiver:streams:). > But if spec is that the method name is track but the type is rtpreceiver then > we'll do that. attachedStreams->streams though Done. https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:231: NSMutableDictionary *_nativeToObjcMediaStream; On 2017/02/15 23:43:48, tkchin_webrtc wrote: > NSMutableDictionary<NSNumber, RTCMediaStream *> *_mediaStreamsByStreamId; Done. https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:612: - (RTCMediaStream *)getOrCreateObjcStream: On 2017/02/15 23:43:48, tkchin_webrtc wrote: > "get" prefix is forbidden in ObjC style > > - (RTCMediaStream > *)mediaStreamForNativeStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream > { > > } Done. https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:625: - (void)removeObjcStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream { On 2017/02/15 23:43:47, tkchin_webrtc wrote: > nit: > - > (void)removeNativeMediaStream:(rtc::scoped_refptr<webrtc::MediaStreamInterface>)stream); > Done. https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2513063003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:109: @optional On 2017/02/15 23:43:48, tkchin_webrtc wrote: > either all these methods should be optional or none of them > > unless there's a specific reason this one needs to be optional? I thought there would be at least some warnings if I add an non-optional method in the protocol and the downstream application doesn't implement it. This could be a temporary fix and I can leave a TODO here. Once the downstream apps add this method, we can remove the "optional". If it is OK to have some warnings or I'm just worrying too much about it, I can remove the "@optional".
https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:217: [peer_connection.delegate peerConnection:peer_connection since you marked it optional, you have to check if the delegate responds to the selector, otherwise this will cause a crash for old clients https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:615: _mediaStreamsByStreamId[[NSNumber numberWithLong:(long)stream.get()]]; per Taylor's comment, can we use the ID instead of the pointer value? also, use @( ), and use a local var for the key instead of creating multiple NSNumber
Patchset #5 (id:180001) has been deleted
Please take anther look. Thanks. :) https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:217: [peer_connection.delegate peerConnection:peer_connection On 2017/02/22 18:13:18, tkchin_webrtc wrote: > since you marked it optional, you have to check if the delegate responds to the > selector, otherwise this will cause a crash for old clients Done. https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:615: _mediaStreamsByStreamId[[NSNumber numberWithLong:(long)stream.get()]]; On 2017/02/22 18:13:18, tkchin_webrtc wrote: > per Taylor's comment, can we use the ID instead of the pointer value? > also, use @( ), and use a local var for the key instead of creating multiple > NSNumber Sorry I missed this comment. I assume Taylor means "label()". https://cs.chromium.org/chromium/src/third_party/webrtc/api/mediastreaminterf...
https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:615: _mediaStreamsByStreamId[[NSNumber numberWithLong:(long)stream.get()]]; On 2017/02/23 05:26:54, Zhi Huang wrote: > On 2017/02/22 18:13:18, tkchin_webrtc wrote: > > per Taylor's comment, can we use the ID instead of the pointer value? > > also, use @( ), and use a local var for the key instead of creating multiple > > NSNumber > > Sorry I missed this comment. > > I assume Taylor means "label()". > https://cs.chromium.org/chromium/src/third_party/webrtc/api/mediastreaminterf... > Yeah; I forgot, streams have "labels" while tracks have "IDs"
lgtm https://codereview.webrtc.org/2513063003/diff/200001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:617: RTCMediaStream *mediaStream = _mediaStreamsByStreamId[@(stream->label().c_str())]; might need to use NSStringFromStdString unless the string is guaranteed here to behave nicely
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19164)
On 2017/02/23 19:23:36, tkchin_webrtc wrote: > lgtm > > https://codereview.webrtc.org/2513063003/diff/200001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): > > https://codereview.webrtc.org/2513063003/diff/200001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:617: RTCMediaStream > *mediaStream = mailto:_mediaStreamsByStreamId[@(stream->label().c_str())]; > might need to use NSStringFromStdString > > unless the string is guaranteed here to behave nicely Do you mean stringForStdString? NSStringFromStdString is only available on IOS.
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19206)
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2513063003/#ps240001 (title: "replace the NSStringFromStdString with stringForStdString")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by zhihuang@webrtc.org
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": 240001, "attempt_start_ts": 1487969054733160,
"parent_rev": "c9bb7918f6c3dec9b3d583bbf2fc53e74ea875b6", "commit_rev":
"633f6fe0046131ed815098298b9a3120bac1d7a0"}
The CQ bit was unchecked by tkchin@webrtc.org
Message was sent while issue was closed.
Description was changed from ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ========== to ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 Review-Url: https://codereview.webrtc.org/2513063003 Cr-Commit-Position: refs/heads/master@{#16835} Committed: https://chromium.googlesource.com/external/webrtc/+/633f6fe0046131ed815098298... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/633f6fe0046131ed815098298...
Message was sent while issue was closed.
https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:30: #include "webrtc/sdk/objc/Framework/Classes/helpers.h" you don't need this include, NSString+StdString has what you need already https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:619: _mediaStreamsByStreamId[[NSString stringForStdString:stream->label()]]; nit: pull this out into a local so you're not creating multiple strings each time
Message was sent while issue was closed.
On 2017/02/24 20:54:15, tkchin_webrtc wrote: > https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): > > https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:30: #include > "webrtc/sdk/objc/Framework/Classes/helpers.h" > you don't need this include, NSString+StdString has what you need already > > https://codereview.webrtc.org/2513063003/diff/240001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:619: > _mediaStreamsByStreamId[[NSString stringForStdString:stream->label()]]; > nit: pull this out into a local so you're not creating multiple strings each > time I'll have a follow up CL to fix the comments. Sorry about that.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:240001) has been created in https://codereview.webrtc.org/2720753002/ by magjed@webrtc.org. The reason for reverting is: This CL breaks iOS AppRTCMobile. We don't have any automatic tests running on the bots yet, so please try AppRTCMobile locally before relanding. Stack trace: * thread #15: tid = 0x20e933, 0x0000000100488440 AppRTCMobile`webrtc::AudioRtpReceiver::OnFirstPacketReceived(this=0x0000000170156c60, channel=0x000000010511a600) + 48 at rtpreceiver.cc:133, name = 'Thread 0x0x10421b2a0', stop reason = EXC_BAD_ACCESS (code=1, address=0x1a1aac71979) * frame #0: 0x0000000100488440 AppRTCMobile`webrtc::AudioRtpReceiver::OnFirstPacketReceived(this=0x0000000170156c60, channel=0x000000010511a600) + 48 at rtpreceiver.cc:133 frame #1: 0x000000010048a3f8 AppRTCMobile`void sigslot::_opaque_connection::emitter<webrtc::AudioRtpReceiver, cricket::BaseChannel*>(self=0x000000017424b380, args=0x000000010511a600) + 184 at sigslot.h:391 frame #2: 0x00000001005a30ec AppRTCMobile`void sigslot::_opaque_connection::emit<cricket::BaseChannel*>(this=0x000000017424b380, args=0x000000010511a600) const + 56 at sigslot.h:381 frame #3: 0x00000001005a3094 AppRTCMobile`sigslot::signal_with_thread_policy<sigslot::single_threaded, cricket::BaseChannel*>::emit(this=0x000000010511a678, args=0x000000010511a600) + 504 at sigslot.h:615 frame #4: 0x000000010057ef5c AppRTCMobile`sigslot::signal_with_thread_policy<sigslot::single_threaded, cricket::BaseChannel*>::operator(this=0x000000010511a678, args=0x000000010511a600)(cricket::BaseChannel*) + 32 at sigslot.h:621 frame #5: 0x000000010057ef00 AppRTCMobile`cricket::BaseChannel::OnMessage(this=0x000000010511a600, pmsg=0x000000016e676db0) + 600 at channel.cc:1494 frame #6: 0x0000000100584a58 AppRTCMobile`cricket::VoiceChannel::OnMessage(this=0x000000010511a600, pmsg=0x000000016e676db0) + 152 at channel.cc:1909 frame #7: 0x000000010017c0dc AppRTCMobile`rtc::MessageQueue::Dispatch(this=0x000000010421b2a0, pmsg=0x000000016e676db0) + 336 at messagequeue.cc:538 frame #8: 0x00000001001d8efc AppRTCMobile`rtc::Thread::ProcessMessages(this=0x000000010421b2a0, cmsLoop=-1) + 228 at thread.cc:496 frame #9: 0x00000001001d8e08 AppRTCMobile`rtc::Thread::Run(this=0x000000010421b2a0) + 28 at thread.cc:327 frame #10: 0x00000001001d8b0c AppRTCMobile`rtc::Thread::PreRun(pv=0x000000017000f030) + 300 at thread.cc:316 frame #11: 0x00000001843f1850 libsystem_pthread.dylib`_pthread_body + 240 frame #12: 0x00000001843f1760 libsystem_pthread.dylib`_pthread_start + 284 frame #13: 0x00000001843eed94 libsystem_pthread.dylib`thread_start + 4.
Message was sent while issue was closed.
Description was changed from ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 Review-Url: https://codereview.webrtc.org/2513063003 Cr-Commit-Position: refs/heads/master@{#16835} Committed: https://chromium.googlesource.com/external/webrtc/+/633f6fe0046131ed815098298... ========== to ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 Review-Url: https://codereview.webrtc.org/2513063003 Cr-Commit-Position: refs/heads/master@{#16835} Committed: https://chromium.googlesource.com/external/webrtc/+/633f6fe0046131ed815098298... ==========
Description was changed from ========== Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 Review-Url: https://codereview.webrtc.org/2513063003 Cr-Commit-Position: refs/heads/master@{#16835} Committed: https://chromium.googlesource.com/external/webrtc/+/633f6fe0046131ed815098298... ========== to ========== Relanding: Add the OnAddTrack callback for Objective-C wrapper. Created an Obj-C wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ==========
zhihuang@webrtc.org changed reviewers: + skvlad@webrtc.org
The issue of the AppRTC is that the RTCMediaStreams are cached in RTCPeerConnection with a dictionary and they are not updated when the native media streams change such as adding or removing tracks. I have fix for the AppRTCMobile but it is not ideal. I noticed that we don't do caching for MediaStreams such as when adding or removing stream. So I remove the caching as well. Each time when adding a new track to a native stream, a new RTCMediaStream will be create so that it can sync with the native media stream. But from the application's view, the track are added to a different stream. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
