From 7c0073dd775b1aaf4db4c7cfc9728e7cd2f63672 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sat, 9 Apr 2016 00:18:20 -0400 Subject: [PATCH] Unreported bug: memory and file handle leak when importing custom mpeg... I could see this on windows: Import a file such as .aiff format, while "Files of type:" reads "FFmpeg-compatible files". Leave Audacity running. Attempt to delete the file in Windows Exploerer. Get a message from Windows that the file is in use by Audacity and cannot be deleted. --- src/FFmpeg.cpp | 51 +++++++++++++++++------------ src/FFmpeg.h | 12 ++++++- src/import/ImportFFmpeg.cpp | 36 +++++++++----------- src/ondemand/ODDecodeFFmpegTask.cpp | 36 ++++++++++---------- src/ondemand/ODDecodeFFmpegTask.h | 5 +-- 5 files changed, 77 insertions(+), 63 deletions(-) diff --git a/src/FFmpeg.cpp b/src/FFmpeg.cpp index c8d89adef..179ed8657 100644 --- a/src/FFmpeg.cpp +++ b/src/FFmpeg.cpp @@ -209,12 +209,10 @@ static int64_t ufile_seek(void *opaque, int64_t pos, int whence) int ufile_close(AVIOContext *pb) { - wxFile *f = (wxFile *) pb->opaque; + std::unique_ptr f{ (wxFile *)pb->opaque }; - if (f) { + if (f) f->Close(); - delete f; - } return 0; } @@ -222,16 +220,14 @@ int ufile_close(AVIOContext *pb) // Open a file with a (possibly) Unicode filename int ufile_fopen(AVIOContext **s, const wxString & name, int flags) { - wxFile *f; wxFile::OpenMode mode; - f = new wxFile; + auto f = std::make_unique(); if (!f) { return -ENOMEM; } if (flags == (AVIO_FLAG_READ | AVIO_FLAG_WRITE)) { - delete f; return -EINVAL; } else if (flags == AVIO_FLAG_WRITE) { mode = wxFile::write; @@ -240,63 +236,76 @@ int ufile_fopen(AVIOContext **s, const wxString & name, int flags) } if (!f->Open(name, mode)) { - delete f; return -ENOENT; } *s = avio_alloc_context((unsigned char*)av_malloc(32768), 32768, flags & AVIO_FLAG_WRITE, - /*opaque*/f, + /*opaque*/f.get(), ufile_read, ufile_write, ufile_seek); if (!*s) { - delete f; return -ENOMEM; } + f.release(); // s owns the file object now + return 0; } // Detect type of input file and open it if recognized. Routine // based on the av_open_input_file() libavformat function. -int ufile_fopen_input(AVFormatContext **ic_ptr, wxString & name) +int ufile_fopen_input(std::unique_ptr &context_ptr, wxString & name) { + context_ptr.reset(); + auto context = std::make_unique(); + wxFileName f(name); wxCharBuffer fname; const char *filename; - AVIOContext *pb = NULL; int err; fname = f.GetFullName().mb_str(); filename = (const char *) fname; // Open the file to prepare for probing - if ((err = ufile_fopen(&pb, name, AVIO_FLAG_READ)) < 0) { + if ((err = ufile_fopen(&context->pb, name, AVIO_FLAG_READ)) < 0) { goto fail; } - *ic_ptr = avformat_alloc_context(); - (*ic_ptr)->pb = pb; + context->ic_ptr = avformat_alloc_context(); + context->ic_ptr->pb = context->pb; // And finally, attempt to associate an input stream with the file - err = avformat_open_input(ic_ptr, filename, NULL, NULL); + err = avformat_open_input(&context->ic_ptr, filename, NULL, NULL); if (err) { goto fail; } + // success + context_ptr = std::move(context); return 0; fail: - if (pb) { - ufile_close(pb); + return err; +} + +FFmpegContext::~FFmpegContext() +{ + if (FFmpegLibsInst->ValidLibsLoaded()) + { + if (ic_ptr) + avformat_close_input(&ic_ptr); + av_log_set_callback(av_log_default_callback); } - *ic_ptr = NULL; - - return err; + if (pb) { + ufile_close(pb); + av_free(pb); + } } streamContext *import_ffmpeg_read_next_frame(AVFormatContext* formatContext, diff --git a/src/FFmpeg.h b/src/FFmpeg.h index 6c4e562b8..807e87e06 100644 --- a/src/FFmpeg.h +++ b/src/FFmpeg.h @@ -393,8 +393,18 @@ FFmpegLibs *PickFFmpegLibs(); ///! anymore, or just decrements it's reference count void DropFFmpegLibs(); +// This object allows access to the AVFormatContext, +// and its destructor cleans up memory and file handles +struct FFmpegContext { + FFmpegContext() {} + ~FFmpegContext(); + + AVIOContext *pb{}; + AVFormatContext *ic_ptr{}; +}; + int ufile_fopen(AVIOContext **s, const wxString & name, int flags); -int ufile_fopen_input(AVFormatContext **ic_ptr, wxString & name); +int ufile_fopen_input(std::unique_ptr &context_ptr, wxString & name); int ufile_close(AVIOContext *pb); struct streamContext; diff --git a/src/import/ImportFFmpeg.cpp b/src/import/ImportFFmpeg.cpp index fc658c149..b15e6f507 100644 --- a/src/import/ImportFFmpeg.cpp +++ b/src/import/ImportFFmpeg.cpp @@ -262,6 +262,7 @@ public: private: + std::shared_ptr mContext; // An object that does proper IO shutdown in its destructor; may be shared with decoder task. AVFormatContext *mFormatContext; //!< Format description, also contains metadata and some useful info int mNumStreams; //!< mNumstreams is less or equal to mFormatContext->nb_streams ScsPtr mScs; //!< Points to array of pointers to stream contexts, which may be shared with a decoder task. @@ -358,12 +359,20 @@ bool FFmpegImportFileHandle::Init() av_log_set_callback(av_log_wx_callback); - int err = ufile_fopen_input(&mFormatContext, mName); - if (err < 0) + int err; { - wxLogError(wxT("FFmpeg : av_open_input_file() failed for file %s"),mName.c_str()); - return false; + std::unique_ptr tempContext; + err = ufile_fopen_input(tempContext, mName); + if (err < 0) + { + wxLogError(wxT("FFmpeg : av_open_input_file() failed for file %s"), mName.c_str()); + return false; + } + wxASSERT(tempContext.get()); + // Move from unique to shared pointer + mContext.reset(tempContext.release()); } + mFormatContext = mContext->ic_ptr; err = avformat_find_stream_info(mFormatContext, NULL); if (err < 0) @@ -573,7 +582,7 @@ int FFmpegImportFileHandle::Import(TrackFactory *trackFactory, for (const auto &stream : mChannels) { ++s; ODDecodeFFmpegTask* odTask = - new ODDecodeFFmpegTask(mScs, ODDecodeFFmpegTask::FromList(mChannels), mFormatContext, s); + new ODDecodeFFmpegTask(mScs, ODDecodeFFmpegTask::FromList(mChannels), mContext, s); odTask->CreateFileDecoder(mFilename); //each stream has different duration. We need to know it if seeking is to be allowed. @@ -852,21 +861,8 @@ void FFmpegImportFileHandle::GetMetadata(Tags *tags, const wxChar *tag, const ch FFmpegImportFileHandle::~FFmpegImportFileHandle() { -#ifdef EXPERIMENTAL_OD_FFMPEG - //ODDecodeFFmpegTask takes ownership and deltes it there. - if(!mUsingOD) - { -#endif - if (FFmpegLibsInst->ValidLibsLoaded()) - { - if (mFormatContext) avformat_close_input(&mFormatContext); - av_log_set_callback(av_log_default_callback); - } - -#ifdef EXPERIMENTAL_OD_FFMPEG - }//mUsingOD -#endif - + // Do this before unloading the libraries + mContext.reset(); delete mStreamInfo; diff --git a/src/ondemand/ODDecodeFFmpegTask.cpp b/src/ondemand/ODDecodeFFmpegTask.cpp index fda527a06..a71c7f970 100644 --- a/src/ondemand/ODDecodeFFmpegTask.cpp +++ b/src/ondemand/ODDecodeFFmpegTask.cpp @@ -62,7 +62,7 @@ public: ODFFmpegDecoder(const wxString & fileName, const ScsPtr &scs, ODDecodeFFmpegTask::Streams &&channels, - AVFormatContext* formatContext, + const std::shared_ptr &formatContext, int streamIndex); virtual ~ODFFmpegDecoder(); @@ -99,7 +99,7 @@ private: ScsPtr mScs; //!< Pointer to array of pointers to stream contexts. ODDecodeFFmpegTask::Streams mChannels; - AVFormatContext *mFormatContext; //!< Format description, also contains metadata and some useful info + std::shared_ptr mContext; //!< Format description, also contains metadata and some useful info std::vector mDecodeCache; int mNumSamplesInCache; sampleCount mCurrentPos; //the index of the next sample to be decoded @@ -128,11 +128,11 @@ auto ODDecodeFFmpegTask::FromList(const std::list &channels) -> St } //------ ODDecodeFFmpegTask definitions -ODDecodeFFmpegTask::ODDecodeFFmpegTask(const ScsPtr &scs, Streams &&channels, void* formatContext, int streamIndex) +ODDecodeFFmpegTask::ODDecodeFFmpegTask(const ScsPtr &scs, Streams &&channels, const std::shared_ptr &context, int streamIndex) : mChannels(std::move(channels)) { mScs=scs; - mFormatContext = formatContext; + mContext = context; mStreamIndex = streamIndex; } ODDecodeFFmpegTask::~ODDecodeFFmpegTask() @@ -142,7 +142,7 @@ ODDecodeFFmpegTask::~ODDecodeFFmpegTask() std::unique_ptr ODDecodeFFmpegTask::Clone() const { - auto clone = std::make_unique(mScs, Streams{ mChannels }, mFormatContext, mStreamIndex); + auto clone = std::make_unique(mScs, Streams{ mChannels }, mContext, mStreamIndex); clone->mDemandSample=GetDemandSample(); //the decoders and blockfiles should not be copied. They are created as the task runs. @@ -157,7 +157,7 @@ ODFileDecoder* ODDecodeFFmpegTask::CreateFileDecoder(const wxString & fileName) // Open the file for import auto decoder = make_movable(fileName, mScs, ODDecodeFFmpegTask::Streams{ mChannels }, - (AVFormatContext*)mFormatContext, mStreamIndex); + mContext, mStreamIndex); mDecoders.push_back(std::move(decoder)); return mDecoders.back().get(); @@ -206,13 +206,15 @@ bool ODFFmpegDecoder::SeekingAllowed() // url_fseek(mFormatContext->pb,0,SEEK_SET); + std::unique_ptr tempMpegContext; AVFormatContext* tempContext; int err; - err = ufile_fopen_input(&tempContext, mFName); + err = ufile_fopen_input(tempMpegContext, mFName); if (err < 0) { goto test_failed; } + tempContext = tempMpegContext->ic_ptr; err = avformat_find_stream_info(tempContext, NULL); if (err < 0) @@ -222,12 +224,9 @@ bool ODFFmpegDecoder::SeekingAllowed() if(av_seek_frame(tempContext, st->index, 0, 0) >= 0) { mSeekingAllowedStatus = ODFFMPEG_SEEKING_TEST_SUCCESS; - if (tempContext) avformat_close_input(&tempContext); return SeekingAllowed(); } - if (tempContext) avformat_close_input(&tempContext); - test_failed: mSeekingAllowedStatus = ODFFMPEG_SEEKING_TEST_FAILED; return SeekingAllowed(); @@ -239,12 +238,12 @@ test_failed: ODFFmpegDecoder::ODFFmpegDecoder(const wxString & fileName, const ScsPtr &scs, ODDecodeFFmpegTask::Streams &&channels, - AVFormatContext* formatContext, - int streamIndex) + const std::shared_ptr &context, +int streamIndex) :ODFileDecoder(fileName), //mSamplesDone(0), mScs(scs), -mFormatContext(formatContext), +mContext(context), mNumSamplesInCache(0), mCurrentLen(0), mSeekingAllowedStatus(ODFFMPEG_SEEKING_TEST_UNKNOWN), @@ -271,11 +270,8 @@ mStreamIndex(streamIndex) //we have taken ownership, so DELETE the ffmpeg stuff allocated in ImportFFmpeg that was given to us. ODFFmpegDecoder::~ODFFmpegDecoder() { - if (FFmpegLibsInst->ValidLibsLoaded()) - { - if (mFormatContext) avformat_close_input(&mFormatContext); - av_log_set_callback(av_log_default_callback); - } + // Do this before unloading libraries + mContext.reset(); //DELETE our caches. while(mDecodeCache.size()) @@ -295,6 +291,8 @@ ODFFmpegDecoder::~ODFFmpegDecoder() #define kMaxSeekRewindAttempts 8 int ODFFmpegDecoder::Decode(SampleBuffer & data, sampleFormat & format, sampleCount start, sampleCount len, unsigned int channel) { + auto mFormatContext = mContext->ic_ptr; + auto scs = mScs->get(); auto sci = scs[mStreamIndex].get(); format = sci->m_osamplefmt; @@ -592,7 +590,7 @@ streamContext* ODFFmpegDecoder::ReadNextFrame() auto scs = mScs->get(); // This reinterpret_cast to array of plain pointers is innocent return import_ffmpeg_read_next_frame - (mFormatContext, reinterpret_cast(scs), mChannels.size()); + (mContext->ic_ptr, reinterpret_cast(scs), mChannels.size()); } diff --git a/src/ondemand/ODDecodeFFmpegTask.h b/src/ondemand/ODDecodeFFmpegTask.h index 3afad0a0f..404e0b58b 100644 --- a/src/ondemand/ODDecodeFFmpegTask.h +++ b/src/ondemand/ODDecodeFFmpegTask.h @@ -21,6 +21,7 @@ #include "ODDecodeTask.h" #include "ODTaskThread.h" +struct FFmpegContext; class ODFileDecoder; class WaveTrack; /// A class representing a modular task to be used with the On-Demand structures. @@ -33,7 +34,7 @@ public: static Streams FromList(const std::list &channels); /// Constructs an ODTask - ODDecodeFFmpegTask(const ScsPtr &scs, Streams &&channels, void* formatContext, int streamIndex); + ODDecodeFFmpegTask(const ScsPtr &scs, Streams &&channels, const std::shared_ptr &context, int streamIndex); virtual ~ODDecodeFFmpegTask(); std::unique_ptr Clone() const override; @@ -49,7 +50,7 @@ protected: Streams mChannels; ScsPtr mScs; - void* mFormatContext; + std::shared_ptr mContext; int mStreamIndex; }; #endif //__ODDECODEFFMPEGTASK__