[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883

fspadoni · 2019-01-11T16:53:45Z

Add a new video recorder class VideoRecorderFFMPEG to capture videos of simulated scenes.

This new video recorder has two differences from the old recorder.

It depends only on ffmpeg executable and not on any ffmpeg libs.
It is much faster. (only ~20% slowdown when capturing video)

This PR:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings.
does not generate new unit test failures.
does not generate new scene test failures.
does not break API compatibility.
is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

… capture videos of simulated scene. This new video recorder has two differences from the old recorder. 1) It depends only on ffmpeg executable and not on any ffmpeg libs. 2) It is much faster. (only ~20% slowdown)

damienmarchal · 2019-01-11T21:13:14Z

Hi Frederico,

Many thanks for this PR that looks intersting (as usual :)).

damienmarchal · 2019-01-11T21:16:01Z

applications/sofa/gui/BaseViewer.cpp

@@ -132,6 +133,9 @@ void BaseViewer::setPrefix(const std::string& prefix, bool prependDirectory)
 #ifdef SOFA_HAVE_FFMPEG
    videoRecorder.setPrefix(fullPrefix);
 #endif
+#ifdef SOFA_HAVE_FFMPEG_EXEC


I'm not very found of #ifdef #endif is wonder if there is no cleaner alternative without this kind of code branches.

damienmarchal · 2019-01-11T21:18:50Z

So I quicky looked at the implementation and it sounds very appealing.

The only questions I have it about:

do we really want to keep the old version based on libffmepg ?
istn't it possible to have an implementation VideoRecorder that does not leak the implementation details to the other layer so there is no need to have #ifdef in the rest of the code base ?

fspadoni · 2019-01-15T10:20:18Z

Everything here depends on ffmpeg and SOFA can be built with or without ffmpeg.

I tried to follow the SOFA_HAVE_FFMPEG conditional statements and add the SOFA_HAVE_FFMPEG_EXEC for the new implementation. I used a different preprocessor definition because the SOFA_HAVE_FFMPEG depends on the ffmpeg libraries while SOFA_HAVE_FFMPEG_EXEC depends only on the ffmpeg executable (one file).

* do we really want to keep the old version based on libffmepg ?

My proposition for the future is to remove the old version based on libffmepg so all the code depending on SOFA_HAVE_FFMPEG (because the videos capture is much slower) and keep only the code depending on SOFA_HAVE_FFMPEG_EXEC.

* istn't it possible to have an implementation VideoRecorder that does not leak the implementation details to the other layer so there is no need to have #ifdef in the rest of the code base ?

If it's better not to use any preprocessor definition I can propose two solutions:

provide the ffmpeg executable in external dependency package in SOFA (so we are sure it's always found). It should be only one file for each platform supported and maybe a 32 - 64 bits version.

or

create an interface for the video recorder class so that if ffmpeg is found it will be used the ffmpeg implementation otherwise it will be used the screenshots implementation or nothing. This required some changes int the GUI and I never worked on the GUI

fspadoni · 2019-01-15T13:25:39Z

[ci-build] [with-scene-tests] [with-regression-tests]

applications/sofa/gui/BaseViewer.h

…raries

tgaugry · 2019-01-25T12:40:18Z

cmake/Modules/FindFFMPEG.cmake

-# Redistribution and use is allowed according to the terms of the New
-# BSD license.
-#
-if (FFMPEG_LIBRARIES AND FFMPEG_INCLUDE_DIR)


Removing that is gonna break the headless recorder, as it does use FFMPEG.
How about leaving this file as is, and adding current changes in a new file FindFFMPEGEXEC.cmake ?

…exec.cmake to find only the ffmpeg executable

fspadoni · 2019-01-28T13:55:31Z

[ci-build] [with-scene-tests] [with-regression-tests]

tgaugry · 2019-02-04T12:37:30Z

SofaKernel/framework/sofa/helper/gl/VideoRecorderFFMPEG.cpp

+bool VideoRecorderFFMPEG::init(const std::string& filename, int width, int height, unsigned int framerate, unsigned int bitrate, const std::string& codec )
+{
+    std::cout << "START recording to " << filename << " ( ";
+    if (!codec.empty()) std::cout << codec << ", ";


Maybe this should instead return an error, because empty codec will make the execution fail a few lines later.

tgaugry · 2019-02-04T13:55:18Z

SofaKernel/framework/sofa/helper/gl/VideoRecorderFFMPEG.cpp

+    } while (stat(filename.c_str(), &st) == 0);
+    m_counter = c + 1;
+
+    sprintf(buf, "%04d", c);


filename already has correct value at this point.

tgaugry · 2019-02-04T13:56:46Z

SofaKernel/framework/sofa/helper/gl/VideoRecorderFFMPEG.cpp

+VideoRecorderFFMPEG::VideoRecorderFFMPEG()
+    : m_framerate(25)
+    , m_prefix("sofa_video")
+    , m_counter(-1)


What is it used for ? It is only set, but never read

@fspadoni did you take Thierry's remark into account?

I merged the Thierry branch with all his modifications into my branch so I supposed he took into account this remark.

I merged the Thierry branch with all his modifications into my branch so I supposed he took into account this remark.
I send him a review request.

As you can see, that was not included. My commit only had fix to allow execution (correct popen flag for linux, quote on path...).

guparan · 2019-02-13T10:57:39Z

On Linux:

OpenGL viewer: https://files.gitter.im/sofa-framework/sofa/RDcN/caduceus__r20_0001.mp4
QGLViewer is OK.

damienmarchal · 2019-02-26T10:33:05Z

Any news about this PR ?

tgaugry · 2019-02-26T15:46:31Z

asked changes can be found in commit db1b908 on my sofa repo. The openglviewer bug only happen on some display manager, so I think we can merge.

tgaugry · 2019-02-27T15:41:50Z

new bug found :

[libx264 @ 0x561e20af02c0] height not divisible by 2 (1574x965)
Error initializing output stream 0:0 -- Error while opening encoder for output stream #0:0 - maybe incorrect parameters such as bit_rate, rate, width or height

# Conflicts: # SofaKernel/framework/sofa/helper/CMakeLists.txt # SofaKernel/framework/sofa/helper/gl/VideoRecorder.cpp

…height is not divisible by 2

fredroy · 2019-04-17T08:47:09Z

for the problem of width and/or weight not divisible by 2, I would to considerer a bigger buffer and add a pixel on each line and or each column.
Easy to add a new line, but adding a new column implies adding a pixel at every Width pixel.
I would advise to use std::vector instead of unsigned char[] ; it will be easier to insert new data inside the array. And in glReadPixels, you can use .data() (which gives a raw pointer to the underlying array)

…nd height values

hugtalbot · 2019-04-17T13:07:22Z

hi @fspadoni do not hesitate to poke us (me an Fred) whenever you want a final review best

fspadoni · 2019-04-17T14:11:40Z

I implemented what Fred suggested: the use of two different buffers.
One buffer of any size to store data from the viewport and the other buffer of size divisible by 2 to store data for the ffmpeg stream.
I used memcpy to copy block of memory from one buffer to the other and row pointers.

fredroy · 2019-04-17T15:36:10Z

In my case, I would have created only one buffer (with std::vector) of the biggest size, and would have used insert() instead of memcpy :o) . I suppose your implementation is faster but use more memory.
But if it works, I am okay !

hugtalbot · 2019-04-18T08:00:37Z

Fred approved. Ready to me

fspadoni added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Jan 11, 2019

damienmarchal reviewed Jan 11, 2019

View reviewed changes

fspadoni added 3 commits January 15, 2019 11:41

[VideoRecorder] Fix compilation issues

5649e80

[VideoRecorder] Fix compiling error for linux

9a8c1e6

[VideoRecorder] Fix compiling errors for Linux

8064151

guparan reviewed Jan 16, 2019

View reviewed changes

applications/sofa/gui/BaseViewer.h Outdated Show resolved Hide resolved

guparan reviewed Jan 16, 2019

View reviewed changes

applications/sofa/gui/BaseViewer.h Outdated Show resolved Hide resolved

guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Jan 16, 2019

[VideoRecorder] append m_ to the name of new member variables

c5a55b9

fspadoni added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jan 23, 2019

guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Jan 23, 2019

[VideoRecorder] Remove all the files and references to the FFMPEG lib…

9e8cb0a

…raries

tgaugry reviewed Jan 25, 2019

View reviewed changes

fspadoni added 2 commits January 28, 2019 14:19

[VideoRecorder] recover the FindFFMPEG.came file and add a FindFFMPEG…

631f2ef

…exec.cmake to find only the ffmpeg executable

[VideoRecorder] Clean helper/gl/VideoRecorderFFMPEG class

c728d01

fspadoni added 2 commits January 28, 2019 16:31

[VideoRecorder] Compiling error FIX for linux and osx

6641b4f

[VideoRecorder] one more compiling FIX for linux and osx

9ef37a8

fspadoni added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jan 30, 2019

tgaugry reviewed Feb 4, 2019

View reviewed changes

guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Feb 13, 2019

tgaugry and others added 3 commits March 26, 2019 16:47

Add messages to VideoRecorderFFMPEG.cpp + some error checks

979a6b8

Merge branch 'master' into NewVideoRecorder

832963e

# Conflicts: # SofaKernel/framework/sofa/helper/CMakeLists.txt # SofaKernel/framework/sofa/helper/gl/VideoRecorder.cpp

[VideoRecorder] prevent the ffmpeg error when the viewport width and …

0525ecd

…height is not divisible by 2

fspadoni added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 17, 2019

[VideoRecorder] support the video capture for viewport of any width a…

e6cb42c

…nd height values

guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Apr 17, 2019

fspadoni assigned fredroy and hugtalbot Apr 17, 2019

fredroy approved these changes Apr 17, 2019

View reviewed changes

hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status wip Development in the pull-request is still in progress labels Apr 18, 2019

fspadoni assigned tgaugry Apr 18, 2019

tgaugry approved these changes Apr 18, 2019

View reviewed changes

epernod merged commit 9fe34ed into sofa-framework:master Apr 18, 2019

guparan added this to the v19.06 milestone Jun 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883

[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883

fspadoni commented Jan 11, 2019

damienmarchal commented Jan 11, 2019

damienmarchal Jan 11, 2019

damienmarchal commented Jan 11, 2019

fspadoni commented Jan 15, 2019 •

edited

Loading

fspadoni commented Jan 15, 2019

tgaugry Jan 25, 2019

fspadoni commented Jan 28, 2019

tgaugry Feb 4, 2019 •

edited

Loading

tgaugry Feb 4, 2019

tgaugry Feb 4, 2019

hugtalbot Apr 18, 2019

fspadoni Apr 18, 2019

fspadoni Apr 18, 2019

tgaugry Apr 18, 2019

guparan commented Feb 13, 2019 •

edited

Loading

damienmarchal commented Feb 26, 2019

tgaugry commented Feb 26, 2019

tgaugry commented Feb 27, 2019

fredroy commented Apr 17, 2019

hugtalbot commented Apr 17, 2019

fspadoni commented Apr 17, 2019

fredroy commented Apr 17, 2019

hugtalbot commented Apr 18, 2019

[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883

[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883

Conversation

fspadoni commented Jan 11, 2019

damienmarchal commented Jan 11, 2019

Choose a reason for hiding this comment

damienmarchal commented Jan 11, 2019

fspadoni commented Jan 15, 2019 • edited Loading

fspadoni commented Jan 15, 2019

Choose a reason for hiding this comment

fspadoni commented Jan 28, 2019

tgaugry Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guparan commented Feb 13, 2019 • edited Loading

damienmarchal commented Feb 26, 2019

tgaugry commented Feb 26, 2019

tgaugry commented Feb 27, 2019

fredroy commented Apr 17, 2019

hugtalbot commented Apr 17, 2019

fspadoni commented Apr 17, 2019

fredroy commented Apr 17, 2019

hugtalbot commented Apr 18, 2019

fspadoni commented Jan 15, 2019 •

edited

Loading

tgaugry Feb 4, 2019 •

edited

Loading

guparan commented Feb 13, 2019 •

edited

Loading