[camera_avfoundation] fix race condition when starting image stream on iOS #8733
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
js2702
force-pushed
the
fix_startstream_race_condition
branch
from
89cc825 to
e902881
Compare
|
I'm not sure why but I'm getting a 400 error when signing the CLA (Which I thought I had already signed for other PR) |
|
This will require unit test in order to land. |
|
I'm looking for any example test but I don't see anything. As this doesn't affect the Dart part I don't see how to make a test for this, wouldn't this be test exempt? Additionally, this bug in particular manifests as a race condition in the communication with Dart and native, requiring an end to end test, which I'm not seeing either. |
There is no blanket test exemption for native code; if there were, almost nothing in plugins would be tested. Please see https://github.com/flutter/flutter/blob/master/docs/ecosystem/testing/Plugin-Tests.md |
# Conflicts: # packages/camera/camera_avfoundation/example/ios/Runner.xcodeproj/project.pbxproj # packages/camera/camera_avfoundation/example/ios/RunnerTests/StreamingTest.m
|
I added a test for the issue. Everything was working before this last Swift refactor. A few things:
Again, not an expert in Swift/Objective-C. Also, the test was developed in Master and it failed unless you added a delay before the expect. |
|
Ended up adding the async/await because the tests pass with that, I guess it's because of the callback added to the function? |
The async/await api is generated. Can you use completion callbacks to be consistent with other tests? |
…ondition # Conflicts: # packages/camera/camera_avfoundation/CHANGELOG.md
|
@hellohuanlin I updated the tests to use the completion callback, I had to move the expectation creation step of the streaming test because the existing expectation only were run when actually capturing output. |
|
@hellohuanlin I don't even know what is happening with the formating checks in CI, it keeps flipping between: If I make the change the CI will apply the other formatting. |
I think you're conflating the output for two different files: QueueUtils.m, and QueueUtils.h. I see consistent CI output across each file for the last three runs. The formatting between the files isn't consistent, but that's just a quirk of the header not being identified by |
|
@stuartmorgan Thank you but I'm not sure I'm following, what would be the fix? I'm running the provided format command and it doesn't change anything in the current state of the PR. This is the output |
|
The fix is to manually change the files to match what the CI diff output says the lines should look like.
You probably have a newer version of |
| @@ -1197,6 +1200,7 @@ - (void)startImageStreamWithMessenger:(NSObject<FlutterBinaryMessenger> *)messen | |||
|
|
|||
| strongSelf.isStreamingImages = YES; | |||
| strongSelf.streamingPendingFramesCount = 0; | |||
| completion(nil); | |||
There was a problem hiding this comment.
Do you want the \n removed there?
There was a problem hiding this comment.
oh sorry the github web ui shows it weirdly. This is good.
|
Yes, in the current state the test fails. |
packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraInitRaceConditionsTests.swift
Outdated
Show resolved
Hide resolved
| @@ -224,8 +224,7 @@ - (void)initializeCamera:(NSInteger)cameraId | |||
| - (void)startImageStreamWithCompletion:(nonnull void (^)(FlutterError *_Nullable))completion { | |||
| __weak typeof(self) weakSelf = self; | |||
| dispatch_async(self.captureSessionQueue, ^{ | |||
| [weakSelf.camera startImageStreamWithMessenger:weakSelf.messenger]; | |||
| completion(nil); | |||
| [weakSelf.camera startImageStreamWithMessenger:weakSelf.messenger withCompletion:completion]; | |||
There was a problem hiding this comment.
This needs to be converted to a strongSelf, and a codepath added that calls completion(nil) if strongSelf is nil. As written, this will never call completion if weakSelf is nil, which violates the API contract of platform channel completion handlers.
There was a problem hiding this comment.
I made some changes as you suggested, hopefully is OK now.
| @@ -842,7 +842,7 @@ - (void)startVideoRecordingWithCompletion:(void (^)(FlutterError *_Nullable))com | |||
| messengerForStreaming:(nullable NSObject<FlutterBinaryMessenger> *)messenger { | |||
| if (!_isRecording) { | |||
| if (messenger != nil) { | |||
| [self startImageStreamWithMessenger:messenger]; | |||
| [self startImageStreamWithMessenger:messenger withCompletion:completion]; | |||
There was a problem hiding this comment.
Why is completion being passed to a method and also used below? This looks like it will call completion multiple times, which has undefined behavior.
There was a problem hiding this comment.
Good catch, unfortunately providing nil for the completion seems to break the interfaces because _test interface is included in the FLTCam.m. My Objective-C knowledge is quite limited so I put an empty callback there which also works.
If you want a nullable completion I would appreciate if you could finish up this small change.
…condition # Conflicts: # packages/camera/camera_avfoundation/CHANGELOG.md # packages/camera/camera_avfoundation/pubspec.yaml
| createExpectation.fulfill() | ||
| } | ||
|
|
||
| waitForExpectations(timeout: 30, handler: nil) |
There was a problem hiding this comment.
Why are multiple expectation waits required? Usually this only needs to be done at the end of a test.
There was a problem hiding this comment.
Initially I was using async/await to wait for the camera creation in the test, before starting the stream. From an early review it was mentioned not using async/await in the tests so I changed it to two calls for waitForExpectations. The first one waits for the camera initialization and the second one actually checks for the stream initialization.
packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraInitRaceConditionsTests.swift
Outdated
Show resolved
Hide resolved
| finishStartStreamExpectation.fulfill() | ||
| }) | ||
|
|
||
| waitForExpectations(timeout: 30, handler: nil) |
There was a problem hiding this comment.
Same question here; why are there expectation waits?
...ion/ios/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCam.h
Outdated
Show resolved
Hide resolved
...os/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCam_Test.h
Outdated
Show resolved
Hide resolved
| @@ -847,7 +847,10 @@ - (void)startVideoRecordingWithCompletion:(void (^)(FlutterError *_Nullable))com | |||
| messengerForStreaming:(nullable NSObject<FlutterBinaryMessenger> *)messenger { | |||
| if (!_isRecording) { | |||
| if (messenger != nil) { | |||
| [self startImageStreamWithMessenger:messenger]; | |||
| // Start image stream without waiting for result. | |||
There was a problem hiding this comment.
The comment should explain why waiting isn't necessary here, as it is non-obvious to me why this case is different.
There was a problem hiding this comment.
Maybe you are right, how would I wait here so it does not continue before the completion is called?
There was a problem hiding this comment.
Code that needs to run after the method completes needs to be in the completion block; that's the purpose of the parameter.
There was a problem hiding this comment.
Just updated this to call the setup logic in the completion callback. Let me know if this is a proper change. I'm not comfortable with Objective-C.
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
# Conflicts: # packages/camera/camera_avfoundation/CHANGELOG.md # packages/camera/camera_avfoundation/example/ios/Runner.xcodeproj/project.pbxproj # packages/camera/camera_avfoundation/pubspec.yaml
# Conflicts: # packages/camera/camera_avfoundation/CHANGELOG.md # packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraInitRaceConditionsTests.swift
Fixes flutter/flutter#147702
It's not easy to reproduce this with the example application (as shown in the linked issue). We've been able to reproduce it in our own application which makes heavy usage of Platform channels and different native plugins. After digging into the code we notice that:
The Dart code that starts listening to the event channel was being executed before initializing the stream in the native side.
packages/packages/camera/camera_avfoundation/lib/src/avfoundation_camera.dart
Line 271 in 9744c9d
packages/packages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTThreadSafeEventChannel.m
Line 31 in 9744c9d
I don't have much experience with Objective-C, so not sure how to create unit tests for that if necessary but I believe this would be an adecuate solution. I've seen other methods in the plugin that use the same strategy passing the completion object.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.