Fix some error states in VideoCaptureDeviceWin::AllocateAndStart.

media/video/capture/win/video_capture_device_win.cc

Index: media/video/capture/win/video_capture_device_win.cc
diff --git a/media/video/capture/win/video_capture_device_win.cc b/media/video/capture/win/video_capture_device_win.cc
index 29ccf5afcdb8e753d1aa24757387f9ea8d42e6a2..0a3a2f5fe5281de6a813516b3be9f47eddf6f505 100644
--- a/media/video/capture/win/video_capture_device_win.cc
+++ b/media/video/capture/win/video_capture_device_win.cc
@@ -314,7 +314,9 @@ void VideoCaptureDeviceWin::AllocateAndStart(
   // Get the windows capability from the capture device.
   hr = stream_config->GetStreamCaps(
       found_capability.stream_index, media_type.Receive(), caps.get());
-  if (SUCCEEDED(hr)) {
+  if (FAILED(hr)) {

[mcasas, 2014/08/19 08:23:36]

Hmm, I might be overzealous but there's this rationale:

- SUCCEEDED() is a macro checking for success result [1].
- GetStreamCaps() can return S_FALSE, S_OK or a series of error
indications [2]. Both S_FALSE and S_OK would be interpreted 
as SUCCEEDED().
- I think S_FALSE should be handled as a fault condition,
hence this comparison should be changed to
if (hr != S_OK)
or equivalent. Similar case to what happens in [3].


[1]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms687197(v=vs.85).aspx
[2]
http://msdn.microsoft.com/en-us/library/windows/desktop/dd319787(v=vs.85).aspx
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...

[Henrik Grunell, 2014/08/19 08:50:53]

Done.

+      SetErrorState("Failed to get capture device capabilities");

[Henrik Grunell, 2014/08/19 08:50:53]

Note my previous question (repeating here):

Should we not return when hitting these errors? It must be guaranteed that we
only upload UMA stats for failures once per start attempt (Done when calling
OnError on the client). If we shouldn't return, it seems like we should change
the error state to a log.

[mcasas, 2014/08/19 11:20:32]

IMHO we should add a return here for coherence reasons,
but not for UMA-logs unicity...

[Henrik Grunell, 2014/08/19 11:39:47]

Added in both places.

I would though say it should be for the sake of UMA since it depends on zero or
one stop capture log to be useful. *If* it means we don't guarantee that, we
have a problem. Anyhow, now we do.

+  } else {
     if (media_type->formattype == FORMAT_VideoInfo) {
       VIDEOINFOHEADER* h =
           reinterpret_cast<VIDEOINFOHEADER*>(media_type->pbFormat);
@@ -325,11 +327,10 @@ void VideoCaptureDeviceWin::AllocateAndStart(
     sink_filter_->SetRequestedMediaFormat(format);
     // Order the capture device to use this format.
     hr = stream_config->SetFormat(media_type.get());
+    if (FAILED(hr))
+      SetErrorState("Failed to set capture device output format");

[mcasas, 2014/08/19 08:23:36]

Suggestion: Use SystemErrorCodeToString() [0] to
provide more information on the error. This might
be far beyond the scope of this CL though.


[0]
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.cc&sq...

[Henrik Grunell, 2014/08/19 08:50:53]

Are you sure that works for this case? I'd prefer not do it in this CL, since
I'm unsure of this.

[mcasas, 2014/08/19 11:20:32]

The method FormatMessageA() underlying 
SystemErrorCodeToString() seems after Googling
to be the preferred way to translate an HRESULT 
to a human readable string.

Like I said, far beyond the scope of this CL,
what about a TODO+crbug...?

[Henrik Grunell, 2014/08/19 11:39:47]

OK.
Done.

   }
 
-  if (FAILED(hr))
-    SetErrorState("Failed to set capture device output format");
-
   if (format.pixel_format == PIXEL_FORMAT_MJPEG && !mjpg_filter_.get()) {
     // Create MJPG filter if we need it.
     hr = mjpg_filter_.CreateInstance(CLSID_MjpegDec, NULL, CLSCTX_INPROC);