[camera] Fixed a crash when streaming on iOS #4520

zuvola · 2021-11-18T05:27:45Z

Streaming large images on older devices seems to cause the call stack to fill up as the processing cannot keep up.
Added a confirmation of reception for each frame, and skipped frames that could not be processed.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-11-18T05:27:47Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

stuartmorgan-g

Thanks for the contribution!

stuartmorgan-g · 2021-11-30T18:37:15Z

packages/camera/camera/example/integration_test/camera_test.dart

+      });
+
+      await Future.delayed(Duration(seconds: 5));
+      expect(_frame > 30, true);


It seems like this is testing round-trip speed on the device it's running on, rather than anything about the code itself. Where is the value of 30 coming from? How do you know that this will always pass?

Surely 30 is not important, so I removed it.
I wanted to test if I could keep it running for a certain amount of time without crashing.
I'm not sure if 5 seconds is a reasonable number in this case.
Actually, I don't know how to create such environment-dependent tests.

stuartmorgan-g · 2021-11-30T18:37:50Z

packages/camera/camera/example/integration_test/camera_test.dart

@@ -240,4 +240,34 @@ void main() {
    },
    skip: !Platform.isAndroid,
  );
+
+  testWidgets(
+    'Image streaming persistence test on iOS',


Would this test fail if the rest of the PR were reverted? It doesn't seem that way, unless I'm missing something about the test.

It is a device dependent test, so if you test it on an old device, it will crash.

This means that it wouldn't actually fail in our CI, which means there's nothing preventing this from breaking again.

A test for something like this should be creating the conditions to allow for testing that the behavior is what we want regardless of the device, so that it has predictable output. If the goal is to prevent more than N frames from being in flight without response, for instance, then a test should deliberately respond very slowly and assert that N isn't exceeded.

stuartmorgan-g · 2021-11-30T18:38:23Z

packages/camera/camera/CHANGELOG.md

@@ -2,6 +2,7 @@

 * Fixes bug where calling a method after the camera was closed resulted in a Java `IllegalStateException` exception.
 * Fixes integration tests.
+* Fixes crash when streaming in iOS.


Changes cannot be added to versions that have already been published.

stuartmorgan-g · 2021-11-30T18:43:13Z

packages/camera/camera/lib/src/camera_controller.dart

@@ -438,6 +438,13 @@ class CameraController extends ValueNotifier<CameraValue> {
    _imageStreamSubscription =
        cameraEventChannel.receiveBroadcastStream().listen(
      (dynamic imageData) {
+        if (defaultTargetPlatform == TargetPlatform.iOS) {
+          try {
+            _channel.invokeMethod<void>('receivedImageStreamData');


Given that this is asynchronous in both directions I would expect this could cause significant frame loss even on a reasonably powerful device. How much investigation have you done into specifically what the parameters of the crash are? Is it really necessary to have a maximum of a single frame in flight?

It seems likely that this needs a more nuanced approach (maybe a hard limit still but higher than one, maybe something that's tunable by plugin clients). It's hard to determine the right fix without more details about what the failure condition actually is.

I modified the plugin to allow you to set a limit on the maximum number of frames pending processing.
And I tested it on my devices to see how many frames can be processed in 5 seconds. The results are shown in the table below.
The value is the number of frames processed at max quality and is the average of three times.
The horizontal axis is the maximum number of pending frames (frameStack), where 0 means no limit.
With no limit, iPhone6 and iPadAir crashed.
In this case, if frameStack is set to about 4, frame loss can be reduced to some extent.

0 1 2 3 4 5 6

iPhoneX 143 76 108 118 133 138 136

iPhone6 13(crash) 40 53 69 70 55 56

iPadAir-1st 14(crash) 44 62 53 63 52 45

I don't have that many devices, but according to flutter/flutter#44436, it seems to be crashing on iPhone 6, 7, 8, etc. These devices still have a large market share that cannot be ignored. Also, for image analysis processes such as OCR, low or medium quality is not always satisfactory.

Thanks for this testing. Given this table, I'm fine with starting from setting a fixed value—4 looks like a good place to start—with an explanation in the code of where that number came from, and then if it turns out that causes issues in some cases we can add an API to tune it later.

…into fix_ios_streaming

stuartmorgan-g

(Updating review status for the comments above)

…streaming # Conflicts: # packages/camera/camera/example/ios/Runner.xcodeproj/project.pbxproj # packages/camera/camera/ios/Classes/CameraPlugin.m

hellohuanlin

It's looking good

hellohuanlin · 2022-03-16T05:41:55Z

packages/camera/camera/example/ios/RunnerTests/StreamingTest.m

+  id messenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
+  [_camera startImageStreamWithMessenger:messenger imageStreamHandler:handlerMock];
+
+  [self waitForStart];


i highly recommend not to create this helper function since it's making the test a bit harder to read. Unit tests themselves are not unit tested, so it's better to make it as simple as possible.

I see. Fixed.

hellohuanlin · 2022-03-16T05:43:05Z

packages/camera/camera/example/ios/RunnerTests/StreamingTest.m

+  NSPredicate *predicate = [NSPredicate predicateWithFormat:@"isStreamingImages == YES"];
+  XCTNSPredicateExpectation *expectation =
+      [[XCTNSPredicateExpectation alloc] initWithPredicate:predicate object:_camera];
+  XCTWaiterResult result = [XCTWaiter waitForExpectations:@[ expectation ] timeout:3];


i'm curious will it fail if we use 1 second? due to predicate parsing? if so please document it out. otherwise let's keep 1 second.

also i think you can do [self expectationForPredicate...] and [self waitForExpectations:]

Using XCTNSPredicateExpectation, it seems to time out with a 1 second wait.
I tried using KVO this time and it seems to work well.

stuartmorgan-g

Looks good! Just a few minor style issues to resolve.

stuartmorgan-g · 2022-03-22T19:25:47Z

packages/camera/camera/ios/Classes/FLTCam.h

@@ -60,6 +60,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)setExposureModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr;
 - (void)setFocusModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr;
 - (void)applyFocusMode;
+- (void)receivedImageStreamData;


This needs a declaration comment, per style guide.

(I know almost nothing else in this header current has one, but existing style violations aren't a reason to perpetuate new style violations.)

Done.
But, my English isn't very good, so please point out any oddities.

packages/camera/camera/ios/Classes/FLTCam.m

stuartmorgan-g · 2022-03-22T19:27:16Z

packages/camera/camera/ios/Classes/FLTCam_Test.h

@@ -5,6 +5,14 @@
 #import "FLTCam.h"
 #import "FLTSavePhotoDelegate.h"

+@interface FLTImageStreamHandler : NSObject <FlutterStreamHandler>
+// The queue on which `eventSink` property should be accessed


Nit: These are declaration comments, so should follow declaration comment style (which is /// for this file).

stuartmorgan-g · 2022-03-22T19:27:28Z

packages/camera/camera/ios/Classes/FLTCam_Test.h

+@interface FLTImageStreamHandler : NSObject <FlutterStreamHandler>
+// The queue on which `eventSink` property should be accessed
+@property(nonatomic, strong) dispatch_queue_t captureSessionQueue;
+// `eventSink` property should be accessed on `captureSessionQueue`.


Blank line between declarations please.

stuartmorgan-g

Just a few more minor nits on the comments. Thanks for adding them!

stuartmorgan-g · 2022-03-23T15:35:05Z

packages/camera/camera/ios/Classes/FLTCam.h

@@ -60,6 +60,10 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)setExposureModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr;
 - (void)setFocusModeWithResult:(FLTThreadSafeFlutterResult *)result mode:(NSString *)modeStr;
 - (void)applyFocusMode;
+
+/// Sets that a streaming image has been received.


"Sets" implies that there's a boolean toggle here, which isn't the case. And the last sentence of this comment isn't relevant to clients using the method.

I wold replace this whole declaration comment with something like:

/** * Acknowledges the receipt of one image stream frame. * * This should be called each time a frame is received. Failing to call it may * cause later frames to be dropped instead of streamed. */

(This file uses /** ... */ style. Ideally we'd be consistent within the whole project, but we should at least have file-level consistency.)

I learned a lot. Thank you.
I ended up using your suggestion in its entirety.

stuartmorgan-g · 2022-03-23T15:36:05Z

packages/camera/camera/ios/Classes/FLTCam_Test.h

@@ -6,11 +6,14 @@
 #import "FLTSavePhotoDelegate.h"

 @interface FLTImageStreamHandler : NSObject <FlutterStreamHandler>
-// The queue on which `eventSink` property should be accessed
+
+/// The queue on which `eventSink` property should be accessed


Nit: missing period.

stuartmorgan-g · 2022-03-23T15:37:17Z

packages/camera/camera/ios/Classes/FLTCam.m

@@ -135,6 +133,11 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
  _videoFormat = kCVPixelFormatType_32BGRA;
  _inProgressSavePhotoDelegates = [NSMutableDictionary dictionary];

+  // To prevent memory consumption, limit the number of frames pending processing.


Nit: change "prevent" to "limit".

stuartmorgan-g · 2022-03-23T15:39:10Z

packages/camera/camera/ios/Classes/FLTCam_Test.h

+@property(nonatomic, strong) dispatch_queue_t captureSessionQueue;
+
+/// `eventSink` property should be accessed on `captureSessionQueue`.
+/// The block itself should be invoked on the main queue.


A declaration comment should start wit a description of what something is, before getting into how to use it. So:

/// The event sink to stream camera events to Dart. /// /// The property should only be accessed on `captureSessionQueue`. /// The block itself should be invoked on the main queue.

stuartmorgan-g

LGTM, thanks!

…ns#4520)

renanyoy · 2022-05-30T08:42:02Z

is it fixed in flutter 3.0 ?

jmagman · 2022-05-31T20:11:24Z

This was fixed in camera plugin version 0.9.4+18, not a version of Flutter.

renanyoy · 2022-06-01T08:33:25Z

ok, I have still a crash with the last version on iOS (it works on android)

flutter: |L2K|camera initialized
Initialized TensorFlow Lite runtime.
INFO: Initialized TensorFlow Lite runtime.
* thread #59, queue = 'ProviderImageSurfaceCacheQueue', stop reason = EXC_BAD_ACCESS (code=1, address=0x2a9598000)
libsystem_platform.dylib`_platform_memmove:
->  0x22b44fb0c <+188>: ldnp   q0, q1, [x1]
    0x22b44fb10 <+192>: add    x1, x1, #0x20
    0x22b44fb14 <+196>: subs   x2, x2, #0x20
    0x22b44fb18 <+200>: b.hi   0x22b44fb04               ; <+180>

edit: seems more related to googleMLKit addon..

AnimeshHestabit · 2022-07-13T14:42:02Z

I'm getting crash on video recording on camera v0.9.7+1 and the latest one (camera v0.9.8+1)

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: '*** -[AVAssetWriterInputPixelBufferAdaptor appendPixelBuffer:withPresentationTime:] A pixel buffer cannot be appended when readyForMoreMediaData is NO.'
*** First throw call stack:
(0x185349cac 0x19c3b8758 0x18def7a48 0x18def516c 0x18defa8ec 0x10383851c 0x1a7e897ac 0x1a7e74a20 0x1a80a79c8 0x1a823fc3c 0x1850162f0 0x184fb89c8 0x184fca304 0x184fbc41c 0x184fbcff0 0x184fc6ae4 0x1deccef38 0x1decceaa4)
libc++abi: terminating with uncaught exception of type NSException

thread Fix workaround for failing dynamic check in Xcode 7/sdk version 9. #25, queue = 'io.flutter.camera.captureSessionQueue', stop reason = signal SIGABRT
frame #0: 0x00000001be89ea48 libsystem_kernel.dylib__pthread_kill + 8 libsystem_kernel.dylib__pthread_kill:
-> 0x1be89ea48 <+8>: b.lo 0x1be89ea64 ; <+36>
0x1be89ea4c <+12>: stp x29, x30, [sp, #-0x10]!
0x1be89ea50 <+16>: mov x29, sp
0x1be89ea54 <+20>: bl 0x1be89a6d0 ; cerror_nocancel
Target 0: (Runner) stopped.
Lost connection to device.

stuartmorgan-g · 2022-07-13T16:06:56Z

The place to report new crashes is the issue tracker; this is a fix for an unrelated crash.

zuvola added 2 commits November 18, 2021 14:10

Fixed crash when streaming in iOS

29beaa7

Fixed skip judgment

aec0f93

zuvola requested a review from bparrishMines as a code owner November 18, 2021 05:27

flutter-dashboard bot added the needs tests label Nov 18, 2021

google-cla bot added the cla: yes label Nov 18, 2021

github-actions bot added p: camera platform-ios labels Nov 18, 2021

stuartmorgan-g removed the request for review from bparrishMines November 18, 2021 21:21

zuvola added 4 commits November 22, 2021 11:17

Merge branch 'flutter:master' into fix_ios_streaming

0d9f61a

Add test

7cff775

Update CHANGELOG.md

5d1fb56

Fix test

7c0fa9e

zuvola marked this pull request as draft November 24, 2021 00:43

zuvola marked this pull request as ready for review November 24, 2021 00:43

mamuseferha mentioned this pull request Nov 25, 2021

[iOS] camera plugin startImageStream crash in flutter v2.6.0-12.0.pre.837 flutter/flutter#94195

Closed

stuartmorgan-g reviewed Nov 30, 2021

View reviewed changes

zuvola added 6 commits December 10, 2021 13:18

Add frameStack property to startImageStream

3056e17

Merge branch 'flutter:master' into fix_ios_streaming

1a48ffc

Merge branch 'fix_ios_streaming' of https://github.com/zuvola/plugins …

4cb5d6e

…into fix_ios_streaming

Bump version to 0.9.4+6

27957a1

format

97d16c7

Fixed tests

1382d62

zuvola requested a review from stuartmorgan-g December 14, 2021 02:19

godofredoc changed the base branch from master to main January 6, 2022 22:53

stuartmorgan-g suggested changes Jan 27, 2022

View reviewed changes

zuvola added 4 commits January 27, 2022 14:07

Merge branch 'main' into fix_ios_streaming

46d9df0

Restored the API and set the number of pending frames to 4

84dc53d

Fixed tests

3ed1e94

Merge commit 'f7138f7222856623796c2ee2e561f866fcf08d38' into fix_ios_…

9a3e627

…streaming # Conflicts: # packages/camera/camera/example/ios/Runner.xcodeproj/project.pbxproj # packages/camera/camera/ios/Classes/CameraPlugin.m

hellohuanlin reviewed Mar 16, 2022

View reviewed changes

Using KVO to wait

7b8d74f

zuvola requested a review from hellohuanlin March 16, 2022 06:07

hellohuanlin approved these changes Mar 16, 2022

View reviewed changes

stuartmorgan-g suggested changes Mar 22, 2022

View reviewed changes

Add comments

ab233fa

zuvola requested a review from stuartmorgan-g March 23, 2022 05:08

stuartmorgan-g suggested changes Mar 23, 2022

View reviewed changes

Update comments

830d5ad

zuvola requested a review from stuartmorgan-g March 24, 2022 01:01

stuartmorgan-g removed the needs tests label Mar 25, 2022

stuartmorgan-g approved these changes Mar 25, 2022

View reviewed changes

stuartmorgan-g added 2 commits March 25, 2022 16:05

Merge branch 'main' into fix_ios_streaming

095faf3

Version bump

e185988

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Mar 25, 2022

fluttergithubbot merged commit 972e942 into flutter:main Mar 29, 2022

engine-flutter-autoroll mentioned this pull request Mar 29, 2022

Roll Plugins from 86b36210ed03 to 1ddfcc044615 (8 revisions) flutter/flutter#100970

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2022

972e942c0 [camera] Fixed a crash when streaming on iOS (flutter/plugi…

31d1859

…ns#4520)

engine-flutter-autoroll mentioned this pull request Mar 29, 2022

Roll Plugins from 86b36210ed03 to 8a5f9bf6c931 (9 revisions) flutter/flutter#100977

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2022

972e942c0 [camera] Fixed a crash when streaming on iOS (flutter/plugi…

f0c4a90

…ns#4520)

engine-flutter-autoroll mentioned this pull request Mar 30, 2022

Roll Plugins from 86b36210ed03 to b3e37559f966 (13 revisions) flutter/flutter#101050

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 30, 2022

972e942c0 [camera] Fixed a crash when streaming on iOS (flutter/plugi…

1ee1d9a

…ns#4520)

zuvola deleted the fix_ios_streaming branch June 1, 2022 00:11

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[camera] Fixed a crash when streaming on iOS (flutter#4520)

5bee6b5

kwikwag mentioned this pull request Apr 5, 2025

[camera_android] prevent startImageStream OOM error when main thread hangs (flutter#166533) flutter/packages#8998

Open

11 tasks

	0	1	2	3	4	5	6
iPhoneX	143	76	108	118	133	138	136
iPhone6	13(crash)	40	53	69	70	55	56
iPadAir-1st	14(crash)	44	62	53	63	52	45

[camera] Fixed a crash when streaming on iOS #4520

[camera] Fixed a crash when streaming on iOS #4520

Uh oh!

Conversation

zuvola commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Nov 18, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

renanyoy commented May 30, 2022

Uh oh!

jmagman commented May 31, 2022

zuvola commented Nov 18, 2021 •

edited

Loading

renanyoy commented Jun 1, 2022 •

edited

Loading