Image Capture: wire setPhotoOptions() for Win

Image Capture: wire setPhotoOptions() for Win

This CL wires the photo capabilities setPhotoOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivial updates to the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setPhotoOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128
Review-Url: https://codereview.chromium.org/2873143002
Cr-Commit-Position: refs/heads/master@{#471554}
Committed: https://chromium.googlesource.com/chromium/src/+/86adb2c88ba82194458ac09d078a00616c0b22dd

[mcasas, 2017-05-10 01:35:44]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-05-10 01:50:32]

Description was changed from

==========
Image Capture: wire setOptions() for Win



Made |camera_control_| and |video_control_| member variables

BUG=657128
==========

to

==========
Image Capture: wire setOptions() for Win

This CL wires the photo capabilities setOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivially updating the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128
==========

[mcasas, 2017-05-10 17:53:02]

mcasas@chromium.org changed reviewers:
+ robliao@chromium.org

[mcasas, 2017-05-10 17:53:03]

robliao@ PTAL

[robliao, 2017-05-11 05:14:07]

https://codereview.chromium.org/2873143002/diff/20001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2873143002/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:243: // 
DCHECK_NE(used_from_thread_id_, base::kInvalidThreadId)
Should this code instead be deleted or is this an accidental comment out?

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:470: hr =
capture_filter_.CopyTo(info.Receive());
Conflict warning: Receive is being renamed to GetAddressOf(). Monitor when
https://codereview.chromium.org/2870263002/ lands.

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:728:
ScopedComPtr<IAMVideoControl> video_control_;
This declaration shadows the instance variable video_control_ initialized in
AllocateAndStart(). Is this supposed to use that pointer or be a temporary here?

[mcasas, 2017-05-12 18:51:46]

Patchset #2 (id:40001) has been deleted

[mcasas, 2017-05-12 18:52:49]

ptal!

https://codereview.chromium.org/2873143002/diff/20001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2873143002/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:243: // 
DCHECK_NE(used_from_thread_id_, base::kInvalidThreadId)
On 2017/05/11 05:14:07, robliao wrote:
> Should this code instead be deleted or is this an accidental comment out?

Ooops, unintended upload, this was a workaround for

https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:470: hr =
capture_filter_.CopyTo(info.Receive());
On 2017/05/11 05:14:07, robliao wrote:
> Conflict warning: Receive is being renamed to GetAddressOf(). Monitor when
> https://codereview.chromium.org/2870263002/ lands.

gotcha! Will rebase.

https://codereview.chromium.org/2873143002/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:728:
ScopedComPtr<IAMVideoControl> video_control_;
On 2017/05/11 05:14:07, robliao wrote:
> This declaration shadows the instance variable video_control_ initialized in
> AllocateAndStart(). Is this supposed to use that pointer or be a temporary
here?

Oops, unintended change.

[robliao, 2017-05-12 19:34:30]

A few more questions:

Nit: setOptions() -> setPhotoOptions() in description

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:492: i,
IID_PPV_ARGS(camera_control_.GetAddressOf()));
IID_PPV_ARGS(&camera_control) should continue to work. Are you running into
problem with this?

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:698: callback.Run(true);
Should this be called synchronously or should it be posted to a task runner?
What's the goal of this callback?

[mcasas, 2017-05-12 21:26:56]

Description was changed from

==========
Image Capture: wire setOptions() for Win

This CL wires the photo capabilities setOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivially updating the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128
==========

to

==========
Image Capture: wire setPhotoOptions() for Win

This CL wires the photo capabilities setPhotoOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivial updates to the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setPhotoOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128
==========

[mcasas, 2017-05-12 21:27:04]

ptal

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:492: i,
IID_PPV_ARGS(camera_control_.GetAddressOf()));
On 2017/05/12 19:34:30, robliao wrote:
> IID_PPV_ARGS(&camera_control) should continue to work. Are you running into
> problem with this?

Oops, wrong rebase. Reverted and still working.

https://codereview.chromium.org/2873143002/diff/60001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:698: callback.Run(true);
On 2017/05/12 19:34:30, robliao wrote:
> Should this be called synchronously or should it be posted to a task runner?
> What's the goal of this callback?

This callback is a sophisticated ScopedResultCallback [1],
passed around from its origin in image_capture_impl.cc [2],
where it's constructed witht a trampoline function
RunSetOptionsCallbackOnUIThread() and with a 
media::BindToCurrentLoop();

FTR, ScopedResultCallback is a special Callback: it'll
Run() the callback with certain arguments if destroyed and 
not Run() explicitly, the idea being that those arguments 
signify "error". This way, anyone passing by or receiving 
such a ScopedResultCallback can just drop it in case of
error, and it'll do the right thing itself.


[1]
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...
[2]
https://cs.chromium.org/chromium/src/content/browser/image_capture/image_capt...

[robliao, 2017-05-12 21:46:02]

Windows Usage lgtm.

[mcasas, 2017-05-12 22:54:27]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 22:55:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mcasas, 2017-05-13 01:38:23]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2017-05-13 01:38:33]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:39:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 03:59:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494639513082060,
"parent_rev": "9d5f354616dea455ea08345c96b67081275a865d", "commit_rev":
"86adb2c88ba82194458ac09d078a00616c0b22dd"}

[commit-bot: I haz the power, 2017-05-13 03:59:55]

Description was changed from

==========
Image Capture: wire setPhotoOptions() for Win

This CL wires the photo capabilities setPhotoOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivial updates to the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setPhotoOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128
==========

to

==========
Image Capture: wire setPhotoOptions() for Win

This CL wires the photo capabilities setPhotoOptions() method:

- |camera_control_| and |video_control_| are made member 
 variables, which forces trivial updates to the lambdas.

- most of the controls are straightforward except white
balance and exposure: those have a 'manual' and 'auto'
that enable the use of |color_temperature| and 
|exposure_compensation|, resp.  Since either of them can
be configured in subsequent setPhotoOptions() cycles, we need
member flags to keep the manual/auto state.



BUG=657128

Review-Url: https://codereview.chromium.org/2873143002
Cr-Commit-Position: refs/heads/master@{#471554}
Committed:
https://chromium.googlesource.com/chromium/src/+/86adb2c88ba82194458ac09d078a...
==========

[commit-bot: I haz the power, 2017-05-13 03:59:56]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/86adb2c88ba82194458ac09d078a...

[mcasas, 2017-05-13 04:16:29]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/2876303002/ by mcasas@chromium.org.

The reason for reverting is: /video_capture_device_win.obj 
ninja -t msvc -e environment.x86 -- C:\b\c\goma_client/gomacc.exe
"C:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/media/capture/capture_lib/video_capture_device_win.obj.rsp /c
../../media/capture/video/win/video_capture_device_win.cc
/Foobj/media/capture/capture_lib/video_capture_device_win.obj
/Fd"obj/media/capture/capture_lib_cc.pdb"
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(637):
error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(637):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(644):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(654):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(661):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(670):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(676):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(681):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(686):
warning C4189: 'hr': local variable is initialized but not referenced
c:\b\c\b\win_chrome\src\media\capture\video\win\video_capture_device_win.cc(691):
warning C4189: 'hr': local variable is initialized but not referenced.

[findit-for-me, 2017-05-13 04:44:20]

Findit (https://goo.gl/kROfz5) confirmed this CL at revision 471554 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[robliao, 2017-05-15 16:07:47]

On 2017/05/13 04:44:20, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) confirmed this CL at revision 471554 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

DLOG_IF_FAILED_WITH_HRESULT, unlike DCHECK, actually removes the statement from
the compilation. Looks like a few UNREFERENCED_PARAMETER's might be in order.

Alternatively you can structure this as...

if (FAILED(statement)) {
  DLOG(WARNING) << "Messaage here"
}