[camera_windows] Fixes initializing video preview with latest webcam driver#10303
[camera_windows] Fixes initializing video preview with latest webcam driver#10303dasyad00 wants to merge 7 commits intoflutter:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with video preview initialization on Windows for certain webcam drivers. The fix involves explicitly querying for video, photo, and audio stream indices instead of relying on predefined constants. A key change is the addition of a call to SetCurrentDeviceMediaType after determining the best media type for preview. These changes are correctly propagated through the photo, preview, and record handlers, and the corresponding tests have been updated. I've identified one potential issue where the search for an audio stream is unconditional, which could cause problems in video-only capture scenarios.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I've left some initial comments. I'm not very familiar with the Media Foundation APIs, so I may be misunderstanding, but the code definitely needs more explanation about what the code is doing and why.
Looking at the comment in the issue, it sounds like this was only tested against a single device; we'll need more justification in terms of docs or examples about this code being correct for more than just the one case.
| ## NEXT | ||
| ## 0.2.7 | ||
|
|
||
| * Fixes initializing video preview with latest webcam driver [#140014](https://github.com/flutter/flutter/issues/140014) |
There was a problem hiding this comment.
This needs a period, per the linked style guide.
| repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_windows | ||
| issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22 | ||
| version: 0.2.6+2 | ||
| version: 0.2.7 |
There was a problem hiding this comment.
0.2.6+3, since this is a bugfix, per Dart conventions for pre-1.0 versioning.
| // for the actual video capture media type. | ||
| // video_source_stream_index: Integer index of the video source stream in | ||
| // MediaFoundation. audio_source_stream_index: Integer index of the audio | ||
| // source stream in MediaFoundation. |
There was a problem hiding this comment.
These need to be formatted the same as the other comments, with separate lines and proper indenting.
| // sample_callback: A pointer to capture engine listener. | ||
| // This is set as sample callback for preview sink. | ||
| // source_stream_index: Integer index of the preview source stream in | ||
| // MediaFoundation. |
There was a problem hiding this comment.
Please update all of these to do consistent indenting.
There was a problem hiding this comment.
Also, the order should match the parameter order.
| } | ||
| } | ||
|
|
||
| enum class PlatformStreamCategory { video, photo, audio }; |
There was a problem hiding this comment.
Freestanding internal helpers like enums and functions should be declared at the top of the file in an anonymous namespace.
|
|
||
| enum class PlatformStreamCategory { video, photo, audio }; | ||
|
|
||
| HRESULT GetMediaSourceStreamIndex( |
There was a problem hiding this comment.
This needs a declaration comment, per the style guide, explaining what it does and what its overall role is.
| (target_stream_category == PlatformStreamCategory::audio && | ||
| stream_category == MF_CAPTURE_ENGINE_STREAM_CATEGORY_AUDIO)) { | ||
| *source_stream_index = stream_index; | ||
| return S_OK; |
There was a problem hiding this comment.
This is unconditionally returning only the first matching stream; why is that correct? How did you determine that unconditionally using the first stream is never a regression compared to using the preferred stream? That seems counterintuitive.
| if ((target_stream_category == PlatformStreamCategory::video && | ||
| (stream_category == MF_CAPTURE_ENGINE_STREAM_CATEGORY_VIDEO_PREVIEW || | ||
| stream_category == | ||
| MF_CAPTURE_ENGINE_STREAM_CATEGORY_VIDEO_CAPTURE)) || |
There was a problem hiding this comment.
Why are preview and capture streams being treated interchangeably?
There was a problem hiding this comment.
This is a similar behavior used in the Chromium source code (in VideoCaptureDeviceMFWin::FillCapabilities). Though I'm not too sure why they can be treated interchangeably.
There was a problem hiding this comment.
That code is building a list of all relevant sources, it's not unilaterally picking a source to use. There is a huge difference between "a preview source is a potential video source" and "if the first source encountered is a preview source, always use it for video".
It doesn't seem like the approach in this PR has been sufficiently thought through; I suggest writing up a short design document explaining your planned approach so that the approach can be discussed at a high level.
dasyad00
force-pushed
the
fix/camera_windows/140014
branch
from
f651d9c to
405bf6a
Compare
|
Greetings from stale PR triage! 👋 |
|
Thank you for your contribution. Since there are outstanding comments but the PR hasn’t been updated in several months, I’m going to close it so that our PR queue reflects active PRs. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks! |
3 participants
Explanation post: flutter/flutter#140014 (comment)
Resolves #140014.
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3