-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG #883
Conversation
… 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)
Hi Frederico, Many thanks for this PR that looks intersting (as usual :)). |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very found of #ifdef #endif is wonder if there is no cleaner alternative without this kind of code branches.
So I quicky looked at the implementation and it sounds very appealing. The only questions I have it about:
|
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).
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.
If it's better not to use any preprocessor definition I can propose two solutions:
or
|
[ci-build] [with-scene-tests] [with-regression-tests] |
cmake/Modules/FindFFMPEG.cmake
Outdated
# Redistribution and use is allowed according to the terms of the New | ||
# BSD license. | ||
# | ||
if (FFMPEG_LIBRARIES AND FFMPEG_INCLUDE_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
[ci-build] [with-scene-tests] [with-regression-tests] |
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 << ", "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should instead return an error, because empty codec will make the execution fail a few lines later.
} while (stat(filename.c_str(), &st) == 0); | ||
m_counter = c + 1; | ||
|
||
sprintf(buf, "%04d", c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename already has correct value at this point.
VideoRecorderFFMPEG::VideoRecorderFFMPEG() | ||
: m_framerate(25) | ||
, m_prefix("sofa_video") | ||
, m_counter(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it used for ? It is only set, but never read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fspadoni did you take Thierry's remark into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the Thierry branch with all his modifications into my branch so I supposed he took into account this remark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see, that was not included. My commit only had fix to allow execution (correct popen flag for linux, quote on path...).
On Linux:
|
Any news about this PR ? |
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. |
new bug found :
|
# Conflicts: # SofaKernel/framework/sofa/helper/CMakeLists.txt # SofaKernel/framework/sofa/helper/gl/VideoRecorder.cpp
…height is not divisible by 2
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. |
hi @fspadoni do not hesitate to poke us (me an Fred) whenever you want a final review best |
I implemented what Fred suggested: the use of two different buffers. |
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. |
Fred approved. Ready to me |
Add a new video recorder class VideoRecorderFFMPEG to capture videos of simulated scenes.
This new video recorder has two differences from the old recorder.
This PR:
Reviewers will merge only if all these checks are true.