Skip to content

[camera] Remove @throw from iOS implementation #5034

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Sep 29, 2023
Merged

[camera] Remove @throw from iOS implementation #5034

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
741d98b
[camera] Remove `@throw` from iOS implementation
stuartmorgan-g Sep 29, 2023
c1bffdc
Add switch handling
stuartmorgan-g Sep 29, 2023
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_avfoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.9.13+6

* Fixes incorrect use of `NSError` that could cause crashes on launch.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ... cause crashes on launch on iOS 17

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I unfortunately don't know if it's "on iOS 17" or "when compiled against the iOS 17 SDK" or some other slight variant of that, which is why I left it vague.


## 0.9.13+5

* Ignores audio samples until the first video sample arrives.
Expand Down
14 changes: 7 additions & 7 deletions packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraPropertiesTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ - (void)testFLTGetFLTFlashModeForString {
XCTAssertEqual(FLTFlashModeAuto, FLTGetFLTFlashModeForString(@"auto"));
XCTAssertEqual(FLTFlashModeAlways, FLTGetFLTFlashModeForString(@"always"));
XCTAssertEqual(FLTFlashModeTorch, FLTGetFLTFlashModeForString(@"torch"));
XCTAssertThrows(FLTGetFLTFlashModeForString(@"unkwown"));
XCTAssertEqual(FLTFlashModeInvalid, FLTGetFLTFlashModeForString(@"unknown"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should it be FLTFlashModeUnknown? in case we have new flash mode in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we have a new flash mode in the future then we need to update both the Dart and Obj-C code at the same time. This code is purely for manual platform channel conversions, which means the only way to get a value we don't recognize is if someone adds serialization for that mode on the Dart side but not the corresponding deserialization code on the native side, which would make no sense.

If we have flash modes that the Dart side of camera_avfoundation has been updated to send over the method channel but the Obj-C side doesn't know about, then someone has made a serious mistake in adding a new flash mode. We shouldn't explicitly allow for that case with an "unknown", implying that's an okay state to be in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(This is why Pigeon will moot all of this; in Pigeon it's impossible for an enum value defined in the interface to exist on one side but not the other.)

}

- (void)testFLTGetAVCaptureFlashModeForFLTFlashMode {
Expand All @@ -34,27 +34,27 @@ - (void)testFLTGetAVCaptureFlashModeForFLTFlashMode {
- (void)testFLTGetStringForFLTExposureMode {
XCTAssertEqualObjects(@"auto", FLTGetStringForFLTExposureMode(FLTExposureModeAuto));
XCTAssertEqualObjects(@"locked", FLTGetStringForFLTExposureMode(FLTExposureModeLocked));
XCTAssertThrows(FLTGetStringForFLTExposureMode(-1));
XCTAssertNil(FLTGetStringForFLTExposureMode(-1));
}

- (void)testFLTGetFLTExposureModeForString {
XCTAssertEqual(FLTExposureModeAuto, FLTGetFLTExposureModeForString(@"auto"));
XCTAssertEqual(FLTExposureModeLocked, FLTGetFLTExposureModeForString(@"locked"));
XCTAssertThrows(FLTGetFLTExposureModeForString(@"unknown"));
XCTAssertEqual(FLTExposureModeInvalid, FLTGetFLTExposureModeForString(@"unknown"));
}

#pragma mark - focus mode tests

- (void)testFLTGetStringForFLTFocusMode {
XCTAssertEqualObjects(@"auto", FLTGetStringForFLTFocusMode(FLTFocusModeAuto));
XCTAssertEqualObjects(@"locked", FLTGetStringForFLTFocusMode(FLTFocusModeLocked));
XCTAssertThrows(FLTGetStringForFLTFocusMode(-1));
XCTAssertNil(FLTGetStringForFLTFocusMode(-1));
}

- (void)testFLTGetFLTFocusModeForString {
XCTAssertEqual(FLTFocusModeAuto, FLTGetFLTFocusModeForString(@"auto"));
XCTAssertEqual(FLTFocusModeLocked, FLTGetFLTFocusModeForString(@"locked"));
XCTAssertThrows(FLTGetFLTFocusModeForString(@"unknown"));
XCTAssertEqual(FLTFocusModeInvalid, FLTGetFLTFocusModeForString(@"unknown"));
}

#pragma mark - resolution preset tests
Expand All @@ -67,7 +67,7 @@ - (void)testFLTGetFLTResolutionPresetForString {
XCTAssertEqual(FLTResolutionPresetVeryHigh, FLTGetFLTResolutionPresetForString(@"veryHigh"));
XCTAssertEqual(FLTResolutionPresetUltraHigh, FLTGetFLTResolutionPresetForString(@"ultraHigh"));
XCTAssertEqual(FLTResolutionPresetMax, FLTGetFLTResolutionPresetForString(@"max"));
XCTAssertThrows(FLTGetFLTFlashModeForString(@"unknown"));
XCTAssertEqual(FLTResolutionPresetInvalid, FLTGetFLTResolutionPresetForString(@"unknown"));
}

#pragma mark - video format tests
Expand All @@ -89,7 +89,7 @@ - (void)testFLTGetUIDeviceOrientationForString {
XCTAssertEqual(UIDeviceOrientationLandscapeLeft,
FLTGetUIDeviceOrientationForString(@"landscapeRight"));
XCTAssertEqual(UIDeviceOrientationPortrait, FLTGetUIDeviceOrientationForString(@"portraitUp"));
XCTAssertThrows(FLTGetUIDeviceOrientationForString(@"unknown"));
XCTAssertEqual(UIDeviceOrientationUnknown, FLTGetUIDeviceOrientationForString(@"unknown"));
}

- (void)testFLTGetStringForUIDeviceOrientation {
Expand Down
12 changes: 12 additions & 0 deletions packages/camera/camera_avfoundation/ios/Classes/CameraProperties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ typedef NS_ENUM(NSInteger, FLTFlashMode) {
FLTFlashModeAuto,
FLTFlashModeAlways,
FLTFlashModeTorch,
// This should never occur; it indicates an unknown value was received over
// the platform channel.
FLTFlashModeInvalid,
};

/**
Expand All @@ -39,6 +42,9 @@ extern AVCaptureFlashMode FLTGetAVCaptureFlashModeForFLTFlashMode(FLTFlashMode m
typedef NS_ENUM(NSInteger, FLTExposureMode) {
FLTExposureModeAuto,
FLTExposureModeLocked,
// This should never occur; it indicates an unknown value was received over
// the platform channel.
FLTExposureModeInvalid,
};

/**
Expand All @@ -61,6 +67,9 @@ extern FLTExposureMode FLTGetFLTExposureModeForString(NSString *mode);
typedef NS_ENUM(NSInteger, FLTFocusMode) {
FLTFocusModeAuto,
FLTFocusModeLocked,
// This should never occur; it indicates an unknown value was received over
// the platform channel.
FLTFocusModeInvalid,
};

/**
Expand Down Expand Up @@ -100,6 +109,9 @@ typedef NS_ENUM(NSInteger, FLTResolutionPreset) {
FLTResolutionPresetVeryHigh,
FLTResolutionPresetUltraHigh,
FLTResolutionPresetMax,
// This should never occur; it indicates an unknown value was received over
// the platform channel.
FLTResolutionPresetInvalid,
};

/**
Expand Down
63 changes: 13 additions & 50 deletions packages/camera/camera_avfoundation/ios/Classes/CameraProperties.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,7 @@ FLTFlashMode FLTGetFLTFlashModeForString(NSString *mode) {
} else if ([mode isEqualToString:@"torch"]) {
return FLTFlashModeTorch;
} else {
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown flash mode %@", mode]
}];
@throw error;
return FLTFlashModeInvalid;
}
}

Expand All @@ -48,14 +42,11 @@ AVCaptureFlashMode FLTGetAVCaptureFlashModeForFLTFlashMode(FLTFlashMode mode) {
return @"auto";
case FLTExposureModeLocked:
return @"locked";
case FLTExposureModeInvalid:
// This value should never actually be used.
return nil;
}
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown string for exposure mode"]
}];
@throw error;
return nil;
}

FLTExposureMode FLTGetFLTExposureModeForString(NSString *mode) {
Expand All @@ -64,13 +55,7 @@ FLTExposureMode FLTGetFLTExposureModeForString(NSString *mode) {
} else if ([mode isEqualToString:@"locked"]) {
return FLTExposureModeLocked;
} else {
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown exposure mode %@", mode]
}];
@throw error;
return FLTExposureModeInvalid;
}
}

Expand All @@ -82,14 +67,11 @@ FLTExposureMode FLTGetFLTExposureModeForString(NSString *mode) {
return @"auto";
case FLTFocusModeLocked:
return @"locked";
case FLTFocusModeInvalid:
// This value should never actually be used.
return nil;
}
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown string for focus mode"]
}];
@throw error;
return nil;
}

FLTFocusMode FLTGetFLTFocusModeForString(NSString *mode) {
Expand All @@ -98,13 +80,7 @@ FLTFocusMode FLTGetFLTFocusModeForString(NSString *mode) {
} else if ([mode isEqualToString:@"locked"]) {
return FLTFocusModeLocked;
} else {
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown focus mode %@", mode]
}];
@throw error;
return FLTFocusModeInvalid;
}
}

Expand All @@ -120,14 +96,7 @@ UIDeviceOrientation FLTGetUIDeviceOrientationForString(NSString *orientation) {
} else if ([orientation isEqualToString:@"portraitUp"]) {
return UIDeviceOrientationPortrait;
} else {
NSError *error = [NSError
errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
[NSString stringWithFormat:@"Unknown device orientation %@", orientation]
}];
@throw error;
return UIDeviceOrientationUnknown;
}
}

Expand Down Expand Up @@ -163,13 +132,7 @@ FLTResolutionPreset FLTGetFLTResolutionPresetForString(NSString *preset) {
} else if ([preset isEqualToString:@"max"]) {
return FLTResolutionPresetMax;
} else {
NSError *error = [NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown resolution preset %@", preset]
}];
@throw error;
return FLTResolutionPresetInvalid;
}
}

Expand Down
91 changes: 57 additions & 34 deletions packages/camera/camera_avfoundation/ios/Classes/FLTCam.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,16 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
error:(NSError **)error {
self = [super init];
NSAssert(self, @"super init cannot be nil");
@try {
_resolutionPreset = FLTGetFLTResolutionPresetForString(resolutionPreset);
} @catch (NSError *e) {
*error = e;
_resolutionPreset = FLTGetFLTResolutionPresetForString(resolutionPreset);
if (_resolutionPreset == FLTResolutionPresetInvalid) {
*error = [NSError
errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
[NSString stringWithFormat:@"Unknown resolution preset %@", resolutionPreset]
}];
return nil;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is technically a behavioral change since not returning nil on error was clearly wrong, but also this should never be able to happen anyway.

}
_enableAudio = enableAudio;
_captureSessionQueue = captureSessionQueue;
Expand Down Expand Up @@ -162,7 +168,9 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
_motionManager = [[CMMotionManager alloc] init];
[_motionManager startAccelerometerUpdates];

[self setCaptureSessionPreset:_resolutionPreset];
if (![self setCaptureSessionPreset:_resolutionPreset withError:error]) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also technically a behavioral change in that now the error actually gets returned instead of us having an unhandled @throw of an NSError, which... I don't even actually know what that would do. Crash the app probably?

But as above, this should not actually be a thing that can happen as far as I can tell.

return nil;
}
[self updateOrientation];

return self;
Expand Down Expand Up @@ -337,7 +345,7 @@ - (NSString *)getTemporaryFilePathWithExtension:(NSString *)extension
return file;
}

- (void)setCaptureSessionPreset:(FLTResolutionPreset)resolutionPreset {
- (BOOL)setCaptureSessionPreset:(FLTResolutionPreset)resolutionPreset withError:(NSError **)error {
switch (resolutionPreset) {
case FLTResolutionPresetMax:
case FLTResolutionPresetUltraHigh:
Expand Down Expand Up @@ -382,17 +390,17 @@ - (void)setCaptureSessionPreset:(FLTResolutionPreset)resolutionPreset {
_videoCaptureSession.sessionPreset = AVCaptureSessionPresetLow;
_previewSize = CGSizeMake(352, 288);
} else {
NSError *error =
[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
@"No capture session available for current capture session."
}];
@throw error;
*error = [NSError errorWithDomain:NSCocoaErrorDomain
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of these NSErrors are directly moved from the code that was @throwing them, to preserve behavior. As noted in the PR description, I'm deliberately not changing the weird domain and code used in these errors for now.

This was some deeply strange Obj-C code.

code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
@"No capture session available for current capture session."
}];
return NO;
}
}
_audioCaptureSession.sessionPreset = _videoCaptureSession.sessionPreset;
return YES;
}

- (void)captureOutput:(AVCaptureOutput *)output
Expand Down Expand Up @@ -726,11 +734,17 @@ - (void)resumeVideoRecordingWithResult:(FLTThreadSafeFlutterResult *)result {

- (void)lockCaptureOrientationWithResult:(FLTThreadSafeFlutterResult *)result
orientation:(NSString *)orientationStr {
UIDeviceOrientation orientation;
@try {
orientation = FLTGetUIDeviceOrientationForString(orientationStr);
} @catch (NSError *e) {
[result sendError:e];
UIDeviceOrientation orientation = FLTGetUIDeviceOrientationForString(orientationStr);
// "Unknown" should never be sent, so is used to represent an unexpected
// value.
if (orientation == UIDeviceOrientationUnknown) {
[result sendError:[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown device orientation %@",
orientationStr]
}]];
return;
}

Expand All @@ -749,11 +763,14 @@ - (void)unlockCaptureOrientationWithResult:(FLTThreadSafeFlutterResult *)result
}

- (void)setFlashModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr {
FLTFlashMode mode;
@try {
mode = FLTGetFLTFlashModeForString(modeStr);
} @catch (NSError *e) {
[result sendError:e];
FLTFlashMode mode = FLTGetFLTFlashModeForString(modeStr);
if (mode == FLTFlashModeInvalid) {
[result sendError:[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown flash mode %@", modeStr]
}]];
return;
}
if (mode == FLTFlashModeTorch) {
Expand Down Expand Up @@ -800,11 +817,14 @@ - (void)setFlashModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSStri
}

- (void)setExposureModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr {
FLTExposureMode mode;
@try {
mode = FLTGetFLTExposureModeForString(modeStr);
} @catch (NSError *e) {
[result sendError:e];
FLTExposureMode mode = FLTGetFLTExposureModeForString(modeStr);
if (mode == FLTExposureModeInvalid) {
[result sendError:[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown exposure mode %@", modeStr]
}]];
return;
}
_exposureMode = mode;
Expand All @@ -830,11 +850,14 @@ - (void)applyExposureMode {
}

- (void)setFocusModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr {
FLTFocusMode mode;
@try {
mode = FLTGetFLTFocusModeForString(modeStr);
} @catch (NSError *e) {
[result sendError:e];
FLTFocusMode mode = FLTGetFLTFocusModeForString(modeStr);
if (mode == FLTFocusModeInvalid) {
[result sendError:[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey : [NSString
stringWithFormat:@"Unknown focus mode %@", modeStr]
}]];
return;
}
_focusMode = mode;
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_avfoundation/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: camera_avfoundation
description: iOS implementation of the camera plugin.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.9.13+5
version: 0.9.13+6

environment:
sdk: ">=2.19.0 <4.0.0"
Expand Down