Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 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. |
DescriptionAdd video recording wrapper
BUG=webrtc:7203
Review-Url: https://codereview.webrtc.org/2704113004
Cr-Commit-Position: refs/heads/master@{#17200}
Committed: https://chromium.googlesource.com/external/webrtc/+/1b0e3b866de34a46882cd8ca0dbc9c3aff0e3ceb
Patch Set 1 #
Total comments: 51
Patch Set 2 : Addressed comments + cleaned it up a little #
Total comments: 16
Patch Set 3 : Fixed according to comments #
Total comments: 2
Patch Set 4 : fix result file name variable #
Created: 3 years, 9 months ago
Messages
Total messages: 14 (5 generated)
jansson@webrtc.org changed reviewers: + kjellander@webrtc.org
Could you PTAL?
Wow, big script! Mostly nits. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:21: def _parseArgs(): Chromium has two exceptions from the standard google python style guide: * Indentation: use 2, not 4, spaces * Naming: Functions and Method names use CapWords() style, not lower_with_under() The only exception to this rule is the main() function -- we do not use Main() http://dev.chromium.org/chromium-os/python-style-guidelines Please update all functions in this script to align with that. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:46: parser.add_option('--ref_ffmpeg', type='string', When do you need two different ffmpeg binaries? This runs on one machine. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:105: if not os.path.exists(options.ref_ffmpeg): I usually use os.path.isfile instead, since if you would have a directory named this, the check would not trigger. Same below. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:109: parser.error('You most provide location for the test device ffmpeg \ Skip the \, it's not needed if you end the string with ' on this line (and begin with ' on the next) The style guide forbids \ https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:117: generated!') align indent with the above ( https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:120: if not os.path.exists(options.frame_analyzer): options.zxing_path https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:127: """Creates root directories for reference and test recordings and Make the summary line fit on a single line Quoting https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings: "Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description" https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:134: absolute paths. +4 spaces indent before "absolute" here. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:137: if not os.path.exists(options.ref_rec_dir): Use os.path.isdir https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:141: # Create and time-stamp directories for all the output files. +1 blank line before comment https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:159: """Tries to find the provided options.ref_video_device and Same comment here about the docstring summary line. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:200: bind_cmd = ['sudo', 'tee', '/sys/bus/usb/drivers/usb/bind'] Will the script ask for sudo password in a clear way if it's needed here? I'm afraid it might get stuck etc. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:230: ref_file_name = options.app_name + '_' + CURRENT_TIME + '_ref.' + \ Use Python string formatters instead, i.e. ref_file_name = '%s_%s_ref.%s' % (options.app_name, CURRENT_TIME, options.video_container) Then you can linebreak easier (before %, or inside the parentheses). Same applies below. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:275: time.sleep(5) Maybe this should be a flag as well? Constants like this hidden deep in the script is undesired. At least refactor it into a constant in the top of the file, like DELAY_BEFORE_RECORDING_SECONDS = 5 if you don't think a flag is needed. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:277: test_recorder = subprocess.Popen(test_cmd, stderr=None) Why stderr=None ? Don't you risk to ignore errors then? https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:293: ref_file_name, ref_file_location): indent aligning with ( above https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:309: test and reference video files. +4 indent https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:312: # Ref file cropping. +blank line https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:314: cropped_ref_file = os.path.abspath(\ remove \ https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:316: ref_crop_parameters = options.ref_crop_width + ':' + \ use string formatter https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:393: result_file_name = \ remove \ and use string formatter instead. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:461: compare_videos(options, recording_result) This is a bit unexpected. I suggest you at least add: else: print 'Skipping compare videos step due to frame_analyzer, zxing or compare_videos flags were not passed.' https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:462: +1 blank line for top-level statements.
https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:21: def _parseArgs(): On 2017/02/22 12:30:51, kjellander_webrtc wrote: > Chromium has two exceptions from the standard google python style guide: > * Indentation: use 2, not 4, spaces > * Naming: Functions and Method names use CapWords() style, not > lower_with_under() > The only exception to this rule is the main() function -- we do not use Main() > > http://dev.chromium.org/chromium-os/python-style-guidelines > > Please update all functions in this script to align with that. In my opinion the linter should catch these things, enforcing style manually must be a lot of work......Can we update the linter, if so where? Changed it all according to the style. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:46: parser.add_option('--ref_ffmpeg', type='string', On 2017/02/22 12:30:50, kjellander_webrtc wrote: > When do you need two different ffmpeg binaries? This runs on one machine. Initially I could not run the same executable at the same time but that works now, not sure why that was. Removed the 2nd ffmpeg. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:105: if not os.path.exists(options.ref_ffmpeg): On 2017/02/22 12:30:50, kjellander_webrtc wrote: > I usually use os.path.isfile instead, since if you would have a directory named > this, the check would not trigger. > Same below. Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:109: parser.error('You most provide location for the test device ffmpeg \ On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Skip the \, it's not needed if you end the string with ' on this line (and begin > with ' on the next) > The style guide forbids \ Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:117: generated!') On 2017/02/22 12:30:51, kjellander_webrtc wrote: > align indent with the above ( > https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:120: if not os.path.exists(options.frame_analyzer): On 2017/02/22 12:30:50, kjellander_webrtc wrote: > options.zxing_path Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:127: """Creates root directories for reference and test recordings and On 2017/02/22 12:30:49, kjellander_webrtc wrote: > Make the summary line fit on a single line > > Quoting https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings: > "Multi-line docstrings consist of a summary line just like a one-line docstring, > followed by a blank line, followed by a more elaborate description" Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:134: absolute paths. On 2017/02/22 12:30:50, kjellander_webrtc wrote: > +4 spaces indent before "absolute" here. Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:137: if not os.path.exists(options.ref_rec_dir): On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Use os.path.isdir Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:141: # Create and time-stamp directories for all the output files. On 2017/02/22 12:30:50, kjellander_webrtc wrote: > +1 blank line before comment Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:159: """Tries to find the provided options.ref_video_device and On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Same comment here about the docstring summary line. Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:200: bind_cmd = ['sudo', 'tee', '/sys/bus/usb/drivers/usb/bind'] On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Will the script ask for sudo password in a clear way if it's needed here? I'm > afraid it might get stuck etc. Yes it will but for me it's quite clear that it asks for it. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:230: ref_file_name = options.app_name + '_' + CURRENT_TIME + '_ref.' + \ On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Use Python string formatters instead, i.e. > ref_file_name = '%s_%s_ref.%s' % (options.app_name, CURRENT_TIME, > options.video_container) > > Then you can linebreak easier (before %, or inside the parentheses). > > Same applies below. Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:275: time.sleep(5) On 2017/02/22 12:30:51, kjellander_webrtc wrote: > Maybe this should be a flag as well? Constants like this hidden deep in the > script is undesired. > At least refactor it into a constant in the top of the file, like > DELAY_BEFORE_RECORDING_SECONDS = 5 if you don't think a flag is needed. Made it a flag. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:277: test_recorder = subprocess.Popen(test_cmd, stderr=None) On 2017/02/22 12:30:50, kjellander_webrtc wrote: > Why stderr=None ? Don't you risk to ignore errors then? This was just a default setting. Will fix. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:293: ref_file_name, ref_file_location): On 2017/02/22 12:30:50, kjellander_webrtc wrote: > indent aligning with ( above Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:309: test and reference video files. On 2017/02/22 12:30:50, kjellander_webrtc wrote: > +4 indent Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:312: # Ref file cropping. On 2017/02/22 12:30:50, kjellander_webrtc wrote: > +blank line Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:314: cropped_ref_file = os.path.abspath(\ On 2017/02/22 12:30:50, kjellander_webrtc wrote: > remove \ Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:316: ref_crop_parameters = options.ref_crop_width + ':' + \ On 2017/02/22 12:30:51, kjellander_webrtc wrote: > use string formatter Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:393: result_file_name = \ On 2017/02/22 12:30:50, kjellander_webrtc wrote: > remove \ and use string formatter instead. Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:461: compare_videos(options, recording_result) On 2017/02/22 12:30:50, kjellander_webrtc wrote: > This is a bit unexpected. I suggest you at least add: > else: > print 'Skipping compare videos step due to frame_analyzer, zxing or > compare_videos flags were not passed.' Done. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:462: On 2017/02/22 12:30:50, kjellander_webrtc wrote: > +1 blank line for top-level statements. Done.
A few more comments. I'll work on getting the linter improved. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:21: def _parseArgs(): On 2017/03/07 14:23:45, janssonWebRTC wrote: > On 2017/02/22 12:30:51, kjellander_webrtc wrote: > > Chromium has two exceptions from the standard google python style guide: > > * Indentation: use 2, not 4, spaces > > * Naming: Functions and Method names use CapWords() style, not > > lower_with_under() > > The only exception to this rule is the main() function -- we do not use Main() > > > > http://dev.chromium.org/chromium-os/python-style-guidelines > > > > Please update all functions in this script to align with that. > > In my opinion the linter should catch these things, enforcing style manually > must be a lot of work......Can we update the linter, if so where? > > Changed it all according to the style. You're right, I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=7303 and I think it can be improved. Thanks for raising this concern. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:46: parser.add_option('--ref_ffmpeg', type='string', On 2017/03/07 14:23:45, janssonWebRTC wrote: > On 2017/02/22 12:30:50, kjellander_webrtc wrote: > > When do you need two different ffmpeg binaries? This runs on one machine. > > Initially I could not run the same executable at the same time but that works > now, not sure why that was. > > Removed the 2nd ffmpeg. Acknowledged. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:200: bind_cmd = ['sudo', 'tee', '/sys/bus/usb/drivers/usb/bind'] On 2017/03/07 14:23:45, janssonWebRTC wrote: > On 2017/02/22 12:30:50, kjellander_webrtc wrote: > > Will the script ask for sudo password in a clear way if it's needed here? I'm > > afraid it might get stuck etc. > > Yes it will but for me it's quite clear that it asks for it. Acknowledged. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:275: time.sleep(5) On 2017/03/07 14:23:45, janssonWebRTC wrote: > On 2017/02/22 12:30:51, kjellander_webrtc wrote: > > Maybe this should be a flag as well? Constants like this hidden deep in the > > script is undesired. > > At least refactor it into a constant in the top of the file, like > > DELAY_BEFORE_RECORDING_SECONDS = 5 if you don't think a flag is needed. > > Made it a flag. Acknowledged. https://codereview.webrtc.org/2704113004/diff/1/webrtc/tools/run_video_analys... webrtc/tools/run_video_analysis.py:277: test_recorder = subprocess.Popen(test_cmd, stderr=None) On 2017/03/07 14:23:45, janssonWebRTC wrote: > On 2017/02/22 12:30:50, kjellander_webrtc wrote: > > Why stderr=None ? Don't you risk to ignore errors then? > > This was just a default setting. Will fix. Acknowledged. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:136: def RestartMagewellDevices(options): Since you're only using two of the options here, I suggest passing those as strings instead of the whole object (ref_video_device and test_video_device). That way the code becomes more readable and it's obvious what information is needed/used inside this function. For the other methods that use 5-10 options, I think it's better to pass the whole object, since it becomes annoying with that many args. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:140: options.Sest_Rdeo_device devices which use video4linux and then do a soft options.Sest_Rdeo_device does not exist. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:266: assert ref_recorder.returncode == 0, '%s%s' % ('Ref recording failed, ' It looks strange to me to use the string formatter this way. I suggest changing to: assert ref_recorder.returncode == 0, ( 'Ref recording failed, check ffmpeg output and device: %s' % options.ref_video_device) Same below. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:367: result_file_name = os.path.join('%s', '%s_%s%s') % ( Inline os.path.dirname(recording_result['cropped_ref_file']) instead of using %s to insert it with a string formatter for the first argument here. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:373: for param in options.ref_crop_parameters.split('crop'): You're making hard assumptions on the format of the crop parameters here. I suggest adding a few sanity checks in _ParseArgs so the script can exit quickly with an error instead of running for long and then fail at this stage. Such sanity checks would ideally be verified with a unit test. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:438: if (os.path.exists(options.frame_analyzer) or Skip os.path.exists here (you already check that in _ParseArgs) since what you want to check here is if the flag is set or not. It seems you want 'and' rather than 'or' between each condition as well, given that you want the else-clause to fire in case any of them is not set. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:439: os.path.exists(options.compare_videos_script) or +2 space indent on this line and the one below. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:444: 'compare_videos flags were not passed.') indent with ( above.
https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:136: def RestartMagewellDevices(options): On 2017/03/08 07:31:21, kjellander_webrtc wrote: > Since you're only using two of the options here, I suggest passing those as > strings instead of the whole object (ref_video_device and test_video_device). > That way the code becomes more readable and it's obvious what information is > needed/used inside this function. > > For the other methods that use 5-10 options, I think it's better to pass the > whole object, since it becomes annoying with that many args. Done. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:140: options.Sest_Rdeo_device devices which use video4linux and then do a soft On 2017/03/08 07:31:21, kjellander_webrtc wrote: > options.Sest_Rdeo_device does not exist. Done. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:266: assert ref_recorder.returncode == 0, '%s%s' % ('Ref recording failed, ' On 2017/03/08 07:31:21, kjellander_webrtc wrote: > It looks strange to me to use the string formatter this way. I suggest changing > to: > assert ref_recorder.returncode == 0, ( > 'Ref recording failed, check ffmpeg output and device: %s' > % options.ref_video_device) > > Same below. Done. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:367: result_file_name = os.path.join('%s', '%s_%s%s') % ( On 2017/03/08 07:31:21, kjellander_webrtc wrote: > Inline os.path.dirname(recording_result['cropped_ref_file']) instead of using %s > to insert it with a string formatter for the first argument here. Done. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:373: for param in options.ref_crop_parameters.split('crop'): On 2017/03/08 07:31:21, kjellander_webrtc wrote: > You're making hard assumptions on the format of the crop parameters here. I > suggest adding a few sanity checks in _ParseArgs so the script can exit quickly > with an error instead of running for long and then fail at this stage. > > Such sanity checks would ideally be verified with a unit test. Good catch, I've added that as check in _ParseArgs as a parser.error. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:438: if (os.path.exists(options.frame_analyzer) or On 2017/03/08 07:31:21, kjellander_webrtc wrote: > Skip os.path.exists here (you already check that in _ParseArgs) since what you > want to check here is if the flag is set or not. > > It seems you want 'and' rather than 'or' between each condition as well, given > that you want the else-clause to fire in case any of them is not set. Yeah I agree this is confusing, I will leave just checking the that the command line option for the compare videos script is there because it will also check if it's dependencies exists. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:439: os.path.exists(options.compare_videos_script) or On 2017/03/08 07:31:21, kjellander_webrtc wrote: > +2 space indent on this line and the one below. Done. https://codereview.webrtc.org/2704113004/diff/20001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:444: 'compare_videos flags were not passed.') On 2017/03/08 07:31:21, kjellander_webrtc wrote: > indent with ( above. Done.
lgtm but I have one small suggestion. https://codereview.webrtc.org/2704113004/diff/40001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:377: result_file_name = rec_path + '%s_%s%s' % (options.app_name, CURRENT_TIME, It would make more sense to inline '_result.txt' into the format string and also use os.path.join to create the file instead of just using +
https://codereview.webrtc.org/2704113004/diff/40001/webrtc/tools/run_video_an... File webrtc/tools/run_video_analysis.py (right): https://codereview.webrtc.org/2704113004/diff/40001/webrtc/tools/run_video_an... webrtc/tools/run_video_analysis.py:377: result_file_name = rec_path + '%s_%s%s' % (options.app_name, CURRENT_TIME, On 2017/03/10 13:48:38, kjellander_webrtc wrote: > It would make more sense to inline '_result.txt' into the format string and also > use os.path.join to create the file instead of just using + Done.
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/2704113004/#ps60001 (title: "fix result file name variable")
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": 60001, "attempt_start_ts": 1489394707211770,
"parent_rev": "6ef1b34aae1bcd02ed261ebc5daab14cc4b8db1b", "commit_rev":
"1b0e3b866de34a46882cd8ca0dbc9c3aff0e3ceb"}
Message was sent while issue was closed.
Description was changed from ========== Add video recording wrapper BUG=webrtc:7203 ========== to ========== Add video recording wrapper BUG=webrtc:7203 Review-Url: https://codereview.webrtc.org/2704113004 Cr-Commit-Position: refs/heads/master@{#17200} Committed: https://chromium.googlesource.com/external/webrtc/+/1b0e3b866de34a46882cd8ca0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/1b0e3b866de34a46882cd8ca0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
