iOS/MacOS:Refactor metal rendering by extracting common implementation

webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm

Index: webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm
diff --git a/webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm b/webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm
index 1d422eb09aa9b55e8a0b98414b30b9c605ea7e17..bed1f93ba6b151fbca9154edda0c8addd0580c06 100644
--- a/webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm
+++ b/webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm
@@ -18,151 +18,200 @@
 
 // Extension of RTCMTLVideoView for testing purposes.
 @interface RTCMTLVideoView (Testing)
-@property(nonatomic, strong) id<RTCMTLRenderer> renderer;
+@property(nonatomic, strong) id<RTCMTLRenderer> rendererI420;

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

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.

+@property(nonatomic, strong) id<RTCMTLRenderer> rendererNV12;
 @property(nonatomic, strong) UIView* metalView;

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

I would like to avoid exposing |metalView| and |videoFrame| as well, see
comments below.

 @property(atomic, strong) RTCVideoFrame* videoFrame;
 
 + (BOOL)isMetalAvailable;
 + (UIView*)createMetalView:(CGRect)frame;
-+ (id<RTCMTLRenderer>)createMetalRenderer;
++ (id<RTCMTLRenderer>)createNV12Renderer;
++ (id<RTCMTLRenderer>)createI420Renderer;
 - (void)drawInMTKView:(id)view;
 @end
 
 @interface RTCMTLVideoViewTests : NSObject
 @property(nonatomic, strong) id classMock;
 @property(nonatomic, strong) id metalViewMock;
-@property(nonatomic, strong) id rendererMock;
+@property(nonatomic, strong) id rendererNV12Mock;
+@property(nonatomic, strong) id rendererI420Mock;
+@property(nonatomic, strong) id frameMock;
 @end
 
 @implementation RTCMTLVideoViewTests
 
 @synthesize classMock = _classMock;
 @synthesize metalViewMock = _metalViewMock;
-@synthesize rendererMock = _rendererMock;
+@synthesize rendererNV12Mock = _rendererNV12Mock;
+@synthesize rendererI420Mock = _rendererI420Mock;
+@synthesize frameMock = _frameMock;
 
 - (void)setup {
   self.classMock = OCMClassMock([RTCMTLVideoView class]);
-
-  self.metalViewMock = OCMClassMock([RTCMTLVideoViewTests class]);
-  // NOTE: OCMock doesen't provide modern syntax for -ignoringNonObjectArgs.
-  [[[[self.classMock stub] ignoringNonObjectArgs] andReturn:self.metalViewMock]
-      createMetalView:CGRectZero];
-
-  self.rendererMock = OCMProtocolMock(@protocol(RTCMTLRenderer));
-  OCMStub([self.classMock createMetalRenderer]).andReturn(self.rendererMock);
 }
 
 - (void)tearDown {
   [self.classMock stopMocking];
-  [self.rendererMock stopMocking];
+  [self.rendererI420Mock stopMocking];
+  [self.rendererNV12Mock stopMocking];
   [self.metalViewMock stopMocking];
+  [self.frameMock stopMocking];
   self.classMock = nil;
-  self.rendererMock = nil;
+  self.rendererI420Mock = nil;
+  self.rendererNV12Mock = nil;
   self.metalViewMock = nil;
+  self.frameMock = nil;
+}
+
+- (id)frameMockWithNativeHandle:(BOOL)hasNativeHandle {
+  id frameMock = OCMClassMock([RTCVideoFrame class]);
+  if (hasNativeHandle) {
+    OCMStub([frameMock nativeHandle]).andReturn((CVPixelBufferRef)[OCMArg anyPointer]);
+  } else {
+    OCMStub([frameMock nativeHandle]).andReturn((CVPixelBufferRef) nullptr);
+  }
+  return frameMock;
+}
+
+- (id)rendererMockWithSuccessfulSetup:(BOOL)sucess {
+  id rendererMock = OCMProtocolMock(@protocol(RTCMTLRenderer));
+  OCMStub([rendererMock addRenderingDestination:[OCMArg any]]).andReturn(sucess);
+
+  return rendererMock;
 }
 
-- (void)testMetalConfigureNotExecuted {
+#pragma mark - Test cases
+- (void)testMetalConfigureNotExecutedIfMetalUnavailable {
   // when
   OCMStub([self.classMock isMetalAvailable]).andReturn(NO);
   RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];

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

Can we just return nil from RTCMTLVideoView init when Metal is unavailable
instead?

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

See my previous response.

 
   // then
-  EXPECT_TRUE(realView.renderer == nil);
-  EXPECT_TRUE(realView.metalView == nil);
+  EXPECT_EQ(realView.rendererI420, nil);
+  EXPECT_EQ(realView.rendererNV12, nil);
+  EXPECT_EQ(realView.metalView, nil);
 }
 
-- (void)testMetalConfigureExecuted {
+- (void)testRendererNotCreatedAndFrameNotAssignedIfMetalUnavailable {
   // given
-  OCMStub([self.classMock isMetalAvailable]).andReturn(YES);
-  OCMStub([self.rendererMock addRenderingDestination:self.metalViewMock])
-      .andReturn(NO);
+  OCMStub([self.classMock isMetalAvailable]).andReturn(NO);
 
-  // when
   RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];
+  self.frameMock = OCMClassMock([RTCVideoFrame class]);
+  [[self.frameMock reject] nativeHandle];
+
+  // when
+  [realView renderFrame:self.frameMock];
 
   // then
-  EXPECT_TRUE(realView.renderer == nil);
-  EXPECT_TRUE(realView.metalView != nil);
+  EXPECT_EQ(realView.rendererNV12, nil);
+  EXPECT_EQ(realView.rendererI420, nil);
+  EXPECT_TRUE(realView.videoFrame != self.frameMock);
+  [self.frameMock verify];
 }
 
-- (void)testMetalDrawCallback {
+- (void)testRTCVideoRenderNilFrameCallback {
   // given
-  OCMStub([self.classMock isMetalAvailable]).andReturn(NO);
-  OCMExpect([self.rendererMock drawFrame:[OCMArg any]]);
+  OCMStub([self.classMock isMetalAvailable]).andReturn(YES);
   RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];
-  realView.metalView = self.metalViewMock;
-  realView.renderer = self.rendererMock;
+  self.frameMock = OCMClassMock([RTCVideoFrame class]);
+  realView.videoFrame = self.frameMock;

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

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];

+  [[self.frameMock reject] nativeHandle];
 
   // when
-  [realView drawInMTKView:self.metalViewMock];
+  [realView renderFrame:nil];
 
   // then
-  [self.rendererMock verify];
+  EXPECT_EQ(realView.rendererNV12, nil);
+  EXPECT_EQ(realView.rendererI420, nil);
+  EXPECT_EQ(realView.videoFrame, self.frameMock);
+  [self.frameMock verify];
 }
 
-- (void)testRTCVideoRenderNilFrameCallback {
+- (void)testRTCVideoRenderFrameCallbackI420 {
   // given
-  OCMStub([self.classMock isMetalAvailable]).andReturn(NO);
+  OCMStub([self.classMock isMetalAvailable]).andReturn(YES);
+  self.rendererI420Mock = [self rendererMockWithSuccessfulSetup:YES];
+  self.frameMock = [self frameMockWithNativeHandle:NO];
+
+  OCMExpect([self.rendererI420Mock drawFrame:self.frameMock]);
+  OCMExpect([self.classMock createI420Renderer]).andReturn(self.rendererI420Mock);
+  [[self.classMock reject] createNV12Renderer];
+
   RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];
 
   // when
-  [realView renderFrame:nil];
+  [realView renderFrame:self.frameMock];
+  [realView drawInMTKView:self.metalViewMock];
 
   // then
-  EXPECT_TRUE(realView.videoFrame == nil);
+  EXPECT_EQ(realView.videoFrame, self.frameMock);

[magjed_webrtc, 2017/04/21 15:04:58]

I think this check is unnecessary since we already have
OCMExpect([self.rendererI420Mock drawFrame:self.frameMock].

+  EXPECT_EQ(realView.rendererNV12, nil);
+  [self.rendererI420Mock verify];
+  [self.classMock verify];
 }
 
-- (void)testRTCVideoRenderFrameCallback {
+
+- (void)testRTCVideoRenderFrameCallbackNV12 {
   // given
-  OCMStub([self.classMock isMetalAvailable]).andReturn(NO);
+  OCMStub([self.classMock isMetalAvailable]).andReturn(YES);
+  self.rendererNV12Mock = [self rendererMockWithSuccessfulSetup:YES];
+  self.frameMock = [self frameMockWithNativeHandle:YES];
+
+  OCMExpect([self.rendererNV12Mock drawFrame:self.frameMock]);
+  OCMExpect([self.classMock createNV12Renderer]).andReturn(self.rendererNV12Mock);
+  [[self.classMock reject] createI420Renderer];
+
   RTCMTLVideoView *realView = [[RTCMTLVideoView alloc] init];
-  id frame = OCMClassMock([RTCVideoFrame class]);
-  realView.metalView = self.metalViewMock;
-  realView.renderer = self.rendererMock;
-  OCMExpect([self.rendererMock drawFrame:frame]);
 
   // when
-  [realView renderFrame:frame];
+  [realView renderFrame:self.frameMock];
   [realView drawInMTKView:self.metalViewMock];
 
   // then
-  EXPECT_EQ(realView.videoFrame, frame);
-  [self.rendererMock verify];
+  EXPECT_EQ(realView.videoFrame, self.frameMock);

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

Same here, I don't think this check is necessary.

+  EXPECT_EQ(realView.rendererI420, nil);
+  [self.rendererNV12Mock verify];
+  [self.classMock verify];
 }
 @end
 
-TEST(RTCMTLVideoViewTests, MetalConfigureNotExecuted) {
+
+TEST(RTCMTLVideoViewTests, MetalConfigureNotExecutedIfMetalUnavailable) {
   RTCMTLVideoViewTests *test = [[RTCMTLVideoViewTests alloc] init];
   [test setup];
-  [test testMetalConfigureNotExecuted];
+  [test testMetalConfigureNotExecutedIfMetalUnavailable];
   [test tearDown];
 }
 
-
-TEST(RTCMTLVideoViewTests, MetalConfigureExecuted) {
+TEST(RTCMTLVideoViewTests, RendererNotCreatedAndFrameNotAssignedIfMetalUnavailable) {
   RTCMTLVideoViewTests *test = [[RTCMTLVideoViewTests alloc] init];
   [test setup];
-  [test testMetalConfigureExecuted];
+
+  [test testRendererNotCreatedAndFrameNotAssignedIfMetalUnavailable];
   [test tearDown];
 }
 
-TEST(RTCMTLVideoViewTests, MetalDrawCallback) {
+TEST(RTCMTLVideoViewTests, RTCVideoRenderNilFrameCallback) {
   RTCMTLVideoViewTests *test = [[RTCMTLVideoViewTests alloc] init];
   [test setup];
-  [test testMetalDrawCallback];
+  [test testRTCVideoRenderNilFrameCallback];
   [test tearDown];
 }
 
-TEST(RTCMTLVideoViewTests, RTCVideoRenderNilFrameCallback) {
+TEST(RTCMTLVideoViewTests, RTCVideoRenderFrameCallbackI420) {
   RTCMTLVideoViewTests *test = [[RTCMTLVideoViewTests alloc] init];
   [test setup];
-  [test testRTCVideoRenderNilFrameCallback];
+
+  [test testRTCVideoRenderFrameCallbackI420];
   [test tearDown];
 }
 
-TEST(RTCMTLVideoViewTests, RTCVideoRenderFrameCallback) {
+
+TEST(RTCMTLVideoViewTests, RTCVideoRenderFrameCallbackNV12) {
   RTCMTLVideoViewTests *test = [[RTCMTLVideoViewTests alloc] init];
   [test setup];
-  [test testRTCVideoRenderFrameCallback];
+
+  [test testRTCVideoRenderFrameCallbackNV12];
   [test tearDown];
 }