Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362333002:
Reland: Image Capture Linux/CrOs: wire some capabilities set/get (Closed)
|
Created:
4 years, 3 months ago by mcasas Modified:
4 years, 2 months ago Reviewers:
chfremer CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland: Image Capture Linux/CrOs: wire some capabilities set/get
The original CL was reverted due to some unit test
error in large bots. An improved error logging was
introduced in https://crrev.com/2359243002, and this
CL adds more of that, plus waiting for the correct
photo-related event to happen -- as opposed to before,
where the test cases waited on WaitForCapturedFrame().
Original CL description -----------------------------------------------
Image Capture Linux/CrOs: wire some capabilities set/get
This CL implements for Linux/Cros parts of the methods:
- VideoCaptureDeviceWin::GetPhotoCapabilities():
zoom, autofocus on/off, autoexposure on/off, and
auto whitebalance on/off
- VideoCaptureDeviceWin::SetPhotoOptions(): zoom
and adds a small VideoCaptureDeviceTest test case
to retrieve capabilities, enabled for Linux (should be
enabled for Android in another CL).
BUG=647352
Committed: https://crrev.com/ebcc9794508eb9a52a8fcee54b93a0794a61f6a8
Cr-Commit-Position: refs/heads/master@{#421031}
Patch Set 1 : Verbatim https://codereview.chromium.org/2349693003/ #Patch Set 2 : Added more condition Checks and cleaned up unit tests thread-hopping #
Total comments: 26
Patch Set 3 : chfremer@ comments #Patch Set 4 : second round of comments from chfremer@ #Patch Set 5 : Removed unnecessary RunLoop() after AllocateAndStart() #Patch Set 6 : VideoCaptureCamera2: do not return error if stopCapture() finds the CameraCaptureSession closed #
Messages
Total messages: 20 (10 generated)
Description was changed from ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ========== to ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get Also note the improved OS error logging after https://crrev.com/2359243002 Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ==========
Description was changed from ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get Also note the improved OS error logging after https://crrev.com/2359243002 Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ========== to ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get The original CL was reverted due to some unit test error in large bots. An improved error logging was introduced in https://crrev.com/2359243002, and this CL adds more of that, plus waiting for the correct photo-related event to happen -- as opposed to before, where the test cases waited on WaitForCapturedFrame(). Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ==========
mcasas@chromium.org changed reviewers: + chfremer@chromium.org
chfremer@ PTAL at PS1 plz
https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:335: return; This error case silently swallows the call and does not invoke the callback. If it is an unexpected condition it should at least have a DCHECK(false). If it is an expected condition it should report it back to the caller. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:390: photo_capabilities->red_eye_reduction = false; Does the spec allow these "empty ranges" as way of saying "not supported"? https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:399: return; Same as above. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:404: zoom_current.value = settings->zoom / 100; If the result of the division is a float, please write 100.0f (or 100.0 for double), so that readers can understand without having to look up the type of settings->zoom. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.h (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:33: class V4L2CaptureDelegate final Question/suggestion unrelated to this CL: Why is this not an implementation of media::VideoCaptureDevice? It seems the class VideoCaptureDeviceLinux forward all calls to an instance of this class on a different thread. If V4L2CaptureDelegate would implement media::VideoCaptureDevice and VideoCaptureDeviceLinux would depend on the interface instead of the concrete V4L2CaptureDelegate, then this would be the decorator pattern. This would help readers understand the role and contract of V4L2CaptureDelegate. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:62: void SetPhotoOptions(mojom::PhotoSettingsPtr settings, nit: photo options vs. photo settings, could be made consistent https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:175: OnCorrectPhotoTaken(); The expectation on the arriving data is expressed here, while the expectation on the call count is expressed in the actual test cases. This scattering of expectation/verification code between here and the actual test cases makes it hard to understand what is actually verified when reading the code for the test case. As an example, when reading the code for test case MAYBE_TakePhoto it is non-obvious that there is verification happening beyond the expectations set on the call count of OnCorrectPhotoTaken. Would it be possible to express all expectations in one place, i.e. the test case? https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:546: // Starts the camera and takes a photo. This description is not really an answer to the question "What does this test?". It is a description of the exercise, but does not say what is verified. I would think something along the lines of "Tests that TakePhotoCallback is invoked with valid data when calling TakePhoto() on an active capture device." https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:571: WaitForCapturedFrame(); The description of the interface media::VideoCaptureDevice, does not state that it is required to wait for the first captured frame before invoking TakePhoto(). If it is required, we need to update the description. If it is not required, the test should not do it. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:594: return; I assume we do this to not fail on machines that do not have devices attached. But this way it will report the test as passed even though it really was skipped. Is there a way in gtest to bail out of a test and have it marked as skipped?
ptal https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:335: return; On 2016/09/23 21:18:29, chfremer wrote: > This error case silently swallows the call and does not invoke the callback. > If it is an unexpected condition it should at least have a DCHECK(false). > If it is an expected condition it should report it back to the caller. The callback is not a simple thing, but a ScopedResultCallback [1]. which is correctly handled when dropped, IOW, dropping it is the right handling. [1] https://cs.chromium.org/chromium/src/media/capture/video/scoped_result_callba... https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:390: photo_capabilities->red_eye_reduction = false; On 2016/09/23 21:18:29, chfremer wrote: > Does the spec allow these "empty ranges" as way of saying "not supported"? Yes, mojom::Range::New() essentially makes all the fields 0, which will be interpreted as min == max == current == 0. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:399: return; On 2016/09/23 21:18:29, chfremer wrote: > Same as above. Acknowledged. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:404: zoom_current.value = settings->zoom / 100; On 2016/09/23 21:18:29, chfremer wrote: > If the result of the division is a float, please write 100.0f (or 100.0 for > double), so that readers can understand without having to look up the type of > settings->zoom. Ridiculously enough, the setting (and the underlying USB field) are integer, so throwing away the decimals will happen either here or inside the ioctl implementation. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.h (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:33: class V4L2CaptureDelegate final On 2016/09/23 21:18:29, chfremer wrote: > Question/suggestion unrelated to this CL: > Why is this not an implementation of media::VideoCaptureDevice? > It seems the class VideoCaptureDeviceLinux forward all calls to an instance of > this class on a different thread. If V4L2CaptureDelegate would implement > media::VideoCaptureDevice and VideoCaptureDeviceLinux would depend on the > interface instead of the concrete V4L2CaptureDelegate, then this would be the > decorator pattern. > > This would help readers understand the role and contract of V4L2CaptureDelegate. History. It used to be inside VideoCaptureDeviceLinux, but then 2 V4L2 implementations came around, one for single and one for multiplanar versions. The latter was removed, and things never consolidated. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:62: void SetPhotoOptions(mojom::PhotoSettingsPtr settings, On 2016/09/23 21:18:29, chfremer wrote: > nit: photo options vs. photo settings, could be made consistent They follow the idl names, which in turn come from the JS spec. [1], [2]. You can file a spec-bug! :-) https://github.com/w3c/mediacapture-image/issues/new [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagec... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagec... https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:175: OnCorrectPhotoTaken(); On 2016/09/23 21:18:30, chfremer wrote: > The expectation on the arriving data is expressed here, while the expectation on > the call count is expressed in the actual test cases. > This scattering of expectation/verification code between here and the actual > test cases makes it hard to understand what is actually verified when reading > the code for the test case. > > As an example, when reading the code for test case MAYBE_TakePhoto it is > non-obvious that there is verification happening beyond the expectations set on > the call count of OnCorrectPhotoTaken. > > Would it be possible to express all expectations in one place, i.e. the test > case? Yeah, but then we'd have to duplicate this header checking-code, which will in turn force us to put it in a static method, which will bring us back to the beginning. I like to see it as two different verifications: One when we received a photo Blob, make sure it has the appropriate type, and two, expect a certain amount of photo blob callbacks. I can envision cases in which photo blobs are expected, but have the wrong format, and also a case in which a received blob has the right format, but is unexpected. (Note that these are EXPECT, not asserts, so they would "accumulate" as failures, without stopping the test case on the spot). https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:546: // Starts the camera and takes a photo. On 2016/09/23 21:18:30, chfremer wrote: > This description is not really an answer to the question "What does this test?". > It is a description of the exercise, but does not say what is verified. > > I would think something along the lines of "Tests that TakePhotoCallback is > invoked with valid data when calling TakePhoto() on an active capture device." Done. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:571: WaitForCapturedFrame(); On 2016/09/23 21:18:29, chfremer wrote: > The description of the interface media::VideoCaptureDevice, does not state that > it is required to wait for the first captured frame before invoking TakePhoto(). > > If it is required, we need to update the description. > If it is not required, the test should not do it.' Hmm well, it kinda says so: "...and/or interrupting the capture flow", plus the JS side prevents creating an ImageCapturer if the MediaStream(Video)Track is not active, or muted, so the device has to be capturing. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:594: return; On 2016/09/23 21:18:29, chfremer wrote: > I assume we do this to not fail on machines that do not have devices attached. > But this way it will report the test as passed even though it really was > skipped. Is there a way in gtest to bail out of a test and have it marked as > skipped? Hmm that's a good suggestion. This behaviour is pervasive among WebRtc tests, because many bots don't have any camera. I would love to drop off in the SetUp() version of the test, in that case not logging the test's name on the console, but I'm not sure how to do it. I'll investigate. Update: investigated and found out that there is no runtime test-skipping features [1]. The only way to skip it is to mark the test case name with a DISABLED_ prefix. [1] https://github.com/google/googletest/issues/490
lgtm % nits in comments https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:335: return; On 2016/09/23 22:10:57, mcasas wrote: > On 2016/09/23 21:18:29, chfremer wrote: > > This error case silently swallows the call and does not invoke the callback. > > If it is an unexpected condition it should at least have a DCHECK(false). > > If it is an expected condition it should report it back to the caller. > > The callback is not a simple thing, but a > ScopedResultCallback [1]. which is correctly handled > when dropped, IOW, dropping it is the right > handling. > > [1] > https://cs.chromium.org/chromium/src/media/capture/video/scoped_result_callba... Thanks. Now I remember you mentioned that to me before. I like this mechanism. Only downside is that because of the typedef it is somewhat non-obvious whether or not |callback| is a ScopedResultCallback or a plain Callback. I will look out for it next time. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:404: zoom_current.value = settings->zoom / 100; On 2016/09/23 22:10:57, mcasas wrote: > On 2016/09/23 21:18:29, chfremer wrote: > > If the result of the division is a float, please write 100.0f (or 100.0 for > > double), so that readers can understand without having to look up the type of > > settings->zoom. > > Ridiculously enough, the setting (and the > underlying USB field) are integer, so throwing > away the decimals will happen either here or > inside the ioctl implementation. Seems I misinterpreted the value 100 by assuming the range of settings->zoom would be [0:100] and the range of zoom_current.value would be [0,1]. In that case, could you please use a named constexpr instead of the magic number to help me as a reader? https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:175: OnCorrectPhotoTaken(); On 2016/09/23 22:10:57, mcasas wrote: > On 2016/09/23 21:18:30, chfremer wrote: > > The expectation on the arriving data is expressed here, while the expectation > on > > the call count is expressed in the actual test cases. > > This scattering of expectation/verification code between here and the actual > > test cases makes it hard to understand what is actually verified when reading > > the code for the test case. > > > > As an example, when reading the code for test case MAYBE_TakePhoto it is > > non-obvious that there is verification happening beyond the expectations set > on > > the call count of OnCorrectPhotoTaken. > > > > Would it be possible to express all expectations in one place, i.e. the test > > case? > > Yeah, but then we'd have to duplicate this header > checking-code, which will in turn force us to put > it in a static method, which will bring us back > to the beginning. > > I like to see it as two different verifications: > One when we received a photo Blob, make sure it has > the appropriate type, and two, expect a certain > amount of photo blob callbacks. I can envision > cases in which photo blobs are expected, but have > the wrong format, and also a case in which a > received blob has the right format, but is unexpected. > (Note that these are EXPECT, not asserts, so they > would "accumulate" as failures, without stopping > the test case on the spot). Okay to keep it the way it is, especially since it is pretty commonly done this way. I was thinking about having a static method named "VerifyDataHasExpectedFormat()" and then attaching that to the EXPECT_CALL in the test case using Invoke(). This would make it obvious to the reader that this verification is happening. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:571: WaitForCapturedFrame(); On 2016/09/23 22:10:57, mcasas wrote: > On 2016/09/23 21:18:29, chfremer wrote: > > The description of the interface media::VideoCaptureDevice, does not state > that > > it is required to wait for the first captured frame before invoking > TakePhoto(). > > > > If it is required, we need to update the description. > > If it is not required, the test should not do it.' > > Hmm well, it kinda says so: > "...and/or interrupting the capture flow", plus the > JS side prevents creating an ImageCapturer if the > MediaStream(Video)Track is not active, or muted, so > the device has to be capturing. > As per discussion on IM, I think we should remove the WaitForCaptureFrame(), since actual clients do not have to wait, and we want to cover that scenario with our test. Let us add a TODO for this, so we can move forward with this CL.
https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:404: zoom_current.value = settings->zoom / 100; On 2016/09/23 22:48:56, chfremer wrote: > On 2016/09/23 22:10:57, mcasas wrote: > > On 2016/09/23 21:18:29, chfremer wrote: > > > If the result of the division is a float, please write 100.0f (or 100.0 for > > > double), so that readers can understand without having to look up the type > of > > > settings->zoom. > > > > Ridiculously enough, the setting (and the > > underlying USB field) are integer, so throwing > > away the decimals will happen either here or > > inside the ioctl implementation. > > Seems I misinterpreted the value 100 by assuming the range of settings->zoom > would be [0:100] and the range of zoom_current.value would be [0,1]. > In that case, could you please use a named constexpr instead of the magic number > to help me as a reader? |kZoomMultiplier| it is. https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2362333002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:571: WaitForCapturedFrame(); On 2016/09/23 22:48:56, chfremer wrote: > On 2016/09/23 22:10:57, mcasas wrote: > > On 2016/09/23 21:18:29, chfremer wrote: > > > The description of the interface media::VideoCaptureDevice, does not state > > that > > > it is required to wait for the first captured frame before invoking > > TakePhoto(). > > > > > > If it is required, we need to update the description. > > > If it is not required, the test should not do it.' > > > > Hmm well, it kinda says so: > > "...and/or interrupting the capture flow", plus the > > JS side prevents creating an ImageCapturer if the > > MediaStream(Video)Track is not active, or muted, so > > the device has to be capturing. > > > > As per discussion on IM, I think we should remove the WaitForCaptureFrame(), > since actual clients do not have to wait, and we want to cover that scenario > with our test. > > Let us add a TODO for this, so we can move forward with this CL. Oh hark. Let me explain: from the ImageCapture POV, there's no need to wait for the first incoming capture frame; however VideoCaptureDeviceTest::OnFrameCaptured() does try to run_loop_->QuitClosure() and |run_loop_| is allocated... in WaitForCaptureFrame(). So: I'll protect OnFrameCaptured() access to |run_loop_|, and here just do a RunUntilIdle() in a local RunLoop, with a comment on what we want by this.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chfremer@chromium.org Link to the patchset: https://codereview.chromium.org/2362333002/#ps80001 (title: "Removed unnecessary RunLoop() after AllocateAndStart()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chfremer@chromium.org Link to the patchset: https://codereview.chromium.org/2362333002/#ps100001 (title: "VideoCaptureCamera2: do not return error if stopCapture() finds the CameraCaptureSession closed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get The original CL was reverted due to some unit test error in large bots. An improved error logging was introduced in https://crrev.com/2359243002, and this CL adds more of that, plus waiting for the correct photo-related event to happen -- as opposed to before, where the test cases waited on WaitForCapturedFrame(). Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ========== to ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get The original CL was reverted due to some unit test error in large bots. An improved error logging was introduced in https://crrev.com/2359243002, and this CL adds more of that, plus waiting for the correct photo-related event to happen -- as opposed to before, where the test cases waited on WaitForCapturedFrame(). Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get The original CL was reverted due to some unit test error in large bots. An improved error logging was introduced in https://crrev.com/2359243002, and this CL adds more of that, plus waiting for the correct photo-related event to happen -- as opposed to before, where the test cases waited on WaitForCapturedFrame(). Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 ========== to ========== Reland: Image Capture Linux/CrOs: wire some capabilities set/get The original CL was reverted due to some unit test error in large bots. An improved error logging was introduced in https://crrev.com/2359243002, and this CL adds more of that, plus waiting for the correct photo-related event to happen -- as opposed to before, where the test cases waited on WaitForCapturedFrame(). Original CL description ----------------------------------------------- Image Capture Linux/CrOs: wire some capabilities set/get This CL implements for Linux/Cros parts of the methods: - VideoCaptureDeviceWin::GetPhotoCapabilities(): zoom, autofocus on/off, autoexposure on/off, and auto whitebalance on/off - VideoCaptureDeviceWin::SetPhotoOptions(): zoom and adds a small VideoCaptureDeviceTest test case to retrieve capabilities, enabled for Linux (should be enabled for Android in another CL). BUG=647352 Committed: https://crrev.com/ebcc9794508eb9a52a8fcee54b93a0794a61f6a8 Cr-Commit-Position: refs/heads/master@{#421031} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ebcc9794508eb9a52a8fcee54b93a0794a61f6a8 Cr-Commit-Position: refs/heads/master@{#421031} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
