Log error messages in windows video capture

Log error messages in windows video capture

BUG=405016
Committed: https://crrev.com/4c16ae578100980cd2f21a3b2b6070f877e7a3f2
Cr-Commit-Position: refs/heads/master@{#292886}

[magjed_chromium, 2014-08-26 14:45:57]

Hi, can you review this please?

[mcasas, 2014-08-26 15:38:19]

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

[mcasas, 2014-08-26 15:38:19]

Almost there, note that sometimes GetLastSystemErrorCode() will return:
 The operation completed successfully. (0x0)

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:339: DPLOG(ERROR) <<
log_msg;
l.338 will in all probability clear the last error id,
so perhaps here you should do:
DLOG(ERROR) << log_msg << ", OS message: " << last_error_message;

Note DLOG(ERROR) != DVLOG(1)

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

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:442: "capture device";
Did you introduce a tab in this line? =-0

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:558: DPLOG(ERROR) << reason;
Similar comments as in the previous file.

[magjed_chromium, 2014-08-26 17:20:29]

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:339: DPLOG(ERROR) <<
log_msg;
On 2014/08/26 15:38:18, mcasas wrote:
> l.338 will in all probability clear the last error id,
> so perhaps here you should do:
> DLOG(ERROR) << log_msg << ", OS message: " << last_error_message;
> 
> Note DLOG(ERROR) != DVLOG(1) 

Done. Btw, I don't think GetLastSystemErrorCode clears the last error (from
reading the documentation, not testing).

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

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:442: "capture device";
On 2014/08/26 15:38:19, mcasas wrote:
> Did you introduce a tab in this line? =-0

I introduced spaces to align the strings, according to the style guide:
http://www.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/489183003/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:558: DPLOG(ERROR) << reason;
On 2014/08/26 15:38:19, mcasas wrote:
> Similar comments as in the previous file.

Done.

[magjed_chromium, 2014-08-27 08:54:32]

magjed@chromium.org changed reviewers:
+ perkj@chromium.org

[magjed_chromium, 2014-08-27 08:54:32]

Per, can you please take a look?

[perkj_chrome, 2014-08-27 10:45:49]

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

https://codereview.chromium.org/489183003/diff/80001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:563:
client_->OnError(log_msg);
mcasas - you suggested we should move the above lines to os independendent
layer. 

so should we move the line
logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode()); to
VideoCaptureController::VideoCaptureDeviceClient::OnError ?

[mcasas, 2014-08-27 11:34:09]

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

https://codereview.chromium.org/489183003/diff/80001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:563:
client_->OnError(log_msg);
On 2014/08/27 10:45:48, perkj wrote:
> mcasas - you suggested we should move the above lines to os independendent
> layer. 
> 
> so should we move the line
> logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode()); to
> VideoCaptureController::VideoCaptureDeviceClient::OnError ?
> 

Fine by me, just make sure we log both via screen dump and
webrtc log.

[magjed_chromium, 2014-08-27 14:38:54]

magjed@chromium.org changed reviewers:
+ tommi@chromium.org

[magjed_chromium, 2014-08-27 14:38:55]

PTAL

[tommi (sloooow) - chröme, 2014-08-27 14:46:18]

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:523: DLOG(ERROR)
<< log_message;
log_message could be a std::ostringstream which would reduce the amount of
std::string objects being created as a result of using operator+().

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:525: "Error on
video capture: " + log_message);
"Error on video capture: " could be made the first part of log_message which
would reduce one more std::string.

https://codereview.chromium.org/489183003/diff/100001/media/video/capture/win...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/489183003/diff/100001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:189: DPLOG(ERROR) <<
"Failed to GetAllocatedString";
FYI there's also DPLOG_IF.  Here you could remove the if() and do:

DPLOG_IF(FAILED(hr), ERROR) << "Failed to GetAllocatedString".

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

https://codereview.chromium.org/489183003/diff/100001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:71: DPLOG_IF(ERROR,
FAILED(hr)) << "Failed to bind camera filter";
ah, I see you're already using it

[magjed_chromium, 2014-08-27 17:03:28]

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:523: DLOG(ERROR)
<< log_message;
On 2014/08/27 14:46:18, tommi wrote:
> log_message could be a std::ostringstream which would reduce the amount of
> std::string objects being created as a result of using operator+().

Done.

https://codereview.chromium.org/489183003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:525: "Error on
video capture: " + log_message);
On 2014/08/27 14:46:18, tommi wrote:
> "Error on video capture: " could be made the first part of log_message which
> would reduce one more std::string.

Done.

https://codereview.chromium.org/489183003/diff/100001/media/video/capture/win...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/489183003/diff/100001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:189: DPLOG(ERROR) <<
"Failed to GetAllocatedString";
On 2014/08/27 14:46:18, tommi wrote:
> FYI there's also DPLOG_IF.  Here you could remove the if() and do:
> 
> DPLOG_IF(FAILED(hr), ERROR) << "Failed to GetAllocatedString".

Nice. The arguments are reversed though.

[mcasas, 2014-08-27 21:24:21]

LGTM with nit.

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

https://codereview.chromium.org/489183003/diff/160001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:449: DPLOG_IF(ERROR,
FAILED(hr)) << "IAMVideoControl Interface NOT SUPPORTED";
nit: this is a WARNING.

[tommi (sloooow) - chröme, 2014-08-28 08:48:46]

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:523: << ", OS
message: " << last_error_message;
exchange last_error_message with the call to SystemErrorCodeToString.

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:524: DLOG(ERROR)
<< log_message.str();
nit: no need to call .str() here

https://codereview.chromium.org/489183003/diff/180001/media/video/capture/win...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/489183003/diff/180001/media/video/capture/win...
media/video/capture/win/video_capture_device_mf_win.cc:335: const std::string
reason =
move this variable into the scope below.
(alternatively, you can skip using an explicit variable and pass the return
value of StringPrintf() to OnError())

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

https://codereview.chromium.org/489183003/diff/180001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:72: break; // TODO(magjed):
Should we return E_FAIL here instead of break?
well, we certainly shouldn't return if |hr| indicates success...
and in the case of failure, we should return |hr| and not E_FAIL, which is what
we end up doing so I think you can remove that todo.

https://codereview.chromium.org/489183003/diff/180001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:227: DPLOG(ERROR) << "Failed
to create capture filter";
FWIW - all the 'P's in the DPLOG() here are pretty much useless or misleading. 
What's more valuable is the |hr| error code.  A 'last error' value is stored in
thread local storage and is only changed when someone explicitly calls
SetLastError or otherwise modifies the TEB.

So, there's a good chance that the current gle value doesn't have anything to do
with the error we're hitting here, but the value of |hr| will certainly tell us
something.  To get the same benefit of DPLOG (i.e. produce a string from an
error value), you can call SystemErrorCodeToString(hr).  Here you'd do something
like:

DLOG(ERROR) << "GetDeviceFilter failed: " << base::SystemErrorCodeToString(hr);

[magjed_chromium, 2014-08-28 14:50:39]

perkj, tommi: PTAL

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

https://codereview.chromium.org/489183003/diff/160001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:449: DPLOG_IF(ERROR,
FAILED(hr)) << "IAMVideoControl Interface NOT SUPPORTED";
On 2014/08/27 21:24:21, mcasas wrote:
> nit: this is a WARNING.

Done.

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:523: << ", OS
message: " << last_error_message;
On 2014/08/28 08:48:46, tommi wrote:
> exchange last_error_message with the call to SystemErrorCodeToString.

Done.

https://codereview.chromium.org/489183003/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:524: DLOG(ERROR)
<< log_message.str();
On 2014/08/28 08:48:46, tommi wrote:
> nit: no need to call .str() here

Done.

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

https://codereview.chromium.org/489183003/diff/180001/media/video/capture/win...
media/video/capture/win/video_capture_device_win.cc:72: break; // TODO(magjed):
Should we return E_FAIL here instead of break?
On 2014/08/28 08:48:46, tommi wrote:
> well, we certainly shouldn't return if |hr| indicates success...
> and in the case of failure, we should return |hr| and not E_FAIL, which is
what
> we end up doing so I think you can remove that todo.

Done.

[perkj_chrome, 2014-08-29 14:17:34]

lgtm with a small request.

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:521: "Error on
video capture: %s, OS message: %s",
Can we also log the error code?

[tommi (sloooow) - chröme, 2014-08-30 17:39:33]

lgtm % whatever nits might remain.

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:521: "Error on
video capture: %s, OS message: %s",
On 2014/08/29 14:17:34, perkj wrote:
> Can we also log the error code?

The 'last error' on the thread is being included indirectly via
GetLastSystemErrorCode, but it looks like the OnError method doesn't support
providing an alternate error code.  Perhaps the code is expected to be included
in the error message when applicable?

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:189: <<
logging::SystemErrorCodeToString(hr);
nit: it might also be good to include the numeric error value (<< std::hex <<
hr)

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
media/video/capture/win/video_capture_device_mf_win.cc:335: if (client_.get())
since the body of the if() contains more than one line, you need braces here

[magjed_chromium, 2014-09-01 08:34:14]

Regarding including error codes:
In OS_WIN, the error code is already included in SystemErrorCodeToString. 
In OS_POSIX, SystemErrorCodeToString is a wrapper around strerror_r, and does
NOT include the error code. 

So, should I include the error code in the OS independent file: 
content/browser/renderer_host/media/video_capture_controller.cc
If I do, the error code will be outputted twice in OS_WIN.

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:521: "Error on
video capture: %s, OS message: %s",
On 2014/08/29 14:17:34, perkj wrote:
> Can we also log the error code?

See message.

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:189: <<
logging::SystemErrorCodeToString(hr);
On 2014/08/30 17:39:33, tommi wrote:
> nit: it might also be good to include the numeric error value (<< std::hex <<
> hr)

See message.

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
media/video/capture/win/video_capture_device_mf_win.cc:335: if (client_.get())
On 2014/08/30 17:39:33, tommi wrote:
> since the body of the if() contains more than one line, you need braces here

Done.

[tommi (sloooow) - chröme, 2014-09-01 12:04:51]

On 2014/09/01 08:34:14, magjed wrote:
> Regarding including error codes:
> In OS_WIN, the error code is already included in SystemErrorCodeToString. 
> In OS_POSIX, SystemErrorCodeToString is a wrapper around strerror_r, and does
> NOT include the error code. 

Thanks for looking into that.  Looks like we have a discrepancy and it would be
best to fix it at that level (if someone deems it fixworthy that is).

> 
> So, should I include the error code in the OS independent file: 
> content/browser/renderer_host/media/video_capture_controller.cc
> If I do, the error code will be outputted twice in OS_WIN.

Skip adding the error code. This is good as is but I appreciate you checking
this.

> 
>
https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/489183003/diff/260001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_controller.cc:521: "Error on
> video capture: %s, OS message: %s",
> On 2014/08/29 14:17:34, perkj wrote:
> > Can we also log the error code?
> 
> See message.
> 
>
https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
> File media/video/capture/win/video_capture_device_factory_win.cc (right):
> 
>
https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
> media/video/capture/win/video_capture_device_factory_win.cc:189: <<
> logging::SystemErrorCodeToString(hr);
> On 2014/08/30 17:39:33, tommi wrote:
> > nit: it might also be good to include the numeric error value (<< std::hex
<<
> > hr)
> 
> See message.
> 
>
https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
> File media/video/capture/win/video_capture_device_mf_win.cc (right):
> 
>
https://codereview.chromium.org/489183003/diff/260001/media/video/capture/win...
> media/video/capture/win/video_capture_device_mf_win.cc:335: if (client_.get())
> On 2014/08/30 17:39:33, tommi wrote:
> > since the body of the if() contains more than one line, you need braces here
> 
> Done.

lgtm

[magjed_chromium, 2014-09-01 16:18:38]

The CQ bit was checked by magjed@chromium.org

[commit-bot: I haz the power, 2014-09-01 16:19:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/magjed@chromium.org/489183003/300001

[commit-bot: I haz the power, 2014-09-01 17:15:00]

Committed patchset #16 (id:300001) as b5236bc27861bd8e09656e7d6c88fc90f22dd764

[commit-bot: I haz the power, 2014-09-10 03:17:18]

Patchset 16 (id:??) landed as
https://crrev.com/4c16ae578100980cd2f21a3b2b6070f877e7a3f2
Cr-Commit-Position: refs/heads/master@{#292886}