From 5de19dc95292fece133dd675f266584eef0fc0ad Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Tue, 20 Dec 2016 11:03:02 -0500 Subject: [PATCH 1/3] ODDecode task checks blockfile error, making no progress if so... ... also replace explicit mutex locking with RAII and atomics. This is a part of the big project that handles failure to write block files, as from disk exhaustion. ODDecodeBlockFile::WriteODDecodeBlockFile is the one place calling WriteSimpleBlockFile but not (as in SimpleBlockFile constructor) throwing an exception. It is called only when attempting to recover files at open time, or in worker threads in an EXPERIMENTAL code branch. --- src/blockfile/ODDecodeBlockFile.cpp | 66 ++++++++++++----------------- src/blockfile/ODDecodeBlockFile.h | 4 +- 2 files changed, 30 insertions(+), 40 deletions(-) diff --git a/src/blockfile/ODDecodeBlockFile.cpp b/src/blockfile/ODDecodeBlockFile.cpp index 19ba836f0..eb04c676b 100644 --- a/src/blockfile/ODDecodeBlockFile.cpp +++ b/src/blockfile/ODDecodeBlockFile.cpp @@ -47,7 +47,6 @@ ODDecodeBlockFile::ODDecodeBlockFile(wxFileNameWrapper &&baseFileName, wxFileNam mAliasChannel(aliasChannel) { mDecoder = NULL; - mDataAvailable=false; mAudioFileName = std::move(audioFileName); mFormat = int16Sample; } @@ -59,11 +58,11 @@ ODDecodeBlockFile::ODDecodeBlockFile(wxFileNameWrapper &&existingFile, wxFileNam SimpleBlockFile{ std::move(existingFile), aliasLen, min, max, rms }, mType(decodeType), + mDataAvailable( dataAvailable ), mAliasStart(aliasStart), mAliasChannel(aliasChannel) { mDecoder = NULL; - mDataAvailable=dataAvailable; mAudioFileName = std::move(audioFileName); mFormat = int16Sample; } @@ -304,11 +303,7 @@ bool ODDecodeBlockFile::IsSummaryAvailable() const bool ODDecodeBlockFile::IsDataAvailable() const { - bool retval; - mDataAvailableMutex.Lock(); - retval= mDataAvailable; - mDataAvailableMutex.Unlock(); - return retval; + return mDataAvailable; } /// Write the summary to disk, using the derived ReadData() to get the data @@ -321,43 +316,38 @@ int ODDecodeBlockFile::WriteODDecodeBlockFile() // derived classes) to get the sample data SampleBuffer sampleData;// = NewSamples(mLen, floatSample); int ret; - //use the decoder here. - mDecoderMutex.Lock(); - if(!mDecoder) { - mDecoderMutex.Unlock(); - return -1; + //use the decoder here. + ODLocker locker{ &mDecoderMutex }; + + if(!mDecoder) + return -1; + + //sampleData and mFormat are set by the decoder. + ret = mDecoder->Decode(sampleData, mFormat, mAliasStart, mLen, mAliasChannel); + + if(ret < 0) { + printf("ODDecodeBlockFile Decode failure\n"); + return ret; //failure + } } - - //sampleData and mFormat are set by the decoder. - ret = mDecoder->Decode(sampleData, mFormat, mAliasStart, mLen, mAliasChannel); - - mDecoderMutex.Unlock(); - if(ret < 0) { - printf("ODDecodeBlockFile Decode failure\n"); - return ret; //failure + { + //the summary is also calculated here. + ODLocker locker{ &mFileNameMutex }; + //TODO: we may need to write a version of WriteSimpleBlockFile that uses threadsafe FILE vs wxFile + bool bSuccess = + WriteSimpleBlockFile( + sampleData.ptr(), + mLen, + mFormat, + NULL); + if ( !bSuccess ) + return -1; } - //the summary is also calculated here. - mFileNameMutex.Lock(); - //TODO: we may need to write a version of WriteSimpleBlockFile that uses threadsafe FILE vs wxFile - bool bSuccess = - WriteSimpleBlockFile( - sampleData.ptr(), - mLen, - mFormat, - NULL); - wxASSERT(bSuccess); // TODO: Handle failure here by alert to user and undo partial op. - wxUnusedVar(bSuccess); - - mFileNameMutex.Unlock(); - - - mDataAvailableMutex.Lock(); - mDataAvailable=true; - mDataAvailableMutex.Unlock(); + wxAtomicInc( mDataAvailable ); return ret; } diff --git a/src/blockfile/ODDecodeBlockFile.h b/src/blockfile/ODDecodeBlockFile.h index 0ec7150da..06d66f58d 100644 --- a/src/blockfile/ODDecodeBlockFile.h +++ b/src/blockfile/ODDecodeBlockFile.h @@ -32,6 +32,7 @@ Also, see ODPCMAliasBlockFile for a similar file. #include "../ondemand/ODTaskThread.h" #include "../DirManager.h" #include "../ondemand/ODDecodeTask.h" +#include #include /// An AliasBlockFile that references uncompressed data in an existing file @@ -166,8 +167,7 @@ class ODDecodeBlockFile final : public SimpleBlockFile ///The original file the audio came from. wxFileNameWrapper mAudioFileName; - mutable ODLock mDataAvailableMutex; - bool mDataAvailable; + wxAtomicInt mDataAvailable{ 0 }; bool mDataBeingComputed; ODFileDecoder* mDecoder; From 1a3212611bb6edf586a437fb3ec3ea3099a1e110 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Mon, 26 Dec 2016 14:56:32 -0500 Subject: [PATCH 2/3] ODComputeSummaryTask is safe for exceptions; makes no progress then --- src/Project.cpp | 3 ++ src/ondemand/ODComputeSummaryTask.cpp | 40 +++++++++++++++++---------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/Project.cpp b/src/Project.cpp index 400a5e765..134a77eb7 100644 --- a/src/Project.cpp +++ b/src/Project.cpp @@ -4412,6 +4412,9 @@ void AudacityProject::PopState(const UndoState &state) computeTask = make_movable(); odUsed=true; } + // PRL: Is it correct to add all tracks to one task, even if they + // are not partnered channels? Rather than + // make one task for each? computeTask->AddWaveTrack((WaveTrack*)copyTrack); } } diff --git a/src/ondemand/ODComputeSummaryTask.cpp b/src/ondemand/ODComputeSummaryTask.cpp index 606eb6ac7..324f30717 100644 --- a/src/ondemand/ODComputeSummaryTask.cpp +++ b/src/ondemand/ODComputeSummaryTask.cpp @@ -64,32 +64,42 @@ void ODComputeSummaryTask::DoSomeInternal() return; } - sampleCount blockStartSample = 0; - sampleCount blockEndSample = 0; - bool success =false; - mBlockFilesMutex.Lock(); for(size_t i=0; i < mWaveTracks.size() && mBlockFiles.size();i++) { + bool success = false; const auto bf = mBlockFiles[0].lock(); + sampleCount blockStartSample = 0; + sampleCount blockEndSample = 0; + if(bf) { - bf->DoWriteSummary(); + // WriteSummary might throw, but this is a worker thread, so stop + // the exceptions here! success = true; + try { bf->DoWriteSummary(); } + catch(...) { success = false; } blockStartSample = bf->GetStart(); blockEndSample = blockStartSample + bf->GetLength(); } else { + success = true; // The block file disappeared. //the waveform in the wavetrack now is shorter, so we need to update mMaxBlockFiles //because now there is less work to do. mMaxBlockFiles--; } - //take it out of the array - we are done with it. - mBlockFiles.erase(mBlockFiles.begin()); + if (success) + { + //take it out of the array - we are done with it. + mBlockFiles.erase(mBlockFiles.begin()); + } + else + // The task does not make progress + ; //This is a bit of a convenience in case someone tries to terminate the task by closing the trackpanel or window. //ODComputeSummaryTask::Terminate() uses this lock to remove everything, and we don't want it to wait since the UI is being blocked. @@ -97,16 +107,18 @@ void ODComputeSummaryTask::DoSomeInternal() wxThread::This()->Yield(); mBlockFilesMutex.Lock(); - //upddate the gui for all associated blocks. It doesn't matter that we're hitting more wavetracks then we should + //update the gui for all associated blocks. It doesn't matter that we're hitting more wavetracks then we should //because this loop runs a number of times equal to the number of tracks, they probably are getting processed in //the next iteration at the same sample window. - mWaveTrackMutex.Lock(); - for(size_t i=0;iAddInvalidRegion(blockStartSample,blockEndSample); + if (success && bf) { + mWaveTrackMutex.Lock(); + for(size_t i=0;iAddInvalidRegion(blockStartSample,blockEndSample); + } + mWaveTrackMutex.Unlock(); } - mWaveTrackMutex.Unlock(); } mBlockFilesMutex.Unlock(); From 1c91c2b8046c413db5dfcf68c11bc5890848fd8e Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Mon, 26 Dec 2016 15:57:46 -0500 Subject: [PATCH 3/3] ODDecodeTask makes progress unless there is an exception --- src/ondemand/ODDecodeTask.cpp | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/ondemand/ODDecodeTask.cpp b/src/ondemand/ODDecodeTask.cpp index a1e675795..91e7d78fe 100644 --- a/src/ondemand/ODDecodeTask.cpp +++ b/src/ondemand/ODDecodeTask.cpp @@ -42,13 +42,13 @@ void ODDecodeTask::DoSomeInternal() } ODFileDecoder* decoder; - sampleCount blockStartSample = 0; - sampleCount blockEndSample = 0; - bool success =false; for(size_t i=0; i < mWaveTracks.size() && mBlockFiles.size();i++) { const auto bf = mBlockFiles[0].lock(); + sampleCount blockStartSample = 0; + sampleCount blockEndSample = 0; + bool success =false; int ret = 1; @@ -63,6 +63,7 @@ void ODDecodeTask::DoSomeInternal() if(!decoder->IsInitialized()) decoder->Init(); bf->SetODFileDecoder(decoder); + // Does not throw: ret = bf->DoWriteBlockFile(); bf->UnlockRead(); @@ -74,24 +75,31 @@ void ODDecodeTask::DoSomeInternal() } else { + success = true; // The block file disappeared. //the waveform in the wavetrack now is shorter, so we need to update mMaxBlockFiles //because now there is less work to do. mMaxBlockFiles--; } - //Release the refcount we placed on it if we are successful - if(ret >= 0 ) { + if (success) + { //take it out of the array - we are done with it. mBlockFiles.erase(mBlockFiles.begin()); + } + else + // The task does not make progress + ; + //Release the refcount we placed on it if we are successful + if( bf && success ) { //upddate the gui for all associated blocks. It doesn't matter that we're hitting more wavetracks then we should //because this loop runs a number of times equal to the number of tracks, they probably are getting processed in //the next iteration at the same sample window. mWaveTrackMutex.Lock(); for(size_t i=0;iAddInvalidRegion(blockStartSample,blockEndSample); } mWaveTrackMutex.Unlock();