Script to start stubbed loopback video test with Espresso

webrtc/tools/test_stubbed_loopback_video.py

+#!/usr/bin/env python
+# Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+import argparse

[kjellander_webrtc, 2017/01/19 13:55:34]

Please add a module docstring that briefly describes what this script does and
what its expectations are on the environment and flags passed.

[mandermo, 2017/01/19 16:46:42]

Have added a comment now. Anything other format or content I need in it?

+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
+
+WEBRTC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '../../'))

[kjellander_webrtc, 2017/01/19 13:55:34]

Call this SRC_DIR to avoid confusion (there's a webrtc/ dir inside the src/
dir).

[kjellander_webrtc, 2017/01/19 13:55:34]

The way to write this is:
os.path.normpath(os.path.join(SCRIPT_DIR, os.pardir, os.pardir))

[mandermo, 2017/01/19 16:46:42]

Done.

[mandermo, 2017/01/19 16:46:42]

Done.

+
+def _ParseArgs():
+  parser = argparse.ArgumentParser(description='Start loopback video analysis')
+  parser.add_argument('--source_dir',

[kjellander_webrtc, 2017/01/19 13:55:34]

This flag doesn't seem to be needed if we just use SRC_DIR above (it's the same
dir). That however requires a full checkout on the machine that's executing the
test, which is true today but might not be in the future (but let's work on
isolating this in a follow-up CL when/if needed in the future instead).

So just remove this flag and use SRC_DIR instead.

[mandermo, 2017/01/19 16:46:42]

This is an optional argument. It was useful when I was developing to be able to
put the file somewhere else and to be able to inject the the path to the source
dir, which is allowed now. If it is not given then it will use the default which
is give an as an argument to the add_argument() call. Should I remove the
argument and use SRC_DIR directly instead?

[kjellander_webrtc, 2017/01/20 08:18:56]

As it's now defaulting to the SRC_DIR, it's fine to keep it. You could add
"Default: %default." to the help text though, to make it obvious.

[mandermo, 2017/01/23 09:09:30]

Done. Added "." to the end of the help texts also.

+      default=WEBRTC_DIR, help='The path to the WebRTC source directory')
+  parser.add_argument('build_dir_android',
+      help='The path to the build directory for Android')
+  parser.add_argument('--build_dir_x86',
+      help='The path to the build directory for building locally')
+  parser.add_argument('--temp_dir',
+      help='A temporary directory to put the output')
+
+  args = parser.parse_args()

[kjellander_webrtc, 2017/01/19 13:55:34]

Please add parser.error() messages to error out in case a mandatory flag is
missing (I guess all should be mandatory?)

[mandermo, 2017/01/19 16:46:42]

Why should they all be mandatory? Only build_dir_android is currently mandatory
and is a positional argument, maybe it would be more consistent to add it to
add_argument() as "--build_dir_android" instead of as "build_dir_android" and
verify it later and raise an error if it is provided. If it is not given this
happens:
mandermo@mandermo:~/code/webrtc/src$ webrtc/tools/test_stubbed_loopback_video.py
usage: test_stubbed_loopback_video.py [-h] [--source_dir SOURCE_DIR]
                                      [--build_dir_x86 BUILD_DIR_X86]
                                      [--temp_dir TEMP_DIR]
                                      build_dir_android
test_stubbed_loopback_video.py: error: too few arguments

+  return args
+

[kjellander_webrtc, 2017/01/19 13:55:35]

+1 blank line before top-level functions.

[mandermo, 2017/01/19 16:46:42]

Done.

+def main():
+  args = _ParseArgs()
+
+  source_dir = args.source_dir
+  build_dir_android = args.build_dir_android
+  build_dir_x86 = args.build_dir_x86
+  temp_dir = args.temp_dir
+  if not temp_dir:
+    temp_dir = tempfile.mkdtemp()
+  else:
+    if not os.path.exists(temp_dir):
+      os.makedirs(temp_dir)
+
+  if not build_dir_x86:
+    build_dir_x86 = os.path.join(temp_dir, 'LocalBuild')
+    subprocess.check_call(['gn', 'gen', build_dir_x86])
+    subprocess.check_call(['ninja', '-C', build_dir_x86, 'frame_analyzer'])
+
+  # Download ffmpeg and zxing.
+  download_script = os.path.join(source_dir,

[kjellander_webrtc, 2017/01/19 13:55:35]

Please introduce a constant at the top of the file:
TOOLCHAIN_DIR = os.path.join(SRC_DIR, 'tools-webrtc', 'video_quality_toolchain')
and use it below to make the code more readable.

[mandermo, 2017/01/19 16:46:42]

Have introduced a local variable toolchain_dir. If remove the --source_dir
argument, then I can make it global. Should I?

[kjellander_webrtc, 2017/01/20 08:18:56]

This is fine.

+      'tools-webrtc/video_quality_toolchain/download.py')

[kjellander_webrtc, 2017/01/19 13:55:35]

Avoid all / and just use multiple string arguments to os.path.join instead.

[mandermo, 2017/01/19 16:46:42]

Done.

+  subprocess.check_call([download_script])
+
+  # Run the Espresso code.
+  espresso_target = os.path.join(build_dir_android,
+      'bin/run_AppRTCMobileTestStubbedVideoIO')
+  subprocess.check_call([espresso_target])
+
+  # Pull the output video.
+  test_video = os.path.join(temp_dir, 'test_video.y4m')
+  subprocess.check_call(['adb', 'pull', '/sdcard/output.y4m', test_video])

[kjellander_webrtc, 2017/01/19 13:55:35]

Please pass the filename (output.y4m) as a flag, we want to minimize hardcoded
duplicated names in this script.

[mandermo, 2017/01/19 16:46:42]

As a flag to the script? This path is already hard coded in
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests....
Should we communicate that path to ConnectActivityStubbedInputOutputTest? I
thought we did not want that.

[kjellander_webrtc, 2017/01/20 08:18:56]

Argh, you're right. I forgot about that. Hopefully it won't change in the near
future.

+
+  test_video_yuv = os.path.join(temp_dir, 'test_video.yuv')
+
+  ffmpeg_path = os.path.join(source_dir,
+      'tools-webrtc/video_quality_toolchain/linux/ffmpeg')
+
+  def convert_video(input_video, output_video):
+    subprocess.check_call([ffmpeg_path, '-y', '-i', input_video, output_video])
+
+  # Convert the test video.

[kjellander_webrtc, 2017/01/19 13:55:35]

drop this comment, it doesn't add any value.

[mandermo, 2017/01/19 16:46:42]

Done.

+  convert_video(test_video, test_video_yuv)
+
+  reference_video = os.path.join(source_dir,
+      'resources/reference_video_640x360_30fps.y4m')

[kjellander_webrtc, 2017/01/19 13:55:35]

Please pass this string as a flag instead of hardcoding a name here which will
break if the file is renamed/moved.

[mandermo, 2017/01/19 16:46:42]

As flag to this script?

Can be good with flexibility. But thought we decided against flexibility with
regards to the reference video, since we wanted to hard code
reference_video_640x360_30fps.y4m into several places already, like
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/BUILD.gn?q=r...
and
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests....

[kjellander_webrtc, 2017/01/20 08:18:56]

You're right, my bad.

+
+  reference_video_yuv = os.path.join(temp_dir,
+      'reference_video_640x360_30fps.yuv')
+
+  # Convert the reference video.

[kjellander_webrtc, 2017/01/19 13:55:35]

This comment doesn't tell anything the code isn't.

[mandermo, 2017/01/19 16:46:42]

Done.

+  convert_video(reference_video, reference_video_yuv)
+
+  # Run compare script.
+  compare_script = os.path.join(source_dir, 'webrtc/tools/compare_videos.py')
+  zxing_path = os.path.join(source_dir,
+      'tools-webrtc/video_quality_toolchain/linux/zxing')
+
+  # The frame_analyzer binary should be built for local computer and not for
+  # Android
+  frame_analyzer = os.path.join(build_dir_x86, 'frame_analyzer')
+
+  frame_width = 640
+  frame_height = 360
+
+  stats_file_ref = os.path.join(temp_dir, 'stats_ref.txt')
+  stats_file_test = os.path.join(temp_dir, 'stats_test.txt')
+
+  subprocess.check_call([
+      compare_script, '--ref_video', reference_video_yuv,
+      '--test_video', test_video_yuv, '--yuv_frame_width', str(frame_width),
+      '--yuv_frame_height', str(frame_height),
+      '--stats_file_ref', stats_file_ref,
+      '--stats_file_test', stats_file_test, '--frame_analyzer', frame_analyzer,
+      '--ffmpeg_path', ffmpeg_path, '--zxing_path', zxing_path])
+
+  shutil.rmtree(temp_dir)
+

[kjellander_webrtc, 2017/01/19 13:55:34]

+1 blank line before top-level functions and statements.

[mandermo, 2017/01/19 16:46:42]

Done.

+if __name__ == '__main__':
+  sys.exit(main())
+