-
Notifications
You must be signed in to change notification settings - Fork 466
Improve CameraView Performance #2909
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
Improve CameraView Performance #2909
Conversation
* Add PlatformRefreshAvailableCameras * Don't invoke refresh camera task when one is in progress * Remove redundant refresh camera calls in CameraManager
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.
Pull Request Overview
This PR optimizes camera initialization performance by eliminating redundant calls to RefreshAvailableCameras() and implementing concurrent refresh protection. The optimization reduces multiple simultaneous camera refresh operations from up to four calls down to a single execution during initialization.
- Consolidates camera refresh logic to occur only once during initialization in
CameraManager.ConnectCamera() - Implements task-based deduplication in
CameraProviderto prevent concurrent refresh operations - Removes redundant refresh calls from
CameraViewHandlerand platform-specificCameraManagerimplementations
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| CameraView.shared.cs | Simplifies error handling logic in GetAvailableCameras() method |
| CameraProvider.*.cs | Changes RefreshAvailableCameras() from public to private platform implementation and adds shared task deduplication |
| CameraViewHandler.shared.cs | Removes redundant camera refresh call from ConnectHandler() |
| CameraManager.*.cs | Removes redundant refresh calls from platform implementations and consolidates initialization logic |
| CameraViewPage.xaml* | Adds initialization guard to prevent unnecessary refresh operations in sample app |
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.shared.cs
Outdated
Show resolved
Hide resolved
|
As suggested in #2907, I've created this PR to improve the performance for refresh during camera initialization. Can you please take a look, thanks! |
|
@ne0rrmatrix Thanks for the review, I've updated the code accordingly. |
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.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.shared.cs
Outdated
Show resolved
Hide resolved
|
@ne0rrmatrix @TheCodeTraveler Would love to get reviews/feedback on this PR and hopefully get it merged soon, to avoid further merge conflicts, thanks! |
…ctHandler(NativePlatformCameraPreviewView)`
…sync handling Refactored CameraManager to use sealed classes and private partial platform methods, and improved resource disposal logic across all platforms. CameraProvider is now sealed, implements IDisposable, and uses a semaphore to synchronize RefreshAvailableCameras, which now returns Task instead of ValueTask. Updated ICameraProvider interface and related mocks to match the new async signature.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
TheCodeTraveler
left a comment
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.
Thanks @zhitaop!
I added a few tweaks to improve performance and avoid race conditions:
- Added
SemaphoreSlimtoCameraProviderto avoid multi-threading race conditions from writing torefreshAvailableCamerasTaskand reading fromrefreshAvailableCamerasTasksimultaneously - Made
CameraManagerasealed classto improve performance and simplified itsDisposepattern - Made
CameraProviderasealed classand implementDisposableto improve performance and memory management - Call
CameraManager.DisconnectinCameraViewHandler.DisconnectHandler() - Call
CameraProvider.DisposeinCameraViewHandler.Dispose
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
This reverts commit 9ede2ec.
ac8b610
into
CommunityToolkit:main
|
@TheCodeTraveler Thanks for the improvements to the PR, those are definitely very necessary and useful enhancements! However, I've identified 2 regressions that would throw uncaught exceptions and cause the app to crash during my testing of the sample app. As these regressions are very likely to be hit by the developers while using the camera view, I've opened a new PR to address them. Could you please have a look? |
Description of Change
Root cause
Multiple calls to
CameraProvider.RefreshAvailableCameras()are scattered throughout the camera initialization code (i.e. inCameraManagerandCameraViewHandler.), affecting performance. Ideally, the refresh should only occur once during initialization.In Addition, developers may also call
RefreshAvailableCameras()in the application code (e.g. in a ViewModel for the CameraView) at the same time when the camera view is initializing. This results in duplicate concurrent executions.For example, when opening the CameraView page in the sample app, a breakpoint set in
CameraProvider.RefreshAvailableCameras()is hit up to four times from the following call stacks:CameraViewViewModel.RefreshCameras()CameraManager.PlatformConnectCamera()CameraManager.PlatformStartCameraPreview()CameraViewHandler.ConnectHandlerThis PR includes the following changes
RefreshAvailableCameras()calls inCameraManagerandCameraViewHandler. Only call the function once inCameraManager.Shared.ConnectCamera()to ensure camera list is populated before connecting the camera.PlatformRefreshAvailableCamerasandrefreshAvailableCamerasTaskin CameraProvider to prevent duplicate concurrent refreshes. If multiple refreshes are invoked around the same time (e.g., from the ViewModel and the internal camera initialization), subsequent calls simply await the existing task.CameraViewPage. Removed unused code and addisInitializedguard to prevent refresh when navigated back from the photo page to the camera view page.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
This PR has been tested on Windows. Number of invocation of the refresh logic is reduced to one, without altering existing behaviour.