Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by skvlad Modified:
4 years, 7 months ago Reviewers:
Taylor Brandstetter, tkchin_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded the API to create an RTCRtpSender to the Objective C wrapper.
Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams.
BUG=
Committed: https://crrev.com/f3569c8a8f05556e0316596b31fc1bdca5c97418
Cr-Commit-Position: refs/heads/master@{#12570}
Patch Set 1 #Patch Set 2 : Rebased to the new directory structure #
Total comments: 20
Patch Set 3 : Code review feedback #
Total comments: 1
Patch Set 4 : Added constants for Audio and Video #
Total comments: 8
Patch Set 5 : Code review feedback #
Total comments: 4
Created: 4 years, 7 months ago
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders, which gives them more fine grained control than MediaStreams. BUG= ========== to ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams. BUG= ==========
skvlad@webrtc.org changed reviewers: + deadbeef@webrtc.org, tkchin@webrtc.org
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Ping. Please take a look.
https://codereview.webrtc.org/1888633002/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:515: RTCRtpSender *audioSender = [self createAudioSender]; unused locals? https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:317: _peerConnection->CreateSender([NSString stdStringForString:kind], nit: use locals for the stdStrings to make this more readable https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:43: RTCLogError(@"Failed to set parameters %@ for %@", parameters, self); maybe what we want here is the pointer? e.g RTCRtpSender(%p): Failed to set parameters: %@, self, parameters and if we want to track it down we can print info in init e.g. RTCRtpSender(%p): Created sender with id: %s, self, _nativeRtpSender->id() https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:63: return [NSString stringWithFormat:@"RTCRtpSender:%@", self.senderId]; nit: I've been trying to make our descriptions look like: RTCRtpSender { senderId: <senderId> } https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:174: @interface RTCPeerConnection (Media) We broke these out based on subsections of w3c spec. Is this part of media spec? https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:177: - (RTCRtpSender *)senderWithKind:(NSString *)kind streamId:(NSString *)streamId; it would be good to not pass arbitrary strings for kind. This should either be an enum or be pre-defined constants. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:28: @property(nonatomic) RTCRtpParameters *parameters; nit: nonatomic, copy? https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:35: @property(nonatomic) RTCMediaStreamTrack *track; I'm surprised this wasn't throwing a warning - this seems to be nullable since the impl returns nil sometimes. nonatomic, copy, nullable
https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:317: _peerConnection->CreateSender([NSString stdStringForString:kind], On 2016/04/27 20:22:20, tkchin_webrtc wrote: > nit: use locals for the stdStrings to make this more readable Done. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:43: RTCLogError(@"Failed to set parameters %@ for %@", parameters, self); On 2016/04/27 20:22:20, tkchin_webrtc wrote: > maybe what we want here is the pointer? > e.g RTCRtpSender(%p): Failed to set parameters: %@, self, parameters > > and if we want to track it down we can print info in init > e.g. RTCRtpSender(%p): Created sender with id: %s, self, _nativeRtpSender->id() I've made it print the pointer here, and the description in the constructor. Is reading a property in a constructor acceptable for something like the description? https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:63: return [NSString stringWithFormat:@"RTCRtpSender:%@", self.senderId]; On 2016/04/27 20:22:20, tkchin_webrtc wrote: > nit: I've been trying to make our descriptions look like: > > RTCRtpSender { > senderId: <senderId> > } All the descriptions I've found are formatted like this (no field names): RTCRtpSender { <senderId> } I've made it follow the format you mentioned above, though - if that's the eventual plan, it's probably better to start doing it the right way. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:174: @interface RTCPeerConnection (Media) On 2016/04/27 20:22:20, tkchin_webrtc wrote: > We broke these out based on subsections of w3c spec. Is this part of media spec? It is, https://www.w3.org/TR/webrtc/#rtp-media-api. The spec defines "addTransceiver" instead of "addSender", but transceivers are not implemented in the WebRTC stack yet. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:177: - (RTCRtpSender *)senderWithKind:(NSString *)kind streamId:(NSString *)streamId; On 2016/04/27 20:22:20, tkchin_webrtc wrote: > it would be good to not pass arbitrary strings for kind. This should either be > an enum or be pre-defined constants. This is how it is defined in the spec (https://www.w3.org/TR/webrtc/#widl-RTCPeerConnection-addTransceiver-RTCRtpTra...), and how it's implemented in both Java and C++ APIs. We can define constants, but then I believe we should do it across all implementations. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:28: @property(nonatomic) RTCRtpParameters *parameters; On 2016/04/27 20:22:20, tkchin_webrtc wrote: > nit: nonatomic, copy? Good catch, copy is the right thing here. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:35: @property(nonatomic) RTCMediaStreamTrack *track; On 2016/04/27 20:22:20, tkchin_webrtc wrote: > I'm surprised this wasn't throwing a warning - this seems to be nullable since > the impl returns nil sometimes. > > nonatomic, copy, nullable Is "copy" the right annotation for the behavior we do here, where we're pulling the native pointer out of the ObjC object and only storing that? Writing and reading the property effectively produces a copy of the RTCMediaStreamTrack, so I guess "copy" is reasonable...
lgtm % enum or constants for kind https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:43: RTCLogError(@"Failed to set parameters %@ for %@", parameters, self); On 2016/04/27 23:00:58, skvlad wrote: > On 2016/04/27 20:22:20, tkchin_webrtc wrote: > > maybe what we want here is the pointer? > > e.g RTCRtpSender(%p): Failed to set parameters: %@, self, parameters > > > > and if we want to track it down we can print info in init > > e.g. RTCRtpSender(%p): Created sender with id: %s, self, > _nativeRtpSender->id() > > I've made it print the pointer here, and the description in the constructor. Is > reading a property in a constructor acceptable for something like the > description? Yes, after self is available it's safe to access property usually. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:63: return [NSString stringWithFormat:@"RTCRtpSender:%@", self.senderId]; On 2016/04/27 23:00:58, skvlad wrote: > On 2016/04/27 20:22:20, tkchin_webrtc wrote: > > nit: I've been trying to make our descriptions look like: > > > > RTCRtpSender { > > senderId: <senderId> > > } > > All the descriptions I've found are formatted like this (no field names): > RTCRtpSender { > <senderId> > } > > I've made it follow the format you mentioned above, though - if that's the > eventual plan, it's probably better to start doing it the right way. Acknowledged. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:174: @interface RTCPeerConnection (Media) On 2016/04/27 23:00:58, skvlad wrote: > On 2016/04/27 20:22:20, tkchin_webrtc wrote: > > We broke these out based on subsections of w3c spec. Is this part of media > spec? > > It is, https://www.w3.org/TR/webrtc/#rtp-media-api. The spec defines > "addTransceiver" instead of "addSender", but transceivers are not implemented in > the WebRTC stack yet. Acknowledged. https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:177: - (RTCRtpSender *)senderWithKind:(NSString *)kind streamId:(NSString *)streamId; On 2016/04/27 23:00:58, skvlad wrote: > On 2016/04/27 20:22:20, tkchin_webrtc wrote: > > it would be good to not pass arbitrary strings for kind. This should either be > > an enum or be pre-defined constants. > > This is how it is defined in the spec > (https://www.w3.org/TR/webrtc/#widl-RTCPeerConnection-addTransceiver-RTCRtpTra...), > and how it's implemented in both Java and C++ APIs. We can define constants, but > then I believe we should do it across all implementations. We should probably have constants or enums. Otherwise all our consumers will be defining constants of their own just like how we did with the AppRTCDemo. At the very least we should comment on what "kinds" are available (I didn't dig into the spec to see what is available beyond "video" and "audio") https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): https://codereview.webrtc.org/1888633002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:35: @property(nonatomic) RTCMediaStreamTrack *track; On 2016/04/27 23:00:58, skvlad wrote: > On 2016/04/27 20:22:20, tkchin_webrtc wrote: > > I'm surprised this wasn't throwing a warning - this seems to be nullable since > > the impl returns nil sometimes. > > > > nonatomic, copy, nullable > > Is "copy" the right annotation for the behavior we do here, where we're pulling > the native pointer out of the ObjC object and only storing that? Writing and > reading the property effectively produces a copy of the RTCMediaStreamTrack, so > I guess "copy" is reasonable... Sort-of. Since we are overriding the setter these attributes are relatively meaningless and only serve as a hint to the caller that we are not taking a ref onto their object. https://codereview.webrtc.org/1888633002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm (right): https://codereview.webrtc.org/1888633002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpSender.mm:26: if (self = [super init]) { nit: assert nativeRtpSender not nil
I've added the constants. Please let me know if this looks ok to you.
https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCMediaStreamTrack.mm (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCMediaStreamTrack.mm:16: extern NSString * const kRTCMediaStreamTrackKindAudio = extern not needed here. Also, for these you can use syntactic sugar: kRTCMediaStreamTrackKindAudio = @(webrtc::MediaStreamTrackInterface::kAudioKind) https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:25: RTC_EXPORT RTC_EXPORT must be placed immediately before @interface https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:27: extern NSString * const kRTCMediaStreamTrackKindAudio; RTC_EXTERN https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:176: /** Create an RTCRtpSender with the specified kind and media stream ID. */ nit: add "See RTCMediaStreamTrack.h for available kinds."
Patchset #5 (id:120001) has been deleted
https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCMediaStreamTrack.mm (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCMediaStreamTrack.mm:16: extern NSString * const kRTCMediaStreamTrackKindAudio = On 2016/04/28 00:05:27, tkchin_webrtc wrote: > extern not needed here. Also, for these you can use syntactic sugar: > kRTCMediaStreamTrackKindAudio = @(webrtc::MediaStreamTrackInterface::kAudioKind) Didn't know about the @() syntax, very convenient. https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:25: RTC_EXPORT On 2016/04/28 00:05:27, tkchin_webrtc wrote: > RTC_EXPORT must be placed immediately before @interface Oops, sorry - I somehow mistook it for the NS_ASSUME_NONNULL_BEGIN macro. https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:27: extern NSString * const kRTCMediaStreamTrackKindAudio; On 2016/04/28 00:05:27, tkchin_webrtc wrote: > RTC_EXTERN Done. https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/1888633002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:176: /** Create an RTCRtpSender with the specified kind and media stream ID. */ On 2016/04/28 00:05:27, tkchin_webrtc wrote: > nit: add "See RTCMediaStreamTrack.h for available kinds." Done.
lgtm https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:314: - (RTCRtpSender *)senderWithKind:(NSString *)kind don't know if we need to check the kind here, or if the native C++ will error out if you give it a bad one https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h (right): https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:25: RTC_EXTERN NSString * const kRTCMediaStreamTrackKindAudio; const NSString * const?
https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:314: - (RTCRtpSender *)senderWithKind:(NSString *)kind On 2016/04/28 00:32:19, tkchin_webrtc wrote: > don't know if we need to check the kind here, or if the native C++ will error > out if you give it a bad one It will error out and return a null pointer. https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h (right): https://codereview.webrtc.org/1888633002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMediaStreamTrack.h:25: RTC_EXTERN NSString * const kRTCMediaStreamTrackKindAudio; On 2016/04/28 00:32:19, tkchin_webrtc wrote: > const NSString * const? Done.
lgtm still, but remove const NSString * from method args since NSString is an immutable class anyway and it's a nonstandard pattern (but leave it on the extern-ed global as a hint) https://codereview.webrtc.org/1888633002/diff/160001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/NSString+StdString.mm (right): https://codereview.webrtc.org/1888633002/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/NSString+StdString.mm:19: + (std::string)stdStringForString:(const NSString *)nsString { don't really need the const here since NSString is supposed to be immutable https://codereview.webrtc.org/1888633002/diff/160001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1888633002/diff/160001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:314: - (RTCRtpSender *)senderWithKind:(const NSString *)kind likewise, unusual to see const NSString * in method arguments
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888633002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888633002/140001
Message was sent while issue was closed.
Description was changed from ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams. BUG= ========== to ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams. BUG= ========== to ========== Added the API to create an RTCRtpSender to the Objective C wrapper. Objective C applications can now create new RTCRtpSenders and change their tracks, which gives them more fine grained control than MediaStreams. BUG= Committed: https://crrev.com/f3569c8a8f05556e0316596b31fc1bdca5c97418 Cr-Commit-Position: refs/heads/master@{#12570} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f3569c8a8f05556e0316596b31fc1bdca5c97418 Cr-Commit-Position: refs/heads/master@{#12570} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
