Change ownership of video file

Change ownership of video file

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282189

[nduca, 2014-07-08 18:45:08]

https://codereview.chromium.org/379443002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py (left):

https://codereview.chromium.org/379443002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:232:
self._device, self._video_output, megabits_per_second=min_bitrate_mbps)
No.

self._video should not exist at all.

VideoRecorder.Pull() should return a Video object.

Video constructor should accept a NamedTemporaryFile as its only argument.

[Satyanarayana N H Manyam, 2014-07-08 22:19:37]

PTAL

https://codereview.chromium.org/379443002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py (left):

https://codereview.chromium.org/379443002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:232:
self._device, self._video_output, megabits_per_second=min_bitrate_mbps)
Modified the VideoRecorder.Pull() to return a temporaryfile obj.
removed the file_name parameter in the VideoRecorder constructor.

PTAL

[nednguyen, 2014-07-08 22:28:12]

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
build/android/pylib/screenshot.py:28: rotate=False):
There are other place that specifies the host_file param. You need to keep that
param, either in the constructor or make it a param of the Pull(..) method.

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:20: def __init__(self, video_file_obj):
Don't you still need the TODO to clean up this file?

[nduca, 2014-07-08 22:34:22]

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
build/android/pylib/screenshot.py:28: rotate=False):
or update the other callsites to use the new pull model

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
build/android/pylib/screenshot.py:83: Output video file object on the host.
NamedTemporaryFile containing the video, on the host

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:20: def __init__(self, video_file_obj):
tempfiles delete the file on gc

BUT, this code needs an assert isinstance(video_file_obj,
tempfile.NamedTemporaryFile)

otherwise i coudl pass in any old thing

[Satyanarayana N H Manyam, 2014-07-08 22:46:42]

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
build/android/pylib/screenshot.py:28: rotate=False):
Ok added the parameter to the Pull then.

[Satyanarayana N H Manyam, 2014-07-08 22:57:27]

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/40001/build/android/pylib/scre...
build/android/pylib/screenshot.py:83: Output video file object on the host.
Updated the code a bit

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/379443002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:20: def __init__(self, video_file_obj):
On 2014/07/08 22:34:21, nduca wrote:
> tempfiles delete the file on gc
> 
> BUT, this code needs an assert isinstance(video_file_obj,
> tempfile.NamedTemporaryFile)
> 
> otherwise i coudl pass in any old thing

Done.

[nduca, 2014-07-08 23:24:03]

lgtm

[nednguyen, 2014-07-08 23:24:20]

lgtm

Thanks for this clean up!

https://codereview.chromium.org/379443002/diff/60001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/60001/build/android/pylib/scre...
build/android/pylib/screenshot.py:86: self._device.old_interface.GetTimestamp())
The original code convert this to abspath. I am not entirely sure whether it is
necessary but it's safe to the same behavior.

https://codereview.chromium.org/379443002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/379443002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:22: assert isinstance(video_file_obj,
tempfile.NamedTemporaryFile)
You may also want to assert that video_file_obj.close_called is False.

[Satyanarayana N H Manyam, 2014-07-09 00:58:02]

https://codereview.chromium.org/379443002/diff/60001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/379443002/diff/60001/build/android/pylib/scre...
build/android/pylib/screenshot.py:86: self._device.old_interface.GetTimestamp())
On 2014/07/08 23:24:20, nednguyen wrote:
> The original code convert this to abspath. I am not entirely sure whether it
is
> necessary but it's safe to the same behavior.

Done.

https://codereview.chromium.org/379443002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/379443002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:22: assert isinstance(video_file_obj,
tempfile.NamedTemporaryFile)
On 2014/07/08 23:24:20, nednguyen wrote:
> You may also want to assert that video_file_obj.close_called is False.

Done.

https://codereview.chromium.org/379443002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:22: assert isinstance(video_file_obj,
tempfile.NamedTemporaryFile)
class name here (tempfile._TemporaryFileWrapper) is not public, so using delete
attribute of this object as a proxy for this.

[Satyanarayana N H Manyam, 2014-07-09 01:16:39]

The CQ bit was checked by satyanarayana@google.com

[commit-bot: I haz the power, 2014-07-09 01:18:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/satyanarayana@google.com/379443002/80001

[commit-bot: I haz the power, 2014-07-09 02:12:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26238)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30683)

[commit-bot: I haz the power, 2014-07-09 02:14:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-09 02:14:58]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26258)

[Satyanarayana N H Manyam, 2014-07-09 17:19:26]

The CQ bit was checked by satyanarayana@google.com

[commit-bot: I haz the power, 2014-07-09 17:22:00]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/satyanarayana@google.com/379443002/100001

[commit-bot: I haz the power, 2014-07-09 21:50:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 00:27:07]

Change committed as 282189