Fix unguarded availability in Camera plugin #2928

dnfield · 2020-08-12T23:01:08Z

~~The build is failing on this plugin in an unrelated PR currrently due to these issues.~~ This doesn't fail the build because we're specifically ignoring these warnings, so instead I'm enabling the warnings. This does cause crashes for users on older iOS versions, see linked bug.

This does make me wonder if we really should claim to support this plugin on iOS 8.0. Currently, you can't take a picture with the camera plugin on anything lower than iOS 10 - it would cause a runtime crash (now it will just result in an exception that should be recoverable).

Fixes flutter/flutter#20708

jmagman · 2020-08-12T23:09:37Z

This does make me wonder if we really should claim to support this plugin on iOS 8.0. Currently, you can't take a picture with the camera plugin on anything lower than iOS 10 - it would cause a runtime crash (now it will just result in an exception that should be recoverable).

If you changed it to 10.0 then Flutter apps at 8.0 or 9.0 minimums couldn't use the plugin. There would be a CocoaPods error or a build time error. The dependencies need to have a lower than equal requirement.

@dnfield can we get a link to the failure?

dnfield · 2020-08-12T23:11:20Z

Here's an example: https://cirrus-ci.com/task/5249881340116992

I only noticed the warnings because of an unrelated error that @ditman fixed and I hadn't patched in yet.

dnfield · 2020-08-12T23:12:12Z

As it is right now, saving a picture to a file on iOS 8.0 or 9.0 would result in a crash. Video functionality would still work. Dropping support for iOS 8.0 and 9.0 might be too severe, but we may want to implement these methods in some way friendly to the version(s) we support if we support them.

jmagman · 2020-08-12T23:15:28Z

flutter/flutter#20708

jmagman · 2020-08-12T23:22:41Z

If you get rid of --no-analyze camera and/or --ignore-warnings camera does it pass?

plugins/.cirrus.yml

Line 160 in e26634c

    
           - ./script/incremental_build.sh podspecs --no-analyze camera --ignore-warnings camera

jmagman · 2020-08-12T23:29:06Z

No, it doesn't pass.
CameraPlugin.m:66:24: 'AVCapturePhotoOutput' is only available on iOS 10.0 or newer
CameraPlugin.m:69:43: 'AVCaptureResolvedPhotoSettings' is only available on iOS 10.0 or newer
CameraPlugin.m:78:7: 'JPEGPhotoDataRepresentationForJPEGSampleBuffer:previewPhotoSampleBuffer:' is only available on iOS 10.0 or newer
CameraPlugin.m:77:19: 'AVCapturePhotoOutput' is only available on iOS 10.0 or newer
CameraPlugin.m:163:52: This block declaration is not a prototype
CameraPlugin.m:170:32: 'AVCapturePhotoOutput' is only available on iOS 10.0 or newer
CameraPlugin.m:257:26: 'AVCapturePhotoOutput' is only available on iOS 10.0 or newer
CameraPlugin.m:854:10: Block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior

dnfield · 2020-08-12T23:30:40Z

I'll see if I can fix those.

jmagman

Some other warnings:

AVCapturePhotoOutput and AVCaptureResolvedPhotoSettings usage (_capturePhotoOutput, -[captureOutput:::]) need the same availability treatment.
onFrameAvailable property is missing a void

@property(nonatomic, copy) void (^onFrameAvailable)(void);

jmagman · 2020-08-12T23:30:20Z

packages/camera/ios/Classes/CameraPlugin.m

@@ -204,7 +204,7 @@ - (void)startVideoRecordingAtPath:(NSString *)path result:(FlutterResult)result;
 - (void)stopVideoRecordingWithResult:(FlutterResult)result;
 - (void)startImageStreamWithMessenger:(NSObject<FlutterBinaryMessenger> *)messenger;
 - (void)stopImageStream;
- (void)captureToFile:(NSString *)filename result:(FlutterResult)result;
+- (void)captureToFile:(NSString *)filename result:(FlutterResult)result API_AVAILABLE(ios(10));


All the method declarations can be removed from this interface, it just needs the properties. API_AVAILABLE(ios(10.0)) can go in the implementation:

- (void)captureToFile:(NSString *)path result:(FlutterResult)result API_AVAILABLE(ios(10.0)) {

jmagman · 2020-08-12T23:47:56Z

packages/camera/ios/Classes/CameraPlugin.m

-        case AVCaptureDevicePositionUnspecified:
-          lensFacing = @"external";
-          break;
+  if (@available(iOS 10.0, *)) {


If call.method is availableCameras and @available(iOS 10.0, *) isn't true, should it result(@[]);? Or do we want the FlutterMethodNotImplemented (because I guess it isn't, though I don't know the side effects of that)?

I think FlutterMethodNotImplemented is most appropriate, since we haven't implemented the method for that platform.

Today, this is a hard crash for the end user. At least this way they can handle it in application code.

jmagman · 2020-08-12T23:56:49Z

packages/camera/ios/Classes/CameraPlugin.m

@@ -847,7 +836,7 @@ - (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)re
      int64_t textureId = [_registry registerTexture:cam];
      _camera = cam;
      cam.onFrameAvailable = ^{
-        [_registry textureFrameAvailable:textureId];
+        [self.registry textureFrameAvailable:textureId];


Is this a retain cycle?

jmagman · 2020-08-13T00:13:17Z

One more thing to get rid of the warnings:

plugins/packages/camera/ios/camera.podspec

Line 19 in 15b8029

    
           s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES', 'VALID_ARCHS' => 'armv7 arm64 x86_64' }

Should become:

s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES', 'VALID_ARCHS[sdk=iphonesimulator*]' => 'x86_64' }

Like all the other plugins. Example:

plugins/packages/device_info/device_info/ios/device_info.podspec

Line 21 in 42b33ec

    
           s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES', 'VALID_ARCHS[sdk=iphonesimulator*]' => 'x86_64' }

That will fix flutter/flutter#61927.

jmagman

LGTM, thank you!

jmagman · 2020-08-13T00:19:22Z

packages/camera/ios/Classes/CameraPlugin.m

+    } else {
+      result(FlutterMethodNotImplemented);


I think what you had before would have fallen through to FlutterMethodNotImplemented, whatever you prefer though.

Yeah but it'd make the other else blocks not run on iOS 10+ :\

jmagman · 2020-08-13T00:43:22Z

Static analyzer error:

CameraPlugin.m:481:5: Incorrect decrement of the reference count of an object that is not owned at this point by the caller

Here:

plugins/packages/camera/ios/Classes/CameraPlugin.m

Line 494 in 05c2505

CFRelease(sampleBuffer);

It's being released twice if it hits this codepath:

plugins/packages/camera/ios/Classes/CameraPlugin.m

Line 487 in 05c2505

CFRelease(sampleBuffer);

jmagman · 2020-08-13T01:00:16Z

Looks like -[adjustTime::] is making a copy of the CMSampleBufferRef, so the code is actually right. Just need to tell the analyzer that we know what we're doing by either adding "copy" to the method name or a CF_RETURNS_RETAINED.

- (CMSampleBufferRef)adjustTime:(CMSampleBufferRef)sample by:(CMTime)offset CF_RETURNS_RETAINED {

or

- (CMSampleBufferRef)copyBufferAndAdjustTime:(CMSampleBufferRef)sample by:(CMTime)offset {

jmagman

LGTM!

* Fix unguarded availability in Camera plugin

Fix unguarded availability in Camera plugin

0dea11a

dnfield requested review from cyanglaz and bparrishMines August 12, 2020 23:01

googlebot added the cla: yes label Aug 12, 2020

dnfield requested a review from jmagman August 12, 2020 23:04

format

f31a05c

cirrus update

94b45db

jmagman reviewed Aug 12, 2020

View reviewed changes

dnfield added 2 commits August 12, 2020 16:51

more

6732730

jmagman review

ac16333

jmagman reviewed Aug 12, 2020

View reviewed changes

dnfield added 2 commits August 12, 2020 17:02

derp

b97f499

weakSelf

a8710a2

dnfield mentioned this pull request Aug 13, 2020

plugin: camera - crashes on iOS < 10 flutter/flutter#20708

Closed

dnfield added 2 commits August 12, 2020 17:10

format again

a5a18b4

..

76a8389

fix podspec

ef301fa

jmagman approved these changes Aug 13, 2020

View reviewed changes

copy

f320102

dnfield added 2 commits August 12, 2020 21:40

different way

1e3368f

cleanup podspec

c6f8bca

jmagman approved these changes Aug 13, 2020

View reviewed changes

dnfield merged commit 235283b into flutter:master Aug 13, 2020

dnfield deleted the fix_camera branch August 13, 2020 06:24

jmagman mentioned this pull request Aug 13, 2020

[Camera] warning architecture on Xcode. flutter/flutter#61927

Closed

jarrodcolburn pushed a commit to jarrodcolburn/plugins that referenced this pull request Aug 20, 2020

Fix unguarded availability in Camera plugin (flutter#2928)

d80e539

* Fix unguarded availability in Camera plugin

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

Fix unguarded availability in Camera plugin (flutter#2928)

72d5753

* Fix unguarded availability in Camera plugin

jmagman mentioned this pull request Nov 5, 2020

Building Flutter app for iOS - "GeneratedPluginRegistrant.m:10:9: Module 'camera' not found" flutter/flutter#54599

Closed

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

Fix unguarded availability in Camera plugin (flutter#2928)

f387fdf

* Fix unguarded availability in Camera plugin

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unguarded availability in Camera plugin #2928

Fix unguarded availability in Camera plugin #2928

dnfield commented Aug 12, 2020 •

edited

Loading

jmagman commented Aug 12, 2020

dnfield commented Aug 12, 2020

dnfield commented Aug 12, 2020

jmagman commented Aug 12, 2020

jmagman commented Aug 12, 2020 •

edited

Loading

jmagman commented Aug 12, 2020

dnfield commented Aug 12, 2020

jmagman left a comment

jmagman Aug 12, 2020

jmagman Aug 12, 2020

dnfield Aug 12, 2020

jmagman Aug 12, 2020

jmagman commented Aug 13, 2020

jmagman left a comment

jmagman Aug 13, 2020

dnfield Aug 13, 2020

jmagman commented Aug 13, 2020

jmagman commented Aug 13, 2020

jmagman left a comment

Fix unguarded availability in Camera plugin #2928

Fix unguarded availability in Camera plugin #2928

Conversation

dnfield commented Aug 12, 2020 • edited Loading

jmagman commented Aug 12, 2020

dnfield commented Aug 12, 2020

dnfield commented Aug 12, 2020

jmagman commented Aug 12, 2020

jmagman commented Aug 12, 2020 • edited Loading

jmagman commented Aug 12, 2020

dnfield commented Aug 12, 2020

jmagman left a comment

Choose a reason for hiding this comment

jmagman Aug 12, 2020

Choose a reason for hiding this comment

jmagman Aug 12, 2020

Choose a reason for hiding this comment

dnfield Aug 12, 2020

Choose a reason for hiding this comment

jmagman Aug 12, 2020

Choose a reason for hiding this comment

jmagman commented Aug 13, 2020

jmagman left a comment

Choose a reason for hiding this comment

jmagman Aug 13, 2020

Choose a reason for hiding this comment

dnfield Aug 13, 2020

Choose a reason for hiding this comment

jmagman commented Aug 13, 2020

jmagman commented Aug 13, 2020

jmagman left a comment

Choose a reason for hiding this comment

dnfield commented Aug 12, 2020 •

edited

Loading

jmagman commented Aug 12, 2020 •

edited

Loading