From 0964939f20cef48637e6946a89d8bc22a86d196e Mon Sep 17 00:00:00 2001 From: Chad Walker Date: Thu, 6 Feb 2020 14:20:33 -0600 Subject: [PATCH 1/3] add destructor to FFmpegWriter --- include/FFmpegWriter.h | 3 ++- src/FFmpegWriter.cpp | 31 ++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/include/FFmpegWriter.h b/include/FFmpegWriter.h index dc3a2cf7b..d1b89e1c1 100644 --- a/include/FFmpegWriter.h +++ b/include/FFmpegWriter.h @@ -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; @@ -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(); diff --git a/src/FFmpegWriter.cpp b/src/FFmpegWriter.cpp index 245bd9bd6..68509dbbe 100644 --- a/src/FFmpegWriter.cpp +++ b/src/FFmpegWriter.cpp @@ -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) { @@ -102,6 +102,34 @@ FFmpegWriter::FFmpegWriter(std::string path) : auto_detect_format(); } +FFmpegWriter::~FFmpegWriter() { + if (is_open) { + Close(); + } + + 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) { @@ -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 From d522e7e697627fdd41538904fece907aff5b475f Mon Sep 17 00:00:00 2001 From: Chad Walker Date: Fri, 7 Feb 2020 14:43:38 -0600 Subject: [PATCH 2/3] move the resource freeing to a separate method, and call it from both the destructor and close --- include/FFmpegWriter.h | 1 + src/FFmpegWriter.cpp | 93 +++++++++++++++++++++++++----------------- 2 files changed, 57 insertions(+), 37 deletions(-) diff --git a/include/FFmpegWriter.h b/include/FFmpegWriter.h index d1b89e1c1..09ed67702 100644 --- a/include/FFmpegWriter.h +++ b/include/FFmpegWriter.h @@ -241,6 +241,7 @@ namespace openshot { /// write all queued frames void write_queued_frames(); + void free_resources(); public: /// @brief Constructor for FFmpegWriter. Throws one of the following exceptions. diff --git a/src/FFmpegWriter.cpp b/src/FFmpegWriter.cpp index 68509dbbe..e0bbb677f 100644 --- a/src/FFmpegWriter.cpp +++ b/src/FFmpegWriter.cpp @@ -103,31 +103,7 @@ FFmpegWriter::FFmpegWriter(std::string path) : } FFmpegWriter::~FFmpegWriter() { - if (is_open) { - Close(); - } - - 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; - } + free_resources(); } // Open the writer @@ -300,6 +276,7 @@ void FFmpegWriter::SetVideoOptions(bool has_video, std::string codec, Fraction f info.display_ratio.den = size.den; ZmqLogger::Instance()->AppendDebugMethod("FFmpegWriter::SetVideoOptions (" + codec + ")", "width", width, "height", height, "size.num", size.num, "size.den", size.den, "fps.num", fps.num, "fps.den", fps.den); + AV_FREE_CONTEXT(video_codec); // Enable / Disable video info.has_video = has_video; @@ -1020,17 +997,16 @@ void FFmpegWriter::close_audio(AVFormatContext *oc, AVStream *st) } } -// Close the writer -void FFmpegWriter::Close() { - // Write trailer (if needed) - if (!write_trailer) - WriteTrailer(); - +void FFmpegWriter::free_resources() { // Close each codec - if (video_st) + if (video_st) { close_video(oc, video_st); - if (audio_st) + video_st = NULL; + } + if (audio_st) { close_audio(oc, audio_st); + audio_st = NULL; + } // Deallocate image scalers if (image_rescalers.size() > 0) @@ -1041,14 +1017,57 @@ void FFmpegWriter::Close() { avio_close(oc->pb); } + // be sure to free any allocated AVFrames + 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 (LIBAVFORMAT_VERSION_MAJOR >= 58) + if (video_codec) { + AV_FREE_CONTEXT(video_codec); + video_codec = NULL; + } + if (audio_codec) { + AV_FREE_CONTEXT(audio_codec); + audio_codec = NULL; + } + +#elif (LIBAVFORMAT_VERSION_MAJOR <= 55) + if (video_codec) { + AV_FREE_CONTEXT(video_codec); + video_codec = NULL; + } + if (audio_codec) { + AV_FREE_CONTEXT(audio_codec); + audio_codec = NULL; + } +#endif + + if (oc) { + avformat_free_context(oc); + oc = NULL; + } +} + +// Close the writer +void FFmpegWriter::Close() { + // Write trailer (if needed) + if (!write_trailer) + WriteTrailer(); + // Reset frame counters write_video_count = 0; write_audio_count = 0; - // Free the context which frees the streams too - avformat_free_context(oc); - oc = NULL; - // Close writer is_open = false; prepare_streams = false; From c04d67026cb4d52ef255900fd75a2a10b09231b3 Mon Sep 17 00:00:00 2001 From: "FeRD (Frank Dana)" Date: Thu, 5 Mar 2020 21:13:46 -0500 Subject: [PATCH 3/3] FFmpegWriter_Tests: Add Close-reopen unit test --- tests/FFmpegWriter_Tests.cpp | 76 ++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/tests/FFmpegWriter_Tests.cpp b/tests/FFmpegWriter_Tests.cpp index cb75a118a..67133f64f 100644 --- a/tests/FFmpegWriter_Tests.cpp +++ b/tests/FFmpegWriter_Tests.cpp @@ -37,6 +37,7 @@ using namespace std; using namespace openshot; SUITE(FFMpegWriter) { + TEST(Webm) { // Reader @@ -45,7 +46,7 @@ TEST(Webm) FFmpegReader r(path.str()); r.Open(); - /* WRITER ---------------- */ + // Writer FFmpegWriter w("output1.webm"); // Set options @@ -65,7 +66,7 @@ TEST(Webm) FFmpegReader r1("output1.webm"); r1.Open(); - // Verify various settings on new MP4 + // Verify various settings on new video CHECK_EQUAL(2, r1.GetFrame(1)->GetAudioChannelsCount()); CHECK_EQUAL(24, r1.info.fps.num); CHECK_EQUAL(1, r1.info.fps.den); @@ -84,6 +85,28 @@ TEST(Webm) CHECK_CLOSE(255, (int)pixels[pixel_index + 3], 5); } +TEST(Destructor) +{ + FFmpegWriter *w = new FFmpegWriter("output2.webm"); + + // Set video options (no audio) + w->SetVideoOptions(true, "libvpx", Fraction(24,1), 1280, 720, Fraction(1,1), false, false, 30000000); + + // Dummy reader (30 FPS, 1280x720, 48kHz, 2-channel, 10.0 second duration) + DummyReader r(Fraction(30,1), 1280, 720, 48000, 2, 10.0); + r.Open(); + + // Open writer and write frames + w->Open(); + w->WriteFrame(&r, 1, 25); + + // Close and destroy writer + w->Close(); + delete w; + + CHECK(w == nullptr); +} + TEST(Options_Overloads) { // Reader @@ -92,7 +115,7 @@ TEST(Options_Overloads) FFmpegReader r(path.str()); r.Open(); - /* WRITER ---------------- */ + // Writer FFmpegWriter w("output1.mp4"); // Set options @@ -125,4 +148,51 @@ TEST(Options_Overloads) CHECK_EQUAL(true, r1.info.top_field_first); } +TEST(Close_and_reopen) +{ + // Create a Reader source, to get some frames + stringstream path; + path << TEST_MEDIA_PATH << "sintel_trailer-720p.mp4"; + FFmpegReader r(path.str()); + r.Open(); + + // A Writer to dump the frames into + FFmpegWriter w("output2.mp4"); + + // Set options + w.SetAudioOptions("aac", 44100, 188000); + w.SetVideoOptions("libxvid", 720, 480, Fraction(30000, 1001), 5000000); + + // Open writer + w.Open(); + + // Whoops, changed our mind + w.Close(); + + // Set different options + w.SetAudioOptions("aac", 48000, 192000); + w.SetVideoOptions("libx264", 1280, 720, Fraction(30, 1), 8000000); + + // Reopen writer + w.Open(); + + // Write some frames + w.WriteFrame(&r, 45, 90); + + // Close writer & reader + w.Close(); + r.Close(); + + // Read back newly-written video + FFmpegReader r1("output2.mp4"); + r1.Open(); + + // Verify new video's properties match the second Set___Options() calls + CHECK_EQUAL(48000, r1.GetFrame(1)->SampleRate()); + CHECK_EQUAL(30, r1.info.fps.num); + CHECK_EQUAL(1, r1.info.fps.den); + CHECK_EQUAL(1280, r1.info.width); + CHECK_EQUAL(720, r1.info.height); +} + } // SUITE()