iOS: Add new RTCVideoSource interface

iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of RTCVideoCapturer.

BUG=webrtc:7177
TBR=tommi

Review-Url: https://codereview.webrtc.org/2745193002
Cr-Commit-Position: refs/heads/master@{#17409}
Committed: https://chromium.googlesource.com/external/webrtc/+/abb84b81281cfd0d699cde7d1ebbb318f7b630ef

[magjed_webrtc, 2017-03-15 15:21:05]

Description was changed from

==========
iOS: Add new RTCVideoSource interface

BUG=webrtc:7177
==========

to

==========
iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of
RTCVideoCapturer.

BUG=webrtc:7177
==========

[magjed_webrtc, 2017-03-15 15:21:05]

magjed@webrtc.org changed reviewers:
+ denicija@webrtc.org, sakal@webrtc.org

[magjed_webrtc, 2017-03-15 15:21:13]

Please take a look.

[sakal, 2017-03-15 15:52:21]

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:91: -
(RTCVideoSource *)videoSource {
Do we have to use the special naming convention here since we create a new
object?

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/objcvideotracksource.mm (right):

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/objcvideotracksource.mm:80:
std::swap(captured_width, captured_height);
It doesn't look like this code is needed.

[magjed_webrtc, 2017-03-15 20:42:04]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 20:42:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-15 21:14:06]

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

[commit-bot: I haz the power, 2017-03-15 21:14:07]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-03-16 12:07:02]

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:91: -
(RTCVideoSource *)videoSource {
On 2017/03/15 15:52:21, sakal wrote:
> Do we have to use the special naming convention here since we create a new
> object?

Hmm, good point. The existing functions like
avFoundationVideoSourceWithConstraints does not follow that convention though.
Are we leaking all the objects returned from this factory currently? I will ask
Daniela.

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/objcvideotracksource.mm (right):

https://codereview.webrtc.org/2745193002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/objcvideotracksource.mm:80:
std::swap(captured_width, captured_height);
On 2017/03/15 15:52:21, sakal wrote:
> It doesn't look like this code is needed.

True, it's copy-pasted from another class...

[magjed_webrtc, 2017-03-16 16:34:01]

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

[daniela-webrtc, 2017-03-20 14:23:10]

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:151: "objc/Framework/Classes/RTCVideoCapturer.mm",
RTCVideoCapturer.mm -> RTCVideoCapturer.m
There is no need for this class to be objective cpp as it's intended to only
interact with objc interfaces.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:91: -
(RTCVideoSource *)newVideoSource {
No need for the `new` prefix here. ARC will handle this just fine :)

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm:45: -
(void)capturer:(RTCVideoCapturer*)capturer
Pointer spacing

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:20: -
(instancetype)initWithDelegate:(__weak id<RTCVideoCapturerDelegate>)delegate;
We don't need to enforce invokers of the method to pass weak reference as we're
gonna "waekify" it ourselves.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:21:
@property(nonatomic, readonly) id<RTCVideoCapturerDelegate> delegate;
Do we need to expose the delegate as property here? I think not as it's injected
in the initializer.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:24: @protocol
RTCVideoCapturerDelegate
Nit: It's common to declare the protocol on top of the class like so

@class RTCVideoCapturer;

@protocol RTCVideoCapturerDelegate
...
@end

@interface RTCVideoCapturer : NSObject
...
@end

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:25: -
(void)capturer:(RTCVideoCapturer*)capturer
Nit: RTCVideoCapturer* -> RTCVideoCapturer *

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:26:
didCaptureVideoFrame:(RTCVideoFrame*)frame;
Same spacing comment

[daniela-webrtc, 2017-03-20 15:18:31]

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:21:
@property(nonatomic, readonly) id<RTCVideoCapturerDelegate> delegate;
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> Do we need to expose the delegate as property here? I think not as it's
injected
> in the initializer.

Ah just saw that we need it for subclassing. 
In that case
@property (nonatomic, readonly, weak) id<RTCVideoCapturerDelegate> delegate;

Default is strong and we don't want that.

[magjed_webrtc, 2017-03-27 12:35:02]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[magjed_webrtc, 2017-03-27 12:35:11]

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:151: "objc/Framework/Classes/RTCVideoCapturer.mm",
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> RTCVideoCapturer.mm -> RTCVideoCapturer.m
> There is no need for this class to be objective cpp as it's intended to only
> interact with objc interfaces.

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:91: -
(RTCVideoSource *)newVideoSource {
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> No need for the `new` prefix here. ARC will handle this just fine :)

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm:45: -
(void)capturer:(RTCVideoCapturer*)capturer
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> Pointer spacing

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:20: -
(instancetype)initWithDelegate:(__weak id<RTCVideoCapturerDelegate>)delegate;
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> We don't need to enforce invokers of the method to pass weak reference as
we're
> gonna "waekify" it ourselves.

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:21:
@property(nonatomic, readonly) id<RTCVideoCapturerDelegate> delegate;
On 2017/03/20 15:18:30, daniela-webrtc wrote:
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > Do we need to expose the delegate as property here? I think not as it's
> injected
> > in the initializer.
> 
> Ah just saw that we need it for subclassing. 
> In that case
> @property (nonatomic, readonly, weak) id<RTCVideoCapturerDelegate> delegate;
> 
> Default is strong and we don't want that.

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:24: @protocol
RTCVideoCapturerDelegate
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> Nit: It's common to declare the protocol on top of the class like so
> 
> @class RTCVideoCapturer;
> 
> @protocol RTCVideoCapturerDelegate
> ...
> @end
> 
> @interface RTCVideoCapturer : NSObject
> ...
> @end

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:25: -
(void)capturer:(RTCVideoCapturer*)capturer
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> Nit: RTCVideoCapturer* -> RTCVideoCapturer *

Done.

https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:26:
didCaptureVideoFrame:(RTCVideoFrame*)frame;
On 2017/03/20 14:23:10, daniela-webrtc wrote:
> Same spacing comment

Done.

[commit-bot: I haz the power, 2017-03-27 12:35:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2017-03-27 12:38:27]

Patchset #4 (id:60001) has been deleted

[daniela-webrtc, 2017-03-27 13:15:47]

On 2017/03/27 12:35:11, magjed_webrtc wrote:
> https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn
> File webrtc/sdk/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/BUILD.gn#newco...
> webrtc/sdk/BUILD.gn:151: "objc/Framework/Classes/RTCVideoCapturer.mm",
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > RTCVideoCapturer.mm -> RTCVideoCapturer.m
> > There is no need for this class to be objective cpp as it's intended to only
> > interact with objc interfaces.
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:91: -
> (RTCVideoSource *)newVideoSource {
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > No need for the `new` prefix here. ARC will handle this just fine :)
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> File webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm (right):
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Classes/RTCVideoSource.mm:45: -
> (void)capturer:(RTCVideoCapturer*)capturer
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > Pointer spacing
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:20: -
> (instancetype)initWithDelegate:(__weak id<RTCVideoCapturerDelegate>)delegate;
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > We don't need to enforce invokers of the method to pass weak reference as
> we're
> > gonna "waekify" it ourselves.
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:21:
> @property(nonatomic, readonly) id<RTCVideoCapturerDelegate> delegate;
> On 2017/03/20 15:18:30, daniela-webrtc wrote:
> > On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > > Do we need to expose the delegate as property here? I think not as it's
> > injected
> > > in the initializer.
> > 
> > Ah just saw that we need it for subclassing. 
> > In that case
> > @property (nonatomic, readonly, weak) id<RTCVideoCapturerDelegate> delegate;
> > 
> > Default is strong and we don't want that.
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:24: @protocol
> RTCVideoCapturerDelegate
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > Nit: It's common to declare the protocol on top of the class like so
> > 
> > @class RTCVideoCapturer;
> > 
> > @protocol RTCVideoCapturerDelegate
> > ...
> > @end
> > 
> > @interface RTCVideoCapturer : NSObject
> > ...
> > @end
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:25: -
> (void)capturer:(RTCVideoCapturer*)capturer
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > Nit: RTCVideoCapturer* -> RTCVideoCapturer *
> 
> Done.
> 
>
https://codereview.webrtc.org/2745193002/diff/40001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:26:
> didCaptureVideoFrame:(RTCVideoFrame*)frame;
> On 2017/03/20 14:23:10, daniela-webrtc wrote:
> > Same spacing comment
> 
> Done.

LGTM

[sakal, 2017-03-27 13:40:23]

lgtm

[magjed_webrtc, 2017-03-28 07:20:41]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-03-28 07:20:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-28 07:26:41]

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

[commit-bot: I haz the power, 2017-03-28 07:26:42]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15436)

[magjed_webrtc, 2017-03-28 08:33:52]

Description was changed from

==========
iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of
RTCVideoCapturer.

BUG=webrtc:7177
==========

to

==========
iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of
RTCVideoCapturer.

BUG=webrtc:7177
TBR=tommi
==========

[magjed_webrtc, 2017-03-28 08:34:25]

TBRing tommi@ for webrtc/api/video/i420_buffer.cc change.

[magjed_webrtc, 2017-03-28 08:34:32]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-03-28 08:34:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-28 08:56:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490690072179050,
"parent_rev": "c4adacfb15cfd4269c7ca0f0857ddb9bef520182", "commit_rev":
"abb84b81281cfd0d699cde7d1ebbb318f7b630ef"}

[commit-bot: I haz the power, 2017-03-28 08:56:45]

Description was changed from

==========
iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of
RTCVideoCapturer.

BUG=webrtc:7177
TBR=tommi
==========

to

==========
iOS: Add new RTCVideoSource interface

The new RTCVideoSource interface can be used by custom implementations of
RTCVideoCapturer.

BUG=webrtc:7177
TBR=tommi

Review-Url: https://codereview.webrtc.org/2745193002
Cr-Commit-Position: refs/heads/master@{#17409}
Committed:
https://chromium.googlesource.com/external/webrtc/+/abb84b81281cfd0d699cde7d1...
==========

[commit-bot: I haz the power, 2017-03-28 08:56:46]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/abb84b81281cfd0d699cde7d1...

[tkchin_webrtc, 2017-04-06 18:01:52]

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:92:
rtc::scoped_refptr<webrtc::ObjcVideoTrackSource> objc_video_track_source(
ObjC naming in ObjC methods, so camel case

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m:16: 
@synthesize delegate = _delegate;

then you don't need to declare the private ivar above and the getter below

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m:18: if (self = [super
init]) {
assert delegate not nil

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/objcvideotracksource.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/objcvideotracksource.h:14: #import
<WebRTC/RTCVideoFrame.h>
Might be better to forward declare RTCVideoFrame so that this can be #include-ed
in pure C++. There's a macro in RTCMacros.h already for this.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h
(right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:49: -
(RTCVideoSource *)videoSource;
needs doco? How should clients use this?

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:19: @protocol
RTCVideoCapturerDelegate
@protocol RTCVideoCapturerDelegate <NSObject>

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:23: @interface
RTCVideoCapturer : NSObject
The RTC_EXPORT should appear before this one, not the protocol I think.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:25:
@property(nonatomic, readonly, weak) id<RTCVideoCapturerDelegate> delegate;
style: properties before methods

add blank lines to make it more readable

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:21: @interface
RTCVideoSource : RTCMediaSource<RTCVideoCapturerDelegate>
space after RTCMediaSource

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:25: //
RTCVideoCapturerDelegate protocol implementation.
not required (part of decl above). Can be removed.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:26: -
(void)capturer:(RTCVideoCapturer*)capturer
didCaptureVideoFrame:(RTCVideoFrame*)frame;
style:
(RTCVideoCapturer *)
(RTCVideoFrame *)

[magjed_webrtc, 2017-04-10 12:12:50]

I addressed the feedback in a new CL: https://codereview.webrtc.org/2812533003.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:92:
rtc::scoped_refptr<webrtc::ObjcVideoTrackSource> objc_video_track_source(
On 2017/04/06 18:01:51, tkchin_webrtc wrote:
> ObjC naming in ObjC methods, so camel case

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m:16: 
On 2017/04/06 18:01:51, tkchin_webrtc wrote:
> @synthesize delegate = _delegate;
> 
> then you don't need to declare the private ivar above and the getter below

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCVideoCapturer.m:18: if (self = [super
init]) {
On 2017/04/06 18:01:51, tkchin_webrtc wrote:
> assert delegate not nil

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/objcvideotracksource.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/objcvideotracksource.h:14: #import
<WebRTC/RTCVideoFrame.h>
On 2017/04/06 18:01:51, tkchin_webrtc wrote:
> Might be better to forward declare RTCVideoFrame so that this can be
#include-ed
> in pure C++. There's a macro in RTCMacros.h already for this.

Cool, done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h
(right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:49: -
(RTCVideoSource *)videoSource;
On 2017/04/06 18:01:51, tkchin_webrtc wrote:
> needs doco? How should clients use this?

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:19: @protocol
RTCVideoCapturerDelegate
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> @protocol RTCVideoCapturerDelegate <NSObject>

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:23: @interface
RTCVideoCapturer : NSObject
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> The RTC_EXPORT should appear before this one, not the protocol I think.

I think I need it for both actually.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoCapturer.h:25:
@property(nonatomic, readonly, weak) id<RTCVideoCapturerDelegate> delegate;
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> style: properties before methods
> 
> add blank lines to make it more readable

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h (right):

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:21: @interface
RTCVideoSource : RTCMediaSource<RTCVideoCapturerDelegate>
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> space after RTCMediaSource

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:25: //
RTCVideoCapturerDelegate protocol implementation.
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> not required (part of decl above). Can be removed.

Done.

https://codereview.webrtc.org/2745193002/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoSource.h:26: -
(void)capturer:(RTCVideoCapturer*)capturer
didCaptureVideoFrame:(RTCVideoFrame*)frame;
On 2017/04/06 18:01:52, tkchin_webrtc wrote:
> style:
> (RTCVideoCapturer *)
> (RTCVideoFrame *)

Acknowledged.