Win DirectShow: connect antibanding/anti-flicker where supported.

Win DirectShow: connect antibanding/anti-flicker where supported.

Some Win UVC devices implement the 
PROPSETID_VIDCAP_VIDEOPROCAMP API [1]. For them, set 
the anti-banding/anti-flicker/power frequency rejection to
the appropriate frequency (50/60Hz).

This settings sinks all the way into the WebCam DSP so
it's hard to know for sure what is being done, similarly
to http://crrev.com/216263007, although is likely
to avoid certain exposure rates non-multiples of the
power frequency.

IAMVideoProcAmp interface provides access to the
same IKsPropertySet.

[1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff568122(v=vs.85).aspx

BUG=357599
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265646

[mcasas, 2014-04-08 08:02:25]

tommi@ PTAL.

[tommi (sloooow) - chröme, 2014-04-08 10:16:19]

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

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:597: const int
power_line_frequency = GetPowerLineFrequencyForLocation();
This function is getting long.
Can you extract this new code into a separate function?

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:598: if
((power_line_frequency == kPowerLine50Hz) ||
nit: remove superfluous parenthesis

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:601: hr =
ks_propset.QueryFrom(capture_filter_);
handle this return value?

You could do something like:

if (SUCCEEDED(hr = ks_propset.QueryFrom(capture_filter_)) &&
    SUCCEEDED(hr = ks_propset->QuerySupported(...)) &&
    (type_support & KSPROPERTY_SUPPORT_SET)) {
  ...
} else {
  DVLOG(WARNING) << "Anti-flicker setting not supported. "
                 << std::hex << hr;
}

[mcasas, 2014-04-08 10:33:31]

tommi@ PTAL.

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

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:597: const int
power_line_frequency = GetPowerLineFrequencyForLocation();
On 2014/04/08 10:16:19, tommi wrote:
> This function is getting long.
> Can you extract this new code into a separate function?

Done.

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:598: if
((power_line_frequency == kPowerLine50Hz) ||
On 2014/04/08 10:16:19, tommi wrote:
> nit: remove superfluous parenthesis

Done.

https://codereview.chromium.org/220193006/diff/90001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:601: hr =
ks_propset.QueryFrom(capture_filter_);
On 2014/04/08 10:16:19, tommi wrote:
> handle this return value?
> 
> You could do something like:
> 
> if (SUCCEEDED(hr = ks_propset.QueryFrom(capture_filter_)) &&
>     SUCCEEDED(hr = ks_propset->QuerySupported(...)) &&
>     (type_support & KSPROPERTY_SUPPORT_SET)) {
>   ...
> } else {
>   DVLOG(WARNING) << "Anti-flicker setting not supported. "
>                  << std::hex << hr;
> }

Done.

[mcasas, 2014-04-22 14:13:17]

ping.

[tommi (sloooow) - chröme, 2014-04-22 14:55:27]

lgtm % nits

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

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:759: HRESULT hr;
nit: define where you need it

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:762: power_line_frequency ==
kPowerLine60Hz) {
nit: could also turn this into an early return to reduce the number of scopes.

if (power_line_frequency != kPowerLine50Hz &&
    power_line_frequency != kPowerLine60Hz) {
  return;
}

...

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:767:
KSPROPERTY_VIDEOPROCAMP_POWERLINE_FREQUENCY,
not sure about this indent... think it should be:

SUCCEEDED(...
    KSPROPERTY_...

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:779: DVLOG_IF(2, FAILED(hr))
<< "Anti-flicker setting failed.";
DLOG_IF(ERROR, FAILED(hr)) ?

[mcasas, 2014-04-23 10:51:27]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-23 11:00:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/220193006/170001

[commit-bot: I haz the power, 2014-04-23 11:49:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 11:49:43]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mcasas, 2014-04-23 13:13:07]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-23 13:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/220193006/170001

[commit-bot: I haz the power, 2014-04-23 15:36:05]

Change committed as 265646

[mcasas, 2014-04-23 17:04:45]

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

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:759: HRESULT hr;
On 2014/04/22 14:55:27, tommi wrote:
> nit: define where you need it

Done.

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:762: power_line_frequency ==
kPowerLine60Hz) {
On 2014/04/22 14:55:27, tommi wrote:
> nit: could also turn this into an early return to reduce the number of scopes.
> 
> if (power_line_frequency != kPowerLine50Hz &&
>     power_line_frequency != kPowerLine60Hz) {
>   return;
> }
> 
> ...

Done.

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:767:
KSPROPERTY_VIDEOPROCAMP_POWERLINE_FREQUENCY,
On 2014/04/22 14:55:27, tommi wrote:
> not sure about this indent... think it should be:
> 
> SUCCEEDED(...
>     KSPROPERTY_...

Done.

https://codereview.chromium.org/220193006/diff/130001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:779: DVLOG_IF(2, FAILED(hr))
<< "Anti-flicker setting failed.";
On 2014/04/22 14:55:27, tommi wrote:
> DLOG_IF(ERROR, FAILED(hr)) ?

Done.