Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
482413002:
Fix some error states in VideoCaptureDeviceWin::AllocateAndStart. (Closed)
|
Created:
6 years, 4 months ago by Henrik Grunell Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix some error states in VideoCaptureDeviceWin::AllocateAndStart.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290580
Patch Set 1 #
Total comments: 9
Patch Set 2 : Code review #Patch Set 3 : Code review. #
Total comments: 2
Patch Set 4 : Final fix. #Messages
Total messages: 13 (0 generated)
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.
Adding Tommi as reviewer for faster turn-around (hopefully :)). Please see my first comment.
https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:317: if (FAILED(hr)) { 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... https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:331: SetErrorState("Failed to set capture device output format"); 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...
https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:317: if (FAILED(hr)) { On 2014/08/19 08:23:36, mcasas wrote: > 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... Done. https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:318: SetErrorState("Failed to get capture device capabilities"); 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. https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:331: SetErrorState("Failed to set capture device output format"); On 2014/08/19 08:23:36, mcasas wrote: > 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... Are you sure that works for this case? I'd prefer not do it in this CL, since I'm unsure of this.
lgtm. nice catch on the S_FALSE check mcasas.
https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:318: SetErrorState("Failed to get capture device capabilities"); On 2014/08/19 08:50:53, Henrik Grunell wrote: > 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. IMHO we should add a return here for coherence reasons, but not for UMA-logs unicity... https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:331: SetErrorState("Failed to set capture device output format"); On 2014/08/19 08:50:53, Henrik Grunell wrote: > On 2014/08/19 08:23:36, mcasas wrote: > > 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... > > Are you sure that works for this case? I'd prefer not do it in this CL, since > I'm unsure of this. 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...?
https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:318: SetErrorState("Failed to get capture device capabilities"); On 2014/08/19 11:20:32, mcasas wrote: > On 2014/08/19 08:50:53, Henrik Grunell wrote: > > 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. > > IMHO we should add a return here for coherence reasons, > but not for UMA-logs unicity... 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. https://codereview.chromium.org/482413002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:331: SetErrorState("Failed to set capture device output format"); On 2014/08/19 11:20:32, mcasas wrote: > On 2014/08/19 08:50:53, Henrik Grunell wrote: > > On 2014/08/19 08:23:36, mcasas wrote: > > > 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... > > > > Are you sure that works for this case? I'd prefer not do it in this CL, since > > I'm unsure of this. > > The method FormatMessageA() underlying > SystemErrorCodeToString() seems after Googling > to be the preferred way to translate an HRESULT > to a human readable string. OK. > Like I said, far beyond the scope of this CL, > what about a TODO+crbug...? Done.
lgtm https://codereview.chromium.org/482413002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:334: // TODO(mcasas): Log the error. http://crbug.com/405016. nit: s/mcasas/grunell/
https://codereview.chromium.org/482413002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/482413002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:334: // TODO(mcasas): Log the error. http://crbug.com/405016. On 2014/08/19 12:05:23, mcasas wrote: > nit: s/mcasas/grunell/ Fair enough. Done.
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/482413002/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as 290580
Message was sent while issue was closed.
lgtm, thanks for following up on this! |
