New issue
Advanced search Search tips

Issue 862395 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

[Video Capture, Windows] Some capture devices not working with MediaFoundation

Project Member Reported by chfremer@chromium.org, Jul 10

Issue description

During rollout of the new MediaFoundation code path for video capture on Windows, users have reported that certain types of devices stopped working. See comments beginning at https://bugs.chromium.org/p/chromium/issues/detail?id=849636#c9.

The rollout has since been rolled back to mitigate the issue until we can find the root cause and have landed a fix.

Since https://bugs.chromium.org/p/chromium/issues/detail?id=849636 is for tracking a different issue, i.e. an increase in errors during video capture start that happened around May 1st, let us use this new issue entry to track the work on the MediaFoundation code path.

 
I bought the Pinnacle Dazzle to reproduce the issue.
I connected a random video source to it.

I reproduce the issue:
- it works with DirectShow enabled
- it does not work with MediaFoundation enabled

I compared chrome://media-internal output in both modes.
The device is listed in both modes.
When DirectShow is enabled, the Dazzle appears in the camera list with a dozen of video formats.
When MediaFoundation is enabled, the Dazzle still appears in the camera list, but this time with zero video format. 

To fix the issue I propose to count the number of video formats in video_capture_device_factory_win.cc GetDeviceDescriptorsMediaFoundation(), and to fallback on DirectShow or not based on the result.

The fix can take multiple forms:
1- If the MediaFoundation device enumerates zero video formats, fallback to DirectShow
OR
2- If the MediaFoundation device enumerates zero video formats, and the DirectShow device has at least one video format, fallback to DirectShow
OR
3- If the MediaFoundation device has less video formats than the DirectShow equivalent, fallback to DirectShow

I think we should pick fix number 2.
What do you think chfremer@ ?
dazzle_ds.PNG
48.9 KB View Download
dazzle_mf.PNG
12.3 KB View Download
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3772661191b045df5ed2df7aa6fde0a787967fae

commit 3772661191b045df5ed2df7aa6fde0a787967fae
Author: Réda Housni Alaoui <alaoui.rda@gmail.com>
Date: Tue Jul 17 16:28:55 2018

[Video Capture Win] Fallback to DirectShow when MediaFoundation offers
no video format

Devices like Pinnacle Dazzle are listed in DirectShow with their video
formats while listed in MediaFoundation with no available video format
at all.

This makes the capture factory fallback to DirectShow implementation
when it contains at least one video format and its MediaFoundation
counterpart has none.

Bug:  862395 
Change-Id: I5d02f97971d4cf98372e12bb96c0b7ad9cc6d9c3
Reviewed-on: https://chromium-review.googlesource.com/1137817
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575667}
[modify] https://crrev.com/3772661191b045df5ed2df7aa6fde0a787967fae/media/capture/video/win/video_capture_device_factory_win.cc
[modify] https://crrev.com/3772661191b045df5ed2df7aa6fde0a787967fae/media/capture/video/win/video_capture_device_factory_win.h
[modify] https://crrev.com/3772661191b045df5ed2df7aa6fde0a787967fae/media/capture/video/win/video_capture_device_factory_win_unittest.cc

Thank you chfremer@.
Can we backport the fix to 68?
Labels: Merge-Request-68
Status: Fixed (was: Assigned)
I was hoping we might still be able to get this into M68, since it the change is quite safe and is essentially a "hotfix" for the feature rollout which is temporarily disable via service-side configuration flag.

However, we do need to confirm that things are working on at least one Canary build first, and it looks like M68 stable cut is happening before that can be done.

Let's check with the release owner if there is any chance to still get it in.
Project Member

Comment 6 by sheriffbot@chromium.org, Jul 17

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
thanks for the fix. We are cutting release candidate today at 2pm PDT for tomorrow's beta, so it will be tough to get this in. Since this is the last beta, my recommendation is to target this fix for M69. 
abdulsyed@: Hmm, I see. So even if we verify on the next Canary, we would not be able to verify on any M68 beta anymore, leaving a merge directly into the release candidate as the only (probably undesirable) option.

It looks like alaoui.rda@gmail.com has verified the change on his local build, and the touched functionality is covered by unit tests as well. If I go ahead and also verify on a local build of tip of tree right now, would you approve a merge into M68 before the 2pm PDT deadline?


For what it's worth, I have tested a local build of the latest tip of tree, which includes this fix, and found no regression.
Labels: -Merge-Review-68 Merge-Approved-68
confirmed with chfremer@, this is behind a feature flag. It's safe, isolated, and well tested locally. Approved for M68. Branch:3440. But please verify this in Canary and Beta tomorrow. 
Thank you.
I am creating the CL.
re #11 no need, I already did the cherry pick into the branch.
Hmm looks like I spoke too early. The cherry pick through Gerrit gives me an error because of a merge conflict. I'll see if I can resolve it.
Oh cool, just have to wait then :)
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 17

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e3ddfc4b757ca922cb2398db4ad887873210eda

commit 6e3ddfc4b757ca922cb2398db4ad887873210eda
Author: Christian Fremerey <chfremer@chromium.org>
Date: Tue Jul 17 20:36:35 2018

[Video Capture Win] Fallback to DirectShow when MediaFoundation offers no video format

Devices like Pinnacle Dazzle are listed in DirectShow with their video
formats while listed in MediaFoundation with no available video format
at all.

This makes the capture factory fallback to DirectShow implementation
when it contains at least one video format and its MediaFoundation
counterpart has none.

TBR=alaoui.rda@gmail.com

(cherry picked from commit 3772661191b045df5ed2df7aa6fde0a787967fae)

Bug:  862395 
Change-Id: I5d02f97971d4cf98372e12bb96c0b7ad9cc6d9c3
Reviewed-on: https://chromium-review.googlesource.com/1137817
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#575667}
Reviewed-on: https://chromium-review.googlesource.com/1141051
Cr-Commit-Position: refs/branch-heads/3440@{#701}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/6e3ddfc4b757ca922cb2398db4ad887873210eda/media/capture/video/win/video_capture_device_factory_win.cc
[modify] https://crrev.com/6e3ddfc4b757ca922cb2398db4ad887873210eda/media/capture/video/win/video_capture_device_factory_win.h
[modify] https://crrev.com/6e3ddfc4b757ca922cb2398db4ad887873210eda/media/capture/video/win/video_capture_device_factory_win_unittest.cc

Comment 16 Deleted

Cool, the merge was easy to do with git drover. Let's keep a close eye on the beta builder to make sure everything stays green.
Thank you chfremer@

Can the 100% rollout of MediaFoundation be resumed on 68?
We'll have to get a new approval to re-ramp the rollout of the feature once the code change has reached users with M68. I don't assume we will immediately go to 100%. Most likely it will again be 1% first and then increase from there.

Beta builder did succeed with the merge. The first M68 including the fix is 68.0.3440.67, but the latest beta release is still 68.0.3440.59, so we'll have to wait a bit for the newer release.

For M69, the first build including the fix is 69.0.3495.0, which is today's Canary.

alaoui.rda@gmail.com: Could you please try today's Canary and check that everything works as expected?

Looks like the new Beta is now out with version 68.0.3440.68.

alaoui.rda@gmail.com: Would you mind giving it a spin when you get a chance?

Comment 22 Deleted

chfremer@

I just tested both latest Canary and latest Beta with the MediaFoundation flag enabled.
The fix works as expected.
Project Member

Comment 24 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ba070fd460e84e89cabb8af7396f1e28b9ba513

commit 0ba070fd460e84e89cabb8af7396f1e28b9ba513
Author: Christian Fremerey <chfremer@chromium.org>
Date: Tue Aug 14 23:04:29 2018

[VideoCapture, Windows] Blacklist certain legacy Empia chips for MediaFoundation

Devices using certain legacy Empia chips appear to get listed and report valid
formats in both DirectShow and MediaFoundation but only work in DirectShow when
actually trying to open them.

This CL adds a blacklisting mechanism that forces device models known to exhibit
this issue to always get opened with DirectShow instead of MediaFoundation.

Test: capture_unittests.exe --gtest_filter=VideoCaptureDeviceFactoryMFWinTest*
Bug: 792640, 849636,  862395 
Change-Id: Ia70d13a45eb49ed185f27630d0e544f3a484b6f2
Reviewed-on: https://chromium-review.googlesource.com/1174902
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583068}
[modify] https://crrev.com/0ba070fd460e84e89cabb8af7396f1e28b9ba513/media/capture/video/win/video_capture_device_factory_win.cc
[modify] https://crrev.com/0ba070fd460e84e89cabb8af7396f1e28b9ba513/media/capture/video/win/video_capture_device_factory_win_unittest.cc

Labels: Merge-Request-68 Merge-Request-69
Requesting merge of CL from #24 into M69 and maybe even M68.

Justification for the merge is that this is a "hotfix" for unblocking the rollout tracked in bug 792640. It is very safe, since it is small and localized and only affects a code path that is rolled out via Finch flag and is currently at 50% in beta and 0% in stable.
Project Member

Comment 26 by sheriffbot@chromium.org, Aug 16

Labels: -Merge-Request-69 Merge-Review-69
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #25. Please merge ASAP. Thank you.
Project Member

Comment 28 by bugdroid1@chromium.org, Aug 16

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1abb14e34927da38b26ba3087dcb443847c641ac

commit 1abb14e34927da38b26ba3087dcb443847c641ac
Author: Christian Fremerey <chfremer@chromium.org>
Date: Thu Aug 16 23:22:33 2018

[VideoCapture, Windows] Blacklist certain legacy Empia chips for MediaFoundation

Devices using certain legacy Empia chips appear to get listed and report valid
formats in both DirectShow and MediaFoundation but only work in DirectShow when
actually trying to open them.

This CL adds a blacklisting mechanism that forces device models known to exhibit
this issue to always get opened with DirectShow instead of MediaFoundation.

Test: capture_unittests.exe --gtest_filter=VideoCaptureDeviceFactoryMFWinTest*
Bug: 792640, 849636,  862395 
Change-Id: Ia70d13a45eb49ed185f27630d0e544f3a484b6f2
Reviewed-on: https://chromium-review.googlesource.com/1174902
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#583068}(cherry picked from commit 0ba070fd460e84e89cabb8af7396f1e28b9ba513)
Reviewed-on: https://chromium-review.googlesource.com/1179241
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#679}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/1abb14e34927da38b26ba3087dcb443847c641ac/media/capture/video/win/video_capture_device_factory_win.cc
[modify] https://crrev.com/1abb14e34927da38b26ba3087dcb443847c641ac/media/capture/video/win/video_capture_device_factory_win_unittest.cc

Labels: M-70
Labels: M-69
This was actually merged to M69
Project Member

Comment 31 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6854025abb6278daaa3258117cf8c796ec46bba8

commit 6854025abb6278daaa3258117cf8c796ec46bba8
Author: Christian Fremerey <chfremer@chromium.org>
Date: Thu Oct 11 20:46:32 2018

[Video Capture, Win] Update blacklist for MediaFoundation

This CL adds a new entry to the blacklist as per new report from
https://bugs.chromium.org/p/chromium/issues/detail?id=849636#c61

TBR=emircan@chromium.org

Bug: 792640, 849636,  862395 
Change-Id: I47a36dce54993f93059fd833f0ca5ea074037339
Reviewed-on: https://chromium-review.googlesource.com/c/1277589
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598921}
[modify] https://crrev.com/6854025abb6278daaa3258117cf8c796ec46bba8/media/capture/video/win/video_capture_device_factory_win.cc

Sign in to add a comment