This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[camera]call engine API in main thread to fix a crash #4661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hellohuanlin
force-pushed
the
camera_thread_safe
branch
2 times, most recently
from
January 11, 2022 01:44
dc9fd82
to
959eb1a
Compare
hellohuanlin
commented
Jan 11, 2022
hellohuanlin
force-pushed
the
camera_thread_safe
branch
from
January 11, 2022 01:56
959eb1a
to
fe2a14b
Compare
hellohuanlin
commented
Jan 11, 2022
packages/camera/camera/example/ios/RunnerTests/ThreadSafeEventChannelTests.m
Outdated
Show resolved
Hide resolved
hellohuanlin
commented
Jan 11, 2022
hellohuanlin
commented
Jan 11, 2022
hellohuanlin
commented
Jan 11, 2022
hellohuanlin
commented
Jan 11, 2022
Hi @jmagman @cyanglaz and @stuartmorgan, I re-created this PR from last year. I left comments about what's updated from the last PR. The biggest change is to use PTAL again, thank you! |
hellohuanlin
requested review from
jmagman,
stuartmorgan and
cyanglaz
and removed request for
bparrishMines
January 11, 2022 02:05
stuartmorgan
suggested changes
Jan 11, 2022
packages/camera/camera/example/ios/RunnerTests/ThreadSafeEventChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
stuartmorgan
suggested changes
Jan 13, 2022
packages/camera/camera/example/ios/RunnerTests/ThreadSafeEventChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeTextureRegistry.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeTextureRegistry.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeTextureRegistry.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeTextureRegistry.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h
Outdated
Show resolved
Hide resolved
hellohuanlin
force-pushed
the
camera_thread_safe
branch
2 times, most recently
from
January 13, 2022 20:00
05fa6ba
to
a0ccf76
Compare
hellohuanlin
commented
Jan 14, 2022
hellohuanlin
commented
Jan 14, 2022
stuartmorgan
approved these changes
Jan 19, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
packages/camera/camera/example/ios/RunnerTests/ThreadSafeEventChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
jmagman
reviewed
Jan 19, 2022
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/ThreadSafeTextureRegistryTests.m
Outdated
Show resolved
Hide resolved
cyanglaz
reviewed
Jan 20, 2022
packages/camera/camera/ios/Classes/FLTThreadSafeTextureRegistry.m
Outdated
Show resolved
Hide resolved
hellohuanlin
force-pushed
the
camera_thread_safe
branch
6 times, most recently
from
January 20, 2022 20:37
c7470b3
to
39a438a
Compare
FYI I've addressed all nits and will merge in a few hours. |
…st cases, and update wrappers comment
…d QueueHelper refactor
hellohuanlin
force-pushed
the
camera_thread_safe
branch
from
January 20, 2022 22:51
39a438a
to
cf020ad
Compare
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jan 21, 2022
This was referenced Jan 21, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jan 21, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is part of a larger project to fix the thread model for iOS camera plugin. The formal proposal can be found here
The engine APIs used in camera plugin are required to be run on platform thread. Some of these APIs have explicit asserts (e.g. MethodChannel) and some have warning instructions in documentation (e.g. TextureRegistry). However, in camera plugin we are currently calling these APIs in background thread, causing the crash.
There's already a FLTThreadSafeFlutterResult wrapper to invoke FlutterResult in main thread. This PR created similar "thread safe wrappers" for EventChannel, MethodChannel and TextureRegistry.
Issues
flutter/flutter#94723
flutter/flutter#52578
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.