Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746413002:
Improve error handling for ffmpeg operations (Closed)
|
Created:
3 years, 9 months ago by janssonWebRTC Modified:
3 years, 9 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImprove error handling for ffmpeg operations
BUG=webrtc:7203
Review-Url: https://codereview.webrtc.org/2746413002
Cr-Commit-Position: refs/heads/master@{#17258}
Committed: https://chromium.googlesource.com/external/webrtc/+/5c7a623dd45eff06bd173d9e32801188aa1eaf3c
Patch Set 1 #Patch Set 2 : remove debug prints #
Total comments: 16
Patch Set 3 : add custom exceptions #
Total comments: 8
Patch Set 4 : Remove custom exceptions #
Total comments: 6
Patch Set 5 : Add back custom exceptions and remove return #
Total comments: 12
Patch Set 6 : Address comments and cleaned up docstrings comments #
Total comments: 1
Patch Set 7 : Remove blank line #Messages
Total messages: 18 (5 generated)
jansson@webrtc.org changed reviewers: + kjellander@webrtc.org
Could you please a take a look? I just wanted to improve the error handling and cleanup directories and files if something fails.
https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:185: remove line 185. I think the blank-line-before-comment is mainly for readability and not necessarily enforced always (just having try: on the previous line doesn't make it harder to read without a blank line in between). https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:212: except Exception: From http://google.github.io/styleguide/pyguide.html?showone=Exceptions#Exceptions: Never use catch-all except: statements, or catch Exception or StandardError, unless you are re-raising the exception or in the outermost block in your thread (and printing an error message). Python is very tolerant in this regard and except: will really catch everything including misspelled names, sys.exit() calls, Ctrl+C interrupts, unittest failures and all kinds of other exceptions that you simply don't want to catch. Catch something more specific, like the OSError you used to (or multiple types). https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:282: raise Exception('Recording failed, check ffmpeg output') Introduce your own exception in this module. From http://google.github.io/styleguide/pyguide.html?showone=Exceptions#Exceptions: Modules or packages should define their own domain-specific base exception class, which should inherit from the built-in Exception class. The base exception for a module should be called Error. class Error(Exception): pass https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:283: except Exception: Same here as above. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:291: ref_file_location) Fix indentation/wrapping. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:356: raise Exception('Cropping failed, check ffmpeg output') raise a module-specific exception instead of the generic one. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:357: except Exception: Never catch Exception (see above) https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:421: except Exception as e: Don't catch Exception here.
Sorry for not reading the style guide! https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:185: On 2017/03/14 12:45:26, kjellander_webrtc wrote: > remove line 185. I think the blank-line-before-comment is mainly for readability > and not necessarily enforced always (just having try: on the previous line > doesn't make it harder to read without a blank line in between). Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:212: except Exception: On 2017/03/14 12:45:26, kjellander_webrtc wrote: > From > http://google.github.io/styleguide/pyguide.html?showone=Exceptions#Exceptions: > > Never use catch-all except: statements, or catch Exception or StandardError, > unless you are re-raising the exception or in the outermost block in your thread > (and printing an error message). Python is very tolerant in this regard and > except: will really catch everything including misspelled names, sys.exit() > calls, Ctrl+C interrupts, unittest failures and all kinds of other exceptions > that you simply don't want to catch. > > Catch something more specific, like the OSError you used to (or multiple types). Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:282: raise Exception('Recording failed, check ffmpeg output') On 2017/03/14 12:45:26, kjellander_webrtc wrote: > Introduce your own exception in this module. > > From > http://google.github.io/styleguide/pyguide.html?showone=Exceptions#Exceptions: > Modules or packages should define their own domain-specific base exception > class, which should inherit from the built-in Exception class. The base > exception for a module should be called Error. > class Error(Exception): > pass Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:283: except Exception: On 2017/03/14 12:45:26, kjellander_webrtc wrote: > Same here as above. Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:291: ref_file_location) On 2017/03/14 12:45:26, kjellander_webrtc wrote: > Fix indentation/wrapping. Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:356: raise Exception('Cropping failed, check ffmpeg output') On 2017/03/14 12:45:26, kjellander_webrtc wrote: > raise a module-specific exception instead of the generic one. Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:357: except Exception: On 2017/03/14 12:45:26, kjellander_webrtc wrote: > Never catch Exception (see above) Done. https://codereview.webrtc.org/2746413002/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:421: except Exception as e: On 2017/03/14 12:45:26, kjellander_webrtc wrote: > Don't catch Exception here. Done.
https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:220: except MagewellError: catching only this and just re-throwing it doesn't add any value. You might as well skip the try+except encapsulation entirely. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:291: except FfmpegError: Same here. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:298: ref_file_name, ref_file_location) It's either alight with the above ( or use 4 spaces indent when wrapping like this. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:364: except FfmpegError: No point with this raise (if you would have a final part it could make sense, but you don't so there's no point having try+except at all here.
Redid it in a similar fashion as compare_videos and barcode_decoder.py. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:220: except MagewellError: On 2017/03/14 14:39:58, kjellander_webrtc wrote: > catching only this and just re-throwing it doesn't add any value. You might as > well skip the try+except encapsulation entirely. Done. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:291: except FfmpegError: On 2017/03/14 14:39:58, kjellander_webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:298: ref_file_name, ref_file_location) On 2017/03/14 14:39:58, kjellander_webrtc wrote: > It's either alight with the above ( or use 4 spaces indent when wrapping like > this. Done. https://codereview.webrtc.org/2746413002/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:364: except FfmpegError: On 2017/03/14 14:39:58, kjellander_webrtc wrote: > No point with this raise (if you would have a final part it could make sense, > but you don't so there's no point having try+except at all here. Done.
I think PS#3 is much better than 4, returning booleans is confusing and harder to read IMO. See comments. https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:207: if bind.returncode == 0: When returncode != 0 we don't have a defined return value, I guess it will be none. That's quite confusing. https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:275: return False I prefer the exception, it's much easier to read and makes it easier for the user to call the method and know what's happening. The boolean return value should also be documented in the docstring for the method, but that goes for exceptions as well. I think exception is better, since they are exceptional cases (that normally shouldn't happen). All you need to do is to remove the unnecessary try+except cases. That only needs to exist in the calling function (main in this case), if you want to ensure you gracefully handle the error cases. You can also just ignore catching the exceptions in main() as well, in which case they will bubble up to the console running the script (but it's not that nice). https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:415: def test(): Forgot to remove? :)
I totally misunderstood what you meant earlier, my bad. Added back the exceptions but without try+except. https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:207: if bind.returncode == 0: On 2017/03/14 21:20:07, kjellander_webrtc wrote: > When returncode != 0 we don't have a defined return value, I guess it will be > none. That's quite confusing. Done. https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:275: return False On 2017/03/14 21:20:07, kjellander_webrtc wrote: > I prefer the exception, it's much easier to read and makes it easier for the > user to call the method and know what's happening. > > The boolean return value should also be documented in the docstring for the > method, but that goes for exceptions as well. I think exception is better, since > they are exceptional cases (that normally shouldn't happen). > > All you need to do is to remove the unnecessary try+except cases. That only > needs to exist in the calling function (main in this case), if you want to > ensure you gracefully handle the error cases. You can also just ignore catching > the exceptions in main() as well, in which case they will bubble up to the > console running the script (but it's not that nice). Done. https://codereview.webrtc.org/2746413002/diff/60001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:415: def test(): On 2017/03/14 21:20:07, kjellander_webrtc wrote: > Forgot to remove? :) Done.
Much better again. I'm afraid I found some new improvements though. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:22: # Base error class. Please skip the comment for each of these classes. They don't add any value. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:166: test_video_device(string): test recording device path Document that MagewellError may be raised https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:229: Return: nit: You should document the potentially raised FfmpegError as well. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:230: FlipAndCropRecordings(function): Performs cropping and transforms on the This is actually not true, what is returned from this function is the result from the FlipAndCropRecordings function that is invoked on line 293 (the cropped_recordings dict). https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:310: Return: Raises: docstring needed here. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:462: CompareVideos(options, recording_result) It's much more clear if you pass recording_result['cropped_ref_file'] and recording_result['cropped_test_file'] into this function, similar to how you pass the two paths into StartRecordig. It's confusing what CompareVideos is going to do with the result. If you pass two paths it's much more obvious what's going on (ComareVideos doesn't need to know more, even if that's all there is in the dict that is actually passed, it's not obvious to the reader unless he's tracking down all the method calls).
Address your comments and also cleaned up the docstrings in general according to https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:22: # Base error class. On 2017/03/15 11:51:00, kjellander_webrtc wrote: > Please skip the comment for each of these classes. They don't add any value. Done. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:166: test_video_device(string): test recording device path On 2017/03/15 11:51:00, kjellander_webrtc wrote: > Document that MagewellError may be raised Done. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:229: Return: On 2017/03/15 11:51:00, kjellander_webrtc wrote: > nit: You should document the potentially raised FfmpegError as well. Done. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:230: FlipAndCropRecordings(function): Performs cropping and transforms on the On 2017/03/15 11:51:00, kjellander_webrtc wrote: > This is actually not true, what is returned from this function is the result > from the FlipAndCropRecordings function that is invoked on line 293 (the > cropped_recordings dict). Right, was a bit unsure of what to document since it was documented in the FlipAnDCropRecordings function. Fixed. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:310: Return: On 2017/03/15 11:51:00, kjellander_webrtc wrote: > Raises: docstring needed here. Done. https://codereview.webrtc.org/2746413002/diff/80001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:462: CompareVideos(options, recording_result) On 2017/03/15 11:51:00, kjellander_webrtc wrote: > It's much more clear if you pass recording_result['cropped_ref_file'] and > recording_result['cropped_test_file'] into this function, similar to how you > pass the two paths into StartRecordig. > > It's confusing what CompareVideos is going to do with the result. If you pass > two paths it's much more obvious what's going on (ComareVideos doesn't need to > know more, even if that's all there is in the dict that is actually passed, it's > not obvious to the reader unless he's tracking down all the method calls). Done.
lgtm, now it's much cleaner. Just FYI: instead of returning a dict with just two items, you could also return a tuple containing the two strings, but I think either is fine. https://codereview.webrtc.org/2746413002/diff/100001/webrtc/tools/run_video_a... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2746413002/diff/100001/webrtc/tools/run_video_a... webrtc/tools/run_video_analysis.py:96: You can skip this blank line
On 2017/03/15 14:00:29, kjellander_webrtc wrote: > lgtm, now it's much cleaner. > > Just FYI: instead of returning a dict with just two items, you could also return > a tuple containing the two strings, but I think either is fine. Noted! > > https://codereview.webrtc.org/2746413002/diff/100001/webrtc/tools/run_video_a... > File webrtc/tools/run_video_analysis.py (right): > > https://codereview.webrtc.org/2746413002/diff/100001/webrtc/tools/run_video_a... > webrtc/tools/run_video_analysis.py:96: > You can skip this blank line Will fix! Thanks for the patience and review ;)
The CQ bit was checked by jansson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2746413002/#ps120001 (title: "Remove blank line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489588320998710,
"parent_rev": "dbbaa2ad66bd18429c5b4caf2436a61e276971f9", "commit_rev":
"5c7a623dd45eff06bd173d9e32801188aa1eaf3c"}
Message was sent while issue was closed.
Description was changed from ========== Improve error handling for ffmpeg operations BUG=webrtc:7203 ========== to ========== Improve error handling for ffmpeg operations BUG=webrtc:7203 Review-Url: https://codereview.webrtc.org/2746413002 Cr-Commit-Position: refs/heads/master@{#17258} Committed: https://chromium.googlesource.com/external/webrtc/+/5c7a623dd45eff06bd173d9e3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/5c7a623dd45eff06bd173d9e3... |
