Unify the macOS and iOS capturer implementations

Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968, webrtc:6275, webrtc:6333
Committed: https://crrev.com/242d8bdddd77109781cbb70c59d161be7566ac98
Cr-Commit-Position: refs/heads/master@{#14418}

[kthelgason, 2016-09-06 19:04:54]

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

[commit-bot: I haz the power, 2016-09-06 19:05:06]

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, 2016-09-06 19:10:53]

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

[commit-bot: I haz the power, 2016-09-06 19:10:54]

Dry run: Try jobs failed on following builders:
  mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/333)

[kthelgason, 2016-09-07 08:41:41]

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

[commit-bot: I haz the power, 2016-09-07 08:41:51]

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, 2016-09-07 08:54:48]

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

[commit-bot: I haz the power, 2016-09-07 08:54:49]

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

[kthelgason, 2016-09-07 12:50:10]

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

[kthelgason, 2016-09-07 12:50:11]

This CL depends on https://codereview.webrtc.org/2317023002/ landing, to fix the
presubmit warnings.

[magjed_webrtc, 2016-09-08 13:54:50]

Description was changed from

==========
unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275
==========

to

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275
==========

[magjed_webrtc, 2016-09-08 14:07:00]

lgtm. This is a good first step to get rid of QTKit.

After this CL, we still have two AV Foundation capturers. We should try to use
avfoundationvideocapturer.mm only. AFAIK, webrtc/modules/video_capture/ is
legacy that is only used for the video_loopback tool. As a first step, we can
probably make something Mac-specific to unify the iOS/Mac capturers. In the long
run, we should probably try to get rid of VideoCaptureImpl, VideoCaptureModule,
and VideoCaptureExternal altogether and update the Win and Linux capture
implementation to use the new interface.

[kthelgason, 2016-09-08 17:04:47]

On 2016/09/08 14:07:00, magjed_webrtc wrote:
> After this CL, we still have two AV Foundation capturers. We should try to use
> avfoundationvideocapturer.mm only. AFAIK, webrtc/modules/video_capture/ is
> legacy that is only used for the video_loopback tool. As a first step, we can
> probably make something Mac-specific to unify the iOS/Mac capturers. In the
long
> run, we should probably try to get rid of VideoCaptureImpl,
VideoCaptureModule,
> and VideoCaptureExternal altogether and update the Win and Linux capture
> implementation to use the new interface.

Yup, I agree. I thought it would be worthwhile to at least get rid of the stuff
that links against QTKit.

Perhaps it would be worth opening an issue to track getting rid of those
classes?

[magjed_webrtc, 2016-09-09 11:00:59]

On 2016/09/08 17:04:47, kthelgason wrote:
> On 2016/09/08 14:07:00, magjed_webrtc wrote:
> > After this CL, we still have two AV Foundation capturers. We should try to
use
> > avfoundationvideocapturer.mm only. AFAIK, webrtc/modules/video_capture/ is
> > legacy that is only used for the video_loopback tool. As a first step, we
can
> > probably make something Mac-specific to unify the iOS/Mac capturers. In the
> long
> > run, we should probably try to get rid of VideoCaptureImpl,
> VideoCaptureModule,
> > and VideoCaptureExternal altogether and update the Win and Linux capture
> > implementation to use the new interface.
> 
> Yup, I agree. I thought it would be worthwhile to at least get rid of the
stuff
> that links against QTKit.
> 
> Perhaps it would be worth opening an issue to track getting rid of those
> classes?

I filed an issue for getting rid of those classes here:
https://bugs.chromium.org/p/webrtc/issues/detail?id=6333

I think it's best to wait for tkchin@ to review this CL as well. He also has
ownership of webrtc/modules/video_capture/ which you will need regardless.

[magjed_webrtc, 2016-09-09 11:01:16]

Description was changed from

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275
==========

to

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275,webrtc:6333
==========

[perkj_webrtc, 2016-09-10 05:52:23]

perkj@webrtc.org changed reviewers:
+ perkj@webrtc.org, tkchin@webrtc.org

[perkj_webrtc, 2016-09-10 05:54:35]

Have you tried the binary video_loopback with and without this change? (I am not
sure if it worked before this cl either)

[kthelgason, 2016-09-19 07:58:15]

On 2016/09/10 05:54:35, perkj_webrtc wrote:
> Have you tried the binary video_loopback with and without this change? (I am
not
> sure if it worked before this cl either)

Just tested it after the change, and it works perfectly!

[perkj_webrtc, 2016-09-19 08:29:30]

On 2016/09/19 07:58:15, kthelgason wrote:
> On 2016/09/10 05:54:35, perkj_webrtc wrote:
> > Have you tried the binary video_loopback with and without this change? (I am
> not
> > sure if it worked before this cl either)
> 
> Just tested it after the change, and it works perfectly!

awsome- ship it.

[perkj_webrtc, 2016-09-19 08:29:56]

On 2016/09/19 08:29:30, perkj_webrtc wrote:
> On 2016/09/19 07:58:15, kthelgason wrote:
> > On 2016/09/10 05:54:35, perkj_webrtc wrote:
> > > Have you tried the binary video_loopback with and without this change? (I
am
> > not
> > > sure if it worked before this cl either)
> > 
> > Just tested it after the change, and it works perfectly!
> 
> awsome- ship it.

rs lgtm

[kthelgason, 2016-09-19 09:17:01]

On 2016/09/19 08:29:56, perkj_webrtc wrote:
> awsome- ship it.

I will hold on for a stamp from tkchin@ and then (hopefully) land this

[tkchin_webrtc, 2016-09-27 10:56:05]

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/BUILD.gn (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/BUILD.gn:130: "apple/device_info.h",
nit: don't use apple in the codebase, we've been trying to avoid that because
things may change and we could end up needing to write swift at some point
use objc/ instead

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/apple/device_info.h (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/device_info.h:11: #ifndef
WEBRTC_MODULES_VIDEO_CAPTURE_APPLE_DEVICE_INFO_H_
ditto OBJC. Here and rest of CL

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/apple/device_info.mm (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/device_info.mm:99:
strncpy(deviceUniqueIdUTF8, [deviceUniqueId UTF8String],
nit: dot syntax for properties

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm:12: #error "This
file requires ARC support."
Are there plans to delete this file? It should be replaced with
avfoundationvideocapturer

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm:177: ^(void) {
nit: ^{ instead of ^(void) {

[kthelgason, 2016-09-27 11:22:56]

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/BUILD.gn (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/BUILD.gn:130: "apple/device_info.h",
On 2016/09/27 10:56:04, tkchin_webrtc wrote:
> nit: don't use apple in the codebase, we've been trying to avoid that because
> things may change and we could end up needing to write swift at some point
> use objc/ instead

Acknowledged.

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/apple/device_info.mm (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/device_info.mm:99:
strncpy(deviceUniqueIdUTF8, [deviceUniqueId UTF8String],
On 2016/09/27 10:56:04, tkchin_webrtc wrote:
> nit: dot syntax for properties

This is just a reformat, but I will fix it.

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm (right):

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm:12: #error "This
file requires ARC support."
On 2016/09/27 10:56:04, tkchin_webrtc wrote:
> Are there plans to delete this file? It should be replaced with
> avfoundationvideocapturer

Yes, see https://bugs.chromium.org/p/webrtc/issues/detail?id=6333

https://codereview.webrtc.org/2309253005/diff/20001/webrtc/modules/video_capt...
webrtc/modules/video_capture/apple/rtc_video_capture_objc.mm:177: ^(void) {
On 2016/09/27 10:56:04, tkchin_webrtc wrote:
> nit: ^{ instead of ^(void) {

Acknowledged.

[kthelgason, 2016-09-28 09:03:53]

Updated, PTAL

[tkchin_webrtc, 2016-09-28 09:21:06]

lgtm
let's just get rid of the QTKit stuff first like you said, and we can clean up
the rest later. This is all legacy stuff.

https://codereview.webrtc.org/2309253005/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2309253005/diff/40001/PRESUBMIT.py#newcode501
PRESUBMIT.py:501: r".*\bobjc[\\\/].*",
probably don't need the first rule anymore then?

https://codereview.webrtc.org/2309253005/diff/40001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/objc/device_info.h (right):

https://codereview.webrtc.org/2309253005/diff/40001/webrtc/modules/video_capt...
webrtc/modules/video_capture/objc/device_info.h:21: class DeviceInfoIos : public
DeviceInfoImpl {
typically file name = class name
not as familiar with C++ style rules though

[kthelgason, 2016-09-28 11:01:43]

https://codereview.webrtc.org/2309253005/diff/40001/webrtc/modules/video_capt...
File webrtc/modules/video_capture/objc/device_info.h (right):

https://codereview.webrtc.org/2309253005/diff/40001/webrtc/modules/video_capt...
webrtc/modules/video_capture/objc/device_info.h:21: class DeviceInfoIos : public
DeviceInfoImpl {
On 2016/09/28 09:21:06, tkchin_webrtc wrote:
> typically file name = class name
> not as familiar with C++ style rules though

I can't find any specific mention of this in the style guide. I agree with you,
but since this stuff will hopefully all be deleted in the near future I don't
think it's worth it to go through and change this everywhere.

[kthelgason, 2016-09-28 11:02:47]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2016-09-28 11:02:47]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
perkj@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2309253005/#ps60001 (title:
"remove added presubmit filter")

[commit-bot: I haz the power, 2016-09-28 11:02:57]

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, 2016-09-28 12:51:40]

Description was changed from

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275,webrtc:6333
==========

to

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275,webrtc:6333
==========

[commit-bot: I haz the power, 2016-09-28 12:51:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-28 12:51:51]

Description was changed from

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275,webrtc:6333
==========

to

==========
Unify the macOS and iOS capturer implementations

This removes the QTKit based capturer for mac, and removes the need
to link against deprecated system libraries on macOS.

BUG=webrtc:3968,webrtc:6275,webrtc:6333

Committed: https://crrev.com/242d8bdddd77109781cbb70c59d161be7566ac98
Cr-Commit-Position: refs/heads/master@{#14418}
==========

[commit-bot: I haz the power, 2016-09-28 12:51:52]

Patchset 4 (id:??) landed as
https://crrev.com/242d8bdddd77109781cbb70c59d161be7566ac98
Cr-Commit-Position: refs/heads/master@{#14418}

[kthelgason, 2016-09-29 09:42:53]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/2381853002/ by kthelgason@webrtc.org.

The reason for reverting is: Breaks internal project.