Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Handle "create" method call in main thread on iOS #4007

Closed
wants to merge 5 commits into from
Closed

[camera] Handle "create" method call in main thread on iOS #4007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8d7ea0
Handle "create" method call in main thread on iOS.
BeMacized Jun 3, 2021
b6f6d9e
Updated changelog and pubspec version
BeMacized Jun 3, 2021
e846c64
Merge remote-tracking branch 'upstream/master'
BeMacized Jun 4, 2021
b16f1d7
Merge branch 'master' into issue/52578
BeMacized Jun 4, 2021
c336bcb
[camera] Add native iOS unit tests
BeMacized Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/camera/camera/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.8.1+4

* Moved texture registration to the main thread on iOS to improve stability.

## 0.8.1+3

* Do not change camera orientation when iOS device is flat.
Expand Down
108 changes: 108 additions & 0 deletions packages/camera/camera/example/ios/RunnerTests/CameraPluginTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file hasn't been added to the project, so isn't being run.

@import camera;
@import XCTest;

#import <OCMock/OCMock.h>

@interface CameraPlugin ()
- (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry
messenger:(NSObject<FlutterBinaryMessenger> *)messenger;
- (BOOL)handleMethodCallSync:(FlutterMethodCall *)call result:(FlutterResult)result;
- (BOOL)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing with private methods should only be done when a test can't be done with public methods. I don't see why accessing these should be necessary.

@end

@interface CameraPluginTests : XCTestCase
@property(strong, nonatomic) id mockRegistrar;
@property(strong, nonatomic) id mockMessenger;
@property(strong, nonatomic) CameraPlugin *plugin;
@end

@implementation CameraPluginTests

- (void)setUp {
[super setUp];
self.mockRegistrar = OCMProtocolMock(@protocol(FlutterPluginRegistrar));
self.mockMessenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
OCMStub([self.mockRegistrar messenger]).andReturn(self.mockMessenger);
self.plugin = [[CameraPlugin alloc] initWithRegistry:self.mockRegistrar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't make the object under test stateful. Test fixtures should have as little state as possible so that it's obvious what's happening within each test, and that's especially true of seeing how the object under test is being set up.

messenger:self.mockMessenger];
}

- (void)testHandleMethodCallSync_ShouldHandleSyncMethods {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"create");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a mock? Mocking is a tool of last resort; FlutterMethodCall is trivial to instantiate.


BOOL result = [[self plugin] handleMethodCallSync:methodCallMock
result:^(id _Nullable result){
}];

XCTAssertTrue(result);
}

- (void)testHandleMethodCallSync_ShouldNotHandleAsyncMethods {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"initialize");

BOOL result = [[self plugin] handleMethodCallSync:methodCallMock
result:^(id _Nullable result){
}];

XCTAssertFalse(result);
}

- (void)testHandleMethodCallAsync_ShouldHandleAsyncMethods {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"initialize");

BOOL result = [[self plugin] handleMethodCallAsync:methodCallMock
result:^(id _Nullable result){
}];

XCTAssertTrue(result);
}

- (void)testHandleMethodCallAsync_ShouldNotHandleSyncMethods {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"create");

BOOL result = [[self plugin] handleMethodCallAsync:methodCallMock
result:^(id _Nullable result){
}];

XCTAssertFalse(result);
}

- (void)testHandleMethodCall_ShouldNotCallAsyncHandlerForSyncMethod {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"create");
id mockedPlugin = OCMPartialMock(self.plugin);
id result = ^(id _Nullable result) {
};
OCMStub([mockedPlugin handleMethodCallSync:methodCallMock result:result]).andReturn(true);
OCMStub([mockedPlugin handleMethodCallAsync:methodCallMock result:result]).andReturn(false);

[[self plugin] handleMethodCall:methodCallMock result:result];

OCMVerify([mockedPlugin handleMethodCallSync:methodCallMock result:result]);
OCMVerify(never(), [mockedPlugin handleMethodCallAsync:methodCallMock result:result]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these tests are testing for an implementation detail rather than testing that the thing we want to be true—that interactions with the engine are on the main thread. We should test for what we want to assert, not for an implementation that we believe has the properties we want.

As one of many reasons this is true: would these tests fail if someone accidentally swapped which internal method call was done on a queue and which one wasn't?

See #3778 for an example where I fixed a similar issue in another plugin, and handled it with assertions in the code and tests designed to exercise the assertions without assuming things about the implementation details of the plugin itself.

}

- (void)testHandleMethodCall_ShouldCallAsyncHandlerForAsyncMethod {
id methodCallMock = OCMClassMock([FlutterMethodCall class]);
OCMStub([methodCallMock method]).andReturn(@"initialize");
id mockedPlugin = OCMPartialMock(self.plugin);
id result = ^(id _Nullable result) {
};
OCMStub([mockedPlugin handleMethodCallSync:methodCallMock result:result]).andReturn(false);
OCMStub([mockedPlugin handleMethodCallAsync:methodCallMock result:result]).andReturn(true);

[[self plugin] handleMethodCall:methodCallMock result:result];

OCMVerify([mockedPlugin handleMethodCallSync:methodCallMock result:result]);
OCMVerify([mockedPlugin handleMethodCallAsync:methodCallMock result:result]);
}

@end
68 changes: 41 additions & 27 deletions packages/camera/camera/ios/Classes/CameraPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,6 +1271,7 @@ - (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry
_messenger = messenger;
[self initDeviceEventMethodChannel];
[self startOrientationListener];

return self;
}

Expand Down Expand Up @@ -1315,13 +1316,48 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
_dispatchQueue = dispatch_queue_create("io.flutter.camera.dispatchqueue", NULL);
}

// Invoke the plugin on another dispatch queue to avoid blocking the UI.
// Handle method calls on platform thread for those that require it.
if ([self handleMethodCallSync:call result:result]) {
return;
}

// Otherwise invoke the plugin on another dispatch queue to avoid blocking the UI.
dispatch_async(_dispatchQueue, ^{
[self handleMethodCallAsync:call result:result];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commented on the issue to get clarity, but I believe all uses of result on the background queue are also incorrect, so this needs a larger restructure.

Delegating to a queue should very likely be limited to just expensive operations, rather than a default behavior.

});
}

- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)result {
- (BOOL)handleMethodCallSync:(FlutterMethodCall *)call result:(FlutterResult)result {
if ([@"create" isEqualToString:call.method]) {
NSString *cameraName = call.arguments[@"cameraName"];
NSString *resolutionPreset = call.arguments[@"resolutionPreset"];
NSNumber *enableAudio = call.arguments[@"enableAudio"];
NSError *error;
FLTCam *cam = [[FLTCam alloc] initWithCameraName:cameraName
resolutionPreset:resolutionPreset
enableAudio:[enableAudio boolValue]
orientation:[[UIDevice currentDevice] orientation]
dispatchQueue:_dispatchQueue
error:&error];
if (error) {
result(getFlutterError(error));
} else {
if (_camera) {
[_camera close];
}
int64_t textureId = [_registry registerTexture:cam];
_camera = cam;

result(@{
@"cameraId" : @(textureId),
});
}
return true;
}
return false;
}

- (BOOL)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)result {
if ([@"availableCameras" isEqualToString:call.method]) {
if (@available(iOS 10.0, *)) {
AVCaptureDeviceDiscoverySession *discoverySession = [AVCaptureDeviceDiscoverySession
Expand Down Expand Up @@ -1353,31 +1389,7 @@ - (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)re
result(reply);
} else {
result(FlutterMethodNotImplemented);
}
} else if ([@"create" isEqualToString:call.method]) {
NSString *cameraName = call.arguments[@"cameraName"];
NSString *resolutionPreset = call.arguments[@"resolutionPreset"];
NSNumber *enableAudio = call.arguments[@"enableAudio"];
NSError *error;
FLTCam *cam = [[FLTCam alloc] initWithCameraName:cameraName
resolutionPreset:resolutionPreset
enableAudio:[enableAudio boolValue]
orientation:[[UIDevice currentDevice] orientation]
dispatchQueue:_dispatchQueue
error:&error];

if (error) {
result(getFlutterError(error));
} else {
if (_camera) {
[_camera close];
}
int64_t textureId = [_registry registerTexture:cam];
_camera = cam;

result(@{
@"cameraId" : @(textureId),
});
return false;
}
} else if ([@"startImageStream" isEqualToString:call.method]) {
[_camera startImageStreamWithMessenger:_messenger];
Expand Down Expand Up @@ -1483,8 +1495,10 @@ - (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)re
[_camera setFocusPointWithResult:result x:x y:y];
} else {
result(FlutterMethodNotImplemented);
return false;
}
}
return true;
}

@end
2 changes: 1 addition & 1 deletion packages/camera/camera/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description: A Flutter plugin for getting information about and controlling the
and streaming image buffers to dart.
repository: https://github.com/flutter/plugins/tree/master/packages/camera/camera
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.8.1+3
version: 0.8.1+4

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down