Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent Windows apps from crashing when CameraView is used after camera permission has been revoked, by treating UnauthorizedAccessException during MediaCapture initialization as a recoverable failure instead of an unhandled exception.
Changes:
- Updated
InitializeCameraForCameraView(Windows) to return aboolindicating initialization success and to catchUnauthorizedAccessException. - Updated Windows camera enumeration (
PlatformRefreshAvailableCameras) to skip devices whose initialization fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.windows.cs | Skips camera devices that fail MediaCapture initialization during available-camera refresh. |
| src/CommunityToolkit.Maui.Camera/Extensions/CameraViewExtensions.windows.cs | Returns a success flag from initialization and handles UnauthorizedAccessException without throwing. |
Suppressed comments (1)
src/CommunityToolkit.Maui.Camera/Extensions/CameraViewExtensions.windows.cs:52
- The COMException catch says "Camera already initialized" (which was previously treated as non-fatal), but the new bool return reports this as failure. That changes behavior for callers like PlatformRefreshAvailableCameras(), which will now drop/skip that device entirely. If COMException here really means "already initialized", this should be reported as success.
catch (System.Runtime.InteropServices.COMException)
{
// Camera already initialized
return false;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/CommunityToolkit.Maui.Camera/Providers/CameraProvider.windows.cs:28
- When camera initialization fails (e.g., access revoked), this
continueskips adding any entry toavailableCameras. That can leaveAvailableCamerasempty even though a camera device exists, andCameraManager.PlatformStartCameraPreviewwill still throwCameraException("No camera available on device"), which is typically unhandled from the handler path (still an app crash). Consider still adding aCameraInfoentry with conservative defaults when initialization is denied so the camera list isn’t empty, and let preview-start handle the access failure without throwing.
bool success = await mediaCapture.InitializeCameraForCameraView(sourceGroup.Id, token);
if (!success)
{
continue;
}
|
@bijington @TheCodeTraveler How can we proceed with this bugfix? I'm happy to apply any suggestions you make to my PR. Thanks! |
48c6c07 to
b41c069
Compare
|
I rebased the PR on top of latest main branch, reverted the change to InitializeCameraForCameraView() and handled the exceptions instead. Also removed throwing exceptions in |
|
It seems all the builds fail because this unrelated error:
I can't see where the package is referenced in the project, though. |
|
@TheCodeTraveler @bijington @jfversluis Unfortunately I didn't hear back from you. How can we proceed with this PR and the original bug? I'm open for discussion. Thank you! |
|
I think the error you were seeing was from some dependency in another project/the pipeline. I just updated this PR with all the latest changes on main. Let's see if that fixes it for you! |
|
There is one open question for this PR, though, and it's what should be discussed. The original app crash occured because My PR now fixes the app crash by catching all exceptions in There are some possibilities:
|
|
Sorry I missed earlier notifications. I think we should just add an event for all camera related errors for now and pump caught exceptions through it. Then it is at least consistent with the MediaElement to some extent. Maybe something like |
…ons when connecting to cameras
…nymore don't throw exceptions in CameraManager.ConnectCamera(), since no one can handle them and they just crash the whole app
fef18c3 to
3a89bc6
Compare
|
I just rebased my PR branch on top of main and rearranged the commits. The first one adds the Feel free to review. Thank you! |
There was a problem hiding this comment.
🟡 Changes recommended
Broad exception handling suppresses cancellation, and failed Windows initialization leaks its MediaCapture instance.
7 open findings
2 resolved since last review
🧠 Review effort: Balanced
| catch (Exception ex) | ||
| { | ||
| // can't use that camera | ||
| cameraView.OnErrorOccurred(ex); | ||
| return; | ||
| } |
| catch (Exception) | ||
| { | ||
| // can't use that camera | ||
| continue; | ||
| } |
|
|
||
| cameraView.SelectedCamera ??= cameraProvider.AvailableCameras?.FirstOrDefault(); | ||
|
|
||
| if (cameraView.SelectedCamera == null) |
| cameraView.SelectedCamera ??= cameraProvider.AvailableCameras?.FirstOrDefault() ?? throw new CameraException("No camera available on device"); | ||
| cameraView.SelectedCamera ??= cameraProvider.AvailableCameras?.FirstOrDefault(); | ||
|
|
||
| if (cameraView.SelectedCamera == null) |
| /// <summary> | ||
| /// Event args containing all contextual information related to the error occurred event. | ||
| /// </summary> | ||
| /// <param name="ex">The <see cref="Exception"/>exception.</param> |
| } | ||
|
|
||
| /// <summary> | ||
| /// Event that is raised when the an error occurred. |
| void ICameraView.OnErrorOccurred(Exception ex) | ||
| { | ||
| weakEventManager.HandleEvent(this, new ErrorOccurredEventArgs(ex), nameof(ErrorOccurred)); |
|
@MFinkBK Could you please address CoPilot's comments before we merge? |
Description of Change
The change handles the
UnauthorizedAccessExceptioninInitializeCameraForCameraViewand the loop inPlatformRefreshAvailableCameras()is justcontinue'd.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
I'm not sure if catching the exception and just returning a bool is OK here, or if the
PlatformRefreshAvailableCameras()should catch the exceptions instead.There's still an uncatchable
CameraExceptionin theConnectCamera()handler, which should probably also be handled differently.