ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution [chromium/src : master]
Réda Housni Alaoui (Gerrit)
This change is ready for review.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #3 to this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable the force-mediafoundation flag to test this.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 558 insertions(+), 128 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
force-mediafoundation flag is currently broken => https://bugs.chromium.org/p/chromium/issues/detail?id=777880
You will have to force use_media_foundation_ attribute to true at line 343 of video_capture_device_factory_win.cc to test the patch.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #4 to this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable the force-mediafoundation flag to test this.
force-mediafoundation flag is currently broken => https://bugs.chromium.org/p/chromium/issues/detail?id=777880
You will have to force use_media_foundation_ attribute to true at line 343 of video_capture_device_factory_win.cc to test the patch.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 558 insertions(+), 128 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #6 to this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable --force-mediafoundation to test this.
--force-mediafoundation is currently broken => https://bugs.chromium.org/p/chromium/issues/detail?id=777880
You will have to pass --disable-features=MojoVideoCapture to make it work.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 561 insertions(+), 131 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #7 to this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
"--force-mediafoundation" is currently broken => https://bugs.chromium.org/p/chromium/issues/detail?id=777880
You will have to pass "--force-mediafoundation --disable-features=MojoVideoCapture" to make it work.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 561 insertions(+), 131 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui would like Christian Fremerey to review this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
"--force-mediafoundation" is currently broken => https://bugs.chromium.org/p/chromium/issues/detail?id=777880
You will have to pass "--force-mediafoundation --disable-features=MojoVideoCapture" to make it work.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 561 insertions(+), 131 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
I had a quick look and it seemed good, but did not yet have time to fully review the new implementation. It is nice that the implementation allows capturing still images at higher resolutions without having to interrupt an ongoing video stream.
What concerns me is that we currently do not have much test coverage for the MediaFoundation code path. To my knowledge, the only test that exercises this is [1] and that does nothing more than trying to open a single camera at various resolutions and it only runs on the webrtc waterfall. Before landing a rewrite like this, I would like to get this covered and verified by the same tests that currently run on the DirectShow code path.
3 comments:
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #3, Line 35:
preferredVideoPreviewStreamcode style: this is a constant, so name should be kPreferredVideoPreviewStream
Patch Set #3, Line 37:
const DWORD preferredPhotoStream =code style: this is a constant, so name should be kPreferredPhotoStream
Patch Set #3, Line 80:
void OnError(const base::Location& from_here, HRESULT hr);code style: Declare methods before fields.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
I fixed the code style
3 comments:
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #3, Line 35:
kPreferredVideoPreviewStrea
code style: this is a constant, so name should be kPreferredVideoPreviewStream
Done
Patch Set #3, Line 37:
const DWORD kPreferredPhotoStream =
code style: this is a constant, so name should be kPreferredPhotoStream
Done
Patch Set #3, Line 80:
bool capture_;
code style: Declare methods before fields.
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Patch Set 7:
(3 comments)
I had a quick look and it seemed good, but did not yet have time to fully review the new implementation. It is nice that the implementation allows capturing still images at higher resolutions without having to interrupt an ongoing video stream.
What concerns me is that we currently do not have much test coverage for the MediaFoundation code path. To my knowledge, the only test that exercises this is [1] and that does nothing more than trying to open a single camera at various resolutions and it only runs on the webrtc waterfall. Before landing a rewrite like this, I would like to get this covered and verified by the same tests that currently run on the DirectShow code path.
Can you take care of the test coverage or should I do it?
Christian Fremerey (Gerrit)
Patch Set 8:
Patch Set 7:
(3 comments)
I had a quick look and it seemed good, but did not yet have time to fully review the new implementation. It is nice that the implementation allows capturing still images at higher resolutions without having to interrupt an ongoing video stream.
What concerns me is that we currently do not have much test coverage for the MediaFoundation code path. To my knowledge, the only test that exercises this is [1] and that does nothing more than trying to open a single camera at various resolutions and it only runs on the webrtc waterfall. Before landing a rewrite like this, I would like to get this covered and verified by the same tests that currently run on the DirectShow code path.
Can you take care of the test coverage or should I do it?
If you could do it that would be great, since I am currently completely backed up with other tasks. Besides [1], other tests that exercise the platform-specific implementations by running on actual cameras are:
[2] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_unittest.cc?dr=C&q=videoCaptureDeviceTests&l=262
[3] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_image_capture_browsertest.cc?dr=CSs&l=75
Réda Housni Alaoui (Gerrit)
Patch Set 8:
Patch Set 8:
Patch Set 7:
(3 comments)
I had a quick look and it seemed good, but did not yet have time to fully review the new implementation. It is nice that the implementation allows capturing still images at higher resolutions without having to interrupt an ongoing video stream.
What concerns me is that we currently do not have much test coverage for the MediaFoundation code path. To my knowledge, the only test that exercises this is [1] and that does nothing more than trying to open a single camera at various resolutions and it only runs on the webrtc waterfall. Before landing a rewrite like this, I would like to get this covered and verified by the same tests that currently run on the DirectShow code path.
Can you take care of the test coverage or should I do it?
If you could do it that would be great, since I am currently completely backed up with other tasks. Besides [1], other tests that exercise the platform-specific implementations by running on actual cameras are:
[2] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_unittest.cc?dr=C&q=videoCaptureDeviceTests&l=262
[3] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_image_capture_browsertest.cc?dr=CSs&l=75
Ok I will do it ;)
Réda Housni Alaoui (Gerrit)
Patch Set 8:
Patch Set 8:
Patch Set 7:
(3 comments)
I had a quick look and it seemed good, but did not yet have time to fully review the new implementation. It is nice that the implementation allows capturing still images at higher resolutions without having to interrupt an ongoing video stream.
What concerns me is that we currently do not have much test coverage for the MediaFoundation code path. To my knowledge, the only test that exercises this is [1] and that does nothing more than trying to open a single camera at various resolutions and it only runs on the webrtc waterfall. Before landing a rewrite like this, I would like to get this covered and verified by the same tests that currently run on the DirectShow code path.
Can you take care of the test coverage or should I do it?
If you could do it that would be great, since I am currently completely backed up with other tasks. Besides [1], other tests that exercise the platform-specific implementations by running on actual cameras are:
[2] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_unittest.cc?dr=C&q=videoCaptureDeviceTests&l=262
[3] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_image_capture_browsertest.cc?dr=CSs&l=75
Test 1 is ok by running ".\out\Debug\browser_tests.exe --run-manual --gtest_filter=*AcquiringAndReacquiringWebcam*". I didn't find a way to obtain a test report to post it here.
I am going to modify test 3 to have 2 passes, one with DirectShow, the second with MediaFoundation.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #12 to this change.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
M media/capture/video/win/video_capture_device_mf_win.h
4 files changed, 562 insertions(+), 129 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui removed John Abd-El-Malek from this change.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
I added MediaFoundation to test 3 aka WebRtcImageCaptureSucceedsBrowserTest.
Runned command:
.\out\Debug\content_browsertests.exe --gtest_also_run_disabled_tests --gtest_filter=*WebRtcImageCaptureSucceedsBrowserTest*
On 42 tests, 2 iterations on GrabFrame and 1 on TakePhoto fail:
<testcase name="DISABLED_GrabFrame/2" status="run" time="4.997" classname="WebRtcImageCaptureSucceedsBrowserTest">
<failure message="" type=""></failure>
</testcase>
<testcase name="DISABLED_GrabFrame/4" status="run" time="4.118" classname="WebRtcImageCaptureSucceedsBrowserTest">
<failure message="" type=""></failure>
</testcase>
<testcase name="DISABLED_TakePhoto/2" status="run" time="4.985" classname="WebRtcImageCaptureSucceedsBrowserTest">
<failure message="" type=""></failure>
</testcase>
GrabFrame/2 = DirectShow + VideoCaptureService OFF
GrabFrame/4 = MediaFoundation + VideoCaptureService OFF
TakePhoto/2 = DirectShow + VideoCaptureService OFF
Working on test 2 now.
Réda Housni Alaoui (Gerrit)
chfremer,
Test 1, 2 and 3 now cover DirectShow + MediaFoundation.
All tests behave as before the changes except test 3.
On test 3, enabled both DirectShow and MediaFoundation.
Test 3 was only using fake camera before changes on Windows platform.
Here are the results on my Surface Pro.
Test 1 results:
Test modifications: none
Before: 100% OK
After: 100% OK
Test 2 results:
Test modifications: Added the MediaFoundation case
Before: 1 timeout on 7 tests
After: 2 timeouts on 24 tests
The timeouts always concern the DirectShow implementation.
Test 3 results:
Test modifications: Was only using fake camera on Windows. Added DirectShow AND MediaFoundation.
Before: 100 %
After:
On 42 tests, 2 iterations on GrabFrame and 1 on TakePhoto fail.
GrabFrame/2 = DirectShow + VideoCaptureService OFF
GrabFrame/4 = MediaFoundation + VideoCaptureService OFF
TakePhoto/2 = DirectShow + VideoCaptureService OFF
What is the next step?
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui has assigned a change to Christian Fremerey.
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolution
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
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, 642 insertions(+), 154 deletions(-)
Christian Fremerey (Gerrit)
Patch Set 15:
chfremer,
Test 1, 2 and 3 now cover DirectShow + MediaFoundation.
All tests behave as before the changes except test 3.
On test 3, enabled both DirectShow and MediaFoundation.
Test 3 was only using fake camera before changes on Windows platform.Here are the results on my Surface Pro.
Test 1 results:
Test modifications: none
Before: 100% OK
After: 100% OK
Test 2 results:
Test modifications: Added the MediaFoundation case
Before: 1 timeout on 7 tests
After: 2 timeouts on 24 tests
The timeouts always concern the DirectShow implementation.
Test 3 results:
Test modifications: Was only using fake camera on Windows. Added DirectShow AND MediaFoundation.
Before: 100 %
After:
On 42 tests, 2 iterations on GrabFrame and 1 on TakePhoto fail.
GrabFrame/2 = DirectShow + VideoCaptureService OFF
GrabFrame/4 = MediaFoundation + VideoCaptureService OFF
TakePhoto/2 = DirectShow + VideoCaptureService OFFWhat is the next step?
Thanks a lot for adding the test cases.
Regarding Test 3, my mistake, yes these tests were not active for DirectShow either. I guess my question here is would we expect the new MediaFoundation + VideoCaptureService OFF variant to work reliably? If yes, we should investigate that failure of GrabFrame.
I will now take a closer look at the new implementation.
Christian Fremerey (Gerrit)
My comments are mostly code style nits, but there are also a few interesting questions about locks, nullptrs, and refcounting.
26 comments:
Patch Set #16, Line 7:
ImageCapture on Surface Rear Camera (Win) is not using the highest available resolutionPlease change the title of this CL to something indicating what it changes, as opposed to the symptom it is attempting to fix.
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #16, Line 65:
defined(OS_WIN)Considering the tests were not active for Windows DirectShow before, and they seem to be not working or at least flaky, I would say it is okay to keep them disabled in this CL.
File media/capture/video/win/video_capture_device_factory_win.h:
Patch Set #16, Line 34:
// forced via flag, else use DirectShow.I believe with your reworked implementation, now Windows 8 is the minimum requirement for using it, which makes the above comment inaccurate. Personally, I don't think the comment is very useful in the first place, so I would be okay with removing it.
Patch Set #16, Line 36:
bool use_media_foundation_;As a slightly less encapsulation-breaking option for allowing tests to set this configuration, I propose keeping the member private and exposing a setter named set_use_media_foundation_for_testing(bool use).
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #16, Line 40:
class CAPTURE_EXPORT VideoCaptureDeviceMFWin : public VideoCaptureDevice {Just a quick though about unit testing this class.
I think this class contains more than enough complexity to warrant unit testing it in isolation.
Currently all tests that exercise this are very high-level integration/end-to-end tests.
Unit testing would be possible by abstracting out the parts of the MediaFoundation API that are called into from here and using a mock implementation in a unit test.Since this is consistently not done in any of the other platform-specific implementations of VideoCaptureDevice as well, I am not going to insist on it here. But I feel the need to point out that I believe this would be a good idea and the right thing to do.
Patch Set #16, Line 73:
scoped_refptr<MFVideoCallback> callback_;Since we now also have a separate callback object for the photo case, I recommend we name this one |video_callback_|.
Patch Set #16, Line 75:
base::Lock lock_; // Used to guard the below variables.I'd like to make this explanation more concrete, something like:
"Guards the below variables from concurrent access between methods running on |sequence_checker_| and calls to OnIncomingCapturedData() and OnEvent() made by MediaFoundation on threads outside of our control."Patch Set #16, Line 80:
bool capture_;If my understanding is correct that this indicates whether or not the device is started, let us rename this to |is_started_|.
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #16, Line 62:
static bool FillFormat(IMFMediaType* type, VideoCaptureFormat* format) {Sorry to ask for changes in parts that you didn't touch, but since this is the first time I am reviewing this class, there are certain things that confuse me while reading.
I think this method should be named consistently with the two methods above.
As a reader, for me it would be easiest to understand if they were named something like "GetFrameSizeFromMediaType", "GetFrameRateFromMediaType", and "GetFormatFromMediaType".Patch Set #16, Line 63:
GUID majorTypeGuid;For variable and parameter names, please use lower case with underscores as per Chromium style guidelines [1].
[1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
Patch Set #16, Line 97:
static bool ConvertToPhotoMediaType(IMFMediaType* sourceMediaType,Not sure if it understood by all readers that PhotoMediaType implies Jpeg. Maybe we could add this detail to the method name.
Patch Set #16, Line 125:
std::this_thread::sleep_for(std::chrono::milliseconds(50));Use of C++11 <thread> library is banned in Chromium as per [1].
I believe the Chromium equivalent here would be base::PlatformThread::Sleep, see [2].[1] https://chromium-cpp.appspot.com/#library-blacklist
[2] https://cs.chromium.org/chromium/src/base/threading/platform_thread.h?type=cs&q=thread+sleep&l=153I am a bit uncomfortable with this retry loop not having a counter for maximum retries. I think we should add one and give up after a certain number of retries.
In the above block, why are we doing the static_casts? Is this to have the compiler double-check that we are actually implementing this interface?
In case the caller requests IID_IUnknown do we really need the static_cast?
Patch Set #16, Line 269:
DWORD count = 0;|count| is a little unspecific, lets go with |buffer_count| instead.
Patch Set #16, Line 279:
mojom::BlobPtr blob = Blobify(data, length, format_);Can this fail in case, let's say, the data size does not match what would be expected for |format_|?
Patch Set #16, Line 282:
std::move(callback_).Run(std::move(blob));If there is more than one buffer, can it happen that we reach this line a second time and then crash because |callback_| is nullptr?
I guess a follow-up question is what it is supposed to mean if there is more than buffer sent to us as a response to requesting a single still image. Are we supposed to somehow concatenate the buffers? Or is it safe to ignore extra buffers?
Patch Set #16, Line 341:
base::AutoLock lock(lock_);Why do we need to add a lock here? Can this really get called simultaneously with any other code path that accesses the same members?
Patch Set #16, Line 482:
if (capture_) {if (!capture_)
return;This eliminates the need for the long scope and indentation below.
Patch Set #16, Line 545:
new MFPhotoCallback(std::move(callback), std::move(format)));I am not sure if/how a base::RefCountedThreadSafe class works when not wrapped in a scoped_refptr<>. Does it have a refcount of 1 after construction? Does MediaFoundation increase the refcount and release it when it is done with it, or does it only release, or does it do neither?
Patch Set #16, Line 562:
if (capture_) {same as above, return here if !capture_.
Patch Set #16, Line 641:
capture_same as above
Patch Set #16, Line 648:
if (SUCCEEDED(hr) && (settings->has_height || settings->has_width)) {Instead of 4 nested if (SUCCEEDED(hr)), I would prefer to return early on failure, even if it means that we have to duplicate the LogError() call 4 times.
Please extract the code for finding the best match to a separate method, because otherwise the method here gets way too long.
Patch Set #16, Line 711:
base::AutoLock lock(lock_);Is this lock still needed? Before this CL there was access to |capture_| and |reader_|, but now we only access |client_|. Probably still needed to prevent this executing concurrently with StopAndDeAllocate().
Patch Set #16, Line 718:
void VideoCaptureDeviceMFWin::OnEvent(IMFMediaEvent* mediaEvent) {If we use a lock in OnIncomingCaptureData to guard |client_| from concurrent usage/modification, do we need to do it here as well?
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
24 comments:
Patch Set #16, Line 65:
defined(OS_WIN)
Considering the tests were not active for Windows DirectShow before, and they seem to be not working […]
Do you want me to revert all modifications done to this file
OR
to have those tests running on MediaFoundation but not on DirectShow?
File media/capture/video/win/video_capture_device_factory_win.h:
I believe with your reworked implementation, now Windows 8 is the minimum requirement for using it, […]
Done
Patch Set #16, Line 36:
bool use_media_foundation_;
As a slightly less encapsulation-breaking option for allowing tests to set this configuration, I pro […]
Done
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #16, Line 40:
class CAPTURE_EXPORT VideoCaptureDeviceMFWin : public VideoCaptureDevice {
Just a quick though about unit testing this class. […]
Ack
Patch Set #16, Line 73:
scoped_refptr<MFVideoCallback> video_callback_;Since we now also have a separate callback object for the photo case, I recommend we name this one | […]
Done
Patch Set #16, Line 75:
// Guards the below variables from concurrent access between methods runningI'd like to make this explanation more concrete, something like: […]
Done
Patch Set #16, Line 80:
std::unique_ptr<VideoCaptureDevice::Client> client_;If my understanding is correct that this indicates whether or not the device is started, let us rena […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
Sorry to ask for changes in parts that you didn't touch, but since this is the first time I am revie […]
Done
Patch Set #16, Line 63:
static bool GetFormatFromMediaType(IMFMediaType* type,For variable and parameter names, please use lower case with underscores as per Chromium style guide […]
Done
Not sure if it understood by all readers that PhotoMediaType implies Jpeg. […]
Done
Patch Set #16, Line 125:
requested_height = requested_size.height();Use of C++11 <thread> library is banned in Chromium as per [1]. […]
Done
I am a bit uncomfortable with this retry loop not having a counter for maximum retries. […]
Done
Patch Set #16, Line 171:
DWORD media_type_index = 0;In the above block, why are we doing the static_casts? Is this to have the compiler double-check tha […]
Yes it is to double check.
You are correct about Unknown, I removed the static_cast for this case.Patch Set #16, Line 269:
: public base::RefCountedThreadSafe<MFPhotoCallback>,
|count| is a little unspecific, lets go with |buffer_count| instead.
Done
Patch Set #16, Line 279:
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
Can this fail in case, let's say, the data size does not match what would be expected for |format_|?
Yes it can.
But in that case, blob will be a nullptr and we will not pass in if(blob), therefore the callback will not be called.
The callback documentation states "On failure, drops callback without invoking it." IMO this is what it does.Patch Set #16, Line 282:
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);If there is more than one buffer, can it happen that we reach this line a second time and then crash […]
I looked at the documentation and didn't find information about eventual multiple buffers for photo callback.
I propose to only consider the first filled buffer right now.Patch Set #16, Line 341:
const VideoPixelFormat format;Why do we need to add a lock here? Can this really get called simultaneously with any other code pat […]
Done
if (!capture_) […]
Done
same as above, return here if !capture_.
Done
Patch Set #16, Line 641:
max_hesame as above
Done
Patch Set #16, Line 648:
max_width = width;Instead of 4 nested if (SUCCEEDED(hr)), I would prefer to return early on failure, even if it means […]
Done
Please extract the code for finding the best match to a separate method, because otherwise the metho […]
Done
Patch Set #16, Line 711:
kPreferredPhotoStream, current_media_type.GetAddressOf());Is this lock still needed? Before this CL there was access to |capture_| and |reader_|, but now we o […]
Ack
If we use a lock in OnIncomingCaptureData to guard |client_| from concurrent usage/modification, do […]
Correct. Lock added.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #18 to this change.
Replace IMFSourceReader with IMFCaptureEngine in ImageCapture MediaFoundation implementation
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
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, 700 insertions(+), 164 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
1 comment:
Patch Set #16, Line 7:
Replace IMFSourceReader with IMFCaptureEngine in ImageCapture MediaFoundation implementationPlease change the title of this CL to something indicating what it changes, as opposed to the sympto […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #19 to this change.
Replace IMFSourceReader with IMFCaptureEngine in VideoCapture MediaFoundation implementation
- 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
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
Later, it would be nice to see if we still need this flag.
R=mca...@chromium.org
Bug: 730068
Change-Id: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
M AUTHORS
M content/browser/webrtc/webrtc_image_capture_browsertest.cc
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, 700 insertions(+), 164 deletions(-)
To view, visit change 734042. 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 #16, Line 545:
photo_media_type.Get())I am not sure if/how a base::RefCountedThreadSafe class works when not wrapped in a scoped_refptr<>. […]
I don't know :)
If we are not sure, maybe we should take defensive measures?
Could you tell me which ones?
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
I fixed the code according to your comments.
I left one question unanswered.
Christian Fremerey (Gerrit)
Thanks for addressing everything.
The only remaining question is the one about the refcounting. See inline comment.
6 comments:
Patch Set #16, Line 65:
defined(OS_WIN)
Do you want me to revert all modifications done to this file […]
I'd prefer to have the tests run on MediaFoundation.
The fact that they currently don't run on DirectShow is bad, but is of course not an issue of this CL.
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #16, Line 40:
class CAPTURE_EXPORT VideoCaptureDeviceMFWin : public VideoCaptureDevice {
Ack
Added a task for this: https://bugs.chromium.org/p/chromium/issues/detail?id=786854
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #16, Line 282:
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);I looked at the documentation and didn't find information about eventual multiple buffers for photo […]
I am okay with that proposal.
Let's add a brief comment to the break; statement explaining this.
Patch Set #16, Line 545:
photo_media_type.Get())
I don't know :) […]
Yes, we need to make sure that we can be certain that this works as intended.
My recommendation (and really the only reliable way I can think of) is to add a minimal test that verifies our assumptions.Instead of passing in a MFPhotoCallback, the test could pass in a specialized implementation that allows it to verify the refcounts before/during/after usage by MediaFoundation.
File media/capture/video/win/video_capture_device_mf_win.cc:
As per Chromium style guidelines, single line blocks following a control statement are supposed to go without { }.
Patch Set #21, Line 699:
if (settings->has_height || settings->has_width) {We could eliminate the lengthy scope below by inverting this if clause as well and returning early.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
5 comments:
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #16, Line 65:
{DEFAULT},I'd prefer to have the tests run on MediaFoundation. […]
Done
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #16, Line 40:
class CAPTURE_EXPORT VideoCaptureDeviceMFWin : public VideoCaptureDevice {
Added a task for this: https://bugs.chromium. […]
Ack
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #16, Line 282:
if (SUCCEEDED(hr))I am okay with that proposal. […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #21, Line 522:
Location from_here = FROM_HERE;As per Chromium style guidelines, single line blocks following a control statement are supposed to g […]
Done
Patch Set #21, Line 699:
ComPtr<IMFMediaType> current_media_type;
We could eliminate the lengthy scope below by inverting this if clause as well and returning early.
I did it this way because in a near future, we will have to support all the remaining settings, not only height and width.
Do you want to remove the scope despite this?
To view, visit change 734042. 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 #21, Line 699:
ComPtr<IMFMediaType> current_media_type;I did it this way because in a near future, we will have to support all the remaining settings, not […]
It's okay to leave like this. These are really just small nits.
In general, for readability, I think it is good to avoid long indented scopes either by returning early, or by extracting them into a separate method.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
This change is ready for review.
2 comments:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #16, Line 545:
hr = FillCapabilities(kPreferredPhotoStream, source.Get(), &capabilities);Yes, we need to make sure that we can be certain that this works as intended. […]
I created a photo callback factory allowing to pass another implementation of photo callback to MediaFoundation.
I added a test to check the correct release of the callback by MediaFoundation.
The test passes.
Please check that my test assertions are the good ones.
File media/capture/video/win/video_capture_device_mf_win.cc:
It's okay to leave like this. These are really just small nits. […]
Ack
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Test 3 is failing on the surface because the video stream is opened with a max width of 320 [1][2], but since surface camera has a minimal width of 640 and height of 480, the frame is grabbed at 640x480, failing assertion on imageBitmap.width == 320 || imageBitmap.height == 320 [3].
Do you want me to modify this test? Can we remove this strange assertion?
[1] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L10
[2] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L100
[3] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L111
Christian Fremerey (Gerrit)
Patch Set 23:
Test 3 is failing on the surface because the video stream is opened with a max width of 320 [1][2], but since surface camera has a minimal width of 640 and height of 480, the frame is grabbed at 640x480, failing assertion on imageBitmap.width == 320 || imageBitmap.height == 320 [3].
Do you want me to modify this test? Can we remove this strange assertion?
Hmm if not all devices can produce 320 width frames, but all devices can produce 640 width frames, then I think we should change the 320 to 640.
If also not all devices can produce 640, we would have to rethink the test.
Let's try changing it to 640 and see if any bots complain.
>
> [1] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L10
>
> [2] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L100
>
> [3] https://github.com/scheib/chromium/blob/master/content/test/data/media/image_capture_test.html#L111
3 comments:
File media/capture/video/win/photo_callback_factory_mf_win.h:
Patch Set #23, Line 14:
class CAPTURE_EXPORT MFPhotoCallbackFactory {Since the class name is not descriptive enough to convey what this class does, please add a brief description comment.
Patch Set #23, Line 20:
std::unique_ptr<IMFCaptureEngineOnSampleCallback> build(style: Method name should start with capital letter.
naming nit: I like factory methods that start with "Create...". My suggestions would be "CreateFactory".
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #23, Line 517:
hr = photo_sink.Get()->SetSampleCallback(photo_callback.release());The unit test case you added convinces me (as a reader) that AddRef and Release are both called once, which is good.
However, the part that I am still not convinced about (without having to go read the implementation of base::RefCountedThreadSafe<>) is whether or not |photo_callback| actually gets destroyed at the end or if it leaks. That is because I don't know if the |photo_callback| created by build() comes with a refcount of 0 or 1. If it comes out with 1, it would leak.
Having a refcounted object wrapped in a std::unique_ptr<> seems like a confusing and dangerous mix of lifetime management patterns. I guess the reason why refcounted classes typically get a private destructor in Chromium is to explicitly prevent this kind of usage. Unless you can find a better solution, I recommend having build() return a scoped_refptr<> instead of a std::unique_ptr<>. This would indicate to the reader that the returned instance is expected to come out with a refcount of 1, and then it should become clear what to do with it.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
See inline replies.
Réda Housni Alaoui (Gerrit)
Test 3 now uses a max width of 640 and is green.
I also fixed the code according to your latest comments.
3 comments:
File media/capture/video/win/photo_callback_factory_mf_win.h:
Patch Set #23, Line 14:
VideoCaptureFormatSince the class name is not descriptive enough to convey what this class does, please add a brief de […]
Done
Patch Set #23, Line 20:
~MFPhotoCallbackFactory();style: Method name should start with capital letter. […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #23, Line 517:
std::move(format));The unit test case you added convinces me (as a reader) that AddRef and Release are both called once […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Thanks for addressing everything.
LGTM
Patch set 24:Code-Review +1
Réda Housni Alaoui (Gerrit)
Patch set 25:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Merge remote-tracking branch 'origin/master' into mediafoundation_imagecapture" https://chromium-review.googlesource.com/c/734042/25
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/734042/25
Bot data: {"action": "start", "triggered_at": "2017-11-27T23:07:19.0Z", "cq_cfg_revision": "b547f51ef97353cccb06eebcda71133909f61295", "revision": "d20c503d4320ab56a70086c826c5e597d1deab88"}
Commit Bot (Gerrit)
only full committers or CL owner with tryjob access are allowed to trigger CQ
Réda Housni Alaoui (Gerrit)
Thank you Christian.
Merge conflict fixed.
Christian Fremerey (Gerrit)
Hmm, I am not an owner of content/browser/webrtc.
mcasas@: Could you please RS or delegate?
Miguel Casas (Gerrit)
This CL is a tad too large, but I guess now it's too
late to split it up.
16 comments:
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #25, Line 49:
enum Camera {enum class Camera? Otherwise FAKE, DEFAULT are names
that might be confusing in isolation.
File media/capture/video/win/photo_callback_factory_mf_win.h:
/*
Photo callback factory producing MediaFoundation
IMFCaptureEngineOnSampleCallback based on a TakePhotoCallback and a
VideoCaptureFormat
*/No C-style comments, use C++ //
File media/capture/video/win/photo_callback_factory_mf_win.cc:
Patch Set #25, Line 17:
class MFPhotoCallback finalThis class should go in an anonymous namespace
(inside namespace media).HRESULT hr = E_NOINTERFACE;
if (riid == IID_IUnknown) {
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
hr = S_OK;
} else if (riid == IID_IMFCaptureEngineOnSampleCallback) {
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
hr = S_OK;
}
if (SUCCEEDED(hr))
AddRef();
return hr;if (riid == IID_IUnknown || riid == IID_IMFCaptureEngineOnSampleCallback) {
AddRef();
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
return S_OK;
}
return E_NOINTERFACE;
STDMETHOD(OnSample)
(IMFSample* sample) override {Fits in one line?
Patch Set #25, Line 61:
if (buffer.Get()) {if (!buffer.Get())
continue;Patch Set #25, Line 62:
DWORD length = 0, max_length = 0;One definition per line, please.
nullptr
Patch Set #25, Line 85:
format_I think we can make this member const.
File media/capture/video/win/video_capture_device_factory_win.cc:
Patch Set #25, Line 289:
(DWORD)Use C++ static_cast<> please.
Patch Set #25, Line 394:
use_media_foundation_ = use;nit: consider inlining this?
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #25, Line 89:
if (SUCCEEDED(hr)) {if (!SUCCEEDED(hr))
return false;Or
if (FAILED(hr))
return false;Prefer early returns ISO deeply nested scopes.
/*
* Rarely, for some unknown reason, GetAvailableDeviceMediaType returns an
* undocumented MF_E_INVALIDREQUEST. Retrying solves the issue.
*/nit: Use C++ comment style.
while (
This format looks off. Try using clang format before
uploading plz.Patch Set #25, Line 473:
ConvertToPhotoJpegMediaTypeConvertToPhotoJpegMediaType() uses internally S_OK and E_FAIL
but you convert them to true/false before returning; change
this whole chain to using HRESULT.if (height > max_height)
max_height = height;
if (height < min_height)
min_height = height;height = std::max(min_height, std::min(height, max_height)); ?
Same below with |width|.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
16 comments:
File content/browser/webrtc/webrtc_image_capture_browsertest.cc:
Patch Set #25, Line 49:
enum class Caenum class Camera? Otherwise FAKE, DEFAULT are names […]
Done
File media/capture/video/win/photo_callback_factory_mf_win.h:
// Photo callback factory producing MediaFoundation
// IMFCaptureEngineOnSampleCallback based on a TakePhotoCallback and a
// VideoCaptureFormat
class CAPTURE_EXPORT MFPhotoCallbackFactory {
p
No C-style comments, use C++ //
Done
File media/capture/video/win/photo_callback_factory_mf_win.cc:
Patch Set #25, Line 17:
namespace {This class should go in an anonymous namespace […]
Done
STDMETHOD(QueryInterface)(REFIID riid, void** object) override {
if (riid == IID_IUnknown || riid == IID_IMFCaptureEngineOnSampleCallback) {
AddRef();
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
return S_OK;
}
return E_NOINTERFACE;
}
STDMETHOD_(ULONG, AddRef)() override {
base::RefCountedThreadSafe<MFPhotoCallback>::AddRef();
return 1U;if (riid == IID_IUnknown || riid == IID_IMFCaptureEngineOnSampleCallback) { […]
Done
DWORD buffer_count = 0;
sample->GetBufferCount(&buffer_count);Fits in one line?
Done
Patch Set #25, Line 61:
buffer->Lock(&data,if (!buffer.Get()) […]
Done
Patch Set #25, Line 62:
mojom::BlobPtr blob = Blobify(data,
One definition per line, please.
Done
nullptr
Done
Patch Set #25, Line 85:
y::MFPh
I think we can make this member const.
Done
File media/capture/video/win/video_capture_device_factory_win.cc:
Patch Set #25, Line 289:
static_Use C++ static_cast<> please.
Done
Patch Set #25, Line 394:
use_media_foundation_ = use;nit: consider inlining this?
This is the result of git cl format.
I cannot put it in one line.
Maybe I didn't understand what you mean?
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #25, Line 89:
if (FAILED(hr))if (!SUCCEEDED(hr)) […]
Done
// undocumented MF_E_INVALIDREQUEST. Retrying solves the issue.
int retry_count = 0;
while (MF_E_INVALIDREQUEST == (hr = source->GetAvailableDeviceMediaType(
nit: Use C++ comment style.
Done
source, stream, media_type_index, type.GetAddressOf()))) {
VideoCaptureFormat format;
ifThis format looks off. Try using clang format before […]
This is already the result of 'git cl format'.
ConvertToPhotoJpegMediaType() uses internally S_OK and E_FAIL […]
Done
min_width = std::min(width, min_width);
}
photo_capabilities->height =
mojom::Range::New(maheight = std::max(min_height, std::min(height, max_height)); ? […]
I am sorry but I did not understand your proposal.
I am really confused about the variables that you used.I replaced the if with std::max and std::min.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@ I fixed the CL according to your comments
Miguel Casas (Gerrit)
A fresh round of comments to get you going.
11 comments:
Patch Set #27, Line 938:
Cosium <*@cosium.com>Unintended? Remove if so.
File media/capture/video/video_capture_device_unittest.cc:
Patch Set #27, Line 100:
using ::testing::ByMove;Unused? Remove.
bool IsWinMediaFoundation() {
#if defined(OS_WIN)
return std::get<1>(GetParam()) == WIN_MEDIA_FOUNDATION;
#else
return false;
#endif
}Sandwich this whole method in
#if defined(OS_WIN)
#endif
since it's only called in l.777 for OS_WIN.Patch Set #27, Line 775:
/* Verifies that the photo callback is correctly released by MediaFoundation */Please use C++ style comments ISO C style comments throughout.
File media/capture/video/win/photo_callback_factory_mf_win.h:
Patch Set #27, Line 21:
medianit: no need for media:: (check throughout please).
File media/capture/video/win/photo_callback_factory_mf_win.cc:
DWORD length = 0;
DWORD max_length = 0;
BYTE* data = nullptr;micro-nit: if you define them in the same order as you
use them in l.61, it's easier to read: |data|, |max_length|,
|length|.Patch Set #27, Line 66:
// What it is supposed to mean if there is more than buffer sent to usI find this comment hard to parse, is it a question
or a statement? (Try and make a statement if possible.)DISALLOW_COPY_AND_ASSIGN(MFPhotoCallback); ?
MFPhotoCallbackFactory::~MFPhotoCallbackFactory() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}Try to follow the declaration order: please move
this method to l. 88.
File media/capture/video/win/video_capture_device_factory_win.cc:
void VideoCaptureDeviceFactoryWin::set_use_media_foundation_for_testing(
bool use) {
use_media_foundation_ = use;
}By inlining I meant to write, on the .h file:
void set_use_media_foundation_for_testing(bool use) {
use_media_foundation_ = use;
}and remove these lines entirely.
File media/capture/video/win/video_capture_device_mf_win.cc:
min_width = std::min(width, min_width);
}
photo_capabilities->height =
mojom::Range::New(ma
I am sorry but I did not understand your proposal. […]
My bad, I misunderstood these as clamping |height| between
a max and min values. Wrong suggestion, but still is better
to use one liners (more succinct).
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
And here is a fresh round of fixes :)
10 comments:
Patch Set #27, Line 938:
Cosium <*@cosium.com>Unintended? Remove if so.
This CL was created during my work time as Cosium employee.
So it is intended :)
File media/capture/video/video_capture_device_unittest.cc:
Unused? Remove.
Done
bool IsWinMediaFoundation() {
return std::get<1>(GetParam()) == WIN_MEDIA_FOUNDATION;
}
#endif
void ResetWithNewClient() {
Sandwich this whole method in […]
Done
Patch Set #27, Line 775:
return;
Please use C++ style comments ISO C style comments throughout.
nit: no need for media:: (check throughout please).
BYTE* data = nullptr;
DWORD max_length = 0;
DWORD length = 0;micro-nit: if you define them in the same order as you […]
Done
Patch Set #27, Line 66:
// What is it supposed to mean if there is more than one buffer sent tI find this comment hard to parse, is it a question […]
Done
DISALLOW_COPY_AND_ASSIGN(MFPhotoCallback); ?
Done
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Try to follow the declaration order: please move […]
Done
File media/capture/video/win/video_capture_device_factory_win.cc:
// static
VideoCaptureDeviceFactory*
VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory(
By inlining I meant to write, on the .h file: […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@ ready for commit queue? :)
Miguel Casas (Gerrit)
Not ready for CQ just yet.
12 comments:
Patch Set #29, Line 903:
Réda Housni Alaoui <alaou...@gmail.com>It seems like some AUTHORS were added wrongly, this
list should be alphabetically ordered, please correct.Patch Set #29, Line 938:
Cosium <*@cosium.com>Unintended change? Please remove (again).
File media/capture/video/video_capture_device_unittest.cc:
// Wrap the TEST_P macro into another one to allow to preprocess test_name
// macros
#define WRAPPED_TEST_P(test_case_name, test_name) \
TEST_P(test_case_name, test_name)Is this guy really needed? Doesn't seem like any
of its uses has any fancy |test_name|.
File media/capture/video/win/photo_callback_factory_mf_win.h:
Patch Set #29, Line 14:
MFPhotoCallbackFactoryThe class and the file name should match, so this class
should be PhotoCallbackFactoryMFWin.Also this file needs guards, see:
https://cs.chromium.org/chromium/src/media/base/win/mf_helpers.h?type=cs&sq=package:chromium&l=5
File media/capture/video/win/video_capture_device_factory_win.cc:
Patch Set #29, Line 289:
static_cast<DWORD>(MF_SOURCE_READER_FIRST_VIDEO_STREAM),Why this change? You could have left |kFirstVideoStream|.
File media/capture/video/win/video_capture_device_mf_win.cc:
int requested_height = current_size.height();
if (requested_size.height() > 0)
requested_height = requested_size.height();int requested_height = requested_size.height() > 0 ? requested_size.height()
: current_size.height();?
Same for l.123-125
while (MF_E_INVALIDREQUEST == (hr = source->GetAvailableDeviceMediaType(
stream_index, media_type_index, type))) {Hmmm assigning and comparing on the same statement is strange.
What about making this whole while(){}- return loop e.g.:
do {
hr = source->GetAvailableDeviceMediaType(stream_index, media_type_index,
type);
retry_count++;
if (FAILED(hr))
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(50));
} while (hr == MF_E_INVALIDREQUEST && retry_count < 200);
return hr;
} else if (riid == IID_IMFCaptureEngineOnSampleCallback) {
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
hr = S_OK;
} else if (riid == IID_IMFCaptureEngineOnEventCallback) {
*object = static_cast<IMFCaptureEngineOnEventCallback*>(this);
hr = S_OK;
}
} else if (riid == IID_IMFCaptureEngineOnSampleCallback ||
riid == IID_IMFCaptureEngineOnEventCallback) {
*object = static_cast<IMFCaptureEngineOnEventCallback*>(this);
hr = S_OK;
}? Five lines versus 7, no code duplication.
Patch Set #29, Line 354:
Location from_here = FROM_HERE;No need for this guy, just use FROM_HERE everywhere.
if (!engine_.Get())
hr = E_FAIL;Is it OK to continue with the execution of this method
after |engine| has failed?Patch Set #29, Line 370:
if (SUCCEEDED(hr)) {Prefer early return throughout this method:
hr = operationX();
if(FAILED(hr)) {
OnError(FROM_HERE, hr);
return;
}if (is_started_) {
is_started_ = false;
if (engine_.Get())
engine_->StopPreview();
}if (is_started && engine_)
engine_->StopPreview();
is_started_ = false;?
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
12 comments:
Patch Set #29, Line 903:
Myung-jong Kim <mjki...@gmail.com>It seems like some AUTHORS were added wrongly, this […]
Done
Patch Set #29, Line 938:
Vewd Software AS <*@vewd.com>
Unintended change? Please remove (again).
It seems that you did not see my first answer about this.
This is intended since this PR was created on my work hours for company Cosium.
File media/capture/video/video_capture_device_unittest.cc:
// Wrap the TEST_P macro into another one to allow to preprocess test_name
// macros
#define WRAPPED_TEST_P(test_case_name, test_name) \
TEST_P(test_case_name, test_name)
Is this guy really needed? Doesn't seem like any […]
You have multiple test named MAYBE_*.
Example: MAYBE_GetPhotoState
Without this guy, it doesn't compile.
It worked before because tests were TEST_F and not TEST_P.You can have a look at https://github.com/google/googletest/issues/389
File media/capture/video/win/photo_callback_factory_mf_win.h:
Patch Set #29, Line 14:
tory producing MediaFoThe class and the file name should match, so this class […]
Done
File media/capture/video/win/video_capture_device_factory_win.cc:
Patch Set #29, Line 289:
static_cast<DWORD>(MF_SOURCE_READER_FIRST_VIDEO_STREAM),Why this change? You could have left |kFirstVideoStream|.
|kFirstVideoStream| became |kPreferredVideoPreviewStream|:
const DWORD kPreferredVideoPreviewStream = static_cast<DWORD>(MF_CAPTURE_ENGINE_PREFERRED_SOURCE_STREAM_FOR_VIDEO_PREVIEW);
Here I need MF_SOURCE_READER_FIRST_VIDEO_STREAM. This is why.
File media/capture/video/win/video_capture_device_mf_win.cc:
int requested_height = requested_size.height() > 0 ? requested_size.height()
int requested_height = requested_size.height() > 0 ? requested_size.height() […]
Done
type);
retry_count++;Hmmm assigning and comparing on the same statement is strange. […]
Done
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
hr = S_OK;
} else if (riid == IID_IMFCaptureEngineOnEventCallback) {
*object = static_cast<IMFCaptureEngineOnEventCallback*>(this);
hr = S_OK;
}
i
} else if (riid == IID_IMFCaptureEngineOnSampleCallback || […]
Static_cast are not the same between the two cases.
Patch Set #29, Line 354:
OnError(FROM_HERE, E_FAIL);
No need for this guy, just use FROM_HERE everywhere.
Done
Is it OK to continue with the execution of this method […]
It was not continuing since hr is set to E_FAIL and all the remaining code was checking SUCCEEDED(hr).
Nevertheless, with early return, it is clearer.
Prefer early return throughout this method: […]
Done
client_->OnStarted();
is_started_ = true;
}
voiif (is_started && engine_) […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, ready for next round :)
Miguel Casas (Gerrit)
20 comments:
Patch Set #29, Line 938:
Vewd Software AS <*@vewd.com>It seems that you did not see my first answer about this. […]
Ah, ok, ack.
File media/capture/video/video_capture_device_unittest.cc:
// Wrap the TEST_P macro into another one to allow to preprocess test_name
// macros
#define WRAPPED_TEST_P(test_case_name, test_name) \
TEST_P(test_case_name, test_name)
You have multiple test named MAYBE_*. […]
Hmm okay, weird. Would you mind mentioning that
URL here in the comment?
Also: s/test_name/|test_name|/
(variables in comments must be escaped).
File media/capture/video/video_capture_device_unittest.cc:
Patch Set #31, Line 153:
MockMFPhotoCallbackFactorys/MockMFPhotoCallbackFactory/MockPhotoCallbackFactoryMFWin/ ?
std::unique_ptr<IMFCaptureEngineOnSampleCallback> build(
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) {
return std::move(callback_);
}Is this method called? If not, please remove.
Patch Set #31, Line 167:
std::unique_ptr<IMFCaptureEngineOnSampleCallback> callback_;Is this actually needed? If not, please remove.
(You can keep line 784's |callback| alive in the
test case itself).nit: no {} for one-line bodies.
Patch Set #31, Line 790:
MockMFPhotoCallbackFactoryOn second thoughts, and since MockMFPhotoCallbackFactory
doesn't override CreateCallback(), do we actually need
it, or can we just use here a PhotoCallbackFactoryMFWin ?
If yes, this line should be:auto callback_factory =
std::make_unique<PhotoCallbackFactoryMFWin>(std::move(callback));Patch Set #31, Line 813:
.Times(1)Not needed: .Times(1) is the default.
File media/capture/video/win/photo_callback_factory_mf_win.cc:
Patch Set #31, Line 78:
~MFPhotoCallback() {}This is a bit of a personal preference, but Chromium C++'s Dos and Don'ts
recommends using = default; for these cases:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-to-use-defaultPatch Set #31, Line 87:
PhotoCallbackFactoryMFWinThis class is just a wrapper for CreateCallback() and, other
than the |sequence_checker_|, has no state. I wonder if we could
consider:
a) removingf this class
b) moving MFPhotoCallback to an anonymous namespace in
media/capture/video/win/video_capture_device_mf_win.cc
c) adding in the said file a static function
CreateMFPhotoCallback() or some such that would do the
same factory operation as is done here in l.95.WDYT?
Patch Set #31, Line 101:
std::move(format)VideoCaptureFormat is not a move only type, so no
need for std::move().
File media/capture/video/win/video_capture_device_mf_win.cc:
*object = static_cast<IMFCaptureEngineOnSampleCallback*>(this);
hr = S_OK;
} else if (riid == IID_IMFCaptureEngineOnEventCallback) {
*object = static_cast<IMFCaptureEngineOnEventCallback*>(this);
hr = S_OK;
}
i
Static_cast are not the same between the two cases.
Oh, my bad, ack.
File media/capture/video/win/video_capture_device_mf_win.cc:
logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode())
.c_str());
DLOG(ERROR) << log_message;Please use here DPLOG(ERROR); it boils down to PLOG_STREAM() [1]
which automatically appends the GetLastSystemErrorCode() stringified,
see https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Og8-JZlt_ys[1] https://cs.chromium.org/chromium/src/base/logging.h?type=cs&q=DPLOG&sq=package:chromium&l=461
With that and removing the unnecessary parts you could just have
something like:#if !defined(NDEBUG)
void LogError(const Location& from_here, HRESULT hr) {
DPLOG(ERROR) << from_here.ToString() << " hr = "
<< logging::SystemErrorCodeToString(hr);
}
#endif(note that there's no need to say "error" with D{P}LOG(error),
and VideoCaptureDeviceMFWin is already present in the form of
the file name that is prepended by |from_here|.s/0/false/
s/NULL/nullptr/
Patch Set #31, Line 350:
DCHECK_EQ(is_started_, false);DCHECK_EQ(expected, actual);
so this should be
DCHECK_EQ(false, is_started_);0? DWORD is an unsigned int.
Patch Set #31, Line 455:
Location from_here = FROM_HERE;Could you also change this method to be:
hr = ...;
if (FAILED(hr)) {
LogError(FROM_HERE, hr);
return;
}
Otherwise the distance between an error arising and it
being handled is large.Patch Set #31, Line 517:
std::move(format)Same as before, no need for std::move() here.
client_->OnIncomingCapturedData(data, length, capture_video_format_,
rotation, reference_time, timestamp);nit: {} for multi-line bodies.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, ready for next round
18 comments:
// Wrap the TEST_P macro into another one to allow to preprocess |test_name|
// macros
// Needed until https://github.com/google/googletest/issues/389 is fixed
#define WRAPPED_TEST_P(test_case_naHmm okay, weird. Would you mind mentioning that […]
Done
File media/capture/video/video_capture_device_unittest.cc:
s/MockMFPhotoCallbackFactory/MockPhotoCallbackFactoryMFWin/ ?
Done
~MockPhotoCallbackFactoryMFWin() override {}
scoped_refptr<IMFCaptureEngineOnSampleCallback> CreateCallback(
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) override {
return scoped_refptr<IMFCaptureEngineOnSampleCallback>(callback_.release());
Is this method called? If not, please remove.
The method has the wrong name.
It should be called CreateCallback to override its parent.
CreateCallback is called by video_capture_device_mf_win.cc.
It missed virtual keyword on the parent and override on the mock.
FixedPatch Set #31, Line 167:
private:Is this actually needed? If not, please remove. […]
I need it because of CreateCallback
nit: no {} for one-line bodies.
Done
Patch Set #31, Line 790:
que_ptr<MockPhotoCallbackFOn second thoughts, and since MockMFPhotoCallbackFactory […]
We need this because the mock |callback| must be used by MediaFoundation in order to make assertions on |callback| later.
Patch Set #31, Line 813:
EXPECT_CALL(*
Not needed: .Times(1) is the default.
Done
File media/capture/video/win/photo_callback_factory_mf_win.cc:
Patch Set #31, Line 78:
~MFPhotoCallback() =This is a bit of a personal preference, but Chromium C++'s Dos and Don'ts […]
Done
Patch Set #31, Line 87:
PhotoCallbackFactoryMFWin
This class is just a wrapper for CreateCallback() and, other […]
At the beginning of the CL, there was only MFPhotoCallback inside video_capture_device_mf_win.cc. There wasn't PhotoCallbackFactoryMFWin.
Then Christian raised concerns about the fact that we were not sure that MediaFoundation was well behaving with ref counting on the callback. We decided to test the MediaFoundation behaviour.
I pulled out the MFPhotoCallback and added PhotoCallbackFactoryMFWin. This way I could inject a mock MFPhotoCallback registering MediaFoundation behaviour.
I don't see how you could do that with a static factory method.
Patch Set #31, Line 101:
format));VideoCaptureFormat is not a move only type, so no […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
static bool GetFrameSizeFromMediaType(IMFMediaType* type,
Please use here DPLOG(ERROR); it boils down to PLOG_STREAM() [1] […]
Done
s/0/false/
Done
s/NULL/nullptr/
Done
Patch Set #31, Line 350:
OnError(FROM_HERE, E_FAIL);DCHECK_EQ(expected, actual); […]
Done
0? DWORD is an unsigned int.
Done
Could you also change this method to be: […]
Done
Same as before, no need for std::move() here.
Done
base::TimeTicks reference_time,
base::TimeDelta timestamp) {nit: {} for multi-line bodies.
This is multi line because of clang format.
But ok.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Miguel Casas (Gerrit)
1 comment:
File media/capture/video/win/photo_callback_factory_mf_win.cc:
Patch Set #31, Line 87:
PhotoCallbackFactoryMFWinAt the beginning of the CL, there was only MFPhotoCallback inside video_capture_device_mf_win.cc. […]
VideoCaptureDeviceMFWin::photo_callback_factory_ is created
and only held from VideoCaptureDeviceMFWin, so we're not
utilizing its reference counting, right? What I'm proposing
is to have ascoped_refptr<IMFCaptureEngineOnSampleCallback> CreateMFPhotoCallback(
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) {
return scoped_refptr<IMFCaptureEngineOnSampleCallback>(
new MFPhotoCallback(std::move(callback), format));
}in video/win/video_capture_device_mf_win.cc's anonymous
namespace, where MFPhotoCallback would also be, and remove
|photo_callback_factory_|. Then just call this new method
(file-static) from l.523. WDYT?You'd need to square that with the unit tests.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, please take a look at my answer about the photo callback factory. I think I still need clarifications.
1 comment:
Patch Set #31, Line 87:
PhotoCallbackFactoryMFWin
VideoCaptureDeviceMFWin::photo_callback_factory_ is created […]
I am sorry, but I still don't get it.
Each time, the TakePhoto(TakePhotoCallback callback) operation is called, a new IMFCaptureEngineOnSampleCallback is created and consumed by MediaFoundation.
My unit test checks the refcounting on IMFCaptureEngineOnSampleCallback indeed (not on the factory).
Therefore, in my unit test, I need to make MediaFoundation uses MockMFPhotoCallback. I can achieve this through set_photo_callback_factory_for_testing() because I can replace the default PhotoCallbackFactoryMFWin of VideoCaptureDeviceMFWin with MockPhotoCallbackFactoryMFWin because MockPhotoCallbackFactoryMFWin builds MockMFPhotoCallback.If I replace the factory object with a static factory method inside VideoCaptureDeviceMFWin, how can I tell VideoCaptureDeviceMFWin to build and pass MockMFPhotoCallback to MediaFoundation instead of MFPhotoCallback in the context of my unit test ?
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Miguel Casas (Gerrit)
1 comment:
Patch Set #31, Line 87:
PhotoCallbackFactoryMFWin
I am sorry, but I still don't get it. […]
I guess I should elaborate on my "You'd need to square that with the
unit tests.".Add this to video_capture_device_mf_win.*:
- to the public section of the class declaration, and in the appropriate place:
using CreateMFPhotoCallbackCB =
base::Callback(scoped_refptr<IMFCaptureEngineOnSampleCallback>(
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format));void set_photo_callback_factory_for_testing(CreateMFPhotoCallbackCB cb) {
create_mf_photo_callback_ = cb;
}- to the private section of the class declaration:
CreateMFPhotoCallbackCB create_mf_photo_callback_;- to the unnamed namespace of the .cc file:
scoped_refptr<IMFCaptureEngineOnSampleCallback> CreateMFPhotoCallback(
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) {
return scoped_refptr<IMFCaptureEngineOnSampleCallback>(
new MFPhotoCallback(std::move(callback), format));
}
- and finally to the initialization list of the ctor:
create_mf_photo_callback_(base::Bind(&CreateMFPhotoCallback))- and where now you call CreateCallback(), do instead:
create_mf_photo_callback_.Run(std::move(callback), format);
In the unittests:- remove the MockMFPhotoCallbackFactory,
- add a method to VideoCaptureDeviceTest:
#if defined(OS_WIN)
scoped_refptr<IMFCaptureEngineOnSampleCallback> CreateMockPhotoCallback(
MockMFPhotoCallback mock_photo_callback,
VideoCaptureDevice::TakePhotoCallback callback,
VideoCaptureFormat format) override {
return scoped_refptr<IMFCaptureEngineOnSampleCallback>(mock_photo_callback);
}
#endif(note that MockMFPhotoCallback is ref-counted, so please
don't use it via std::unique_ptr<>, it's confusing).- then in the test case itself:
static_cast<VideoCaptureDeviceMFWin*>(device.get())
->set_photo_callback_factory_for_testing(
base::Bind(&VideoCaptureDeviceTest::CreateMockPhotoCallback,
base::Unretained(this),
callback));where we're currying the first parameter:
https://chromium.googlesource.com/chromium/src/+/lkcr/docs/callback.md
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, thank you for the detailed explanations. The photo callback factory has been replaced.
1 comment:
I guess I should elaborate on my "You'd need to square that with the […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Miguel Casas (Gerrit)
Almost there !
10 comments:
File media/capture/video/video_capture_device_unittest.cc:
Patch Set #34, Line 93:
// Needed until https://github.com/google/googletest/issues/389 is fixednit: . at the end of the sentences. I think it could fit in two lines?
STDMETHOD(QueryInterface)(REFIID riid, void** object) override {
return DoQueryInterface(riid, object);
}
STDMETHOD_(ULONG, AddRef)() override { return DoAddRef(); }
STDMETHOD_(ULONG, Release)() override { return DoRelease(); }
STDMETHOD(OnSample)(IMFSample* sample) override { return DoOnSample(sample); }Just curious: can we mock directly these methods,
or is GMock confused by the STDMETHOD() macro ?#endif
#if defined(OS_WIN)#elif defined(OS_WIN)
Patch Set #34, Line 353:
IsWinMediaFoundationWith this name I'd expect to see a comparison of the Win version
or so. What about calling this method UseWinMediaFoundation() ?
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #34, Line 224:
retry_count++;nit: We could do this together with the while() condition.
Patch Set #34, Line 346:
{GUID_ContainerFormatJpeg, PIXEL_FORMAT_MJPEG},I'm a bit confused because you're hijacking this list
and the associated function to do the photo specific
format translation, but IIUC video format translation
in l.245 should never see this GUID_ format and photo format
translation in l.564 should never see the MFVideo...
formats, right?
Instead, remove this and compare explicitly in l.564
photo_media_type.Get() with GUID_ContainerFormatJpeg.Patch Set #34, Line 638:
if (SUCCEEDED(hr)) {if (FAILED(hr) {
LogError(FROM_HERE, hr);
return;
}
here and transform the previous
if (SUCCEEDED(hr)) {
do_something();
from_here = FROM_HERE;
}into
hr = do_something();
if (FAILED(hr)) {
LogError();
bail
}
pleaseDefine HRESULT hr = ...
here instead of in l.758.if (SUCCEEDED(hr)) {
if (event_type == MF_CAPTURE_ENGINE_ERROR)
media_event->GetStatus(&hr);
}if (SUCCEEDED(hr) && event_type == MF_CAPTURE_ENGINE_ERROR) {
media_event->GetStatus(&hr);
}
?
client_->OnError(
from_here,
base::StringPrintf("VideoCaptureDeviceMFWin: %s",
logging::SystemErrorCodeToString(hr).c_str()));Don't modify the LHS, multi-line bodies need {} per
style guide.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, your turn :)
10 comments:
File media/capture/video/video_capture_device_unittest.cc:
Patch Set #34, Line 93:
// fixed.nit: . at the end of the sentences. […]
clang format turns it into 3 lines.
I added the dots.
STDMETHOD(QueryInterface)(REFIID riid, void** object) override {
return DoQueryInterface(riid, object);
}
STDMETHOD_(ULONG, AddRef)() override { return DoAddRef(); }
STDMETHOD_(ULONG, Release)() override { return DoRelease(); }
STDMETHOD(OnSample)(IMFSample* sample) override { return DoOnSample(sample); }
Just curious: can we mock directly these methods, […]
By turning the first mock into:
MOCK_METHOD2(STDMETHOD(QueryInterface), HRESULT(REFIID, void**));
I have an error on MOCK_METHOD2 saying that "'virtual' is not authorized".
#elif defined(OS_WIN)
static_cast<Vid#elif defined(OS_WIN)
Done
Patch Set #34, Line 353:
urn std::get<1>(GetPWith this name I'd expect to see a comparison of the Win version […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #34, Line 224:
if (FAILED(hr)
nit: We could do this together with the while() condition.
Done
Patch Set #34, Line 346:
{MFVideoFormat_YV12, PIXEL_FORMAT_YV12},I'm a bit confused because you're hijacking this list […]
In the photo context, GetFormatFromMediaType() is used to retrieve |pixel_format| AND the |frame_size|. If I remove this "hijack" from FormatFromGuid(), it would return false, therefore I can't use GetFormatFromMediaType() anymore in photo context and have to duplicate code to build a photo VideoCaptureFormat.
Is that what you expect?
if (FAILED(hr) { […]
Done
Define HRESULT hr = ... […]
Done
if (SUCCEEDED(hr) && event_type == MF_CAPTURE_ENGINE_ERROR)
media_event->GetStatus(&hr);
if (SUCCEEDED(hr) && event_type == MF_CAPTURE_ENGINE_ERROR) { […]
I did that to let the scope open for other event_type specific processing. But ok.
(
from_here,
base::StringPrintf("VideoCaptureDeviceMFWin: %s",
logging::SystemErrorCodeToString(hr).c_str()));
}
Don't modify the LHS, multi-line bodies need {} per […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Miguel Casas (Gerrit)
Last round.
9 comments:
STDMETHOD(QueryInterface)(REFIID riid, void** object) override {
return DoQueryInterface(riid, object);
}
STDMETHOD_(ULONG, AddRef)() override { return DoAddRef(); }
STDMETHOD_(ULONG, Release)() override { return DoRelease(); }
STDMETHOD(OnSample)(IMFSample* sample) override { return DoOnSample(sample); }
By turning the first mock into: […]
Yeah, STDMETHOD(x) expands to sth like [1]
virtual HRESULT calltype x()
so the mock should be, if anything:
MOCK_METHOD2(QueryInterface, HRESULT(REFIID, void**));
because MOCK_METHODn is essentially an override operation.
[1] https://msdn.microsoft.com/en-us/library/ms693342(VS.85).aspx
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #35, Line 75:
photo_callback_factorys/photo_callback_factory/create_mf_photo_callback/
mutators and accessors follow the name of the members
they touch.
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #34, Line 346:
{MFVideoFormat_YV12, PIXEL_FORMAT_YV12},In the photo context, GetFormatFromMediaType() is used to retrieve |pixel_format| AND the |frame_siz […]
Ack
File media/capture/video/win/video_capture_device_mf_win.cc:
for (const CapabilityWin& capability : capabilities) {
int height = capability.supported_format.frame_size.height();
int width = capability.supported_format.frame_size.width();
if (std::abs(height - requested_height) <=
std::abs(height -
best_match->supported_format.frame_size.height()) &&
std::abs(width - requested_width) <=
std::abs(width - best_match->supported_format.frame_size.width())) {
best_match = &capability;
}
}Suggest:
for (const CapabilityWin& capability : capabilities) {
const int height = capability.supported_format.frame_size.height();
const int width = capability.supported_format.frame_size.width();
const int best_height = best_match->supported_format.frame_size.height();
const int best_width = best_match->supported_format.frame_size.width();
if (std::abs(height - requested_height) <= std::abs(height - best_height) &&
std::abs(width - requested_width) <= std::abs(width - best_width)) {
best_match = &capability;
}
}Patch Set #35, Line 228:
++retry_count && retry_count < 200In one operation?
retry_count++ < 200= 0;
= 0; ?
Patch Set #35, Line 606:
.Get()No need for .Get()
Also, sometimes you compare/dcheck with Get() (e.g. l.425, l.376)
and sometimes without. I'd use without throughout.int min_height = current_size.height();
int current_height = current_size.height();
int max_height = current_size.height();
int min_width = current_size.width();
int current_width = current_size.width();
int max_width = current_size.width();
for (const CapabilityWin& capability : capabilities) {
int height = capability.supported_format.frame_size.height();
max_height = std::max(height, max_height);
min_height = std::min(height, min_height);
int width = capability.supported_format.frame_size.width();
max_width = std::max(width, max_width);
min_width = std::min(width, min_width);
}
photo_capabilities->height =
mojom::Range::New(max_height, min_height, current_height, 1);
photo_capabilities->width =
mojom::Range::New(max_width, min_width, current_width, 1);Suggest:
gfx::Size min_size = current_size;
gfx::Size max_size = current_size;
for (const CapabilityWin& capability : capabilities) {
min_size.SetToMin(capability.supported_format.frame_size);
max_size.SetToMax(capability.supported_format.frame_size);
}
photo_capabilities->height = mojom::Range::New(
max_size.height(), min_size.height(), current_size.height(), 1);
photo_capabilities->width = mojom::Range::New(
max_size.width(), min_size.width(), current_size.width(), 1);
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, everything fixed
7 comments:
File media/capture/video/win/video_capture_device_mf_win.h:
Patch Set #35, Line 75:
create_mf_photo_callbas/photo_callback_factory/create_mf_photo_callback/ […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
for (const CapabilityWin& capability : capabilities) {
int height = capability.supported_format.frame_size.height();
int width = capability.supported_format.frame_size.width();
int best_height = best_match->supported_format.frame_size.height();
int best_width = best_match->supported_format.frame_size.width();
if (std::abs(height - requested_height) <= std::abs(height - best_height) &&
std::abs(width - requested_width) <= std::abs(width - best_width)) {
best_match = &capability;
}
}
Suggest: […]
Done
Patch Set #35, Line 228:
retry_count++ < 200);In one operation? […]
Done
Done
= 0; ?
Done
No need for .Get() […]
Done
gfx::Size max_size = current_size;
for (const CapabilityWin& capability : capabilities) {
min_size.SetToMin(capability.supported_format.frame_size);
max_size.SetToMax(capability.supported_format.frame_size);
}
photo_capabilities->height = mojom::Range::New(
max_size.height(), min_size.height(), current_size.height(), 1);
photo_capabilities->width = mojom::Range::New(
max_size.width(), min_size.width(), current_size.width(), 1);
photo_capabilities->exposure_compensation = mojom::Range::New();
photo_capabilities->color_temperature = mojom::Range::New();
photo_capabilities->iso = mojom::Range::New();
photo_capabilities->brightness = mojom::Range::New();
photo_capabilities->contrast = mojom::Range::New();
photo_capabilities->saturation = mojom::Range::New();
photo_capabilities->sharpness = mojom::Range::New();
photo_capabilities->zoom = mojom::Range::New();
photo_capabilities->red_eye_reduction = mojom::RedEyeReduction::NEVER;Suggest: […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, just added copy operations of gfx::Size before modification
Miguel Casas (Gerrit)
lgtm! \o/
Patch set 37:Code-Review +1Commit-Queue +1
3 comments:
Patch Set #37, Line 7:
Replace IMFSourceReader with IMFCaptureEngine in VideoCapture MediaFoundation implementationSuggestion:
"Win video capture: use IMFCaptureEngine for Media Foundation"
or some such, so QA folks and sheriffs taking a look at this
CL, possibly bundled with a ton of others, can quickly scan
the name and decide if this CL is guilty of any breakage or
regression.- 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 IMFCaptureEngineNit: reflow to 80 chars wide.
MediaFoundation implementation is still hidden behind a flag.
You will need to enable "--force-mediafoundation" to test this.
Later, it would be nice to see if we still need this flag.Instead, write how you tested this, e.g.
TEST=adapted video_capture_device_unittest.cc and/or
webrtc_image_capture_browsertest.cc; launch Chrome with
--force-mediafoundation on Win7+ and capture video using
e.g. https://webrtc.github.io/samples/src/content/getusermedia/gum/The idea being to help future readers of this CL to
understand how you validated your changes/what was supposed
to be accomplished.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Réda Housni Alaoui uploaded patch set #39 to this change.
Win video capture: use IMFCaptureEngine for Media Foundation
- 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: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
---
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, 794 insertions(+), 176 deletions(-)
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
mcasas@, commit message fixed.
Also, I think I fixed the compilation error.
Can you resubmit to Tryjobs?
3 comments:
Patch Set #37, Line 7:
Win video capture: use IMFCaptureEngine for Media FoundationSuggestion: […]
Done
- Full rewrite of the MediaFoundation implementation video part to use
IMFCaptureEngine
- Implementation of takePhoto, setPhotoOptions and getPhotoCapabilities
Nit: reflow to 80 chars wide.
Done
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
Instead, write how you tested this, e.g. […]
Done
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
InitializeMediaFoundation() crashes the test.
Of course I do not have the issue on both of my machines.
I am investigating.
[ RUN ] VideoCaptureDeviceTests/VideoCaptureDeviceTest.AllocateBadSize/1
Received fatal exception 0xc06d007e
Backtrace:
RaiseException [0x00007FF9F9CD7788+68]
__delayLoadHelper2 [0x00007FF7FAFD6BB8+1b8] (f:\dd\vctools\delayimp\delayhlp.cpp:143)
_tailMerge_MFPlat_DLL [0x00007FF7FAFA27B5+3f]
media::InitializeMediaFoundation [0x00007FF7FAF014BF+df]
media::VideoCaptureDeviceFactoryWin::`scalar deleting destructor'
[0x00007FF7FAA4F5E0+40]
media::VideoCaptureDeviceFactoryWin::GetDeviceDescriptors [0x00007FF7FAA4E4F2+122]
media::VideoCaptureDeviceTest::FindUsableDeviceDescriptor [0x00007FF7FA9FFBD7+37]
media::VideoCaptureDeviceTest_AllocateBadSize_Test::TestBody [0x00007FF7FAA00430+30]
testing::Test::Run [0x00007FF7FAA3F737+b7]
testing::TestInfo::Run [0x00007FF7FAA3FFB3+d3]
testing::TestCase::Run [0x00007FF7FAA403AA+fa]
testing::internal::UnitTestImpl::RunAllTests [0x00007FF7FAA44C75+275]
testing::UnitTest::Run [0x00007FF7FAA44932+a2]
base::TestSuite::Run [0x00007FF7FAF20733+73]
base::LaunchUnitTests [0x00007FF7FAF2179F+af]
main [0x00007FF7FAF20572+8e]
__scrt_common_main_seh [0x00007FF7FAFA31D9+11d]
(f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:11b)
BaseThreadInitThunk [0x00007FF9FD408364+14]
RtlUserThreadStart [0x00007FF9FD785E91+21]
Réda Housni Alaoui (Gerrit)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.
1 comment:
File media/capture/video/win/video_capture_device_factory_win.cc:
Patch Set #40, Line 88:
!VideoCaptureDeviceFactoryWin::PlatformSupportsMediaFoundation() ||Added a check on MediaFoundation dll presence before MF init.
It should fix the build failure.
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Réda Housni Alaoui (Gerrit)
Patch Set 40:
(1 comment)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.
I found https://bugs.chromium.org/p/chromium/issues/detail?id=411227 that relates the same issue.
Miguel Casas (Gerrit)
ping chfremer@
1 comment:
Patch Set #40, Line 88:
!VideoCaptureDeviceFactoryWin::PlatformSupportsMediaFoundation() ||
Added a check on MediaFoundation dll presence before MF init. […]
This was a correct step.
chfremer@, what's the plan to enable testing in bots with MF ?
To view, visit change 734042. To unsubscribe, or for help writing mail filters, visit settings.
Christian Fremerey (Gerrit)
Patch Set 40:
(1 comment)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.
I am surprised that a Win10 machine does not have MediaFoundation. I assumed that IMFCaptureEngine would be available on all Windows version 8 or higher.
A quick web search revealed that some versions of Win10, e.g an "N" version, may not come with Media Foundation [1], which kind of kills any hope to be able to eventually retire the DirectShow capture path.
mcasas@: Who would be the right person to ask which exact version of Win10 is installed on that bot?
Réda Housni Alaoui (Gerrit)
Patch Set 40:
Patch Set 40:
(1 comment)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.I am surprised that a Win10 machine does not have MediaFoundation. I assumed that IMFCaptureEngine would be available on all Windows version 8 or higher.
A quick web search revealed that some versions of Win10, e.g an "N" version, may not come with Media Foundation [1], which kind of kills any hope to be able to eventually retire the DirectShow capture path.
mcasas@: Who would be the right person to ask which exact version of Win10 is installed on that bot?
I am as surprised as you.
I thought this kn edition thing was from the past.
I think we can still check for dlls presence instead of Windows version to decide which implementation to use.
Christian Fremerey (Gerrit)
Patch Set 40:
Patch Set 40:
Patch Set 40:
(1 comment)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.I am surprised that a Win10 machine does not have MediaFoundation. I assumed that IMFCaptureEngine would be available on all Windows version 8 or higher.
A quick web search revealed that some versions of Win10, e.g an "N" version, may not come with Media Foundation [1], which kind of kills any hope to be able to eventually retire the DirectShow capture path.
mcasas@: Who would be the right person to ask which exact version of Win10 is installed on that bot?
I am as surprised as you.
I thought this kn edition thing was from the past.I think we can still check for dlls presence instead of Windows version to decide which implementation to use.
I filed a ticket [1] on infra troopers to see if we can figure out which exact version of Windows is installed on this builder.
Yes, falling back to DirectShow when the DLLs are not found would be an option, but IMO not a good one. I think this is okay for the tests for now, so that we can move forward with landing this CL, but we need clarity on which versions of Windows are out there that do not have Media Foundation or at least not in the way we use it right now.
I created issue [2] to track the work for figuring out and enabling the tests.
reda@, please refer to the bug url in a comment in the test to remind us to enable it when this is resolved.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=791610
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=791615
Réda Housni Alaoui (Gerrit)
Patch Set 40:
Patch Set 40:
Patch Set 40:
Patch Set 40:
(1 comment)
mcasas@, I think win10_chromium_x64_rel_ng is missing MediaFoundation dlls.
I added a dll check to make the build pass.
But it also means that MF will not be tested by win10_chromium_x64_rel_ng bot.I am surprised that a Win10 machine does not have MediaFoundation. I assumed that IMFCaptureEngine would be available on all Windows version 8 or higher.
A quick web search revealed that some versions of Win10, e.g an "N" version, may not come with Media Foundation [1], which kind of kills any hope to be able to eventually retire the DirectShow capture path.
mcasas@: Who would be the right person to ask which exact version of Win10 is installed on that bot?
I am as surprised as you.
I thought this kn edition thing was from the past.I think we can still check for dlls presence instead of Windows version to decide which implementation to use.
I filed a ticket [1] on infra troopers to see if we can figure out which exact version of Windows is installed on this builder.
Yes, falling back to DirectShow when the DLLs are not found would be an option, but IMO not a good one. I think this is okay for the tests for now, so that we can move forward with landing this CL, but we need clarity on which versions of Windows are out there that do not have Media Foundation or at least not in the way we use it right now.
I created issue [2] to track the work for figuring out and enabling the tests.
reda@, please refer to the bug url in a comment in the test to remind us to enable it when this is resolved.[1] https://bugs.chromium.org/p/chromium/issues/detail?id=791610
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=791615
chfremer@, in the current state of the CL (Patchset 40) the test can stay enabled.
Since I added a check on the dlls presence, it shouldn't fail even on win10_chromium_x64_rel_ng.
Do you want me to disable the test and add a comment anyway ?
Christian Fremerey (Gerrit)
Oh, sorry for being unclear. By "enabled" I meant making sure that the test takes the MediaFoundation code path, as we would expect on a Win10 machine.
Not sure where a good place to add a comment for this would be, but if there is no better option, we could add it here: https://chromium-review.googlesource.com/c/chromium/src/+/734042/39..40/media/capture/video/win/video_capture_device_factory_win.cc#88
Réda Housni Alaoui (Gerrit)
chfremer@, mcasas@, the comment has been added.
Can we submit to CQ again?
Christian Fremerey (Gerrit)
Patch set 41:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Add reference to https://bugs.chromium.org/p/chromium/issues/detail?id=791615" https://chromium-review.googlesource.com/c/734042/41
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/734042/41
Bot data: {"action": "start", "triggered_at": "2017-12-04T19:15:21.0Z", "cq_cfg_revision": "b547f51ef97353cccb06eebcda71133909f61295", "revision": "45f667e36f297bb8d910e9cc0c9a66b5266c8a2b"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Win video capture: use IMFCaptureEngine for Media Foundation
- 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: Ib8e7f475d8120a63dd08c7b215c1eaf2c6f3d800
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-Commit-Position: refs/heads/master@{#521435}
---
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, 800 insertions(+), 177 deletions(-)