Refactor all video related processing to its own class

Refactor all video related processing to its own class

BUG=

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

[nednguyen, 2014-05-29 18:52:22]

https://codereview.chromium.org/303043002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/303043002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:232: return
self._video_output
I think it's an issue with the current system that this file is not cleaned up
after use. With this change, maybe we should add  documentation saying that the
caller takes the ownership of the video_output.

[tonyg, 2014-05-29 18:58:49]

Can you explain how this will be used? It seems we wanted to abstract away the
video format in this api by having the timestamp/bitmap list and this then
exposes the video implementation details to the api.

[Satyanarayana N H Manyam, 2014-05-29 20:02:32]

We want to collect the video file/trace data once the test is completed, so that
we can get more visibility into the metrics.

[nednguyen, 2014-05-29 20:46:13]

On 2014/05/29 20:02:32, Satyanarayana N H Manyam wrote:
> We want to collect the video file/trace data once the test is completed, so
that
> we can get more visibility into the metrics.

With the uber tracing, we also need to integrate the video with the trace. I
haven't figured out all the details yet, but I suspect the processing of the
video can happen outside of the android_platform_backend.

[Satyanarayana N H Manyam, 2014-05-30 00:25:18]

https://codereview.chromium.org/303043002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/303043002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:232: return
self._video_output
On 2014/05/29 18:52:22, nednguyen wrote:
> I think it's an issue with the current system that this file is not cleaned up
> after use. With this change, maybe we should add  documentation saying that
the
> caller takes the ownership of the video_output.

Done.

[tonyg, 2014-05-30 16:49:36]

The thing is that we could implement video capture in any number of different
ways and so it seems undesirable to expose it in the core api in this way.
Callers might be tempted to open it and do something with it that we don't want
to support.

We have a general desire to be able to save all artifacts generated by a run. I
wonder whether we could better accomplish that via some sort of API that uses
something like results.AddArtifact('video', video_path).

This is really just me thinking aloud about the right way to do this. I'm really
curious to hear Nat and Dave's opinions on this too.

[Satyanarayana N H Manyam, 2014-05-30 17:35:20]

On 2014/05/30 16:49:36, tonyg wrote:
> The thing is that we could implement video capture in any number of different
> ways and so it seems undesirable to expose it in the core api in this way.
> Callers might be tempted to open it and do something with it that we don't
want
> to support.
Sounds reasonable.
> 
> We have a general desire to be able to save all artifacts generated by a run.
I
> wonder whether we could better accomplish that via some sort of API that uses
> something like results.AddArtifact('video', video_path).
Most likely this is what we might doing at the end of the day, but since results
object is not available, at this point we passed it back as return value of the
function.

I can see following options.
1) Add documentation to startTracing that return value should not read directly.
2) Add a separate property for video_file_name and document it saying this
should only not be used for reading the video file. (same a above but make it
more clear)
3) Send the results object as parameter to StartTracing(...)
4) Any other way of doing this??

Any preference? 
> 
> This is really just me thinking aloud about the right way to do this. I'm
really
> curious to hear Nat and Dave's opinions on this too.

[nednguyen, 2014-05-30 17:38:26]

On 2014/05/30 16:49:36, tonyg wrote:
> The thing is that we could implement video capture in any number of different
> ways and so it seems undesirable to expose it in the core api in this way.
> Callers might be tempted to open it and do something with it that we don't
want
> to support.
> 
> We have a general desire to be able to save all artifacts generated by a run.
I
> wonder whether we could better accomplish that via some sort of API that uses
> something like results.AddArtifact('video', video_path).
> 
> This is really just me thinking aloud about the right way to do this. I'm
really
> curious to hear Nat and Dave's opinions on this too.

With the ubertracing vision, I think we will put all the APIs for accessing
artifacts to timeline model or maybe some UberTimelineModel which the timeline
model is a part of. FYI, my plan is to create a tracing_controller in which
TracingController.Start(..) could open trace, video,... &
TracingController.Stop(..) will return that UberTimelineModel.

[nednguyen, 2014-05-30 17:41:30]

On 2014/05/30 17:35:20, Satyanarayana N H Manyam wrote:
> On 2014/05/30 16:49:36, tonyg wrote:
> > The thing is that we could implement video capture in any number of
different
> > ways and so it seems undesirable to expose it in the core api in this way.
> > Callers might be tempted to open it and do something with it that we don't
> want
> > to support.
> Sounds reasonable.
> > 
> > We have a general desire to be able to save all artifacts generated by a
run.
> I
> > wonder whether we could better accomplish that via some sort of API that
uses
> > something like results.AddArtifact('video', video_path).
> Most likely this is what we might doing at the end of the day, but since
results
> object is not available, at this point we passed it back as return value of
the
> function.
> 
> I can see following options.
> 1) Add documentation to startTracing that return value should not read
directly.
> 2) Add a separate property for video_file_name and document it saying this
> should only not be used for reading the video file. (same a above but make it
> more clear)
> 3) Send the results object as parameter to StartTracing(...)
> 4) Any other way of doing this??

For now, may be instead of returning the raw string that contains the file path
to the video, return a object that wrap around the path with some APIs to make
sure people can't mess it up? What do you guys think of this?

> Any preference? 
> > 
> > This is really just me thinking aloud about the right way to do this. I'm
> really
> > curious to hear Nat and Dave's opinions on this too.

[nednguyen, 2014-05-31 00:25:22]

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

https://codereview.chromium.org/303043002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:249: yield
frame
I think you can just let the StartVideoCapture alone and returns a VideoOutput
object here which is a wrapper around video_output, and VideoOutput class has
the API for streaming the frames. With this, you can later add other APIs to
VideoOutput to support what you want.

[tonyg, 2014-05-31 00:45:15]

On 2014/05/31 00:25:22, nednguyen wrote:
>
https://codereview.chromium.org/303043002/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/303043002/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:249: yield
> frame
> I think you can just let the StartVideoCapture alone and returns a VideoOutput
> object here which is a wrapper around video_output, and VideoOutput class has
> the API for streaming the frames. With this, you can later add other APIs to
> VideoOutput to support what you want.

sgtm

Let's just call it video.py (to match bitmap.py).

[nednguyen, 2014-06-03 22:41:09]

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

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:13: class Video(object):
It looks like the video still doesn't get clean up. This patch is the change to
fix it. Can you either make a clean up method to remove the video file, or even
better, implement the __enter__ and __exit__ methods for this class?

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:18: self.video_file_path =
video_file_path
Can you make these members private? I think what don't need to be exposed
shouldn't be exposed.

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:23: return self.video_file_path
Why do we need to expose the video_file_name? The idea is this class should
abstract intricate methods of processing video. Later if you want to do other
things with the video_file, you can just add other methods to this class.

[tonyg, 2014-06-03 23:42:19]

I really like the new approach. Thanks :)

Feel free to land with Ned's approval whenever he's happy.

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

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:1: # Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2014

[Satyanarayana N H Manyam, 2014-06-07 21:41:44]

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

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:1: # Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/06/03 23:42:18, tonyg wrote:
> 2014

Done.

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:13: class Video(object):
We want to copy the file to cloud storage right, i am not completely sure when
that will happen, and whether this object is alive at that point. Added a TODO.

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:18: self.video_file_path =
video_file_path
On 2014/06/03 22:41:09, nednguyen wrote:
> Can you make these members private? I think what don't need to be exposed
> shouldn't be exposed.

Done.

https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:23: return self.video_file_path
I want to upload the video file to cloud storage.

[nednguyen, 2014-06-08 03:50:40]

On 2014/06/07 21:41:44, Satyanarayana N H Manyam wrote:
>
https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/video.py (right):
> 
>
https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video.py:1: # Copyright (c) 2013 The Chromium
> Authors. All rights reserved.
> On 2014/06/03 23:42:18, tonyg wrote:
> > 2014
> 
> Done.
> 
>
https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video.py:13: class Video(object):
> We want to copy the file to cloud storage right, i am not completely sure when
> that will happen, and whether this object is alive at that point. Added a
TODO.
> 
>
https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video.py:18: self.video_file_path =
> video_file_path
> On 2014/06/03 22:41:09, nednguyen wrote:
> > Can you make these members private? I think what don't need to be exposed
> > shouldn't be exposed.
> 
> Done.
> 
>
https://codereview.chromium.org/303043002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video.py:23: return self.video_file_path
> I want to upload the video file to cloud storage.

Can you make a method like UploadToCloudStorage(..) instead of exposing the
video_file_path?

[nednguyen, 2014-06-08 04:48:38]

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:168: Returns a
telemetry.core.video.Video object.
nit: can you follow the style in
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:44: bmp, bitmap.WEB_PAGE_TEST_ORANGE)
The Highlight bounding box color of is based on tab's implementation. Instead of
hard coding this, it seems more reasonable for this to be set in Tab's
StopVideoCapture method.
Maybe: 
GetVideoFrameIter(self, initial_flash_color):
"""
Args:
   initial_flash_color...
...

[Satyanarayana N H Manyam, 2014-06-10 01:08:37]

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:168: Returns a
telemetry.core.video.Video object.
On 2014/06/08 04:48:38, nednguyen wrote:
> nit: can you follow the style in
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

Done.

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:44: bmp, bitmap.WEB_PAGE_TEST_ORANGE)
Actually this is called by speedindex not tab, and i don't think it has anything
to do with tab, just that video processing code is present in tab class

[nednguyen, 2014-06-10 02:03:35]

On 2014/06/10 01:08:37, Satyanarayana N H Manyam wrote:
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/platform/__init__.py (right):
> 
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/platform/__init__.py:168: Returns a
> telemetry.core.video.Video object.
> On 2014/06/08 04:48:38, nednguyen wrote:
> > nit: can you follow the style in
> >
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...
> 
> Done.
> 
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/video.py (right):
> 
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/video.py:44: bmp, bitmap.WEB_PAGE_TEST_ORANGE)
> Actually this is called by speedindex not tab, and i don't think it has
anything
> to do with tab, just that video processing code is present in tab class

The choice of WEB_PAGE_TEST_ORANGE in here and tab still seems like a magic
value to me. 

Either the StartVideoCapture method of tab should reference this as a constant
defined in video class, or video needs to references this from tab class. I am a
little inclined to the first choice.

[nednguyen, 2014-06-10 02:03:41]

lgtm

[Satyanarayana N H Manyam, 2014-06-11 01:55:49]

On 2014/06/10 02:03:35, nednguyen wrote:
> On 2014/06/10 01:08:37, Satyanarayana N H Manyam wrote:
> >
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/core/platform/__init__.py (right):
> > 
> >
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/core/platform/__init__.py:168: Returns a
> > telemetry.core.video.Video object.
> > On 2014/06/08 04:48:38, nednguyen wrote:
> > > nit: can you follow the style in
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/core/video.py (right):
> > 
> >
>
https://codereview.chromium.org/303043002/diff/120001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/core/video.py:44: bmp,
bitmap.WEB_PAGE_TEST_ORANGE)
> > Actually this is called by speedindex not tab, and i don't think it has
> anything
> > to do with tab, just that video processing code is present in tab class
> 
> The choice of WEB_PAGE_TEST_ORANGE in here and tab still seems like a magic
> value to me. 
> 
> Either the StartVideoCapture method of tab should reference this as a constant
> defined in video class, or video needs to references this from tab class. I am
a
> little inclined to the first choice.

Fixed. PTAL

[nednguyen, 2014-06-11 01:59:59]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-06-11 02:04:38]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/satyanarayana@google.com/303043002/170001

[commit-bot: I haz the power, 2014-06-11 08:09:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  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_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-06-11 08:12:47]

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

[commit-bot: I haz the power, 2014-06-11 08:12:47]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Satyanarayana N H Manyam, 2014-06-11 18:52:11]

The CQ bit was checked by satyanarayana@google.com

[commit-bot: I haz the power, 2014-06-11 18:54:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/satyanarayana@google.com/303043002/180001

[Satyanarayana N H Manyam, 2014-06-11 20:02:40]

The CQ bit was checked by satyanarayana@google.com

[Satyanarayana N H Manyam, 2014-06-11 20:05:36]

The CQ bit was checked by satyanarayana@google.com

[commit-bot: I haz the power, 2014-06-11 20:08:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/satyanarayana@google.com/303043002/220001

[commit-bot: I haz the power, 2014-06-11 21:30:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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_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...)
  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/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15601)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18739)

[commit-bot: I haz the power, 2014-06-11 21:33:59]

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

[commit-bot: I haz the power, 2014-06-11 21:34:01]

Try jobs failed on following builders:
  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_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15663)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18796)

[Satyanarayana N H Manyam, 2014-06-12 17:10:38]

The CQ bit was checked by satyanarayana@google.com

[Satyanarayana N H Manyam, 2014-06-12 17:11:20]

The CQ bit was unchecked by satyanarayana@google.com

[Satyanarayana N H Manyam, 2014-06-12 18:00:43]

The CQ bit was checked by satyanarayana@google.com

[commit-bot: I haz the power, 2014-06-12 18:02:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/satyanarayana@google.com/303043002/240001

[commit-bot: I haz the power, 2014-06-12 22:54:05]

Change committed as 276839