Tweaks for new Objective-C API.

Tweaks for new Objective-C API.

BUG=

Committed: https://crrev.com/a2f7798ec2fe29652bade4ba6b3f29c65328360e
Cr-Commit-Position: refs/heads/master@{#11872}

[hjon_webrtc, 2016-02-12 23:55:36]

hjon@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[tkchin_webrtc, 2016-02-16 19:11:02]

lgtm

[hjon_webrtc, 2016-02-19 20:55:34]

ptal

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
File webrtc/api/objc/RTCVideoTrack.mm (right):

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
webrtc/api/objc/RTCVideoTrack.mm:36: self = [super initWithNativeTrack:track
type:RTCMediaStreamTrackTypeVideo];
If |source| was actually an RTCAVFoundationVideoSource, calling self resulted in
|_source| becoming an RTCVideoSource instead. In turn, that caused the video
preview in AppRTCDemo to not appear because of this line:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....

For the time being, I was concentrating on getting AppRTCDemo working, so there
might be a better way to fix this that I haven't seen yet.

[tkchin_webrtc, 2016-02-19 23:26:38]

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
File webrtc/api/objc/RTCVideoTrack.mm (right):

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
webrtc/api/objc/RTCVideoTrack.mm:36: self = [super initWithNativeTrack:track
type:RTCMediaStreamTrackTypeVideo];
On 2016/02/19 20:55:34, hjon_webrtc wrote:
> If |source| was actually an RTCAVFoundationVideoSource, calling self resulted
in
> |_source| becoming an RTCVideoSource instead. In turn, that caused the video
> preview in AppRTCDemo to not appear because of this line:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> 
> For the time being, I was concentrating on getting AppRTCDemo working, so
there
> might be a better way to fix this that I haven't seen yet.

you don't need to re-init the adapters, that should already be there.

to avoid the double source assign/init, lazily load the source in the getter for
the initWithNative case, and then assign source directly for the initWithFactory
case like you already have it.

- (RTCVideoSource *)source {
  if (!_source) {
    _source = ...<stuff from other ctor>
  }
  return _source;
}

[hjon_webrtc, 2016-02-19 23:58:36]

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
File webrtc/api/objc/RTCVideoTrack.mm (right):

https://codereview.webrtc.org/1696673003/diff/20001/webrtc/api/objc/RTCVideoT...
webrtc/api/objc/RTCVideoTrack.mm:36: self = [super initWithNativeTrack:track
type:RTCMediaStreamTrackTypeVideo];
On 2016/02/19 23:26:38, tkchin_webrtc wrote:
> On 2016/02/19 20:55:34, hjon_webrtc wrote:
> > If |source| was actually an RTCAVFoundationVideoSource, calling self
resulted
> in
> > |_source| becoming an RTCVideoSource instead. In turn, that caused the video
> > preview in AppRTCDemo to not appear because of this line:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > 
> > For the time being, I was concentrating on getting AppRTCDemo working, so
> there
> > might be a better way to fix this that I haven't seen yet.
> 
> you don't need to re-init the adapters, that should already be there.
> 
> to avoid the double source assign/init, lazily load the source in the getter
for
> the initWithNative case, and then assign source directly for the
initWithFactory
> case like you already have it.
> 
> - (RTCVideoSource *)source {
>   if (!_source) {
>     _source = ...<stuff from other ctor>
>   }
>   return _source;
> }

Done.

[tkchin_webrtc, 2016-02-20 00:17:32]

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
File webrtc/api/objc/RTCConfiguration.h (left):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
webrtc/api/objc/RTCConfiguration.h:60: @property(nonatomic, assign)
RTCIceTransportPolicy iceTransportPolicy;
did compiler complain?

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
webrtc/api/objc/RTCPeerConnection.mm:206: nativeConstraints.get(),
get or release?

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCVideoT...
File webrtc/api/objc/RTCVideoTrack.mm (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCVideoT...
webrtc/api/objc/RTCVideoTrack.mm:100:
self.nativeVideoTrack->RemoveRenderer(adapterToRemove.nativeVideoRenderer);
grab the adapter based on the index since we're already tracking that

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/base/objc/RTCSSLAd...
File webrtc/base/objc/RTCSSLAdapter.h (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/base/objc/RTCSSLAd...
webrtc/base/objc/RTCSSLAdapter.h:17: #if defined(__cplusplus)
add something to the prefix header or separate macro file along the lines of

#if defined(__cplusplus)
#define RTC_EXPORT extern "C"
#else
#define RTC_EXPORT extern
#endif

[hjon_webrtc, 2016-02-22 23:03:06]

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
File webrtc/api/objc/RTCConfiguration.h (left):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
webrtc/api/objc/RTCConfiguration.h:60: @property(nonatomic, assign)
RTCIceTransportPolicy iceTransportPolicy;
On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> did compiler complain?

No, was just trying to be consistent as it seemed we were going without assign
in other places.

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
webrtc/api/objc/RTCPeerConnection.mm:206: nativeConstraints.get(),
On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> get or release?

If I'm thinking about this correctly (and correctly tracing through C++), I
think it should be get. It looks like C++ is merely finding the mandatory and
optional constraints, but not taking ownership.

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCVideoT...
File webrtc/api/objc/RTCVideoTrack.mm (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCVideoT...
webrtc/api/objc/RTCVideoTrack.mm:100:
self.nativeVideoTrack->RemoveRenderer(adapterToRemove.nativeVideoRenderer);
On 2016/02/20 00:17:32, tkchin_webrtc wrote:
> grab the adapter based on the index since we're already tracking that

Done.

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/base/objc/RTCSSLAd...
File webrtc/base/objc/RTCSSLAdapter.h (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/base/objc/RTCSSLAd...
webrtc/base/objc/RTCSSLAdapter.h:17: #if defined(__cplusplus)
On 2016/02/20 00:17:32, tkchin_webrtc wrote:
> add something to the prefix header or separate macro file along the lines of
> 
> #if defined(__cplusplus)
> #define RTC_EXPORT extern "C"
> #else
> #define RTC_EXPORT extern
> #endif

Done.

[tkchin_webrtc, 2016-02-23 00:22:26]

lgtm

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
File webrtc/api/objc/RTCConfiguration.h (left):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
webrtc/api/objc/RTCConfiguration.h:60: @property(nonatomic, assign)
RTCIceTransportPolicy iceTransportPolicy;
On 2016/02/22 23:03:06, hjon_webrtc wrote:
> On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> > did compiler complain?
> 
> No, was just trying to be consistent as it seemed we were going without assign
> in other places.

Where?

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCPeerCo...
webrtc/api/objc/RTCPeerConnection.mm:206: nativeConstraints.get(),
On 2016/02/22 23:03:06, hjon_webrtc wrote:
> On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> > get or release?
> 
> If I'm thinking about this correctly (and correctly tracing through C++), I
> think it should be get. It looks like C++ is merely finding the mandatory and
> optional constraints, but not taking ownership.

Ok.

https://codereview.webrtc.org/1696673003/diff/100001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1696673003/diff/100001/webrtc/api/api.gyp#newco...
webrtc/api/api.gyp:197: 'link_settings': {
why do we need this?

[hjon_webrtc, 2016-02-23 21:06:12]

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
File webrtc/api/objc/RTCConfiguration.h (left):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
webrtc/api/objc/RTCConfiguration.h:60: @property(nonatomic, assign)
RTCIceTransportPolicy iceTransportPolicy;
On 2016/02/23 00:22:26, tkchin_webrtc wrote:
> On 2016/02/22 23:03:06, hjon_webrtc wrote:
> > On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> > > did compiler complain?
> > 
> > No, was just trying to be consistent as it seemed we were going without
assign
> > in other places.
> 
> Where?

I took another look and we currently have a mix of using assign or not. Would
you prefer assign for all enums and primitives?

https://codereview.webrtc.org/1696673003/diff/100001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1696673003/diff/100001/webrtc/api/api.gyp#newco...
webrtc/api/api.gyp:197: 'link_settings': {
On 2016/02/23 00:22:26, tkchin_webrtc wrote:
> why do we need this? 

Your question prompted me to dig into this again. Without this, I received
linker errors about undefined symbols (see below for an example). I compared
against the libjingle_peerconnection_objc target, which also included this link
setting, so I included it to see if it fixed the problem, which it did.

That confuses me a bit, though. Shouldn't linking against libc++ work? As far as
I can tell, this is the only place in AppRTCDemo's dependency chain where
libstdc++ is explicitly used.

It might be easier to show you IRL, so I'll plan on doing that tomorrow.

Undefined symbols for architecture armv7:
  "void std::__1::__sort<std::__1::__less<signed char, signed char>&, signed
char*>(signed char*, signed char*, std::__1::__less<signed char, signed
char>&)", referenced from:
      webrtc::Sort(void*, unsigned int, webrtc::Type) in
libsystem_wrappers.a(system_wrappers.sort.o)
      void boost::detail::spread_sort_rec<signed char*, int, signed char>(signed
char*, signed char*, std::__1::vector<signed char*, std::__1::allocator<signed
char*> >&, unsigned int, std::__1::vector<unsigned long,
std::__1::allocator<unsigned long> >&) in
libsystem_wrappers.a(system_wrappers.sort.o)

[tkchin_webrtc, 2016-02-24 22:45:12]

re: libstdc++ if the old target had it then we can carry it forward safely.

still lgtm

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
File webrtc/api/objc/RTCConfiguration.h (left):

https://codereview.webrtc.org/1696673003/diff/80001/webrtc/api/objc/RTCConfig...
webrtc/api/objc/RTCConfiguration.h:60: @property(nonatomic, assign)
RTCIceTransportPolicy iceTransportPolicy;
On 2016/02/23 21:06:12, hjon_webrtc wrote:
> On 2016/02/23 00:22:26, tkchin_webrtc wrote:
> > On 2016/02/22 23:03:06, hjon_webrtc wrote:
> > > On 2016/02/20 00:17:31, tkchin_webrtc wrote:
> > > > did compiler complain?
> > > 
> > > No, was just trying to be consistent as it seemed we were going without
> assign
> > > in other places.
> > 
> > Where?
> 
> I took another look and we currently have a mix of using assign or not. Would
> you prefer assign for all enums and primitives?

I'd prefer having the explicit assign.

[hjon_webrtc, 2016-02-25 17:49:39]

Description was changed from

==========
Tweaks for new Objective-C API.

Add convenience methods for converting between RTCSdpType and NSString.
Allow nil to be passed to -renderFrame:.

BUG=
==========

to

==========
Tweaks for new Objective-C API.

BUG=
==========

[hjon_webrtc, 2016-02-25 22:40:11]

ptal

[tkchin_webrtc, 2016-02-26 00:45:25]

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCDataC...
File webrtc/api/objc/RTCDataChannel.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCDataC...
webrtc/api/objc/RTCDataChannel.h:71: @property(nonatomic, assign, readonly) BOOL
isOrdered;
don't need assign for readonly stuff since assign only applies to setters

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCIceCa...
File webrtc/api/objc/RTCIceCandidate.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCIceCa...
webrtc/api/objc/RTCIceCandidate.h:28: @property(nonatomic, assign, readonly)
NSInteger sdpMLineIndex;
ditto no assign + readonly

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCMedia...
File webrtc/api/objc/RTCMediaStreamTrack.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.h:41: @property(nonatomic, assign, readonly)
RTCMediaStreamTrackState readyState;
ditto no assign + readonly

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCPeerC...
File webrtc/api/objc/RTCPeerConnection.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCPeerC...
webrtc/api/objc/RTCPeerConnection.h:115: @property(nonatomic, assign, readonly)
RTCSignalingState signalingState;
ditto no assign + readonly

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCSessi...
File webrtc/api/objc/RTCSessionDescription.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCSessi...
webrtc/api/objc/RTCSessionDescription.h:28: @property(nonatomic, assign,
readonly) RTCSdpType type;
ditto no assign + readonly

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCStats...
File webrtc/api/objc/RTCStatsReport.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCStats...
webrtc/api/objc/RTCStatsReport.h:20: @property(nonatomic, assign, readonly)
CFTimeInterval timestamp;
ditto no assign + readonly

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCVideo...
File webrtc/api/objc/RTCVideoFrame.h (right):

https://codereview.webrtc.org/1696673003/diff/140001/webrtc/api/objc/RTCVideo...
webrtc/api/objc/RTCVideoFrame.h:19: @property(nonatomic, assign, readonly)
size_t width;
ditto no assign + readonly

[hjon_webrtc, 2016-02-26 16:44:01]

ptal

[tkchin_webrtc, 2016-02-26 20:10:05]

On 2016/02/26 16:44:01, hjon_webrtc wrote:
> ptal

lgtm

[hjon_webrtc, 2016-02-26 20:59:29]

The CQ bit was checked by hjon@webrtc.org

[commit-bot: I haz the power, 2016-02-26 20:59:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/160001

[commit-bot: I haz the power, 2016-02-26 21:03:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 21:03:07]

Exceeded global retry quota

[hjon_webrtc, 2016-02-29 18:30:45]

The CQ bit was checked by hjon@webrtc.org

[commit-bot: I haz the power, 2016-02-29 18:30:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/160001

[commit-bot: I haz the power, 2016-02-29 18:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-29 18:33:59]

Try jobs failed on following builders:
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12732)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13082)

[hjon_webrtc, 2016-03-01 21:46:39]

The CQ bit was checked by hjon@webrtc.org

[hjon_webrtc, 2016-03-01 21:46:39]

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/1696673003/#ps180001 (title:
"Don't use object subscripting")

[commit-bot: I haz the power, 2016-03-01 21:46:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/180001

[commit-bot: I haz the power, 2016-03-01 23:47:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 23:47:26]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hjon_webrtc, 2016-03-03 22:59:38]

The CQ bit was checked by hjon@webrtc.org

[commit-bot: I haz the power, 2016-03-03 22:59:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/180001

[commit-bot: I haz the power, 2016-03-04 01:00:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 01:00:26]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hjon_webrtc, 2016-03-04 01:03:01]

The CQ bit was checked by hjon@webrtc.org

[commit-bot: I haz the power, 2016-03-04 01:03:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/180001

[commit-bot: I haz the power, 2016-03-04 03:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 03:04:11]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hjon_webrtc, 2016-03-04 15:06:59]

The CQ bit was checked by hjon@webrtc.org

[commit-bot: I haz the power, 2016-03-04 15:07:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696673003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696673003/180001

[commit-bot: I haz the power, 2016-03-04 15:09:13]

Description was changed from

==========
Tweaks for new Objective-C API.

BUG=
==========

to

==========
Tweaks for new Objective-C API.

BUG=
==========

[commit-bot: I haz the power, 2016-03-04 15:09:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-04 15:09:25]

Description was changed from

==========
Tweaks for new Objective-C API.

BUG=
==========

to

==========
Tweaks for new Objective-C API.

BUG=

Committed: https://crrev.com/a2f7798ec2fe29652bade4ba6b3f29c65328360e
Cr-Commit-Position: refs/heads/master@{#11872}
==========

[commit-bot: I haz the power, 2016-03-04 15:09:26]

Patchset 10 (id:??) landed as
https://crrev.com/a2f7798ec2fe29652bade4ba6b3f29c65328360e
Cr-Commit-Position: refs/heads/master@{#11872}