Fix some error states in VideoCaptureDeviceWin::AllocateAndStart.

media/video/capture/win/video_capture_device_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/video/capture/win/video_capture_device_win.h"
 
 #include <ks.h>
 #include <ksmedia.h>
 
 #include <algorithm>
@@ skipping 296 matching lines @@
     SetErrorState("Failed to GetNumberOfCapabilities");
     return;
   }
 
   scoped_ptr<BYTE[]> caps(new BYTE[size]);
   ScopedMediaType media_type;
 
   // 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);
       if (format.frame_rate > 0)
         h->AvgTimePerFrame = kSecondsToReferenceTime / format.frame_rate;
     }
     // Set the sink filter to request this format.
     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);
 
     if (SUCCEEDED(hr)) {
       input_mjpg_pin_ = GetPin(mjpg_filter_, PINDIR_INPUT, GUID_NULL);
       output_mjpg_pin_ = GetPin(mjpg_filter_, PINDIR_OUTPUT, GUID_NULL);
       hr = graph_builder_->AddFilter(mjpg_filter_, NULL);
     }
 
@@ skipping 201 matching lines @@
   }
 }
 
 void VideoCaptureDeviceWin::SetErrorState(const std::string& reason) {
   DCHECK(CalledOnValidThread());
   DVLOG(1) << reason;
   state_ = kError;
   client_->OnError(reason);
 }
 }  // namespace media