Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[camera_windows] Support camera on windows desktop platform #4641

Merged
merged 102 commits into from Feb 3, 2022

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented Jan 5, 2022

This PR adds a Windows Desktop implementation of camera plugin using Media Foundation and CaptureEngine interface.

The implementation supports multiple cameras, video preview, and capturing photos and videos. Works with camera plugin and camera example.

The current implementation uses FlutterDesktopPixelBuffer, but could be switched to use shared textures or Platform views after those are supported.

Unit tests are partially done; for the CaptureControllerImpl class all falilure code paths are not yet covered.

Still missing some features, like exposure and focus controls. These can be done later with interfaces IAMCameraControl and IAMVideoProcAmp,

Resolves flutter/flutter#41709: [camera] Add Windows support

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 22:09
@jokerttu jokerttu marked this pull request as draft January 10, 2022 11:28
@jokerttu jokerttu marked this pull request as ready for review January 16, 2022 19:12
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I did another pass over the metadata and Dart and skimmed the C++ headers. I found a few things that I'd missed before; mostly they are very minor but the main things are:

  • The Dart code not having unit tests
  • The C++ code not having class comments

Both are maintainability issues, so definitely need to be addressed.

packages/camera/camera_windows/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/camera/camera_windows/lib/camera_windows.dart Outdated Show resolved Hide resolved
packages/camera/camera_windows/pubspec.yaml Outdated Show resolved Hide resolved
packages/camera/camera_windows/windows/camera.cpp Outdated Show resolved Hide resolved
packages/camera/camera_windows/windows/com_heap_ptr.h Outdated Show resolved Hide resolved
packages/camera/camera_windows/windows/photo_handler.h Outdated Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

stuartmorgan commented Feb 2, 2022

Having also tried out the example: something we should file an issue to track for follow-up if it's not already filed is preview mirroring. This has been an issue on mobile with front-facing cameras, and on desktop that's going to be the normal case, so we'll want to handle it.

@jokerttu
Copy link
Contributor Author

jokerttu commented Feb 2, 2022

@stuartmorgan mirror mode is actually already supported (by software flip) but by default set to false.
It could be option on camera initialization but its not supported by the camera_platform_interface so its not really usable attribute.
We should detect the camera facing, this is possible by reading registry or via api for windows10+ devices and use mirror mode as default for front-facing cameras. There is issue for this.
Or we can set the default to true, and have mirrored camera by default for now. Not sure for what people are going to use this for, so this is a bit weird to put it as mirror by default.
I can make it true as default for now if it is better option and most used usecase.

@cbracken
Copy link
Member

cbracken commented Feb 3, 2022

Overall LGTM modulo a few minor fixes that I've prepared in a branch to make them easier to patch in.

That branch includes minor fixes for three categories of issues:

  1. Adds/tweaks documentation comments.
  2. Makes some C++ methods const, where possible.
  3. Switches a few C++ string parameters to pass by const reference, where they were previously passing by value.

You should be able to patch them in via:

git remote add cbracken git@github.com:cbracken/flutter_plugins.git
git fetch cbracken
git cherry-pick b6aeb9de5b1d41351040d88256d93d1a54512335
git cherry-pick f2e0575fe8bda5cdb4403ad1381209b2dcc74a8d
git cherry-pick 8e01d05ff787d88cd92da3a12a7eea1ac6e56a9c

Apologies if the above is old-hat to you, it's always hard to tell how much git-related pain any given contributor has suffered through in the past :)

Once those commits are merged, I think this looks good to me!

@jokerttu
Copy link
Contributor Author

jokerttu commented Feb 3, 2022

@cbracken thanks, good tweaks.
Those are now cherry picked and committed.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Changes LGTM (I didn't do a full re-review)

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

@HosseinMohebbikhah

This comment was marked as off-topic.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants