964814 - Cap eideticker generated movies at 60fps

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect, P2)

Description

[William Lachance (:wlach)]

With the pointgrey cameras, we sometimes want to capture at a rate higher than 60fps to more reliably detect scrolling changes. However, we should probably cap the movie fps at 60fps since (1) higher fps movies don't seem to compress very well and (2) ffmpeg itself doesn't seem to support generating movies at more than 100fps.

Comment 1

[William Lachance (:wlach)]

Attachment: Cap framerate of generated video to 60fps even if camera is capturing at higher rate;r=mchang

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8366960 [details] [diff] [review]
Cap framerate of generated video to 60fps even if camera is capturing at higher rate;r=mchang

Review of attachment 8366960 [details] [diff] [review]:
-----------------------------------------------------------------

This is a pretty straightforward patch. For now it is limited to the one-off runs that we generate, but it should be fairly straightforward to extend this to the dashboard updater when we want to run fps tests there.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Hrm, that's a bummer re: the ffmpeg deal. I thought the movie was what was analyzed? 

Do we at least break out and save the original frames first so that we have something to go back to in order to hand-validate exceptional results? I'd like to be able to see 120fps of something and not lose the information...

Comment 4

[William Lachance (:wlach)]

(In reply to Geo Mealer [:geo] from comment #3)
> Hrm, that's a bummer re: the ffmpeg deal. I thought the movie was what was
> analyzed? 

No, we keep copies of the raw data captured by the camera in .png format for analysis. The video is just for visual inspection.
 
> Do we at least break out and save the original frames first so that we have
> something to go back to in order to hand-validate exceptional results? I'd
> like to be able to see 120fps of something and not lose the information...

Yup, that was possible and will remain possible after this patch lands. :)

Comment 5

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8366960 [details] [diff] [review]
Cap framerate of generated video to 60fps even if camera is capturing at higher rate;r=mchang

Review of attachment 8366960 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, most of it makes sense. The only big issue I have is making sure we're calculating videoTime correctly. Can you verify that, and if so then I'll switch to r+. The other minor nit for me was 'videoFPS' and 'fps' names. Can you change the videoFPS name to generatedVideoFPS? Thanks!

::: src/dashboard/js/framediff.js
@@ +102,5 @@
>      function datumSelected(datum, series) {
>        plot.unhighlight();
>        plot.highlight(series, datum);
>        currentTime = datum[0];
> +      var videoTime = currentTime * (fps/videoFPS);

I think this calculation is wrong. Let's assume the camera is capturing at 90 fps and the video FPS is 60. Let's say time t = 2s of video. With this formulation, the video time would be (90 / 60 ) * 2 = 3s, which seems way off. Unless the purpose was to capture every frame and extend out the generated video with the extra frames? Otherwise, this equation only works for currentTime < 1, in which case, can you append a new frame for every new 'second' rather than currentTime?

@@ +116,5 @@
>                                                  'eventName': series.label }));
>        var modal = $('#videoDetailModal');
>        if (modal.length) {
>          var largeVideo = $("#large-video").get(0);
> +        console.log(currentTime * (fps/videoFPS));

Nit - If you want to keep the console.log, use videoTime since it's already been calculated.

::: src/videocapture/videocapture/capture.py
@@ +58,5 @@
>  
>      @property
> +    def video_fps(self):
> +        return self.metadata.get('videoFPS', self.fps)
> +

If you want to cap the video FPS at 60, can we make the default value 60 instead of self.fps? Is there a case where we would have the fps data but not the video_fps data?

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to William Lachance (:wlach) from comment #4)
> (In reply to Geo Mealer [:geo] from comment #3)
> > Hrm, that's a bummer re: the ffmpeg deal. I thought the movie was what was
> > analyzed? 
> 
> No, we keep copies of the raw data captured by the camera in .png format for
> analysis. The video is just for visual inspection.
>  
> > Do we at least break out and save the original frames first so that we have
> > something to go back to in order to hand-validate exceptional results? I'd
> > like to be able to see 120fps of something and not lose the information...
> 
> Yup, that was possible and will remain possible after this patch lands. :)

Awesome, thanks for clarifying!

Comment 7

[William Lachance (:wlach)]

(In reply to Mason Chang [:mchang] from comment #5)
> Comment on attachment 8366960 [details] [diff] [review]
> Cap framerate of generated video to 60fps even if camera is capturing at
> higher rate;r=mchang
> 
> Review of attachment 8366960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, most of it makes sense. The only big issue I have is making sure
> we're calculating videoTime correctly. Can you verify that, and if so then
> I'll switch to r+. The other minor nit for me was 'videoFPS' and 'fps'
> names. Can you change the videoFPS name to generatedVideoFPS? Thanks!
> 
> ::: src/dashboard/js/framediff.js
> @@ +102,5 @@
> >      function datumSelected(datum, series) {
> >        plot.unhighlight();
> >        plot.highlight(series, datum);
> >        currentTime = datum[0];
> > +      var videoTime = currentTime * (fps/videoFPS);
> 
> I think this calculation is wrong. Let's assume the camera is capturing at
> 90 fps and the video FPS is 60. Let's say time t = 2s of video. With this
> formulation, the video time would be (90 / 60 ) * 2 = 3s, which seems way
> off. Unless the purpose was to capture every frame and extend out the
> generated video with the extra frames? Otherwise, this equation only works
> for currentTime < 1, in which case, can you append a new frame for every new
> 'second' rather than currentTime?

Hmm, 3 seconds sounds right to me. :) In your example, the video is 1.5 times slower than the actual capture (1 second of captured video == 1.5 seconds of played back video). So second 3 in the video seems to correspond exactly to second 2 in the actual capture.

> 
> @@ +116,5 @@
> >                                                  'eventName': series.label }));
> >        var modal = $('#videoDetailModal');
> >        if (modal.length) {
> >          var largeVideo = $("#large-video").get(0);
> > +        console.log(currentTime * (fps/videoFPS));
> 
> Nit - If you want to keep the console.log, use videoTime since it's already
> been calculated.

I'll just kill the console.log. It's just leftover debugging stuff which shouldn't be there.

> ::: src/videocapture/videocapture/capture.py
> @@ +58,5 @@
> >  
> >      @property
> > +    def video_fps(self):
> > +        return self.metadata.get('videoFPS', self.fps)
> > +
> 
> If you want to cap the video FPS at 60, can we make the default value 60
> instead of self.fps? Is there a case where we would have the fps data but
> not the video_fps data?

Yes, for older captures this might be the case, since we were assuming that the video was being played back at the same rate we were capturing. I think in the past this has always been 60fps so I guess we could use that as a default though in a way that seems more "magical" than just falling back to the capture fps. I think I'll keep this the way it is in my followup patch, if you still disagree with my reasoning let me know and we can chat more about it. :)

Comment 8

[William Lachance (:wlach)]

Attachment: Updated patch to remove debugging info and rename some vars

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8368887 [details] [diff] [review]
Updated patch to remove debugging info and rename some vars

Review of attachment 8368887 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for the fixes.

Comment 10

[William Lachance (:wlach)]

Pushed: https://github.com/mozilla/eideticker/commit/5649425434e5a982dc26d49d06e1b9b7ffb93466