iOS/MacOS:Refactor metal rendering by extracting common implementation

iOS/MacOS:Refactor metal rendering by extracting common implementation
in separate super class.

Included changes:
- Fix rendering on iOS to support NV12 and i420 frames
- Improve code style
- Update build targets
- Update tests

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

[daniela-webrtc, 2017-03-30 12:07:18]

Description was changed from

==========
iOS/MacOS:Refactor metal rendering by extracting common implementation
in separate super class.

Included changes:
- Improve code style
- Update build targets
- Update tests

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

to

==========
iOS/MacOS:Refactor metal rendering by extracting common implementation
in separate super class.

Included changes:
- Fix rendering on iOS to support NV12 and i420 frames
- Improve code style
- Update build targets
- Update tests

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

[daniela-webrtc, 2017-03-30 12:07:18]

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

[magjed_webrtc, 2017-04-04 08:07:36]

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm (right):

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm:115: RTC_CHECK(0) <<
"Virtual method not implemented in subclass.";
nit: Use RTC_NOTREACHED instead.

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm:124: _offset =
offsetForRotation((webrtc::VideoRotation)frame.rotation);
Use static_cast<webrtc::VideoRotation> instead. Or we should possibly introduce
convert functions in RTCVideoFrame for RTCVideoRotation <->
webrtc::VideoRotation that switches on the enum values.

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

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:74: RTCMTLNV12Renderer
*renderer = [[RTCMTLNV12RendererClass alloc] init];
nit: Maybe just return the result directly?

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:90: _rendererI420 =
[RTCMTLVideoView createI420Renderer];
Are these expensive to set up? If they are, we could do it lazily instead based
on self.videoFrame.nativeHandle, since we will either only receive I420 or NV12
frames (unlikely that the source provides mixed formats).

[daniela-webrtc, 2017-04-05 11:49:31]

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm (right):

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm:115: RTC_CHECK(0) <<
"Virtual method not implemented in subclass.";
On 2017/04/04 08:07:36, magjed_webrtc wrote:
> nit: Use RTC_NOTREACHED instead.

Done.

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLRenderer.mm:124: _offset =
offsetForRotation((webrtc::VideoRotation)frame.rotation);
On 2017/04/04 08:07:36, magjed_webrtc wrote:
> Use static_cast<webrtc::VideoRotation> instead. Or we should possibly
introduce
> convert functions in RTCVideoFrame for RTCVideoRotation <->
> webrtc::VideoRotation that switches on the enum values.

Actually I'll use RTCVideoRotation directly instead of webrtc::VideoRotation.
This was done (I think) before RTCVideoRotation was added.
Great catch!

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

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:90: _rendererI420 =
[RTCMTLVideoView createI420Renderer];
On 2017/04/04 08:07:36, magjed_webrtc wrote:
> Are these expensive to set up? If they are, we could do it lazily instead
based
> on self.videoFrame.nativeHandle, since we will either only receive I420 or
NV12
> frames (unlikely that the source provides mixed formats).

Sadly the configuration is quite expensive and entails setup of the buffers,
texture caches, loading of shader programs and setup of command encoders.

[magjed_webrtc, 2017-04-05 12:10:38]

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

https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:90: _rendererI420 =
[RTCMTLVideoView createI420Renderer];
On 2017/04/05 11:49:31, daniela-webrtc wrote:
> On 2017/04/04 08:07:36, magjed_webrtc wrote:
> > Are these expensive to set up? If they are, we could do it lazily instead
> based
> > on self.videoFrame.nativeHandle, since we will either only receive I420 or
> NV12
> > frames (unlikely that the source provides mixed formats).
> 
> Sadly the configuration is quite expensive and entails setup of the buffers,
> texture caches, loading of shader programs and setup of command encoders. 

Ok, so can we try to move the configuration of the shaders to renderFrame on the
first arrival of a frame?

[daniela-webrtc, 2017-04-10 07:14:19]

On 2017/04/05 12:10:38, magjed_webrtc wrote:
>
https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
> File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):
> 
>
https://codereview.webrtc.org/2784243003/diff/1/webrtc/sdk/objc/Framework/Cla...
> webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:90: _rendererI420 =
> [RTCMTLVideoView createI420Renderer];
> On 2017/04/05 11:49:31, daniela-webrtc wrote:
> > On 2017/04/04 08:07:36, magjed_webrtc wrote:
> > > Are these expensive to set up? If they are, we could do it lazily instead
> > based
> > > on self.videoFrame.nativeHandle, since we will either only receive I420 or
> > NV12
> > > frames (unlikely that the source provides mixed formats).
> > 
> > Sadly the configuration is quite expensive and entails setup of the buffers,
> > texture caches, loading of shader programs and setup of command encoders. 
> 
> Ok, so can we try to move the configuration of the shaders to renderFrame on
the
> first arrival of a frame?

Done

[magjed_webrtc, 2017-04-10 09:48:59]

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

https://codereview.webrtc.org/2784243003/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:136: if
(![RTCMTLVideoView isMetalAvailable] || self.errorSetupingRenderer) {
Is errorSetupingRenderer necessary? Can't we just try to re-create the renderer
on the next frame if we fail? I guess we will likely fail again, but I don't
think that's worse behavior then silently dropping all frames.

https://codereview.webrtc.org/2784243003/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:146: if
(frame.nativeHandle) {
We should still handle changing frame formats (although it probably won't happen
in practice). Maybe it's easiest to keep separate variables for the I420 and
NV12 renderer as it used to be. At least we need a way to remember what render
type we have allocated here.

https://codereview.webrtc.org/2784243003/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:153:
self.errorSetupingRenderer = YES;
We should probably log something here.

[daniela-webrtc, 2017-04-21 11:31:14]

Magnus can you take another look at this?

[magjed_webrtc, 2017-04-21 15:03:33]

On 2017/04/21 11:31:14, daniela-webrtc wrote:
> Magnus can you take another look at this?

Sorry, I haven't seen that a new patch set was ready. You need to Publish
comments or Reply in order for this to show up in bold in the review page.

[magjed_webrtc, 2017-04-21 15:04:59]

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

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:83: RTCLog("Metal
unavailable");
Can we make the init functions return nil for this case?

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right):

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:21:
@property(nonatomic, strong) id<RTCMTLRenderer> rendererI420;
I would like to avoid testing these internal variables directly. When testing
that shaders are created lazily, it's enough to test that createI420Renderer()
isn't called, we don't need EXPECT_EQ(realView.rendererI420, nil) as well. Same
for NV12.

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:23:
@property(nonatomic, strong) UIView* metalView;
I would like to avoid exposing |metalView| and |videoFrame| as well, see
comments below.

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:87: RTCMTLVideoView
*realView = [[RTCMTLVideoView alloc] init];
Can we just return nil from RTCMTLVideoView init when Metal is unavailable
instead?

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:118:
realView.videoFrame = self.frameMock;
I think we should only test:
RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];
[[self.classMock reject] createI420Renderer]
[[self.classMock reject] createNV12Renderer]
[realView renderFrame:nil];
[realView drawInMTKView:self.metalViewMock];

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:148:
EXPECT_EQ(realView.videoFrame, self.frameMock);
I think this check is unnecessary since we already have
OCMExpect([self.rendererI420Mock drawFrame:self.frameMock].

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:172:
EXPECT_EQ(realView.videoFrame, self.frameMock);
Same here, I don't think this check is necessary.

[daniela-webrtc, 2017-04-26 13:17:06]

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

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:83: RTCLog("Metal
unavailable");
On 2017/04/21 15:04:58, magjed_webrtc wrote:
> Can we make the init functions return nil for this case?

That was my first intention, but initWithFrame is defined as returning nonnull
instance so it won't be possible.

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right):

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:87: RTCMTLVideoView
*realView = [[RTCMTLVideoView alloc] init];
On 2017/04/21 15:04:59, magjed_webrtc wrote:
> Can we just return nil from RTCMTLVideoView init when Metal is unavailable
> instead?

See my previous response.

[magjed_webrtc, 2017-04-26 13:27:32]

lgtm

Thanks for trying my testing approach! Let's evaluate in a quarter or six
months.

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

https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:83: RTCLog("Metal
unavailable");
On 2017/04/26 13:17:05, daniela-webrtc wrote:
> On 2017/04/21 15:04:58, magjed_webrtc wrote:
> > Can we make the init functions return nil for this case?
> 
> That was my first intention, but initWithFrame is defined as returning nonnull
> instance so it won't be possible.

Ok, maybe we can NSAssert instead? Clients shouldn't create instances of this
class if Metal is unavailable.

[daniela-webrtc, 2017-04-26 14:02:06]

On 2017/04/26 13:27:32, magjed_webrtc wrote:
> lgtm
> 
> Thanks for trying my testing approach! Let's evaluate in a quarter or six
> months.
> 
>
https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
> File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right):
> 
>
https://codereview.webrtc.org/2784243003/diff/80001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:83: RTCLog("Metal
> unavailable");
> On 2017/04/26 13:17:05, daniela-webrtc wrote:
> > On 2017/04/21 15:04:58, magjed_webrtc wrote:
> > > Can we make the init functions return nil for this case?
> > 
> > That was my first intention, but initWithFrame is defined as returning
nonnull
> > instance so it won't be possible.
> 
> Ok, maybe we can NSAssert instead? Clients shouldn't create instances of this
> class if Metal is unavailable.

Good idea. I added NSAssert check.

[daniela-webrtc, 2017-04-27 09:22:33]

The CQ bit was checked by denicija@webrtc.org

[daniela-webrtc, 2017-04-27 09:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784243003/#ps120001 (title:
"Add assertion when metal unavailable")

[commit-bot: I haz the power, 2017-04-27 09:22:36]

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-04-27 09:24:06]

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

[commit-bot: I haz the power, 2017-04-27 09:24:07]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19844)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/19640)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25154)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24015)

[daniela-webrtc, 2017-04-28 08:05:38]

The CQ bit was checked by denicija@webrtc.org

[daniela-webrtc, 2017-04-28 08:05:39]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784243003/#ps140001 (title:
"Fix spacing")

[commit-bot: I haz the power, 2017-04-28 08:05:44]

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-04-28 08:07:54]

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

[commit-bot: I haz the power, 2017-04-28 08:07:55]

Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/24224)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21079)

[daniela-webrtc, 2017-04-28 08:12:35]

The CQ bit was checked by denicija@webrtc.org

[daniela-webrtc, 2017-04-28 08:12:36]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784243003/#ps160001 (title:
"Fix dependency in build file")

[commit-bot: I haz the power, 2017-04-28 08:12:45]

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-04-28 08:17:54]

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

[commit-bot: I haz the power, 2017-04-28 08:17:55]

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

[daniela-webrtc, 2017-04-28 08:44:03]

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

[commit-bot: I haz the power, 2017-04-28 08:44: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-04-28 09:09:20]

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

[commit-bot: I haz the power, 2017-04-28 09:09:21]

Dry run: This issue passed the CQ dry run.

[daniela-webrtc, 2017-04-28 09:12:11]

The CQ bit was checked by denicija@webrtc.org

[daniela-webrtc, 2017-04-28 09:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784243003/#ps180001 (title:
"Fix build linkage")

[commit-bot: I haz the power, 2017-04-28 09:12:19]

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-04-28 09:14:55]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493370731859400,
"parent_rev": "7a38fd2628eb6711a9adae9ce93b41c85f1521b0", "commit_rev":
"d2088156853edb3bef0834f5631fd1ebd56ff66f"}

[commit-bot: I haz the power, 2017-04-28 09:14:58]

Description was changed from

==========
iOS/MacOS:Refactor metal rendering by extracting common implementation
in separate super class.

Included changes:
- Fix rendering on iOS to support NV12 and i420 frames
- Improve code style
- Update build targets
- Update tests

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

to

==========
iOS/MacOS:Refactor metal rendering by extracting common implementation
in separate super class.

Included changes:
- Fix rendering on iOS to support NV12 and i420 frames
- Improve code style
- Update build targets
- Update tests

BUG=webrtc:7079

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

[commit-bot: I haz the power, 2017-04-28 09:14:59]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/d2088156853edb3bef0834f56...