[camera_android] Wait for creating capture session when initializing #8894
Conversation
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
This approach seems reasonable to me! Is it possible to add a test to ensure that sendCameraInitializedEvent gets called when expected?
|
@camsim99 I did try to look into the testing part. The other tests in the plugin don't need that many mocks as this one. |
|
@camsim99 I managed to get a test working. Hopefully that test style with mockito is ok. |
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
Sorry for the delay on this. The test looks good to me, thanks!
| private void startRegularPreview() throws CameraAccessException { | ||
| private void startRegularPreview(@Nullable Runnable onSuccessCallback) | ||
| throws CameraAccessException { | ||
| if (pictureImageReader == null || pictureImageReader.getSurface() == null) return; |
There was a problem hiding this comment.
Are the early returns here and below catastrophic situations where never sending the initialization event is the correct behavior? It's at least theoretically a change in behavior from the previous implementation.
There was a problem hiding this comment.
Good point. I'm not familiar with the state of the camera plugin during initialization. Should that early return code path be treated as an error, throwing on the camera.initialize() or should it finish successfully?
Up until now camera.initialize() would not throw, but it seems that executing would mean we are initializing an already closed camera, right?
There was a problem hiding this comment.
@stuartmorgan-g I've updated those code paths, treating them as no-ops during initialization, like before.
stuartmorgan-g
added
the
autosubmit
flutter/packages@e800da7...b2ce3b0 2025-05-07 [email protected] [camera_android] Wait for creating capture session when initializing (flutter/packages#8894) 2025-05-07 [email protected] [various] Delete discontinued packages (flutter/packages#9215) 2025-05-07 [email protected] [camera_avfoundation] Implementation swift migration - part 3 (flutter/packages#9182) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#8894) As discussed in the linked issue, this is an attempt to palliate one of the thread race conditions. In particular, returning to Dart possibly before creating the session and then let other function like "startImageStream" create a different session in parallel and interleave, causing the `java.lang.IllegalArgumentException: CaptureRequest contains unconfigured Input/Output Surface!` error. Fixes flutter/flutter#165092 cc @camsim99 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…lutter#8894) As discussed in the linked issue, this is an attempt to palliate one of the thread race conditions. In particular, returning to Dart possibly before creating the session and then let other function like "startImageStream" create a different session in parallel and interleave, causing the `java.lang.IllegalArgumentException: CaptureRequest contains unconfigured Input/Output Surface!` error. Fixes flutter/flutter#165092 cc @camsim99 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…lutter#8894) As discussed in the linked issue, this is an attempt to palliate one of the thread race conditions. In particular, returning to Dart possibly before creating the session and then let other function like "startImageStream" create a different session in parallel and interleave, causing the `java.lang.IllegalArgumentException: CaptureRequest contains unconfigured Input/Output Surface!` error. Fixes flutter/flutter#165092 cc @camsim99 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
3 participants
As discussed in the linked issue, this is an attempt to palliate one of the thread race conditions. In particular, returning to Dart possibly before creating the session and then let other function like "startImageStream" create a different session in parallel and interleave, causing the
java.lang.IllegalArgumentException: CaptureRequest contains unconfigured Input/Output Surface!error.Fixes flutter/flutter#165092
cc @camsim99
Pre-Review Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3