Maui icon indicating copy to clipboard operation
Maui copied to clipboard

Fix CameraView crash when switching camera on Windows and other CameraView enhancements

Open zhitaop opened this issue 10 months ago • 3 comments

Description of Change

  • Fixes #2559: In CameraProvider.windows.cs, only one instance of MediaCapture is instantiated when iterating through the videoCaptureSourceGroup. This causes only the first mediaCapture.InitializeCameraForCameraView() to be successful. Wrong camera meta data from the first camera, such as the supported resolutions, are added to the subsequent CameraInfos.

    • Proposed solution: instantiate new MediaCapture in each iteration and properly dispose it after use
  • Fixes #2519:

    As per https://github.com/CommunityToolkit/Maui/issues/2519#issuecomment-2764251471:

    CameraViewHandler.ConnectHandler doesn't actually check the result of the permission request

    • Proposed solution: bubble-up a PermissionException and let consumers respond
  • Also found that unnecessary calls to CameraProvider.RefreshAvailableCameras() are scattered throughout the code, which is a time consuming operation especially on Windows. IMO this call is only necessary when the CameraProvider is initialized, or when consumer of the API knows that available cameras on device has been updated (e.g. external camera has been plugged in after the initialization)

    • Proposed solution: add InitializeAsync task in CameraProvider, which is automatically started to initialize the camera list, and can be awaited by the consumer. (This might need more consideration as it change the API ?)
    • Remove all other RefreshAvailableCameras() calls throughout CameraManager
    • Updated inline documentation for to better explain purposes of each function in CameraProvider. This could potential be the solution to https://github.com/CommunityToolkit/Maui/issues/2272?
  • Also fixes a few issues in CameraViewPage and CameraViewViewModel:

    • Add camera switcher on the camera view page
    • Make Cameras observable in the view model, and make sure it's assigned after the CameraProvider is initialized so that camera list is not empty.

Linked Issues

  • Fixes #2559
  • Fixes #2519

PR Checklist

  • [x] Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • [x] Has tests (if omitted, state reason in description)
  • [x] Has samples (if omitted, state reason in description)
  • [x] Rebased on top of main at time of PR
  • [x] Changes adhere to coding standard
  • [ ] Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

zhitaop avatar Apr 17 '25 11:04 zhitaop

Any guess when this will deploy to main? Hoping it fixes some issues I have with Windows camera.

VR-Architect avatar Apr 29 '25 10:04 VR-Architect

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?

bijington avatar Apr 29 '25 12:04 bijington

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.

VR-Architect avatar May 06 '25 13:05 VR-Architect

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.

ruc12127 avatar Jul 15 '25 01:07 ruc12127

Hi @bijington, could this PR be assigned for review soon? As mentioned in https://github.com/CommunityToolkit/Maui/pull/2634#issuecomment-3071604323, the branch has been tested that it does solve the original crash issue.

zhitaop avatar Jul 16 '25 03:07 zhitaop

Hi @bijington, could this PR be assigned for review soon? As mentioned in https://github.com/CommunityToolkit/Maui/pull/2634#issuecomment-3071604323, the branch has been tested that it does solve the original crash issue.

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

bijington avatar Jul 16 '25 05:07 bijington

Sorry it's taken time to get to this. I'll try and sort it tomorrow or next week

bijington avatar Jul 26 '25 06:07 bijington

I this it is same issue with recent code on Android

vovanb avatar Aug 05 '25 11:08 vovanb

Hi @bijington, I've addressed some of your comments and provide explanation for others, could you please take a look again? Thanks!

zhitaop avatar Aug 20 '25 23:08 zhitaop

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.

bijington avatar Aug 21 '25 07:08 bijington

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?

TheCodeTraveler avatar Aug 28 '25 21:08 TheCodeTraveler

@TheCodeTraveler Thanks for the review and code update! I have raised a PR to update the documentation: https://github.com/MicrosoftDocs/CommunityToolkit/pull/593

zhitaop avatar Sep 01 '25 13:09 zhitaop

Hi @bijington @TheCodeTraveler, is there any update to the status of this PR?

zhitaop avatar Oct 10 '25 07:10 zhitaop

Hi Zhitao! Yes we discussed it in our standup this month.

We've decided not to keep the breaking changes to the API in this PR and to only keep the bug fixes. The new Initialize method doesn't add much value for developers.

I will be closing this PR and reopening it with just the bug fix for Windows.

TheCodeTraveler avatar Oct 10 '25 15:10 TheCodeTraveler

@TheCodeTraveler Thanks for the update!

Just to clarify, in my understanding, adding the InitializeAsync() method wouldn’t be a breaking change - it’s a new API that doesn’t affect any existing behaviour. Existing code using RefreshAvailableCameras() will continue to work as before without modification, developers do not have to use the new method.

The intent for adding the new method was to give developers more flexibility, as both methods serve different use cases:

  • InitializeAsync only performs a one-time discovery of cameras and populates the AvailableCameras property. Subsequent calls will simply reuse the cached lists (a no-op).
  • RefreshAvailableCameras forces a refresh. It always re-runs camera discovery to ensure the list is up to date

Camera discovery can be expensive, especially on Windows (it needs to initialize a mediaCapture object for each camera). In addition, camera availability on a device rarely changes during an app’s lifecycle, so InitializeAsync() provides the option to ensure the list is only initialized once.

For example, a view model for a camera view page can call InitializeAsync to populate a list of cameras when the page opens for the first time. Subsequent openings of the page do not need to refresh the cameras, increasing performance.

public async Task InitializeViewModelAsync()
{
  await cameraProvider.InitializeAsync(CancellationToken.None); 
// alternatively, call camepraProvider.RefreshAvailableCameras() to ensure the list is always up to date
  Cameras = cameraProvider.AvailableCameras ?? [];
} 

That said, if the concern is it adds unnecessary confusion to the API usage without adding too much value, I’m happy to update the PR and remove the InitializeAsync() method to keep it focused on the bug fix.

zhitaop avatar Oct 13 '25 12:10 zhitaop

Thanks @zhitaop! No worries - I've opened the new PR here and would love your review! https://github.com/CommunityToolkit/Maui/pull/2907

TheCodeTraveler avatar Oct 13 '25 17:10 TheCodeTraveler