WIP: add destructor to FFmpegWriter #429
Conversation
|
well, not surprising that FFmpeg4 on Ubuntu 18.04 passed, as that's what I'm running... I'm setting up some docker containers to try the others |
src/FFmpegWriter.cpp
Outdated
| @@ -102,6 +102,34 @@ FFmpegWriter::FFmpegWriter(std::string path) : | |||
| auto_detect_format(); | |||
| } | |||
|
|
|||
| FFmpegWriter::~FFmpegWriter() { | |||
| if (is_open) { | |||
| Close(); | |||
There was a problem hiding this comment.
I've only just taken a cursory look at this, but one thing I will say: If at all possible, it would be better to avoid calling Close() in the destructor. There's too much of that already in libopenshot, and we need to avoid it because our Close() methods are waaaay too heavy. They do a lot of housekeeping "stuff" in preparation for being reopened again, which of course makes no sense in a destructor.
See the discussion at #378, and in particular #378 (comment), for more on that.
It may mean having to reimplement some parts of Close() in the destructor, or separate them out into a new method (Finalize()?) that Close() also calls, but it would be good to avoid all of the unnecessary "stuff" in Close().
There was a problem hiding this comment.
Hmm. FFmpegWriter's Close() is actually pretty good, as libopenshot Close() methods go. It doesn't do too much rearranging of deck chairs on the Titanic, as the saying goes, and most of what it does is properly conditional.
It does unconditionally do this, though:
libopenshot/src/FFmpegWriter.cpp
Lines 1020 to 1022 in 49972b2
...which may be a problem in a destructor, if there is no valid oc to free. May be as simple as slapping an if (oc) around it.
There was a problem hiding this comment.
It also calls ZeroMQ as its very last action:
libopenshot/src/FFmpegWriter.cpp
Line 1030 in 49972b2
which DEFINITELY should not be done from a destructor. But since that's just a debug printf() with no useful information, really it can just be removed.
There was a problem hiding this comment.
Actually, I take it back. Close() itself is fairly lightweight, but the methods it calls are WAY too heavy. WriteTrailer(), close_video(), close_audio(), they all do a lot of processing that will bite us in a destructor.
If the video file hasn't been successfully written by the time the destructor is called, I don't think it's realistic for the destructor to try and complete the encoding process before it goes away. That's what an explicit Close() is for, and if the caller hasn't called it by the time the FFmpegWriter object is being destroyed, IMHO it should just close the output stream and drop everything on the floor, not try and write it all out first.
|
It's band-aid. There is nothing to fix. It should be rewritten completely. If program uses resources that it can't free in time, then something is wrong here. |
|
the ordering of freeing things changed in ffmpeg 2-4; last night I got the tests to pass on all of them, but I'll go through @ferdnyc's comments and move/copy some of the freeing to the destructor |
|
I built ./configure --prefix=/ --disable-libvmaf --enable-cuvid --disable-libfdk-aac --enable-nonfree --enable-libfreetype --enable-libdrm --enable-gnutls --enable-gcrypt --enable-fontconfig --enable-libmp3lame --enable-libgsm --enable-libopenjpeg --enable-librsvg --enable-libssh --enable-libsoxr --enable-libspeex --enable-libtheora --enable-libzimg --enable-avfilter --enable-avresample --enable-postproc --enable-pthreads --disable-static --enable-shared --disable-debug --disable-stripping --arch=x86_64Then I ran a Editing $ make test
[ 1%] Automatic MOC for target openshot
[ 1%] Built target openshot_autogen
[ 78%] Built target openshot
Scanning dependencies of target openshot-test
[ 79%] Building CXX object tests/CMakeFiles/openshot-test.dir/FFmpegWriter_Tests.cpp.o
[ 81%] Linking CXX executable openshot-test
/usr/bin/ld: warning: libavutil.so.56, needed by /usr/lib64/libpostproc.so, may conflict with libavutil.so.55
[100%] Built target openshot-test
----------------------------
RUNNING ALL TESTS
----------------------------
[mp4 @ 0x1de01a0] Using AVStream.codec.time_base as a timebase hint to the muxer is deprecated. Set AVStream.time_base instead.
[aac @ 0x1b98f40] Qavg: 192.111
[aac @ 0x1b98f40] 1 frames left in the queue on closing
double free or corruption (!prev)
make[3]: *** [tests/CMakeFiles/test.dir/build.make:57: tests/CMakeFiles/test] Aborted (core dumped)
make[2]: *** [CMakeFiles/Makefile2:687: tests/CMakeFiles/test.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:694: tests/CMakeFiles/test.dir/rule] Error 2
make: *** [Makefile:420: test] Error 2Some of that output (especially the "double free..." line) may also be helpful. I'll try and look into it as well, see if I can find anything. |
|
Here's the output of a |
|
Actually, more informative might be this, from the output of the Presumably, if multiple threads are executing at once, then multiple threads are executing the destructor. Which is an excellent reason to avoid calling |
|
Mark this PR as work-in-progress, because it has unresolved issues (crash in tests, for example). |
ferdnyc
changed the title
|
Fair point, though a failing PR can't be merged anyway. Still... done. |
… the destructor and close
Codecov Report
@@ Coverage Diff @@
## develop #429 +/- ##
===========================================
- Coverage 49.45% 42.43% -7.03%
===========================================
Files 129 129
Lines 10261 13292 +3031
===========================================
+ Hits 5075 5640 +565
- Misses 5186 7652 +2466
Continue to review full report at Codecov.
|
|
Should still be a WIP, I didn't address the leaks in ffmpeg3 |
|
It's coming along nicely, though! As a sort of coarse metric, here are the results of
|
|
(I can say that the remaining "definitely lost" in |
|
BTW, not to contradict your latest commit message, but it seems Though, before going down that road, we should probably add a Especially since the fix for OpenShot/openshot-qt#2881 will likely introduce a Writer |
|
OK, attached are the complete output of the following two commands, run in my worktree for this PR and built against FFmpeg 4.2.2: valgrind -s --track-origins=yes --leak-check=full --show-leak-kinds=all tests/openshot-test \
|& tee valgrind_openshot-test.txt
valgrind -s --track-origins=yes --leak-check=full --show-leak-kinds=all src/openshot-example \
|& tee valgrind_openshot-example.txt( valgrind_openshot-test.txt FFmpegWriter barely appears in either file, though you'll have to search the output files to see which of these are the top of their respective stacks and which are farther down: $ grep FFmpegWriter valgrind_openshot-test.txt
==71597== by 0x494A68A: openshot::FFmpegWriter::write_audio_packets(bool) [clone ._omp_fn.0] (FFmpegWriter.cpp:1534)
==71597== by 0x4948BC8: openshot::FFmpegWriter::write_audio_packets(bool) (FFmpegWriter.cpp:1522)
==71597== by 0x49427A3: openshot::FFmpegWriter::WriteTrailer() (FFmpegWriter.cpp:768)
==71597== by 0x4944838: openshot::FFmpegWriter::Close() (FFmpegWriter.cpp:1065)
==71597== by 0x436C63: TestFFmpegWriter_Test_Webm::RunImpl() const (FFmpegWriter_Tests.cpp:61)
==71597== by 0x493D846: openshot::FFmpegWriter::auto_detect_format() (FFmpegWriter.cpp:139)
==71597== by 0x493D3FA: openshot::FFmpegWriter::FFmpegWriter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (FFmpegWriter.cpp:102)
==71597== by 0x436AC7: TestFFmpegWriter_Test_Webm::RunImpl() const (FFmpegWriter_Tests.cpp:48)
==71597== by 0x49425C2: openshot::FFmpegWriter::WriteFrame(openshot::ReaderBase*, long, long) (FFmpegWriter.cpp:754)
==71597== by 0x436C54: TestFFmpegWriter_Test_Webm::RunImpl() const (FFmpegWriter_Tests.cpp:58)
==71597== by 0x49425C2: openshot::FFmpegWriter::WriteFrame(openshot::ReaderBase*, long, long) (FFmpegWriter.cpp:754)
==71597== by 0x436C54: TestFFmpegWriter_Test_Webm::RunImpl() const (FFmpegWriter_Tests.cpp:58)
$ grep FFmpegWriter valgrind_openshot-example.txt
==94857== by 0x494A68A: openshot::FFmpegWriter::write_audio_packets(bool) [clone ._omp_fn.0] (FFmpegWriter.cpp:1534)
==94857== by 0x4948BC8: openshot::FFmpegWriter::write_audio_packets(bool) (FFmpegWriter.cpp:1522)
==94857== by 0x49427A3: openshot::FFmpegWriter::WriteTrailer() (FFmpegWriter.cpp:768)
==94857== by 0x4944838: openshot::FFmpegWriter::Close() (FFmpegWriter.cpp:1065)
==94857== by 0x493D846: openshot::FFmpegWriter::auto_detect_format() (FFmpegWriter.cpp:139)
==94857== by 0x493D3FA: openshot::FFmpegWriter::FFmpegWriter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (FFmpegWriter.cpp:102) |
|
I've been using address sanitizer (libasan) for leak detection, but it misses some libav stuff. Maybe I should get valgrind working. This came up because at athenascope, we use libopenshot to make some clips, and if we make more than X clips in a single process it gets OOM killed ;) So this is my current top priority, I think @ckirmse and I should be able to get it down. |
|
Is this PR still WIP or are we getting close to a final version? Looking good! |
|
^ For the record, I got most of the way through writing said test, then got derailed coming up with CheckPixel tests for the resulting video file. But it doesn't really need CheckPixel tests, it's the metadata that's important. I'll just wrap that up now, so I can be done with it. |
|
My I wanted to also write a unit test for the new destructor, but unit-testing destructors is a deceptively tricky problem that I'm not feeling up to solving tonight. |
|
@ferdnyc This one has been here a while. What are your thoughts on this one? I don't want to introduce a crash into the C++ side at this point, and this looks a bit scary without testing it myself. |
|
Merge conflicts have been detected on this PR, please resolve. |
This fixes up some memory leaks, specifically in the video and audio codec contexts