Reland: Win video capture: use IMFCaptureEngine for Media Foundation [chromium/src : master]
Réda Housni Alaoui (Gerrit)
mcasas@, chfremer@,
Initial CL [1] was reverted because of test failures on "Win8 Tester" [2].
At that time, I was testing CL [1] on machines [M1] and [M2].
IMO, failures in [2] come from error "VideoCaptureDeviceMFWin: The stream number provided was invalid. (0xC00D36B3)".
Since I was not able to reproduce on [M1] and [M2], I added [M3], [M4] and [M5].
Patchset 1 is the exact patch that initially landed in [1].
Using [M3], I found a new issue. The Logitech C920 is not able to take a still photo according to its offered sub media types.
It was causing an "Invalid media type error" in the TakePhoto().
Patchset 2, adds a check at device startup to see if the device offers at least one still photo compliant media type.
If the device does not, like does the DirectShow implementation, the TakePhoto operation fallbacks to a GrabFrame operation.
I could not reproduce the "Win8 Tester" failure in [M4], but I did reproduce it on [M5].
So it looks like "Win8 Tester" runs Windows 2012 R2 or Windows 8.0.
Patchset 3, adds a device stream counting at device startup. If there is only one stream available, then there are no video stream and photo stream, therefore we must always use stream 0 in any case.
So to sum up:
- Patchset 2 solves an issue affecting all windows versions
- Patchset 3 solves an issue specific to Windows 2012 R2 and below
If you think that Patchset 3 fix is too cumbersome, an alternative would be to restrict MF implementation usage to Windows 8.1 and above.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/734042
[2] https://build.chromium.org/deprecated/chromium.webrtc/builders/Win8%20Tester/builds/38865
[M1] Windows 10 Microsoft Surface Pro 4 with its embedded webcams
[M2] Windows 10 VM without any webcam
[M3] Windows 10 VM with Logitech HD Pro Webcam C920
[M4] Windows 8.1 VM with Logitech HD Pro Webcam C920
[M5] Windows 2012 R2 + MF package feature VM with Logitech HD Pro Webcam C920
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
A threading question on PatchSet 2, see inline.
3 comments:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #2, Line 144:
return false;Second part of condition can be simplified to the following?
(major_type_guid != MFMediaType_Image &&
(stream == kPreferredPhotoStream ||
!GetFrameRateFromMediaType(type, &format->frame_rate)))Patch Set #2, Line 673:
gfx::Size current_size;Why is this change needed? Do we need to set the |step| of this mojom::Range to 0 if |max_size.height() == min_size.height()|?
Is there potential concurrent access of |video_stream_take_photo_callbacks_|?
Here we are inside |lock_|, but in method TakePhoto() where we also modify it, we are not.
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch set 3:Commit-Queue +1
2 comments:
File media/capture/video/win/video_capture_device_mf_win.cc:
Is there potential concurrent access of |video_stream_take_photo_callbacks_|? […]
With the complexity of this class increasing, the need for unit test increases as well. We definitely need to do that before launching, but can be a follow up CL to this one.
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #3, Line 452:
photo_stream_ = 0;This confused me at first read through. I somehow assumed that these things are IDs, but being able to use 0 here indicates that these are more like indices. If that is the case, could we rename the members to |video_stream_index_| and |photo_stream_index_| to make it more clear?
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
@chfremer, I added an auto lock on takePhoto(), let you a question about the step and fixed the rest.
4 comments:
File media/capture/video/win/video_capture_device_mf_win.cc:
Second part of condition can be simplified to the following? […]
Done
Patch Set #2, Line 673:
return;Why is this change needed? Do we need to set the |step| of this mojom::Range to 0 if |max_size. […]
I did that to match grab frame behaviour of DirectShow implementation.
We may have consumers checking the step value to know if the photo resolution can be different from the video resolution.But I can rollback to step valued at 1 if you want. Just tell me.
Patch Set #2, Line 783:
return;With the complexity of this class increasing, the need for unit test increases as well. […]
Yes it is possible.
Added an AutoLock on TakePhoto().
File media/capture/video/win/video_capture_device_mf_win.cc:
This confused me at first read through. […]
Done
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #2, Line 673:
return;I did that to match grab frame behaviour of DirectShow implementation. […]
I don't understand. If the max and min of a range are the same, it should not matter what the step is set to. I also don't understand how a client would conclude that photo resolution can or cannot be different from video resolution based on this value?
Also, the std::min() expression seems quite complicated for something that can be either 0 or 1.
Please clarify.
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
Patch Set #2, Line 673:
return;
I don't understand. […]
I do agree with you about the fact that it shouldn't matter.
It is just that, today, in the current implementation, when a client use ImageCapture api, it receives always a step of 0 [1].Yes, the client should not build any logic on top of that about the potential resolutions. I just didn't want to break existing clients.
So let's keep the value to 1.
To view, visit change 810766. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch set 5:Code-Review +1Commit-Queue +1
Réda Housni Alaoui (Gerrit)
chfremer@ thanks for the +1 but it seems that the commit is still not in master.
We may need mcasas@ +1 like the last time?
Miguel Casas (Gerrit)
Patch Set 5:
chfremer@ thanks for the +1 but it seems that the commit is still not in master.
We may need mcasas@ +1 like the last time?
+1 then
Patch set 5:Code-Review +1
Christian Fremerey (Gerrit)
Hehe, yes we needed the +1 from mcasas@, but to get the CL submitted we even need a +2, which I am triggering now :)
Patch set 5:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland:
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Original description:
- Full rewrite of the MediaFoundation implementation video part to use
IMFCaptureEngine
- Implementation of takePhoto, setPhotoOptions and getPhotoCapabilities
- takePhoto triggers a still image capture with the highest available
resolution without stopping the video stream thanks to IMFCaptureEngine
TEST=adapted video_capture_device_unittest.cc and
webrtc_image_capture_browsertest.cc; launch Chrome with
--force-mediafoundation on Win8+ and capture video using
e.g. https://webrtc.github.io/samples/src/content/getusermedia/gum/
R=mca...@chromium.org
Bug: 730068
Change-Id: I3835a48ca8516fc66a6a8394b3709d4dea862f89
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Commit-Position: refs/heads/master@{#524417}
---
M AUTHORS
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
8 files changed, 874 insertions(+), 184 deletions(-)
Réda Housni Alaoui (Gerrit)
Tests that failed on Win 8 and Win 10:
- WebRtcWebcamBrowserTests/WebRtcWebcamBrowserTest.MANUAL_TestAcquiringAndReacquiringWebcam/1
- VideoCaptureDeviceTests/VideoCaptureDeviceTest.CaptureWithSize/3
Most of them timed out.
We can see in [1] that for |kVideoCallConstraints720p|, no video frame is received while no error is traced.
I managed to reproduce this behaviour in 1080p with one of my Logitech webcams on Windows 10. This was weird, because there was no error coming from IMFCaptureEngine synchronously or asynchronously. Using MFTrace, I could see an error about a failing MFTransform configuration from NV12 Input to MJPEG output:
2108,26D8 22:45:57.59128 CMFTransformDetours::SetInputType @0000000033C8A540 Succeeded MT:
MF_MT_FRAME_SIZE=8246337209400
(1920,1080);MF_MT_MAJOR_TYPE=MEDIATYPE_Video;MF_MT_AM_FORMAT_TYPE=FORMAT_VideoInfo;MF_MT_FIX
ED_SIZE_SAMPLES=1;MF_MT_VIDEO_NOMINAL_RANGE=1;MF_MT_FRAME_RATE=128849018881
(30,1);MF_MT_PIXEL_ASPECT_RATIO=4294967297
(1,1);MF_MT_ALL_SAMPLES_INDEPENDENT=1;MF_MT_FRAME_RATE_RANGE_MIN=128849018881
(30,1);MF_MT_INTERLACE_MODE=2;MF_MT_FRAME_RATE_RANGE_MAX=128849018881
(30,1);MF_MT_SUBTYPE=MFVideoFormat_NV12
2108,26D8 22:45:57.59130 CMFTransformDetours::SetOutputType @0000000033C8A540 Failed MT:
MF_MT_FRAME_SIZE=8246337209400
(1920,1080);MF_MT_AVG_BITRATE=1492992000;MF_MT_MAJOR_TYPE=MEDIATYPE_Video;MF_MT_AM_FORMAT_TY
PE=FORMAT_VideoInfo;MF_MT_FIXED_SIZE_SAMPLES=1;MF_MT_FRAME_RATE=128849018881
(30,1);MF_MT_PIXEL_ASPECT_RATIO=4294967297
(1,1);MF_MT_ALL_SAMPLES_INDEPENDENT=1;MF_MT_FRAME_RATE_RANGE_MIN=128849018881
(30,1);MF_MT_SAMPLE_SIZE=6220800;MF_MT_INTERLACE_MODE=2;MF_MT_FRAME_RATE_RANGE_MAX=128849018
881 (30,1);MF_MT_SUBTYPE=MFVideoFormat_MJPG
I finally found out that IMFMediaEvent::GetExtendedType is not the best way to catch an error event. This is fixed by Patchset 1. Thanks to this, the MFTransform error is not properly detected and logged. The error was "No suitable transform was found to encode or decode the content error".
I think this was caused because of the absence of MJPEG codec on my machine. On this Logitech webcam, an alternate format exists for NV12. But NV12 was missing from the pixel format mapping and therefore skipped. After adding NV12 to the mapping in Patchset 2, the issue was fixed.
I think this is what happened in "Win 8 Tester" and "Win 10 Tester" but I can't be 100% sure since I couldn't manage to find out which camera is used by these bots.
To view, visit change 843974. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
FYI, Patchset 1 is the one that landed with CL [1]
[1] https://chromium-review.googlesource.com/c/chromium/src/+/810766
Réda Housni Alaoui (Gerrit)
Patch Set 3:
Sorry, I meant Patchset 2 instead of Patchset 1 and Patchset 3 instead of Patchset 2.
Réda Housni Alaoui (Gerrit)
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #3, Line 387:
DCHECK(attributes);This is an omission.
I removed it in Patchset 4.
To view, visit change 843974. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch set 4:Code-Review +1
Réda Housni Alaoui (Gerrit)
mcasas@, I think I need your review :)
Miguel Casas (Gerrit)
RS lgtm
Patch set 4:Code-Review +1
Réda Housni Alaoui (Gerrit)
+2 🙏
Réda Housni Alaoui (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
only full committers or CL owner with tryjob access are allowed to trigger CQ
Christian Fremerey (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
Try jobs failed on following builders:
win7_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/73470)
Réda Housni Alaoui (Gerrit)
chfremer@, mcasas@,
In the win 7 build, failing steps fail with or without patch.
Can you submit to CQ again please?
Réda Housni Alaoui (Gerrit)
Don't forget me please :)
Christian Fremerey (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland number 2:
- "Win10 Tester" browser_tests_functional failure
- "Win10 Tester" capture_unittests failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 1:
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Original description:
- Full rewrite of the MediaFoundation implementation video part to use
IMFCaptureEngine
- Implementation of takePhoto, setPhotoOptions and getPhotoCapabilities
- takePhoto triggers a still image capture with the highest available
resolution without stopping the video stream thanks to IMFCaptureEngine
TEST=adapted video_capture_device_unittest.cc and
webrtc_image_capture_browsertest.cc; launch Chrome with
--force-mediafoundation on Win8+ and capture video using
e.g. https://webrtc.github.io/samples/src/content/getusermedia/gum/
R=mca...@chromium.org
Bug: 730068
Change-Id: I454f6a87c43e0148d50c671911bc214c969f6610
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Commit-Position: refs/heads/master@{#527139}
---
M AUTHORS
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
8 files changed, 872 insertions(+), 184 deletions(-)
Réda Housni Alaoui (Gerrit)
Tests are failing with:
./../media/capture/video/video_capture_device_unittest.cc(483): error: Mock function called
more times than expected - taking default
action specified at:
../../media/capture/video/video_capture_device_unittest.cc(170):
Function call: OnError(@0873F778 16-byte object <2C-54 EE-00 A8-53 EE-00 29-03 00-00 E0-
BC A7-00>, @0873F740
"VideoCaptureDeviceMFWin: No suitable transform was found to encode or decode the content.
(0xC00D5212)")
Expected: to be never called
Actual: called once - over-saturated and active
I experienced this issue with C920 at 1920*1080 and with MJPEG format.
I found why!
Unlike other implementations, IMFCaptureEngine in preview mode performs video decoding itself.
The video stream follow the path camera -> IMFCaptureSource -> IMFTransform -> IMFCaptureSink -> Chromium.
So, the video media type set in IMFCaptureSource must match one exposed by the camera (i.e. 1080p NV12, 1080 MJPEG, ...).
And the video media type set in IMFCaptureSink must be any RAW/uncompressed video format.
IMFCaptureEngine will automatically setup an IMFTransform taking camera stream as input and decoded stream as output.
The IMFCaptureSink media type must always be an uncompressed format, but which one? From what I understood reading VideoCaptureClient implementation, any retrieved frame will be converted to I420. Therefore, I decided to fix the video format output to I420 to avoid performing the same operation multiple times.
Video capture device MF implementation will always emit I420 frames.
To view, visit change 852455. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch set 2:Code-Review +1Commit-Queue +1
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #2, Line 189:
GUID sink_mf_video_format_) {As a reader I find it slightly confusing that the output parameter |sink_media_type| appears in the middle between two parameters that serve as read-only inputs. I recommend putting the output parameter last.
To view, visit change 852455. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #2, Line 189:
IMFMediaType* sink_media_type) {As a reader I find it slightly confusing that the output parameter |sink_media_type| appears in the […]
Done
To view, visit change 852455. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
It seems that I canceled the tryjob with my last commit
Réda Housni Alaoui (Gerrit)
I don't have CQ permission, so don't forget this please 8-)
Miguel Casas (Gerrit)
Patch set 4:Code-Review +1Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland number 3:
- "Win10 Tester" browser_tests_functional failure
- "Win10 Tester" capture_unittests failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 2:
- "Win10 Tester" browser_tests_functional failure
- "Win10 Tester" capture_unittests failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 1:
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Original description:
- Full rewrite of the MediaFoundation implementation video part to use
IMFCaptureEngine
- Implementation of takePhoto, setPhotoOptions and getPhotoCapabilities
- takePhoto triggers a still image capture with the highest available
resolution without stopping the video stream thanks to IMFCaptureEngine
TEST=adapted video_capture_device_unittest.cc and
webrtc_image_capture_browsertest.cc; launch Chrome with
--force-mediafoundation on Win8+ and capture video using
e.g. https://webrtc.github.io/samples/src/content/getusermedia/gum/
R=mca...@chromium.org
Bug: 730068
Change-Id: I13fa632b25a345a20465e95a48162064eb2c4cae
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Original-Commit-Position: refs/heads/master@{#527139}
Reviewed-on: https://chromium-review.googlesource.com/852455
Commit-Queue: Miguel Casas <mca...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528005}
---
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
7 files changed, 915 insertions(+), 184 deletions(-)
Réda Housni Alaoui (Gerrit)
Now, MF VideoCaptureDevice implementation only enumerates I420 descriptor from the point of view of the client.
It should fix the capture_unittests failure.
For the browser_tests_functional, since I can't reproduce, I changed the logging (Patchset 6) to be able to understand what goes wrong.
To view, visit change 858138. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Christian Fremerey (Gerrit)
Patch set 6:Code-Review +1
Réda Housni Alaoui (Gerrit)
Ready for submit?
Christian Fremerey (Gerrit)
I am unfortunately not (yet) in the owners list for content/browser/webrtc, so we need mcasas@ signoff.
I'll to the bots in the meantime.
Patch set 6:Commit-Queue +1
Miguel Casas (Gerrit)
RS lgtm
In the future, you can ping explicitly each reviewer in
the message, e.g.:
mcasas@ RS please
or
chfremer@ PTAL at bla_bla.cc
etc
Patch set 6:Commit-Queue +1
To view, visit change 858138. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Patch Set 6: Commit-Queue+1
RS lgtm
In the future, you can ping explicitly each reviewer in
the message, e.g.:
mcasas@ RS please
or
chfremer@ PTAL at bla_bla.cc
etc
Ok I will.
But what is the meaning of "RS"?
Réda Housni Alaoui (Gerrit)
mcasas@, chfremer@,
Following recent developments [1] + [2], should we rollback the log addition (patchset 6) and instead wait for test renaming before merging?
If yes, I guess the renaming should be done in another CL?
By chfremer@?
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=730068#c57
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=733582#c15
Réda Housni Alaoui (Gerrit)
Patch Set 6:
mcasas@, chfremer@,
Following recent developments [1] + [2], should we rollback the log addition (patchset 6) and instead wait for test renaming before merging?
If yes, I guess the renaming should be done in another CL?
By chfremer@?[1] https://bugs.chromium.org/p/chromium/issues/detail?id=730068#c57
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=733582#c15
Oh I forgot that this is the CL which enable real camera tests on Windows.
Do you want me to rename the test as recommended in [1]?
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=733582#c15
Réda Housni Alaoui (Gerrit)
Patch Set 6:
Patch Set 6:
mcasas@, chfremer@,
Following recent developments [1] + [2], should we rollback the log addition (patchset 6) and instead wait for test renaming before merging?
If yes, I guess the renaming should be done in another CL?
By chfremer@?[1] https://bugs.chromium.org/p/chromium/issues/detail?id=730068#c57
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=733582#c15Oh I forgot that this is the CL which enable real camera tests on Windows.
Do you want me to rename the test as recommended in [1]?[1] https://bugs.chromium.org/p/chromium/issues/detail?id=733582#c15
Please consider comment [1].
Maybe I should only reduce the real camera test scope to MediaFoundation, evicting DirectShow from functional test.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=730068#c59
Réda Housni Alaoui (Gerrit)
I misread the test.
DirectShow is only using fake camera.
Therefore I am back with my first question: Do we rename the test to disable concurrency?
Sorry for the spam :)
Miguel Casas (Gerrit)
Patch Set 6:
Patch Set 6: Commit-Queue+1
RS lgtm
In the future, you can ping explicitly each reviewer in
the message, e.g.:
mcasas@ RS please
or
chfremer@ PTAL at bla_bla.cc
etc
Ok I will.
But what is the meaning of "RS"?
RS means RubberStamp. Also, sorry, I forgot to +1 :-P
Patch set 6:Code-Review +1
Réda Housni Alaoui (Gerrit)
mcasas@, chfremer@,
Following CL "Prefix content_browsertests that use real webcam with UsingRealWebcam" [1], I rebased the current CL on master.
RS + CQ please :)
[1] https://chromium-review.googlesource.com/c/chromium/src/+/872260
Daniel Parada (Gerrit)
Patch set 8:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase on master" https://chromium-review.googlesource.com/c/858138/8
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/858138/8
Bot data: {"action": "start", "triggered_at": "2018-01-21T15:10:13.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "b404e911109fb0020c3340ece01d88d52e3a1643"}
Commit Bot (Gerrit)
only full committers or CL owner with tryjob access are allowed to trigger CQ
Réda Housni Alaoui (Gerrit)
mcasas@ RS please :)
Christian Fremerey (Gerrit)
1 comment:
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #8, Line 250:
testing::ValuesIn(kTargetVideoCaptureImplementations)));Hmm, this will make the tests using the fake device run twice on Windows. We might be able to solve this by adding a |kTargetVideoCaptureImplementationsForFakeDevice| and |kTargetVideoCaptureImplementationsForRealWebcam| instead of the single |kTargetVideoCaptureImplementations|.
To view, visit change 858138. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #8, Line 250:
// --gtest_filter=WebRtc*Hmm, this will make the tests using the fake device run twice on Windows. […]
Done
To view, visit change 858138. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Thanks. This looks good to me.
Assuming that mcasas@ does not have any objections, I can send this to the commit queue so we can see if the sequentialization of the real webcam tests helps with the remaining failures.
Patch set 9:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Make the distinction between 2 lists of values for TargetVideoCaptureImplementation" https://chromium-review.googlesource.com/c/858138/9
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/858138/9
Bot data: {"action": "start", "triggered_at": "2018-01-22T20:15:10.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "2d4d83358f50a0d2fc7885df53d145d9788ff2f4"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/380005)
Réda Housni Alaoui (Gerrit)
chfremer@, mcasas@,
The build failed with:
../../content/browser/webrtc/webrtc_image_capture_browsertest.cc:82:5: error: unused
variable 'kTargetVideoCaptureImplementationsForRealWebcam' [-Werror,-Wunused-const-variable]
kTargetVideoCaptureImplementationsForRealWebcam[] = {
^
1 error generated.
I moved |kTargetVideoCaptureImplementationsForRealWebcam| near the test.
Please RS again.
Christian Fremerey (Gerrit)
Patch set 10:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Move kTargetVideoCaptureImplementationsForRealWebcam near RealWebcam test" https://chromium-review.googlesource.com/c/858138/10
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/858138/10
Bot data: {"action": "start", "triggered_at": "2018-01-22T21:34:21.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "2edf0e95e5a2629ef8dc59f44f954e9ce0230cc0"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/380144)
Réda Housni Alaoui (Gerrit)
The ios-simulator failure is not caused by this patch :/
Evertyhing else is passing.
Réda Housni Alaoui (Gerrit)
android build seems flaky.
Can you RS again please?
Christian Fremerey (Gerrit)
Patch set 10:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Move kTargetVideoCaptureImplementationsForRealWebcam near RealWebcam test" https://chromium-review.googlesource.com/c/858138/10
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/858138/10
Bot data: {"action": "start", "triggered_at": "2018-01-23T01:09:52.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "2edf0e95e5a2629ef8dc59f44f954e9ce0230cc0"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland number 4:
- "Win10 Tester" browser_tests_functional failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 3:
- "Win10 Tester" browser_tests_functional failure
- "Win10 Tester" capture_unittests failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 2:
- "Win10 Tester" browser_tests_functional failure
- "Win10 Tester" capture_unittests failure
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Fixes for reland number 1:
- "Win8 Tester" browser_tests_functional failure
- "Win8 Tester" capture_unittests failure
Original description:
- Full rewrite of the MediaFoundation implementation video part to use
IMFCaptureEngine
- Implementation of takePhoto, setPhotoOptions and getPhotoCapabilities
- takePhoto triggers a still image capture with the highest available
resolution without stopping the video stream thanks to IMFCaptureEngine
TEST=adapted video_capture_device_unittest.cc and
webrtc_image_capture_browsertest.cc; launch Chrome with
--force-mediafoundation on Win8+ and capture video using
e.g. https://webrtc.github.io/samples/src/content/getusermedia/gum/
R=mca...@chromium.org
Bug: 730068
Change-Id: I8da39896ae7b914537f78ce809c9376248409741
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Original-Original-Commit-Position: refs/heads/master@{#527139}
Reviewed-on: https://chromium-review.googlesource.com/852455
Commit-Queue: Miguel Casas <mca...@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#528005}
Reviewed-on: https://chromium-review.googlesource.com/858138
Cr-Commit-Position: refs/heads/master@{#531109}
---
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
7 files changed, 937 insertions(+), 192 deletions(-)
Réda Housni Alaoui (Gerrit)
Remaining failing test prints:
[2548:3320:0122/194550.387:INFO:CONSOLE(13)] "Requesting doGetUserMedia: constraints:
{"video":{"mandatory":{"minWidth":320,"maxWidth":320,"minHeight":240,"maxHeight":240}}}",
source: http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194550.679:INFO:CONSOLE(13)] "Returning request-callback-granted to test.",
source: http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194550.683:INFO:CONSOLE(13)] "Returning ok-got-stream to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194550.684:INFO:CONSOLE(13)] "Returning ok-started to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194550.685:INFO:CONSOLE(13)] "Returning video-not-playing to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194550.936:INFO:CONSOLE(13)] "Returning video-not-playing to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194551.189:INFO:CONSOLE(13)] "Returning video-playing to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194551.190:INFO:CONSOLE(13)] "Returning ok-320x240 to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194551.191:INFO:CONSOLE(13)] "Returning ok-stopped to test.", source:
http://127.0.0.1:54733/webrtc/test_functions.js (13)
[2548:3320:0122/194551.191:INFO:CONSOLE(13)] "Requesting doGetUserMedia: constraints:
{"video":{"mandatory":{"minWidth":640,"maxWidth":640,"minHeight":480,"maxHeight":480}}}",
source: http://127.0.0.1:54733/webrtc/test_functions.js (13)
[5736:5640:0122/194551.497:ERROR:video_capture_device_mf_win.cc(118)]
AllocateAndStart@../../media/capture/video/win/video_capture_device_mf_win.cc:494 hr = The s
tream number provided was invalid. (0xC00D36B3)
[2548:3320:0122/194551.498:INFO:CONSOLE(13)] "GetUserMedia FAILED: Maybe the camera is in
use by another process?", source: http://127.0.0.1:54733/webrtc/test_functions.js (13)
The test with the first resolution, then fails with the second.
Therefore, it seems that the stream enumeration is unstable.
So I chose to catch the INVALIDSTREAMNUMBER instead of relying on the stream count.
mcasas@, chfremer@, please RS :)
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch set 4:Commit-Queue +1
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #4, Line 480:
photo_stream_index_ = 0;Can it happen that even index 0 is not available? Or would we have caught that somewhere before reaching here?
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
Patch Set #4, Line 480:
photo_stream_index_ = 0;
Can it happen that even index 0 is not available? Or would we have caught that somewhere before reac […]
I don't know.
When you look at MSDN documentations, every function that uses a stream index as an argument is supposed to accept value 0: [1]Specifies which stream to query. The value can be any of the following.
0–0xFFFFFFFB
The zero-based index of a stream. To get the number of streams, call
IMFCaptureSource::GetDeviceStreamCount.I never saw a device without stream.
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/hh447893(v=vs.85).aspx
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
1 comment:
Patch Set #4, Line 480:
photo_stream_index_ = 0;
I don't know. […]
Hmm, so basically the issue we are seeing in the test is that for the first resolution, the call to FillCapabilities(photo_stream_index_, ...) worked, but for the second resolution it didn't.
Since we don't know what GetDeviceStreamCount() returned, we can't tell if we tried MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_PHOTO or 0 as the stream index when it failed.Now, the relevant change for this reland is that we fall back to trying to use the video stream instead of the photo stream, with the video stream being either MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_VIDEO_PREVIEW or also 0. I feel this has only a relatively small chance of succeeding, and I don't like that it does not really help us much in understanding how exactly the MF API calls behavior in the problematic situation.
My recommendation would be to temporarily land a version of this CL that does the following here:
- Call GetDeviceStreamCount() and LOG(WARNING) the result.
- Call FillCapabilities(MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_PHOTO) and LOG whether or not it succeeded.
- If it did not succeed, call FillCapabilities(MF_CAPTURE_ENGINE_FIRST_SOURCE_PHOTO_STREAM) and LOG
- If that did not succeed either, call FillCapabilities(0) and LOG whether or not it succeeded.
Same for video case. First try MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_VIDEO_PREVIEW, then MF_CAPTURE_ENGINE_FIRST_SOURCE_VIDEO_STREAM, then 0.
With this logging in place, we should be able to see enough to decide if there is any way we can convince the APIs to work in the problematic case, or if things are really in a bad state where erroring out is the only option. If that happens, the next interesting question would be what exactly it is that brings things into this bad state, but let's think about it when we get there.
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #4, Line 480:
return;Hmm, so basically the issue we are seeing in the test is that for the first resolution, the call to […]
I agree with that.
Done.Please know that |MF_CAPTURE_ENGINE_FIRST_SOURCE_PHOTO_STREAM| and |MF_CAPTURE_ENGINE_FIRST_SOURCE_VIDEO_STREAM| exists only MSDN documentation. I never found a header containing those constant.
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Cool. Ignoring style nits, since this is really just for the investigation, and we expect to have to revert. Let me try and push that to the commit queue so we can see what it gives us in the logs.
Patch set 5:Code-Review +1Commit-Queue +2
Commit Bot (Gerrit)
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/644263)
Christian Fremerey (Gerrit)
Christian Fremerey uploaded patch set #6 to the change originally created by Réda Housni Alaoui.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland number 5:
TBR=mca...@chromium.org
Bug: 730068
Change-Id: If081d29402e9f595a3fd1906e45a3bec1d712b3a
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Original-Original-Commit-Position: refs/heads/master@{#527139}
Reviewed-on: https://chromium-review.googlesource.com/852455
Commit-Queue: Miguel Casas <mca...@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#528005}
Reviewed-on: https://chromium-review.googlesource.com/858138
Cr-Commit-Position: refs/heads/master@{#531109}
---
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
7 files changed, 959 insertions(+), 192 deletions(-)
To view, visit change 880944. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Setting mcasas@ as TBR since there is no change in webrtc_image_capture_browsertest.cc since the last reland.
Patch set 6:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/880944/6
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/880944/6
Bot data: {"action": "start", "triggered_at": "2018-01-24T23:25:23.0Z", "cq_cfg_revision": "39be8832cf39acdcc5b8dada8799c7bf02eb68a1", "revision": "2b89d1d535e76c03c616f13ba208647e62a40fc7"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Cr-Original-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#527139}
Reviewed-on: https://chromium-review.googlesource.com/852455
Commit-Queue: Miguel Casas <mca...@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#528005}
Reviewed-on: https://chromium-review.googlesource.com/858138
Cr-Original-Commit-Position: refs/heads/master@{#531109}
Reviewed-on: https://chromium-review.googlesource.com/880944
Cr-Commit-Position: refs/heads/master@{#531791}
---
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
7 files changed, 959 insertions(+), 192 deletions(-)
Réda Housni Alaoui (Gerrit)
In the previous CL, I found that I missed an error path:
DWORD stream_count = 0;
source->GetDeviceStreamCount(&stream_count);
if (FAILED(hr)) {
OnError(FROM_HERE, hr);
return;
}
I was not checking the HRESULT returned by GetDeviceStreamCount. This was fixed in the previous CL [1].
Thanks to this and added logs, the functional failures can be explained.
Sometimes GetDeviceStreamCount fails the same way GetAvailableDeviceMediaType with error MF_E_INVALIDREQUEST. This is the fix that we produced for GetAvailableDeviceMediaType.
HRESULT hr;
// Rarely, for some unknown reason, GetAvailableDeviceMediaType returns an
// undocumented MF_E_INVALIDREQUEST. Retrying solves the issue.
int retry_count = 0;
do {
hr = source->GetAvailableDeviceMediaType(stream_index, media_type_index,
type);
if (FAILED(hr))
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(50));
// Give up after ~10 seconds
} while (hr == MF_E_INVALIDREQUEST && retry_count++ < 200);
return hr;
When GetDeviceStreamCount failed, the number of stream was kept to 0 and therefore |video_stream_index_| and |photo_stream_index_| were changed to 0.
During this whole time, GetDeviceStreamCount failure allowed the rest of the process to work correctly. Indeed, when GetDeviceStreamCount succeeds, it returns a count of 2. But nevertheless, |MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_VIDEO_PREVIEW| and |MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_PHOTO| were still KO.
The logs in browser_functional_tests, which pass now, show that the C920 always fallback to stream 0.
So my proposal is to fallback on stream 0 when MF_E_INVALIDSTREAMNUMBER is returned.
To view, visit change 885815. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Patch Set 3:
Or I could enumerate all streams, filter them by categories. [1][2]
I just don't understand the difference between those two:
MF_CAPTURE_ENGINE_STREAM_CATEGORY_PHOTO_INDEPENDENT
Specifies an independent photo stream.
MF_CAPTURE_ENGINE_STREAM_CATEGORY_PHOTO_DEPENDENT
Specifies a dependent photo stream.
Let's investigate.
Marking the CL in WIP for now.
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/jj159897(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/jj159898(v=vs.85).aspx
Réda Housni Alaoui (Gerrit)
Finally, I iterate on all streams and filter them by categories.
It works fine on my surface cameras + my C920.
chfremer@, mcasas, RS please :)
Miguel Casas (Gerrit)
Patch set 4:Code-Review +1
Christian Fremerey (Gerrit)
lgtm % nits
Patch set 4:Code-Review +1Commit-Queue +1
5 comments:
File media/capture/video/win/capability_list_win.h:
Patch Set #4, Line 24:
stream_index() {}Not away of any mention of this in coding style guidelines, but this is the first time I have seen a primitive initialized using a default constructor. I'd prefer explicitly initializing it with 0.
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #4, Line 116:
#if !defined(NDEBUG)Is the #if needed here? I was assuming that the D in DPLOG already means that it resolves to no-op in debug builds.
Patch Set #4, Line 247:
HRESULT RequestWithRetries(base::RepeatingCallback<HRESULT()> requester) {Naming nit: |requester| sounds to me like it is referring to whoever is requesting. I propose to name this
HRESULT ExecuteHresultCallbackWithRetries(base::RepeatingCallback<HRESULT()> callback)
Patch Set #4, Line 250:
// undocumented MF_E_INVALIDREQUEST. Retrying solves the issue.Since the method is now generalized, this comment should be moved to the invocation specific to GetAvailableDeviceMediaType.
Patch Set #4, Line 264:
return RequestWithRetries(base::BindRepeating(I recommend adding some comment explaining why we believe retrying is the best option we have here. If I understand correctly, it is because either MediaFoundation itself or the underlying device can be in a state where they reject these calls and there is no event sent by MediaFoundation that would tell us when that state is reached and how long it lasts.
To view, visit change 885815. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
5 comments:
File media/capture/video/win/capability_list_win.h:
Patch Set #4, Line 24:
stream_index(0) {}Not away of any mention of this in coding style guidelines, but this is the first time I have seen a […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #4, Line 116:
DPLOG(ERROR) << from_here.ToString()Is the #if needed here? I was assuming that the D in DPLOG already means that it resolves to no-op i […]
Done
Patch Set #4, Line 247:
// Retry callback execution on MF_E_INVALIDREQUEST.Naming nit: |requester| sounds to me like it is referring to whoever is requesting. […]
Done
Patch Set #4, Line 250:
// state that reject these calls. Since MediaFoundation gives no intel aboutSince the method is now generalized, this comment should be moved to the invocation specific to GetA […]
Done
I recommend adding some comment explaining why we believe retrying is the best option we have here. […]
Done
To view, visit change 885815. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, chfremer@,
Do you know when we can expect the end of trybot maintenance?
I don’t have permission to read the associated monorail ticket.
Or do the build only need to be launched again?
Réda Housni Alaoui (Gerrit)
chfremer@, mcasas@,
CQ please :)
Christian Fremerey (Gerrit)
Patch set 5:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix nits" https://chromium-review.googlesource.com/c/885815/5
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/885815/5
Bot data: {"action": "start", "triggered_at": "2018-01-26T19:35:16.0Z", "cq_cfg_revision": "9544bf3c7cb0639977b7c56b6f9db1b272cef60b", "revision": "acd31f7b74c49b4cbe080cc2f3c305b3ff38cd7b"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Reland: Win video capture: use IMFCaptureEngine for Media Foundation
Fixes for reland number 6:
- "Win10 Tester" capture_unittests failure
R=mca...@chromium.org
Bug: 730068
Change-Id: I7b7ff88f2db8d71f46428a2ecbb733e18a25a334
Reviewed-on: https://chromium-review.googlesource.com/734042
Commit-Queue: Christian Fremerey <chfr...@chromium.org>
Reviewed-by: Miguel Casas <mca...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Cr-Original-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#521435}
Reviewed-on: https://chromium-review.googlesource.com/810766
Cr-Original-Original-Original-Original-Commit-Position: refs/heads/master@{#524417}
Reviewed-on: https://chromium-review.googlesource.com/843974
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#527139}
Reviewed-on: https://chromium-review.googlesource.com/852455
Commit-Queue: Miguel Casas <mca...@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#528005}
Reviewed-on: https://chromium-review.googlesource.com/858138
Cr-Original-Commit-Position: refs/heads/master@{#531109}
Reviewed-on: https://chromium-review.googlesource.com/885815
Cr-Commit-Position: refs/heads/master@{#532040}
---
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
M content/test/data/media/image_capture_test.html
M media/capture/video/video_capture_device_unittest.cc
M media/capture/video/win/capability_list_win.h
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win.h
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
M media/capture/video/win/video_capture_device_win.cc
9 files changed, 1,010 insertions(+), 208 deletions(-)
Réda Housni Alaoui (Gerrit)
Thank you !