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.

Sign up for GitHub

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

Jump to bottom

WIP: add destructor to FFmpegWriter #429

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

WIP: add destructor to FFmpegWriter #429

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0964939
add destructor to FFmpegWriter
Feb 6, 2020
d522e7e
move the resource freeing to a separate method, and call it from both…
Feb 7, 2020
3dc444a
Merge branch 'develop' into FFmpegWriter-memory
ferdnyc Mar 3, 2020
c04d670
FFmpegWriter_Tests: Add Close-reopen unit test
ferdnyc Mar 6, 2020
d26aafb
Merge branch 'develop' into FFmpegWriter-memory
ferdnyc Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/FFmpegWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ namespace openshot {
AVStream *audio_st, *video_st;
AVCodecContext *video_codec;
AVCodecContext *audio_codec;
SwsContext *img_convert_ctx;
double audio_pts, video_pts;
int16_t *samples;
uint8_t *audio_outbuf;
Expand Down Expand Up @@ -248,6 +247,8 @@ namespace openshot {
/// @param path The file path of the video file you want to open and read
FFmpegWriter(std::string path);

~FFmpegWriter();

/// Close the writer
void Close();

Expand Down
31 changes: 30 additions & 1 deletion src/FFmpegWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ static int set_hwframe_ctx(AVCodecContext *ctx, AVBufferRef *hw_device_ctx, int6
FFmpegWriter::FFmpegWriter(std::string path) :
path(path), fmt(NULL), oc(NULL), audio_st(NULL), video_st(NULL), audio_pts(0), video_pts(0), samples(NULL),
audio_outbuf(NULL), audio_outbuf_size(0), audio_input_frame_size(0), audio_input_position(0),
initial_audio_input_frame_size(0), img_convert_ctx(NULL), cache_size(8), num_of_rescalers(32),
initial_audio_input_frame_size(0), cache_size(8), num_of_rescalers(32),
rescaler_position(0), video_codec(NULL), audio_codec(NULL), is_writing(false), write_video_count(0), write_audio_count(0),
original_sample_rate(0), original_channels(0), avr(NULL), avr_planar(NULL), is_open(false), prepare_streams(false),
write_header(false), write_trailer(false), audio_encoder_buffer_size(0), audio_encoder_buffer(NULL) {
Expand All @@ -102,6 +102,34 @@ FFmpegWriter::FFmpegWriter(std::string path) :
auto_detect_format();
}

FFmpegWriter::~FFmpegWriter() {
if (is_open) {
Close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Free the context which frees the streams too
avformat_free_context(oc);
oc = NULL;

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also calls ZeroMQ as its very last action:

libopenshot/src/FFmpegWriter.cpp

Line 1030 in 49972b2

ZmqLogger::Instance()->AppendDebugMethod("FFmpegWriter::Close");

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

for (auto pair : av_frames) {
AVFrame *frame = pair.second;
AV_FREE_FRAME(&frame);
}

spooled_audio_frames.clear();
spooled_video_frames.clear();
queued_audio_frames.clear();
queued_video_frames.clear();
processed_frames.clear();
deallocate_frames.clear();
av_frames.clear();

if (video_codec) {
AV_FREE_CONTEXT(video_codec);
video_codec = NULL;
}
if (audio_codec) {
AV_FREE_CONTEXT(audio_codec);
audio_codec = NULL;
}
}

// Open the writer
void FFmpegWriter::Open() {
if (!is_open) {
Expand Down Expand Up @@ -2122,6 +2150,7 @@ void FFmpegWriter::InitScalers(int source_width, int source_height) {
scale_mode = SWS_BICUBIC;
}

SwsContext *img_convert_ctx;
// Init software rescalers vector (many of them, one for each thread)
for (int x = 0; x < num_of_rescalers; x++) {
// Init the software scaler from FFMpeg
Expand Down