Skip to content
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

Merged
merged 14 commits into from Apr 18, 2019

Conversation

@fspadoni
Copy link
Contributor

commented Jan 11, 2019

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

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 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.

[VideoRecorder] Add a new video recorder class VideoRecorderFFMPEG to…
… 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

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

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

This comment has been minimized.

Copy link
@damienmarchal

damienmarchal Jan 11, 2019

Contributor

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

@damienmarchal

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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:

  1. 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

  1. 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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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

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

This comment has been minimized.

Copy link
@tgaugry

tgaugry Jan 25, 2019

Contributor

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 ?

@fspadoni

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

[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 << ", ";

This comment has been minimized.

Copy link
@tgaugry

tgaugry Feb 4, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@tgaugry

tgaugry Feb 4, 2019

Contributor

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

This comment has been minimized.

Copy link
@hugtalbot

hugtalbot Apr 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@fspadoni

fspadoni Apr 18, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@fspadoni

fspadoni Apr 18, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@tgaugry

tgaugry Apr 18, 2019

Contributor

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

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@damienmarchal

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Any news about this PR ?

@tgaugry

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

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

tgaugry and others added some commits Feb 5, 2019

Merge branch 'master' into NewVideoRecorder
# Conflicts:
#	SofaKernel/framework/sofa/helper/CMakeLists.txt
#	SofaKernel/framework/sofa/helper/gl/VideoRecorder.cpp
@fredroy

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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)

@hugtalbot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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

@fspadoni

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Fred approved. Ready to me

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

7 checks passed

Dashboard Builds triggered.
Details
[with-regression-tests] Triggered in latest build.
Details
[with-scene-tests] Triggered in latest build.
Details
centos_clang-5_options Build OK. FIXME: 0 unit, 0 scene, 4 regressions
Details
mac_clang-3.5_options Build OK. FIXME: 3 units, 3 scenes, 4 regressions
Details
ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit, 0 scene, 3 regressions
Details
windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit, 1 scene, 3 regressions
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.