Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[camerax] Small fixes to starting/stopping video capture #6068

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Feb 6, 2024

Fixes small issues I noticed in starting/stopping video capture while working on #6059, notably:

  • Change all remaining unawaited calls to await to avoid any racy behavior.
  • Update camera info after VideoCapture use case is bound to the lifecycle of the plugin's ProcessCameraProvider to make sure it is up to date.
  • Unbind VideoCapture use case when video recording stops since it was suggested to lazily load it for performance reasons (open to pushback on this). this would require potentially more changes than I originally thought
  • Make tests checking that async methods throw exceptions actually wait for those exceptions as this may cause flaky test behavior.

Fixes flutter/flutter#132499 as this PR removes any remaining unawaited calls.

Pre-launch Checklist

@camsim99 camsim99 changed the title [camx] Small fixes to starting/stopping video capture [camerax] Small fixes to starting/stopping video capture Feb 7, 2024
@camsim99 camsim99 marked this pull request as ready for review February 8, 2024 20:10
@camsim99 camsim99 requested a review from gmackall February 8, 2024 20:10
Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with only comment being, do we often put issue links in changelogs? If so then carry on, otherwise replace with a description of what is being solved

@camsim99
Copy link
Contributor Author

camsim99 commented Feb 8, 2024

LGTM with only comment being, do we often put issue links in changelogs? If so then carry on, otherwise replace with a description of what is being solved

Ah Reid recommended this for another PR but that's the only time I've done it. I have no prob just writing the fixes out, though, so I can do that :)

@camsim99
Copy link
Contributor Author

camsim99 commented Feb 9, 2024

@gmackall I assumed that unbinding VideoCapture would work based on our original lazy loading philosophy but it actually is causing errors, so I'm going to revert that change. If you think it's worth exploring further, I can file an issue, but I'm okay leaving as is since it still will not be loaded until it is used. I assume the issue comes somewhere from setup I didn't include (like dealing with the Recorder, Recording, etc.)

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024
@auto-submit auto-submit bot merged commit cd45100 into flutter:main Feb 9, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 9, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 9, 2024
flutter/packages@29d8cc0...11152d2

2024-02-09 [email protected] [webview_flutter] Add interface for showing javascript dialog message (flutter/packages#4704)
2024-02-09 [email protected] [pigeon] Implement Screaming Snake Case Conversion for Kotlin Enum Cases (flutter/packages#5918)
2024-02-09 [email protected] [camerax] Small fixes to starting/stopping video capture (flutter/packages#6068)
2024-02-08 [email protected] Roll Flutter from 8431cae to eb5d0a4 (33 revisions) (flutter/packages#6079)

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],[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
auto-submit bot pushed a commit that referenced this pull request Feb 21, 2024
…eOffset` (#6059)

This PR implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` and makes some small fixes here and there, each of which I have left a comment about for context.

Part of flutter/flutter#120468 & flutter/flutter#120467.

~NOTE: Should land after #6068 done :)
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
Fixes small issues I noticed in starting/stopping video capture while working on flutter#6059, notably:

- Change all remaining `unawaited` calls to `await` to avoid any racy behavior.
- Update `camera` info after `VideoCapture` use case is bound to the lifecycle of the plugin's `ProcessCameraProvider` to make sure it is up to date.
- ~Unbind `VideoCapture` use case when video recording stops since it was suggested to lazily load it for performance reasons (open to pushback on this).~ this would require potentially more changes than I originally thought
- Make tests checking that async methods throw exceptions actually wait for those exceptions as this may cause flaky test behavior.

Fixes flutter/flutter#132499 as this PR removes any remaining `unawaited` calls.
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…eOffset` (flutter#6059)

This PR implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` and makes some small fixes here and there, each of which I have left a comment about for context.

Part of flutter/flutter#120468 & flutter/flutter#120467.

~NOTE: Should land after flutter#6068 done :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[camerax] Change unawaited usages to await for calls to stop asynchronous camera operations
2 participants