Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 9 months ago by daniela-webrtc Modified:
3 years, 9 months ago Reviewers:
magjed_webrtc, kthelgason CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd unit tests for RTCMTLVideoView.
To properly test the functionality, following changes were needed
- Make RTCMTLVideoView compiliable for all cpu architectures not just arm64.
This is needed so that the test can run on any device and on simulator as well.
- Refactor RTCMTLVideoView to have mockable class methods.
The unittest class, RTCMTLVideoViewTests was designed to provide easy transition
to XCTest when the time comes for that.
To transition to XCTest it would suffice to inherit from XCTestCase and remove
the gtest methods.
BUG=webrtc:7079
Review-Url: https://codereview.webrtc.org/2723903003
Cr-Commit-Position: refs/heads/master@{#17014}
Committed: https://chromium.googlesource.com/external/webrtc/+/0ebe0199acd1070f17ca2abc5bc22fdd8b0861ca
Patch Set 1 #Patch Set 2 : Fix property vs ivar use #
Total comments: 13
Patch Set 3 : Replace superclass types with proper types #
Total comments: 1
Patch Set 4 : Sort imports #Patch Set 5 : Fix unittest target to include test for ios only #Patch Set 6 : Rebase #
Created: 3 years, 9 months ago
Messages
Total messages: 25 (11 generated)
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
Please review.
It's not completely clear to me when and on which platforms we include metal. IIUC it should only be compiled on 64-bit arm for iOS, right? https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:262: if (current_cpu != "arm64") { Isn't this conditional the wrong way around? https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:24: #define RTCMTLNV12RendererClass NSClassFromString(@"RTCMTLNV12Renderer") This frightens me a little bit. Can't we just not compile this file on architectures without metal?
On 2017/03/02 08:23:05, kthelgason wrote: > It's not completely clear to me when and on which platforms we include metal. > IIUC it should only be compiled on 64-bit arm for iOS, right? The metal framework is supported only for arm64. To avoid linking issues on other cpu's we are also restricting the classes that link metal (RTCMTLVideoView and RTCMTLRenrerer) to compile only for arm64. For testing however, I've included RTCMTLVIdeoView for all cpus so that we can exercise the test on simulator as well. > https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn > File webrtc/sdk/BUILD.gn (right): > > https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn#newco... > webrtc/sdk/BUILD.gn:262: if (current_cpu != "arm64") { > Isn't this conditional the wrong way around? It's the right way. The dependencies include it for arm64 already, the test should include it for the rest. > https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): > > https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:24: #define > RTCMTLNV12RendererClass NSClassFromString(@"RTCMTLNV12Renderer") > This frightens me a little bit. Can't we just not compile this file on > architectures without metal? I'd prefer to have the tests up and running on the bots. When we have tests running on devices, then perhaps we can have this class compiling for arm64 only and remove the objc/runtime magic/
https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:263: sources += [ "objc/Framework/Classes/Metal/RTCMTLVideoView.m" ] Is it ok to always do: sources += [ "objc/Framework/Classes/Metal/RTCMTLVideoView.m" ] unconditionally or will that lead to an error on arm64 where it's added twice to sources? https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:28: @property(nonatomic, strong) UIView *metalView; Why can't this still be an MTKView when we mock it? https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:93: if ([_renderer addRenderingDestination:_metalView]) { do: return [_renderer addRenderingDestination:_metalView]; instead https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:100: [self.rendererMock verify]; what does this check? https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:129: EXPECT_TRUE(realView.videoFrame = frame); I guess this should be 'realView.videoFrame == frame'. Can you use EXPECT_EQ here? Would the output of that be helpful?
https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:263: sources += [ "objc/Framework/Classes/Metal/RTCMTLVideoView.m" ] On 2017/03/02 13:28:18, magjed_webrtc wrote: > Is it ok to always do: > sources += [ "objc/Framework/Classes/Metal/RTCMTLVideoView.m" ] > unconditionally or will that lead to an error on arm64 where it's added twice to > sources? Yes, it's added twice for arm64 in that case and compilation fails (duplicate symbols) https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:28: @property(nonatomic, strong) UIView *metalView; On 2017/03/02 13:28:18, magjed_webrtc wrote: > Why can't this still be an MTKView when we mock it? I thought it will give linking errors when using the proper type, but I was wrong :) Thanks for pointing it out. https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:93: if ([_renderer addRenderingDestination:_metalView]) { On 2017/03/02 13:28:18, magjed_webrtc wrote: > do: > return [_renderer addRenderingDestination:_metalView]; > instead Done. https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:100: [self.rendererMock verify]; On 2017/03/02 13:28:18, magjed_webrtc wrote: > what does this check? Verifies that the expectation in line 92 is fulfilled i.e that [rendererMock drawFrame] was called. https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:129: EXPECT_TRUE(realView.videoFrame = frame); On 2017/03/02 13:28:18, magjed_webrtc wrote: > I guess this should be 'realView.videoFrame == frame'. Can you use EXPECT_EQ > here? Would the output of that be helpful? Good catch. Yes, I should have used EXPECT_EQ in this case.
lgtm, thanks for adding these!
lgtm https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right): https://codereview.webrtc.org/2723903003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:100: [self.rendererMock verify]; On 2017/03/03 08:47:02, denicija-webrtc wrote: > On 2017/03/02 13:28:18, magjed_webrtc wrote: > > what does this check? > > Verifies that the expectation in line 92 is fulfilled i.e that [rendererMock > drawFrame] was called. Acknowledged. https://codereview.webrtc.org/2723903003/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm (right): https://codereview.webrtc.org/2723903003/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCMTLVideoViewTests.mm:16: #include <WebRTC/RTCMTLVideoView.h> nit: Sort includes.
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2723903003/#ps60001 (title: "Sort imports")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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/19494)
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2723903003/#ps80001 (title: "Fix unittest target to include test for ios only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:
While running git apply --index -p1;
error: patch failed:
webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:53
error: webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m: patch does
not apply
Patch: webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
Index: webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
diff --git a/webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
b/webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
index
2a18736b25071bba0b2636b9b2bc975295e47dc3..5f389ba9f822e15c0bbd156547b99505424b5c14
100644
--- a/webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
+++ b/webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m
@@ -18,15 +18,18 @@
#import "RTCMTLNV12Renderer.h"
+// To avoid unreconized symbol linker errors, we're taking advantage of the
objc runtime.
+// Linking errors occur when compiling for architectures that don't support
Metal.
+#define MTKViewClass NSClassFromString(@"MTKView")
+#define RTCMTLNV12RendererClass NSClassFromString(@"RTCMTLNV12Renderer")
+
@interface RTCMTLVideoView () <MTKViewDelegate>
-@property(nonatomic, strong) id<RTCMTLRenderer> renderer;
+@property(nonatomic, strong) RTCMTLNV12Renderer *renderer;
@property(nonatomic, strong) MTKView *metalView;
@property(atomic, strong) RTCVideoFrame *videoFrame;
@end
-@implementation RTCMTLVideoView {
- id<RTCMTLRenderer> _renderer;
-}
+@implementation RTCMTLVideoView
@synthesize renderer = _renderer;
@synthesize metalView = _metalView;
@@ -53,16 +56,46 @@
+ (BOOL)isMetalAvailable {
#if defined(__OBJC__) && COREVIDEO_SUPPORTS_METAL
return YES;
-#elif
+#else
return NO;
#endif
}
++ (MTKView *)createMetalView:(CGRect)frame {
+ MTKView *view = [[MTKViewClass alloc] initWithFrame:frame];
+ return view;
+}
+
++ (RTCMTLNV12Renderer *)createMetalRenderer {
+ RTCMTLNV12Renderer *renderer = [[RTCMTLNV12RendererClass alloc] init];
+ return renderer;
+}
+
- (void)configure {
- if ([RTCMTLVideoView isMetalAvailable]) {
- _metalView = [[MTKView alloc] initWithFrame:self.bounds];
- [self addSubview:_metalView];
+ if (![RTCMTLVideoView isMetalAvailable]) {
+ RTCLog("Metal unavailable");
+ return;
+ }
+
+ _metalView = [RTCMTLVideoView createMetalView:self.bounds];
+ _renderer = [RTCMTLVideoView createMetalRenderer];
+
+ if ([self configureMetalRenderer]) {
+ [self configureMetalView];
+ } else {
+ _renderer = nil;
+ RTCLogError("Metal configuration falied.");
+ }
+}
+
+- (BOOL)configureMetalRenderer {
+ return [_renderer addRenderingDestination:_metalView];
+}
+
+- (void)configureMetalView {
+ if (_metalView) {
_metalView.delegate = self;
+ [self addSubview:_metalView];
_metalView.contentMode = UIViewContentModeScaleAspectFit;
_metalView.translatesAutoresizingMaskIntoConstraints = NO;
UILayoutGuide *margins = self.layoutMarginsGuide;
@@ -70,20 +103,14 @@
[_metalView.bottomAnchor
constraintEqualToAnchor:margins.bottomAnchor].active = YES;
[_metalView.leftAnchor constraintEqualToAnchor:margins.leftAnchor].active =
YES;
[_metalView.rightAnchor constraintEqualToAnchor:margins.rightAnchor].active
= YES;
-
- _renderer = [[RTCMTLNV12Renderer alloc] init];
- if (![(RTCMTLNV12Renderer *)_renderer addRenderingDestination:_metalView])
{
- _renderer = nil;
- };
- } else {
- RTCLogError("Metal configuration falied.");
}
}
+
#pragma mark - MTKViewDelegate methods
- (void)drawInMTKView:(nonnull MTKView *)view {
NSAssert(view == self.metalView, @"Receiving draw callbacks from foreign
instance.");
- [_renderer drawFrame:self.videoFrame];
+ [self.renderer drawFrame:self.videoFrame];
}
- (void)mtkView:(MTKView *)view drawableSizeWillChange:(CGSize)size {
@@ -92,8 +119,8 @@
#pragma mark - RTCVideoRenderer
- (void)setSize:(CGSize)size {
- _metalView.drawableSize = size;
- [_metalView draw];
+ self.metalView.drawableSize = size;
+ [self.metalView draw];
}
- (void)renderFrame:(nullable RTCVideoFrame *)frame {
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2723903003/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488556095171190,
"parent_rev": "446fb136dc5ec7c350e528363ec8c116f7e256ed", "commit_rev":
"0ebe0199acd1070f17ca2abc5bc22fdd8b0861ca"}
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for RTCMTLVideoView. To properly test the functionality, following changes were needed - Make RTCMTLVideoView compiliable for all cpu architectures not just arm64. This is needed so that the test can run on any device and on simulator as well. - Refactor RTCMTLVideoView to have mockable class methods. The unittest class, RTCMTLVideoViewTests was designed to provide easy transition to XCTest when the time comes for that. To transition to XCTest it would suffice to inherit from XCTestCase and remove the gtest methods. BUG=webrtc:7079 ========== to ========== Add unit tests for RTCMTLVideoView. To properly test the functionality, following changes were needed - Make RTCMTLVideoView compiliable for all cpu architectures not just arm64. This is needed so that the test can run on any device and on simulator as well. - Refactor RTCMTLVideoView to have mockable class methods. The unittest class, RTCMTLVideoViewTests was designed to provide easy transition to XCTest when the time comes for that. To transition to XCTest it would suffice to inherit from XCTestCase and remove the gtest methods. BUG=webrtc:7079 Review-Url: https://codereview.webrtc.org/2723903003 Cr-Commit-Position: refs/heads/master@{#17014} Committed: https://chromium.googlesource.com/external/webrtc/+/0ebe0199acd1070f17ca2abc5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/0ebe0199acd1070f17ca2abc5...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.webrtc.org/2733953006/ by kthelgason@webrtc.org. The reason for reverting is: This CL depends on a reverted CL.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
