[camera_windows] Set device media type for video preview explicitly #7447

dasyad00 · 2024-08-19T15:58:17Z

Fixes preview aspect ratio on Windows on some webcams (See flutter/flutter#122966).

Before (main branch 7c1a05c):

After:

Pre-launch Checklist

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

stuartmorgan · 2024-08-21T11:50:21Z

Thanks for the contribution! This will need a test, per the PR checklist, before it can be reviewed.

dasyad00 · 2024-08-21T18:53:18Z

Since the bug arose due to a missing function call to Microsoft's Media Foundation API, the only test I can reasonably add is to verify the function is now called.

stuartmorgan · 2024-10-01T19:46:53Z

From triage: @cbracken it looks like this should be ready for review.

cbracken

Looks great -- thanks for your contribution!

stuartmorgan · 2024-10-04T18:29:41Z

packages/camera/camera_windows/CHANGELOG.md

@@ -1,6 +1,7 @@
-## NEXT
+## 0.2.6


For versions <1, Dart convention is to use the build number for bugfixes, so this should be 0.2.5+1.

I have fixed the versioning in 4ffbedd

stuartmorgan · 2024-10-04T18:30:31Z

packages/camera/camera_windows/CHANGELOG.md


 * Updates minimum supported SDK version to Flutter 3.19/Dart 3.3.
+* Fix black bars on camera preview [#122966](https://github.com/flutter/flutter/issues/122966)


"Fixes". Also, this is missing a period. Please see the CHANGELOG style guide linked from the PR checklist.

The changelog should now be following the style guide as of 4ffbedd

stuartmorgan · 2024-10-04T18:33:28Z

packages/camera/camera_windows/windows/capture_controller.cpp

@@ -479,6 +479,13 @@ HRESULT CaptureControllerImpl::FindBaseMediaTypes() {
    return E_FAIL;
  }

+  hr = source->SetCurrentDeviceMediaType(


It seems extremely surprising to have a method called FindBaseMediaTypes() have the side-effect of setting a media type. Why is this method the place where this is called?

SetCurrentDeviceMediaType is a class function of IMFCaptureSource which was conveniently already accessible in the function scope FindBaseMediaType. For the bug fix, the function only needs to be called after base_preview_media_type_ is set, which is when FindBestMediaType is called with it.

We can alternatively move it inside the function CaptureControllerImpl::StartPreview after base_preview_media_type_ is set, but then we would need to prepare a IMFCaptureSource object for this scope.

For the bug fix, the function only needs to be called after base_preview_media_type_ is set, which is when FindBestMediaType is called with it.

My primary concern is the maintainability of this code in the long term, rather than minimizing the diff. Having a method indicate that it gets a value have a side effect of changing a value is the kind of thing that makes maintaining code correctly a lot harder over the longer term.

We can alternatively move it inside the function CaptureControllerImpl::StartPreview

That sounds like a much clearer location for setting values related to displaying the preview.

stuartmorgan · 2024-10-29T19:54:11Z

@dasyad00 Are you still planning on updating this to address the remaining feedback?

dasyad00 · 2024-10-31T07:10:11Z

@stuartmorgan Sorry, I was occupied with other things lately. I have just pushed the refactor commit.

stuartmorgan

Thanks, the refactoring makes for a much clearer flow! Just minor comments.

stuartmorgan · 2024-11-12T21:21:00Z

packages/camera/camera_windows/windows/capture_controller.h

@@ -180,6 +180,7 @@ class CaptureControllerImpl : public CaptureController,
  // Enumerates video_sources media types and finds out best resolution
  // for preview and video capture.
  HRESULT FindBaseMediaTypes();
+  HRESULT FindBaseMediaTypes(IMFCaptureSource* source);


Please give these different names, and their own declaration comments explaining what they do. Function overloading should only be used when the methods do exactly the same thing, just with different input types.

The new method could be called FindBaseMediaTypesForSource.

I have renamed the method in ~~ff678d6~~ 2445b24

stuartmorgan · 2024-11-12T21:22:12Z

packages/camera/camera_windows/windows/capture_controller.cpp

+  hr = capture_engine_->GetSource(&source);
+  if (FAILED(hr)) {
+    return OnPreviewStarted(GetCameraResult(hr),
+                            "Failed to initialize video preview");


Please don't use the same error message for three different failures; we want to be able to distinguish what failed. Each case in the method should describe what specifically failed.

I have clarified the error messages in ~~79610aa~~ 923fc4d

stuartmorgan

LGTM, thanks!

auto-submit · 2024-11-13T14:10:34Z

auto label is removed for flutter/packages/7447, due to - The status or check suite Mac_x64 ios_build_all_packages stable has failed. Please fix the issues identified (or deflake) before re-applying this label.

…plicitly (flutter/packages#7447)

flutter/packages@72356fd...26e123a 2024-11-13 [email protected] [camera_windows] Set device media type for video preview explicitly (flutter/packages#7447) 2024-11-13 [email protected] [go_router] Add support for relative routes (flutter/packages#6825) 2024-11-13 [email protected] [vector_graphics_compiler] fix a renamed method parameter lint (flutter/packages#8070) 2024-11-12 [email protected] [in_app_purchase] Add expiration date to Transaction (flutter/packages#8030) 2024-11-12 [email protected] [various] Clean up contributing guides (flutter/packages#8032) 2024-11-12 [email protected] [ci] Remove web renderer option from tools. (flutter/packages#8055) 2024-11-12 [email protected] [url_launcher] Update Pigeon version for Linux (flutter/packages#8065) 2024-11-12 [email protected] [go_router] Add support for preloading branches of StatefulShellRoute (revised solution) (flutter/packages#6467) 2024-11-12 [email protected] [pigeon] Make Linux type declarations public (flutter/packages#8040) 2024-11-11 [email protected] Roll Flutter from 73546b3 to c8510f2 (30 revisions) (flutter/packages#8042) 2024-11-11 [email protected] Use dependabot multi-directory configuration for Android package updates (flutter/packages#8048) 2024-11-11 [email protected] [tools] Run `pub get` before `format` (flutter/packages#8052) 2024-11-11 [email protected] [file_selector] Fix Linux cancel regression (flutter/packages#8051) 2024-11-09 [email protected] [shared_preferences] Fix confusing language in README (flutter/packages#8049) 2024-11-08 [email protected] Use dependabot multi-directory configuration for Android example gradle updates (flutter/packages#8036) 2024-11-08 [email protected] [animations] Remove `.flutter-plugins` reference from example app (flutter/packages#8002) 2024-11-08 [email protected] Group dependabot github-action update PRs, delete dead docker updates (flutter/packages#8044) 2024-11-08 [email protected] [vector_graphics_compiler] fix-null-exception (flutter/packages#8006) 2024-11-08 [email protected] [tools] Format Dart per-package (flutter/packages#8043) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added p: camera platform-windows labels Aug 19, 2024

dasyad00 force-pushed the fix/camera_windows/black_bars branch from 0cf1e06 to 6dd71c9 Compare August 19, 2024 18:18

dasyad00 marked this pull request as ready for review August 19, 2024 18:19

dasyad00 requested a review from cbracken as a code owner August 19, 2024 18:19

cbracken approved these changes Oct 2, 2024

View reviewed changes

stuartmorgan requested changes Oct 4, 2024

View reviewed changes

dasyad00 force-pushed the fix/camera_windows/black_bars branch from 0b73a32 to 4ffbedd Compare October 4, 2024 20:21

dasyad00 and others added 5 commits November 8, 2024 09:37

Set device media type for video preview explicitly

b79eea5

Bump camera_windows version and document changes

d887b8f

Verify usage of SetCurrentDeviceMediaType

b01c17c

Move SetCurrentDeviceMediaType to StartPreview

1f17a42

Fix file formatting

cdf4f5c

dasyad00 force-pushed the fix/camera_windows/black_bars branch from 1ecdcf7 to cdf4f5c Compare November 8, 2024 09:06

stuartmorgan requested changes Nov 12, 2024

View reviewed changes

dasyad00 added 2 commits November 13, 2024 14:35

Rename overloaded function with suffix

2445b24

Clarify error messages for StartPreview

923fc4d

dasyad00 force-pushed the fix/camera_windows/black_bars branch from 79610aa to 923fc4d Compare November 13, 2024 13:36

Typo fix

74cbd43

stuartmorgan approved these changes Nov 13, 2024

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024

auto-submit bot merged commit 26e123a into flutter:main Nov 13, 2024
77 checks passed

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024

engine-flutter-autoroll mentioned this pull request Nov 13, 2024

Roll Packages from 72356fda4684 to 26e123a6ce03 (19 revisions) flutter/flutter#158626

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2024

26e123a6c [camera_windows] Set device media type for video preview ex…

8e5cf5d

…plicitly (flutter/packages#7447)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera_windows] Set device media type for video preview explicitly #7447

[camera_windows] Set device media type for video preview explicitly #7447

dasyad00 commented Aug 19, 2024 •

edited

Loading

stuartmorgan commented Aug 21, 2024

dasyad00 commented Aug 21, 2024

stuartmorgan commented Oct 1, 2024

cbracken left a comment

stuartmorgan Oct 4, 2024

dasyad00 Oct 4, 2024

stuartmorgan Oct 4, 2024

dasyad00 Oct 4, 2024

stuartmorgan Oct 4, 2024

dasyad00 Oct 4, 2024

stuartmorgan Oct 5, 2024

stuartmorgan commented Oct 29, 2024

dasyad00 commented Oct 31, 2024

stuartmorgan left a comment

stuartmorgan Nov 12, 2024

dasyad00 Nov 13, 2024 •

edited

Loading

stuartmorgan Nov 12, 2024

dasyad00 Nov 13, 2024 •

edited

Loading

stuartmorgan left a comment

auto-submit bot commented Nov 13, 2024


		* Updates minimum supported SDK version to Flutter 3.19/Dart 3.3.
		* Fix black bars on camera preview [#122966](https://github.com/flutter/flutter/issues/122966)

[camera_windows] Set device media type for video preview explicitly #7447

[camera_windows] Set device media type for video preview explicitly #7447

Conversation

dasyad00 commented Aug 19, 2024 • edited Loading

Pre-launch Checklist

stuartmorgan commented Aug 21, 2024

dasyad00 commented Aug 21, 2024

stuartmorgan commented Oct 1, 2024

cbracken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Oct 29, 2024

dasyad00 commented Oct 31, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasyad00 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasyad00 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

auto-submit bot commented Nov 13, 2024

dasyad00 commented Aug 19, 2024 •

edited

Loading

dasyad00 Nov 13, 2024 •

edited

Loading

dasyad00 Nov 13, 2024 •

edited

Loading