Add Metal video view in AppRTCMobile and metal availability macro.

Add Metal video view in AppRTCMobile and Metal availability macro.

- The RTC_SUPPORTS_METAL macro allows consumers to gracefully handle compilation for different archs that are not supporting Metal.

BUG=webrtc:7079
Review-Url: https://codereview.webrtc.org/2722583002
Cr-Commit-Position: refs/heads/master@{#17004}
Committed: https://chromium.googlesource.com/external/webrtc/+/154a7bb877abefada22afe28b0a755cf621e8d9e

[daniela-webrtc, 2017-02-27 16:36:38]

Description was changed from

==========
Add Metal video view in AppRTCMobile and metal availability macro.

- The RTC_SUPPORTS_METAL allows consumers to gracefully handle compilation
for different archs that are not supporting Metal.

BUG=webrtc:7079
==========

to

==========
Add Metal video view in AppRTCMobile and Metal availability macro.

- The RTC_SUPPORTS_METAL macro allows consumers to gracefully handle compilation
for different archs that are not supporting Metal.

BUG=webrtc:7079
==========

[daniela-webrtc, 2017-02-27 16:36:38]

denicija@webrtc.org changed reviewers:
+ kthelgason@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org

[kthelgason, 2017-02-28 10:03:10]

It's cool that we can now support both types of renderer!

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.h (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.h:14: #import
<WebRTC/RTCVideoRenderer.h>
Stick to quotes or angle brackets, let's not mix them.

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m:20: 
nit: empty line

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m:47: #if
defined(RTC_SUPPORTS_METAL) && RTC_SUPPORTS_METAL
just #if RTC_SUPPORTS_METAL should be enough.

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m:50: RTCEAGLVideoView
*remoteView = [[RTCEAGLVideoView alloc] initWithFrame:CGRectZero];
this temp variable looks unnecessary, can we assign to _remoteVideoView
directly?

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:56: #elif
should be #else

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Hea...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Hea...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:15: RTC_EXPORT
This should be right before the exported symbol, in this case the @interface.
Although the NS_CLASS_AVAILABLE macro can come in between.

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Hea...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:23: #endif
I'd want to set this in gn if possible. Are there any drawbacks to that approach
that I'm not thinking through?

[daniela-webrtc, 2017-02-28 14:27:12]

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/examples/objc/AppRTCMo...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallView.m:50: RTCEAGLVideoView
*remoteView = [[RTCEAGLVideoView alloc] initWithFrame:CGRectZero];
On 2017/02/28 10:03:10, kthelgason wrote:
> this temp variable looks unnecessary, can we assign to _remoteVideoView
> directly?

The temp is to avoid casting.

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:56: #elif
On 2017/02/28 10:03:10, kthelgason wrote:
> should be #else

Done.

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Hea...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right):

https://codereview.webrtc.org/2722583002/diff/1/webrtc/sdk/objc/Framework/Hea...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:23: #endif
On 2017/02/28 10:03:10, kthelgason wrote:
> I'd want to set this in gn if possible. Are there any drawbacks to that
approach
> that I'm not thinking through?

This is intended for the consumers of the API, to be able to determine in
compile time if this class is available for the particular architecture they are
building.
Otherwise they'd get symbol unavailable exceptions or they'd need to do some
sort of runtime creation of the class, like NSClassFromString() to prevent the
compiler from complaining about the missing symbols.

[magjed_webrtc, 2017-02-28 17:28:15]

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:18: #define
RTC_SUPPORTS_METAL 1
Don't indent and just do:
#if defined(__arm64__) && __arm64__
#define RTC_SUPPORTS_METAL
#endif
and then:
#if defined(RTC_SUPPORTS_METAL)

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h:1: 
nit: remove

[tkchin_webrtc, 2017-02-28 18:47:33]

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:54: #if
RTC_SUPPORTS_METAL
Do we need to expose the define in the header if it's returned in a function
like this? (Or should we just remove this function?)
Also, why are the two defines different?

[daniela-webrtc, 2017-03-01 13:40:22]

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:54: #if
RTC_SUPPORTS_METAL
On 2017/02/28 18:47:33, tkchin_webrtc wrote:
> Do we need to expose the define in the header if it's returned in a function
> like this? (Or should we just remove this function?)
> Also, why are the two defines different?

The previous define I realized is not correct. COREVIDEO_SUPPORTS_METAL checks
only if TARGET_OS_IPHONE.
That's not enough because we need to check for cpu architecture as well (this
check will fail for armv7 for instance).
More info in this radar report http://openradar.appspot.com/18498494.
The isMetalAvailable is just convenience to not have #ifdef checks through the
class

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:18: #define
RTC_SUPPORTS_METAL 1
On 2017/02/28 17:28:15, magjed_webrtc wrote:
> Don't indent and just do:
> #if defined(__arm64__) && __arm64__
> #define RTC_SUPPORTS_METAL
> #endif
> and then:
> #if defined(RTC_SUPPORTS_METAL)

Done.

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h:1: 
On 2017/02/28 17:28:15, magjed_webrtc wrote:
> nit: remove

Done.

[tkchin_webrtc, 2017-03-01 19:16:38]

lgtm

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):

https://codereview.webrtc.org/2722583002/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:54: #if
RTC_SUPPORTS_METAL
On 2017/03/01 13:40:22, denicija-webrtc wrote:
> On 2017/02/28 18:47:33, tkchin_webrtc wrote:
> > Do we need to expose the define in the header if it's returned in a function
> > like this? (Or should we just remove this function?)
> > Also, why are the two defines different?
> 
> The previous define I realized is not correct. COREVIDEO_SUPPORTS_METAL checks
> only if TARGET_OS_IPHONE.
> That's not enough because we need to check for cpu architecture as well (this
> check will fail for armv7 for instance).
> More info in this radar report http://openradar.appspot.com/18498494.
> The isMetalAvailable is just convenience to not have #ifdef checks through the
> class

Gotcha. Ok, if we're sure that Metal support is known at compile time vs runtime
then I'm good with exposing the define publicly. (but please check that that is
true - e.g. iOS10SDK on older device like iPhone 5C)

[kthelgason, 2017-03-03 12:38:40]

lgtm % small nit

https://codereview.webrtc.org/2722583002/diff/60001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right):

https://codereview.webrtc.org/2722583002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:17: #if
defined(__arm64__) && __arm64__
nit: just defined(__arm64__) should be enough. Also, chromium uses __aarch64__
instead, because that's also defined when using gcc. Maybe we should use that
just for the sake of staying consistent.

[magjed_webrtc, 2017-03-03 12:51:17]

lgtm

[daniela-webrtc, 2017-03-03 13:37:11]

The CQ bit was checked by denicija@webrtc.org

[daniela-webrtc, 2017-03-03 13:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org,
magjed@webrtc.org, kthelgason@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2722583002/#ps80001 (title:
"Replace macro check to be gnu compatible as well.")

[commit-bot: I haz the power, 2017-03-03 13:37:20]

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-03 14:11:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488548231337730,
"parent_rev": "3a2c803dc341a2bc266effb07df9863c14a7aeaa", "commit_rev":
"154a7bb877abefada22afe28b0a755cf621e8d9e"}

[commit-bot: I haz the power, 2017-03-03 14:11:14]

Description was changed from

==========
Add Metal video view in AppRTCMobile and Metal availability macro.

- The RTC_SUPPORTS_METAL macro allows consumers to gracefully handle compilation
for different archs that are not supporting Metal.

BUG=webrtc:7079
==========

to

==========
Add Metal video view in AppRTCMobile and Metal availability macro.

- The RTC_SUPPORTS_METAL macro allows consumers to gracefully handle compilation
for different archs that are not supporting Metal.

BUG=webrtc:7079

Review-Url: https://codereview.webrtc.org/2722583002
Cr-Commit-Position: refs/heads/master@{#17004}
Committed:
https://chromium.googlesource.com/external/webrtc/+/154a7bb877abefada22afe28b...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/154a7bb877abefada22afe28b...

[kthelgason, 2017-03-08 13:51:51]

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

The reason for reverting is: Breaks AppRTCMobile.