-
Notifications
You must be signed in to change notification settings - Fork 460
Fix CameraView crash when switching camera on Windows and other CameraView enhancements #2634
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
base: main
Are you sure you want to change the base?
Fix CameraView crash when switching camera on Windows and other CameraView enhancements #2634
Conversation
* Add automatic initialization in CameraProvider * Remove unnecessary refresh cameras calls in CameraManager * Add camera switcher in camera sample page
Any guess when this will deploy to main? Hoping it fixes some issues I have with Windows camera. |
Perhaps you could help us and test this branch on your machine to see if it helps? |
I would love to help but don't know how do download the branch to test. Let me Google it. |
* Unbind ProcessCameraProvider when dispose
I have tested the CommunityToolkitSampleApp using https://github.com/zhitaop/CommunityToolkitMaui/tree/fix/camera-provider-refresh branch on Surface Book 3 (Windows 10). The camera switcher is working as expected without any crashes. |
Hi @bijington, could this PR be assigned for review soon? As mentioned in #2634 (comment), the branch has been tested that it does solve the original crash issue. |
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.shared.cs
Outdated
Show resolved
Hide resolved
I've added some comments I didn't have a chance to take a look on a laptop so I'll try and do a more in depth check later in the week |
Sorry it's taken time to get to this. I'll try and sort it tomorrow or next week |
I this it is same issue with recent code on Android |
Hi @bijington, I've addressed some of your comments and provide explanation for others, could you please take a look again? Thanks! |
Sorry things became really busy and then I went on vacation. I return at the weekend so I've added a reminder to take a look at this next week for you. |
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.
@zhitaop apologies for the delay in reviewing this. It looks good to me now!
…avoid async/await race conditions, Implement `IDisplosable`
Thanks @zhitaop! This is a breaking change that requires the docs to be updated before we can merge the PR: https://github.com/MicrosoftDocs/CommunityToolkit/pulls @bijington - could you work with @zhitaop on the updating the docs? |
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 fixes a critical CameraView crash on Windows when switching cameras and implements significant camera provider enhancements. The crash was caused by reusing a single MediaCapture instance across multiple cameras, leading to incorrect metadata assignment and resolution conflicts.
Key changes:
- Fixed Windows MediaCapture disposal and instantiation to prevent camera switching crashes
- Refactored camera initialization logic to reduce redundant refresh calls and improve performance
- Enhanced sample app with camera switcher UI and improved initialization patterns
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
CameraProvider.windows.cs | Fixed MediaCapture lifecycle management by creating new instances per camera and proper disposal |
CameraProvider.shared.cs | Added thread-safe initialization logic with semaphore and task management |
ICameraProvider.shared.cs | Extended interface with InitializeAsync method and IsInitialized property |
CameraManager.*.cs | Removed redundant RefreshAvailableCameras calls and consolidated initialization logic |
CameraViewHandler.shared.cs | Simplified connection logic by removing duplicate camera provider refresh |
Sample app files | Enhanced UI with camera picker and improved initialization patterns |
refreshAvailableCamerasTask?.Dispose(); | ||
refreshAvailableCamerasTask = null; |
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.
Setting the task reference to null after disposing it creates a potential race condition. If another thread is checking refreshAvailableCamerasTask?.IsCompletedSuccessfully
in the IsInitialized
property while disposal is happening, it could access a disposed task. Consider setting the reference to null before disposing, or use a different synchronization approach.
refreshAvailableCamerasTask?.Dispose(); | |
refreshAvailableCamerasTask = null; | |
var task = refreshAvailableCamerasTask; | |
refreshAvailableCamerasTask = null; | |
task?.Dispose(); |
Copilot uses AI. Check for mistakes.
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.shared.cs
Outdated
Show resolved
Hide resolved
…nd improve initialization logic
…er semaphore usage
@TheCodeTraveler Thanks for the review and code update! I have raised a PR to update the documentation: MicrosoftDocs/CommunityToolkit#593 |
Description of Change
Fixes [BUG] CameraView App crashes when switching camera on Windows #2559:
Root cause :
In
CameraProvider.windows.cs
, only one instance ofMediaCapture
is instantiated when iterating through thevideoCaptureSourceGroup
. This causes only the firstmediaCapture.InitializeCameraForCameraView()
to be successful. Wrong camera meta data from the first camera, such as the supported resolutions, are added to the subsequentCameraInfo
s (i.e. all subsequentCameraInfo
contains metadata from the firstCameraInfo
). This causes the following exception when the camera is being updated with incorrect resolution:Code changes:
MediaCapture
in each iteration and properly dispose it after useEnhancement: refactor camera refresh logic
Root cause:
Multiple calls to
CameraProvider.RefreshAvailableCameras()
are scattered throughout the camera initialization code (i.e.inCameraManager.ConnectCamera
,CameraManager.StartCameraPreview
,CameraManager.UpdateResolution
), which are unnecessary as the available cameras are unlikely to change during initialization, and also time consuming especially on Windows as it needs to initializeMediaCapture
.In Addition, consumers of the camera view may also call
RefreshAvailableCameras()
(e.g. to populate the list for camera switcher), usually at the same time when the camera view itself is initializing , resulting in more redundant code execution.Code changes:
CameraProvider
:refreshTask
andPlatformRefreshAvailableCameras()
InitializeAsync()
PlatformRefreshAvailableCameras
.refreshTask
is reused for both initialization and refresh, ensuring only one refresh operation runs at a time.InitializeAsync()
only refreshes the cameras if the camera provide has not been initialized, i.e. therefreshTask
is not completed successfullyRefreshAvailableCameras()
only refreshes the cameras if the refresh task is not currently in progress, i.e.refreshTask
is null or is completed (could be completed successfully, faulted or canceled)RefreshAvailableCameras()
calls inCameraManager
andCameraViewHandler
. Instead, callInitializeAsync()
inCameraManager.ConnectCamera
to ensure available cameras are initialized.Example usage by consumer:
CameraViewPage
andCameraViewViewModel
:Cameras
observable in the view model, and make sure it's assigned after theCameraProvider
is initialized so that camera list is not empty.Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information