Improve error handling for ffmpeg operations

webrtc/tools/run_video_analysis.py

Index: webrtc/tools/run_video_analysis.py
diff --git a/webrtc/tools/run_video_analysis.py b/webrtc/tools/run_video_analysis.py
index 350bc556fb71cd4a461639eb045d14432133a85b..33e50e423972daf3718bb70feb8813f167543e95 100755
--- a/webrtc/tools/run_video_analysis.py
+++ b/webrtc/tools/run_video_analysis.py
@@ -14,6 +14,7 @@ import sys
 import time
 import glob
 import re
+import shutil
 
 # Used to time-stamp output files and directories
 CURRENT_TIME = time.strftime("%d_%m_%Y-%H:%M:%S")
@@ -180,11 +181,16 @@ def RestartMagewellDevices(ref_video_device, test_video_device):
     if re.match(r'^\d-\d$', directory):
       magewell_usb_ports.append(directory)
 
-  print '\nResetting USB ports where magewell devices are connected...'
-
-  # Use the USB bus and port ID (e.g. 4-3) to unbind and bind the USB devices
-  # (i.e. soft eject and insert).
   try:
+

[kjellander_webrtc, 2017/03/14 12:45:26]

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).

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+    # Abort early if no devices are found.
+    if len(magewell_usb_ports) == 0:
+      raise Exception('No magewell devices found.')
+
+    print '\nResetting USB ports where magewell devices are connected...'
+
+    # Use the USB bus and port ID (e.g. 4-3) to unbind and bind the USB devices
+    # (i.e. soft eject and insert).
     for usb_port in magewell_usb_ports:
       echo_cmd = ['echo', usb_port]
       unbind_cmd = ['sudo', 'tee', '/sys/bus/usb/drivers/usb/unbind']
@@ -203,14 +209,13 @@ def RestartMagewellDevices(ref_video_device, test_video_device):
       echo_bind.stdout.close()
       bind.communicate()
       bind.wait()
-  except OSError as e:
-    print 'Error while resetting magewell devices: ' + e
+  except Exception:

[kjellander_webrtc, 2017/03/14 12:45:26]

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).

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

     raise
-
-  print 'Reset done!\n'
+  else:
+    print 'Reset done!\n'
 
 
-def StartRecording(options, record_paths):
+def StartRecording(options, ref_file_location, test_file_location):
   """Starts recording from the two specified video devices.
 
   Args:
@@ -220,13 +225,11 @@ def StartRecording(options, record_paths):
   """
   ref_file_name = '%s_%s_ref.%s' % (options.app_name, CURRENT_TIME,
     options.video_container)
-  ref_file_location = os.path.join(record_paths['ref_rec_location'],
-      ref_file_name)
+  ref_file = os.path.join(ref_file_location, ref_file_name)
 
   test_file_name = '%s_%s_test.%s' % (options.app_name, CURRENT_TIME,
     options.video_container)
-  test_file_location = os.path.join(record_paths['test_rec_location'],
-      test_file_name)
+  test_file = os.path.join(test_file_location, test_file_name)
 
   # Reference video recorder command line.
   ref_cmd = [
@@ -240,7 +243,7 @@ def StartRecording(options, record_paths):
     '-s', options.frame_width + 'x' + options.frame_height,
     '-t', options.ref_duration,
     '-framerate', options.framerate,
-    ref_file_location
+    ref_file
   ]
 
   # Test video recorder command line.
@@ -255,34 +258,38 @@ def StartRecording(options, record_paths):
     '-s', options.frame_width + 'x' + options.frame_height,
     '-t', options.test_duration,
     '-framerate', options.framerate,
-    test_file_location
+    test_file
   ]
-  print 'Trying to record from reference recorder...'
-  ref_recorder = subprocess.Popen(ref_cmd, stderr=sys.stderr)
-
-  # Start the 2nd recording a little later to ensure the 1st one has started.
-  # TODO(jansson) Check that the ref_recorder output file exists rather than
-  # using sleep.
-  time.sleep(options.time_between_recordings)
-  print 'Trying to record from test recorder...'
-  test_recorder = subprocess.Popen(test_cmd, stderr=sys.stderr)
-  test_recorder.wait()
-  ref_recorder.wait()
-
-  # ffmpeg does not abort when it fails, need to check return code.
-  assert ref_recorder.returncode == 0, (
-    'Ref recording failed, check ffmpeg output and device: %s'
-    % options.ref_video_device)
-  assert test_recorder.returncode == 0, (
-    'Test recording failed, check ffmpeg output and device: %s'
-    % options.test_video_device)
-
-  print 'Ref file recorded to: ' + os.path.abspath(ref_file_location)
-  print 'Test file recorded to: ' + os.path.abspath(test_file_location)
-  print 'Recording done!\n'
-  return FlipAndCropRecordings(options, test_file_name,
-    record_paths['test_rec_location'], ref_file_name,
-    record_paths['ref_rec_location'])
+  try:
+    print 'Trying to record from reference recorder...'
+    ref_recorder = subprocess.Popen(ref_cmd, stderr=sys.stderr)
+
+    # Start the 2nd recording a little later to ensure the 1st one has started.
+    # TODO(jansson) Check that the ref_recorder output file exists rather than
+    # using sleep.
+    time.sleep(options.time_between_recordings)
+    print 'Trying to record from test recorder...'
+    test_recorder = subprocess.Popen(test_cmd, stderr=sys.stderr)
+    test_recorder.wait()
+    ref_recorder.wait()
+
+    # ffmpeg does not abort when it fails, need to check return code.
+    if ref_recorder.returncode != 0 or test_recorder.returncode != 0:
+
+      # Cleanup recording directories.
+      shutil.rmtree(ref_file_location)
+      shutil.rmtree(test_file_location)
+      raise Exception('Recording failed, check ffmpeg output')

[kjellander_webrtc, 2017/03/14 12:45:26]

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

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+  except Exception:

[kjellander_webrtc, 2017/03/14 12:45:26]

Same here as above.

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+    raise
+  else:
+    print 'Ref file recorded to: ' + os.path.abspath(ref_file)
+    print 'Test file recorded to: ' + os.path.abspath(test_file)
+    print 'Recording done!\n'
+    return FlipAndCropRecordings(options, test_file_name,
+      test_file_location, ref_file_name,
+      ref_file_location)

[kjellander_webrtc, 2017/03/14 12:45:26]

Fix indentation/wrapping.

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+
 
 
 def FlipAndCropRecordings(options, test_file_name, test_file_location,
@@ -334,13 +341,23 @@ def FlipAndCropRecordings(options, test_file_name, test_file_location,
     cropped_test_file
   ]
 
-  ref_crop = subprocess.Popen(ref_video_crop_cmd)
-  ref_crop.wait()
-  print 'Ref file cropped to: ' + cropped_ref_file
-
   try:
+    ref_crop = subprocess.Popen(ref_video_crop_cmd)
+    ref_crop.wait()
     test_crop = subprocess.Popen(test_video_crop_cmd)
     test_crop.wait()
+
+    # ffmpeg does not abort when it fails, need to check return code.
+    if ref_crop.returncode != 0 or test_crop.returncode != 0:
+
+      # Cleanup recording directories.
+      shutil.rmtree(ref_file_location)
+      shutil.rmtree(test_file_location)
+      raise Exception('Cropping failed, check ffmpeg output')

[kjellander_webrtc, 2017/03/14 12:45:26]

raise a module-specific exception instead of the generic one.

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+  except Exception:

[kjellander_webrtc, 2017/03/14 12:45:26]

Never catch Exception (see above)

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

+    raise
+  else:
+    print 'Ref file cropped to: ' + cropped_ref_file
     print 'Test file cropped to: ' + cropped_test_file
     print 'Cropping done!\n'
 
@@ -349,11 +366,7 @@ def FlipAndCropRecordings(options, test_file_name, test_file_location,
       'cropped_test_file' : cropped_test_file,
       'cropped_ref_file' : cropped_ref_file
     }
-
     return cropped_recordings
-  except subprocess.CalledProcessError as e:
-    print 'Something went wrong during cropping: ' + e
-    raise
 
 
 def CompareVideos(options, recording_result):
@@ -378,8 +391,8 @@ def CompareVideos(options, recording_result):
   result_file_name = os.path.join(rec_path, '%s_%s_result.txt') % (
     options.app_name, CURRENT_TIME)
 
-  # Find the crop dimensions (950 and 420) in the ref crop parameter string:
-  # 'hflip, crop=950:420:130:56'
+  # Find the crop dimensions (e.g. 950 and 420) in the ref crop parameter
+  # string: 'hflip, crop=950:420:130:56'
   for param in options.ref_crop_parameters.split('crop'):
     if param[0] == '=':
       crop_width = param.split(':')[0].split('=')[1]
@@ -405,11 +418,12 @@ def CompareVideos(options, recording_result):
     with open(result_file_name, 'w') as f:
       compare_video_recordings = subprocess.Popen(compare_cmd, stdout=f)
       compare_video_recordings.wait()
-      print 'Result recorded to: ' + os.path.abspath(result_file_name)
-      print 'Comparison done!'
-  except subprocess.CalledProcessError as e:
+  except Exception as e:

[kjellander_webrtc, 2017/03/14 12:45:26]

Don't catch Exception here.

[janssonWebRTC, 2017/03/14 13:57:19]

Done.

     print 'Something went wrong when trying to compare videos: ' + e
     raise
+  else:
+    print 'Result recorded to: ' + os.path.abspath(result_file_name)
+    print 'Comparison done!'
 
 
 def main():
@@ -442,7 +456,8 @@ def main():
   options = _ParseArgs()
   RestartMagewellDevices(options.ref_video_device, options.test_video_device)
   record_paths = CreateRecordingDirs(options)
-  recording_result = StartRecording(options, record_paths)
+  recording_result = StartRecording(options, record_paths['ref_rec_location'],
+    record_paths['test_rec_location'])
 
   # Do not require compare_video.py script to run, no metrics will be generated.
   if options.compare_videos_script: