From 875e8e09843474831db2f742bc3d167e88eebb16 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Tue, 7 Jul 2020 16:41:50 -0400 Subject: [PATCH] Cross project copy data loss (#604) * Need only CloseLock now, not old Lock and Unlock... ... which were for cross-project cut and paste, but they no longer work and we need another solution. So delete much old code. * Fix dangling reference to AudacityProject completely! ... ... in SqliteSampleBlockFactory: retain ONLY the shared pointer to ProjectFileIO, then pass that, not project, to constructors of blocks. completing the work of 127696879dcc5ca687ec50a4ccef7acbed563926 * Restore part of the Bug2436 fix... ... which needs the non-default arguments to WaveTrack::EmptyCopy that got lost at d39590cf41e1e1eac02fc52d88a1ad018824f77b So that pasted WaveTracks refer to the correct SampleBlockFactory and database for their project But this is not yet a sufficient re-fix for the bug * Complete the fix for cross-project copies and 2436... ... by duplicating sample blocks, in Sequence.cpp, when it is wrong just to share them. And to determine which case it is, see whether source and destination Sequences have the same sample block factories when doing Copy or Paste. Duplicate when the factories are different. Otherwise sharing is safe and more space efficient. This does the analogous to what DirManager::CopyBlockFile did before commit d39590c. --- src/SampleBlock.h | 2 - src/Sequence.cpp | 83 +++++++++++++++++++++++---------------- src/Sequence.h | 13 +++--- src/SqliteSampleBlock.cpp | 37 ++++++----------- src/Track.h | 3 ++ src/WaveClip.cpp | 16 +------- src/WaveClip.h | 7 +--- src/WaveTrack.cpp | 17 -------- src/WaveTrack.h | 26 +----------- src/menus/EditMenus.cpp | 26 ++---------- 10 files changed, 78 insertions(+), 152 deletions(-) diff --git a/src/SampleBlock.h b/src/SampleBlock.h index 8cec62e24..55d1037fb 100644 --- a/src/SampleBlock.h +++ b/src/SampleBlock.h @@ -46,8 +46,6 @@ class SampleBlock public: virtual ~SampleBlock(); - virtual void Lock() = 0; - virtual void Unlock() = 0; virtual void CloseLock() = 0; virtual SampleBlockID GetBlockID() = 0; diff --git a/src/Sequence.cpp b/src/Sequence.cpp index 425bcbee3..c6f8bffe7 100644 --- a/src/Sequence.cpp +++ b/src/Sequence.cpp @@ -83,14 +83,6 @@ size_t Sequence::GetIdealBlockSize() const return mMaxSamples; } -bool Sequence::Lock() -{ - for (unsigned int i = 0; i < mBlock.size(); i++) - mBlock[i].sb->Lock(); - - return true; -} - bool Sequence::CloseLock() { for (unsigned int i = 0; i < mBlock.size(); i++) @@ -99,14 +91,6 @@ bool Sequence::CloseLock() return true; } -bool Sequence::Unlock() -{ - for (unsigned int i = 0; i < mBlock.size(); i++) - mBlock[i].sb->Unlock(); - - return true; -} - sampleFormat Sequence::GetSampleFormat() const { return mSampleFormat; @@ -380,13 +364,21 @@ float Sequence::GetRMS(sampleCount start, sampleCount len, bool mayThrow) const return sqrt(sumsq / length.as_double() ); } -std::unique_ptr Sequence::Copy(sampleCount s0, sampleCount s1) const +// Must pass in the correct factory for the result. If it's not the same +// as in this, then block contents must be copied. +std::unique_ptr Sequence::Copy( const SampleBlockFactoryPtr &pFactory, + sampleCount s0, sampleCount s1) const { - auto dest = std::make_unique(mpFactory, mSampleFormat); + // Make a new Sequence object for the specified factory: + auto dest = std::make_unique(pFactory, mSampleFormat); if (s0 >= s1 || s0 >= mNumSamples || s1 < 0) { return dest; } + // Decide whether to share sample blocks or make new copies, when whole block + // contents are used -- must copy if factories are different: + auto pUseFactory = (pFactory == mpFactory) ? nullptr : pFactory.get(); + int numBlocks = mBlock.size(); int b0 = FindBlock(s0); @@ -404,7 +396,7 @@ std::unique_ptr Sequence::Copy(sampleCount s0, sampleCount s1) const int blocklen; - // Do the first block + // Do any initial partial block const SeqBlock &block0 = mBlock[b0]; if (s0 != block0.start) { @@ -421,13 +413,15 @@ std::unique_ptr Sequence::Copy(sampleCount s0, sampleCount s1) const else --b0; - // If there are blocks in the middle, copy the blockfiles directly + // If there are blocks in the middle, use the blockfiles whole for (int bb = b0 + 1; bb < b1; ++bb) - AppendBlock(dest->mBlock, dest->mNumSamples, mBlock[bb]); + AppendBlock(pUseFactory, mSampleFormat, + dest->mBlock, dest->mNumSamples, mBlock[bb]); // Increase ref count or duplicate file // Do the last block if (b1 > b0) { + // Probable case of a partial block const SeqBlock &block = mBlock[b1]; const auto &sb = block.sb; // s1 is within block: @@ -439,8 +433,9 @@ std::unique_ptr Sequence::Copy(sampleCount s0, sampleCount s1) const dest->Append(buffer.ptr(), mSampleFormat, blocklen); } else - // Special case, copy exactly - AppendBlock(dest->mBlock, dest->mNumSamples, block); + // Special case of a whole block + AppendBlock(pUseFactory, mSampleFormat, + dest->mBlock, dest->mNumSamples, block); // Increase ref count or duplicate file } @@ -454,13 +449,27 @@ namespace { { return numSamples > wxLL(9223372036854775807); } + + SampleBlockPtr ShareOrCopySampleBlock( + SampleBlockFactory *pFactory, sampleFormat format, SampleBlockPtr sb ) + { + if ( pFactory ) { + // must copy contents to a fresh SampleBlock object in another database + auto sampleCount = sb->GetSampleCount(); + SampleBuffer buffer{ sampleCount, format }; + sb->GetSamples( buffer.ptr(), format, 0, sampleCount ); + sb = pFactory->Create( buffer.ptr(), sampleCount, format ); + } + else + // Can just share + ; + return sb; + } } void Sequence::Paste(sampleCount s, const Sequence *src) // STRONG-GUARANTEE { - auto &factory = *mpFactory; - if ((s < 0) || (s > mNumSamples)) { wxLogError( @@ -501,6 +510,11 @@ void Sequence::Paste(sampleCount s, const Sequence *src) const size_t numBlocks = mBlock.size(); + // Decide whether to share sample blocks or make new copies, when whole block + // contents are used -- must copy if factories are different: + auto pUseFactory = + (src->mpFactory == mpFactory) ? nullptr : mpFactory.get(); + if (numBlocks == 0 || (s == mNumSamples && mBlock.back().sb->GetSampleCount() >= mMinSamples)) { // Special case: this track is currently empty, or it's safe to append @@ -513,8 +527,8 @@ void Sequence::Paste(sampleCount s, const Sequence *src) for (unsigned int i = 0; i < srcNumBlocks; i++) // AppendBlock may throw for limited disk space, if pasting from // one project into another. - AppendBlock(newBlock, samples, srcBlock[i]); - // Increase ref count or duplicate file + AppendBlock(pUseFactory, mSampleFormat, + newBlock, samples, srcBlock[i]); CommitChangesIfConsistent (newBlock, samples, wxT("Paste branch one")); @@ -549,7 +563,7 @@ void Sequence::Paste(sampleCount s, const Sequence *src) splitPoint, length - splitPoint, true); // largerBlockLen is not more than mMaxSamples... - block.sb = factory.Create( + block.sb = mpFactory->Create( buffer.ptr(), largerBlockLen.as_size_t(), mSampleFormat); @@ -605,7 +619,7 @@ void Sequence::Paste(sampleCount s, const Sequence *src) // The final case is that we're inserting at least five blocks. // We divide these into three groups: the first two get merged // with the first half of the split block, the middle ones get - // copied in as is, and the last two get merged with the last + // used whole, and the last two get merged with the last // half of the split block. const auto srcFirstTwoLen = @@ -630,7 +644,9 @@ void Sequence::Paste(sampleCount s, const Sequence *src) for (i = 2; i < srcNumBlocks - 2; i++) { const SeqBlock &block = srcBlock[i]; - newBlock.push_back(SeqBlock(block.sb, block.start + s)); + auto sb = ShareOrCopySampleBlock( + pUseFactory, mSampleFormat, block.sb ); + newBlock.push_back(SeqBlock(sb, block.start + s)); } auto lastStart = penultimate.start; @@ -710,14 +726,15 @@ void Sequence::InsertSilence(sampleCount s0, sampleCount len) Paste(s0, &sTrack); } -void Sequence::AppendBlock(BlockArray &mBlock, sampleCount &mNumSamples, const SeqBlock &b) +void Sequence::AppendBlock( SampleBlockFactory *pFactory, sampleFormat format, + BlockArray &mBlock, sampleCount &mNumSamples, const SeqBlock &b) { // Quick check to make sure that it doesn't overflow if (Overflows((mNumSamples.as_double()) + ((double)b.sb->GetSampleCount()))) THROW_INCONSISTENCY_EXCEPTION; - // Bump ref count - SeqBlock newBlock(b.sb, mNumSamples); + auto sb = ShareOrCopySampleBlock( pFactory, format, b.sb ); + SeqBlock newBlock(sb, mNumSamples); // We can assume newBlock.sb is not null diff --git a/src/Sequence.h b/src/Sequence.h index 9920baa4d..e37eca00e 100644 --- a/src/Sequence.h +++ b/src/Sequence.h @@ -95,7 +95,10 @@ class PROFILE_DLL_API Sequence final : public XMLTagHandler{ size_t len, const sampleCount *where) const; // Return non-null, or else throw! - std::unique_ptr Copy(sampleCount s0, sampleCount s1) const; + // Must pass in the correct factory for the result. If it's not the same + // as in this, then block contents must be copied. + std::unique_ptr Copy( const SampleBlockFactoryPtr &pFactory, + sampleCount s0, sampleCount s1) const; void Paste(sampleCount s0, const Sequence *src); size_t GetIdealAppendLen() const; @@ -125,10 +128,7 @@ class PROFILE_DLL_API Sequence final : public XMLTagHandler{ // for details. // - bool Lock(); - bool Unlock(); - - bool CloseLock();//similar to Lock but should be called upon project close. + bool CloseLock();//should be called upon project close. // not balanced by unlocking calls. // @@ -199,7 +199,8 @@ class PROFILE_DLL_API Sequence final : public XMLTagHandler{ int FindBlock(sampleCount pos) const; - static void AppendBlock(BlockArray &blocks, + static void AppendBlock(SampleBlockFactory *pFactory, sampleFormat format, + BlockArray &blocks, sampleCount &numSamples, const SeqBlock &b); diff --git a/src/SqliteSampleBlock.cpp b/src/SqliteSampleBlock.cpp index 2e430c362..e28272c60 100644 --- a/src/SqliteSampleBlock.cpp +++ b/src/SqliteSampleBlock.cpp @@ -22,11 +22,9 @@ class SqliteSampleBlock final : public SampleBlock { public: - explicit SqliteSampleBlock(AudacityProject *project); + explicit SqliteSampleBlock(ProjectFileIO &io); ~SqliteSampleBlock() override; - void Lock() override; - void Unlock() override; void CloseLock() override; void SetSamples(samplePtr src, size_t numsamples, sampleFormat srcformat); @@ -83,7 +81,7 @@ private: bool mValid; bool mDirty; bool mSilent; - int mRefCnt; + bool mLocked = false; SampleBlockID mBlockID; @@ -131,13 +129,11 @@ public: const wxChar **attrs) override; private: - AudacityProject &mProject; std::shared_ptr mpIO; }; SqliteSampleBlockFactory::SqliteSampleBlockFactory( AudacityProject &project ) - : mProject{ project } - , mpIO{ ProjectFileIO::Get(project).shared_from_this() } + : mpIO{ ProjectFileIO::Get(project).shared_from_this() } { } @@ -147,7 +143,7 @@ SqliteSampleBlockFactory::~SqliteSampleBlockFactory() = default; SampleBlockPtr SqliteSampleBlockFactory::DoCreate( samplePtr src, size_t numsamples, sampleFormat srcformat ) { - auto sb = std::make_shared(&mProject); + auto sb = std::make_shared(*mpIO); sb->SetSamples(src, numsamples, srcformat); return sb; } @@ -155,7 +151,7 @@ SampleBlockPtr SqliteSampleBlockFactory::DoCreate( SampleBlockPtr SqliteSampleBlockFactory::DoCreateSilent( size_t numsamples, sampleFormat srcformat ) { - auto sb = std::make_shared(&mProject); + auto sb = std::make_shared(*mpIO); sb->SetSilent(numsamples, srcformat); return sb; } @@ -164,7 +160,7 @@ SampleBlockPtr SqliteSampleBlockFactory::DoCreateSilent( SampleBlockPtr SqliteSampleBlockFactory::DoCreateFromXML( sampleFormat srcformat, const wxChar **attrs ) { - auto sb = std::make_shared(&mProject); + auto sb = std::make_shared(*mpIO); sb->mSampleFormat = srcformat; int found = 0; @@ -230,17 +226,16 @@ SampleBlockPtr SqliteSampleBlockFactory::DoCreateFromXML( SampleBlockPtr SqliteSampleBlockFactory::DoGet( SampleBlockID sbid ) { - auto sb = std::make_shared(&mProject); + auto sb = std::make_shared(*mpIO); sb->Load(sbid); return sb; } -SqliteSampleBlock::SqliteSampleBlock(AudacityProject *project) -: mIO(ProjectFileIO::Get(*project)) +SqliteSampleBlock::SqliteSampleBlock(ProjectFileIO &io) +: mIO(io) { mValid = false; mSilent = false; - mRefCnt = 0; mBlockID = 0; @@ -258,7 +253,7 @@ SqliteSampleBlock::SqliteSampleBlock(AudacityProject *project) SqliteSampleBlock::~SqliteSampleBlock() { // See ProjectFileIO::Bypass() for a description of mIO.mBypass - if (mRefCnt == 0 && !mIO.ShouldBypass()) + if (!mLocked && !mIO.ShouldBypass()) { // In case Delete throws, don't let an exception escape a destructor, // but we can still enqueue the delayed handler so that an error message @@ -269,19 +264,9 @@ SqliteSampleBlock::~SqliteSampleBlock() } } -void SqliteSampleBlock::Lock() -{ - ++mRefCnt; -} - -void SqliteSampleBlock::Unlock() -{ - --mRefCnt; -} - void SqliteSampleBlock::CloseLock() { - Lock(); + mLocked = true; } SampleBlockID SqliteSampleBlock::GetBlockID() diff --git a/src/Track.h b/src/Track.h index 2babc2486..254631107 100644 --- a/src/Track.h +++ b/src/Track.h @@ -1581,6 +1581,9 @@ class AUDACITY_DLL_API TrackFactory final TrackFactory( const TrackFactory & ) PROHIBITED; TrackFactory &operator=( const TrackFactory & ) PROHIBITED; + const SampleBlockFactoryPtr &GetSampleBlockFactory() const + { return mpFactory; } + private: const ProjectSettings &mSettings; SampleBlockFactoryPtr mpFactory; diff --git a/src/WaveClip.cpp b/src/WaveClip.cpp index c94d54c2b..9e575bb50 100644 --- a/src/WaveClip.cpp +++ b/src/WaveClip.cpp @@ -182,7 +182,7 @@ WaveClip::WaveClip(const WaveClip& orig, orig.TimeToSamplesClip(t0, &s0); orig.TimeToSamplesClip(t1, &s1); - mSequence = orig.mSequence->Copy(s0, s1); + mSequence = orig.mSequence->Copy(factory, s0, s1); mEnvelope = std::make_unique( *orig.mEnvelope, @@ -1652,13 +1652,6 @@ void WaveClip::OffsetCutLines(double t0, double len) } } -void WaveClip::Lock() -{ - GetSequence()->Lock(); - for (const auto &cutline: mCutLines) - cutline->Lock(); -} - void WaveClip::CloseLock() { GetSequence()->CloseLock(); @@ -1666,13 +1659,6 @@ void WaveClip::CloseLock() cutline->CloseLock(); } -void WaveClip::Unlock() -{ - GetSequence()->Unlock(); - for (const auto &cutline: mCutLines) - cutline->Unlock(); -} - void WaveClip::SetRate(int rate) { mRate = rate; diff --git a/src/WaveClip.h b/src/WaveClip.h index 3b4ca8796..03411a147 100644 --- a/src/WaveClip.h +++ b/src/WaveClip.h @@ -328,12 +328,7 @@ public: /// Offset cutlines right to time 't0' by time amount 'len' void OffsetCutLines(double t0, double len); - /// Lock all blockfiles - void Lock(); - /// Unlock all blockfiles - void Unlock(); - - void CloseLock(); //similar to Lock but should be called when the project closes. + void CloseLock(); //should be called when the project closes. // not balanced by unlocking calls. ///Delete the wave cache - force redraw. Thread-safe diff --git a/src/WaveTrack.cpp b/src/WaveTrack.cpp index 614632cac..8dfce5259 100644 --- a/src/WaveTrack.cpp +++ b/src/WaveTrack.cpp @@ -1699,14 +1699,6 @@ bool WaveTrack::GetErrorOpening() return false; } -bool WaveTrack::Lock() const -{ - for (const auto &clip : mClips) - clip->Lock(); - - return true; -} - bool WaveTrack::CloseLock() { for (const auto &clip : mClips) @@ -1715,15 +1707,6 @@ bool WaveTrack::CloseLock() return true; } - -bool WaveTrack::Unlock() const -{ - for (const auto &clip : mClips) - clip->Unlock(); - - return true; -} - AUDACITY_DLL_API sampleCount WaveTrack::TimeToLongSamples(double t0) const { return sampleCount( floor(t0 * mRate + 0.5) ); diff --git a/src/WaveTrack.h b/src/WaveTrack.h index 551798598..71715be7d 100644 --- a/src/WaveTrack.h +++ b/src/WaveTrack.h @@ -304,31 +304,7 @@ private: // doing a copy and paste between projects. // - bool Lock() const; - bool Unlock() const; - - struct WaveTrackLockDeleter { - inline void operator () (const WaveTrack *pTrack) { pTrack->Unlock(); } - }; - using LockerBase = std::unique_ptr< - const WaveTrack, WaveTrackLockDeleter - >; - - // RAII object for locking. - struct Locker : private LockerBase - { - friend LockerBase; - Locker (const WaveTrack *pTrack) - : LockerBase{ pTrack } - { pTrack->Lock(); } - Locker(Locker &&that) : LockerBase{std::move(that)} {} - Locker &operator= (Locker &&that) { - (LockerBase&)(*this) = std::move(that); - return *this; - } - }; - - bool CloseLock(); //similar to Lock but should be called when the project closes. + bool CloseLock(); //should be called when the project closes. // not balanced by unlocking calls. /** @brief Convert correctly between an (absolute) time in seconds and a number of samples. diff --git a/src/menus/EditMenus.cpp b/src/menus/EditMenus.cpp index f28284c98..a27cf2717 100644 --- a/src/menus/EditMenus.cpp +++ b/src/menus/EditMenus.cpp @@ -76,6 +76,7 @@ bool DoPasteNothingSelected(AudacityProject &project) { auto &tracks = TrackList::Get( project ); auto &trackFactory = TrackFactory::Get( project ); + auto &pSampleBlockFactory = trackFactory.GetSampleBlockFactory(); auto &selectedRegion = ViewInfo::Get( project ).selectedRegion; auto &window = ProjectWindow::Get( project ); @@ -85,24 +86,17 @@ bool DoPasteNothingSelected(AudacityProject &project) else { const auto &clipboard = Clipboard::Get(); - auto clipboardProject = clipboard.Project().lock(); auto clipTrackRange = clipboard.GetTracks().Any< const Track >(); if (clipTrackRange.empty()) return true; // nothing to paste Track* pFirstNewTrack = NULL; for (auto pClip : clipTrackRange) { - Optional locker; - Track::Holder uNewTrack; Track *pNewTrack; pClip->TypeSwitch( [&](const WaveTrack *wc) { - if ((clipboardProject.get() != &project)) - // Cause duplication of block files on disk, when copy is - // between projects - locker.emplace(wc); - uNewTrack = wc->EmptyCopy(); + uNewTrack = wc->EmptyCopy( pSampleBlockFactory ); pNewTrack = uNewTrack.get(); }, #ifdef USE_MIDI @@ -381,6 +375,7 @@ void OnPaste(const CommandContext &context) auto &project = context.project; auto &tracks = TrackList::Get( project ); auto &trackFactory = TrackFactory::Get( project ); + auto &pSampleBlockFactory = trackFactory.GetSampleBlockFactory(); auto &selectedRegion = ViewInfo::Get( project ).selectedRegion; const auto &settings = ProjectSettings::Get( project ); auto &window = ProjectWindow::Get( project ); @@ -416,7 +411,6 @@ void OnPaste(const CommandContext &context) auto pC = clipTrackRange.begin(); size_t nnChannels=0, ncChannels=0; - auto clipboardProject = clipboard.Project().lock(); while (*pN && *pC) { auto n = *pN; auto c = *pC; @@ -504,15 +498,9 @@ void OnPaste(const CommandContext &context) ff = n; wxASSERT( n && c && n->SameKindAs(*c) ); - Optional locker; - n->TypeSwitch( [&](WaveTrack *wn){ const auto wc = static_cast(c); - if (clipboardProject.get() != &project) - // Cause duplication of block files on disk, when copy is - // between projects - locker.emplace(wc); bPastedSomething = true; wn->ClearAndPaste(t0, t1, wc, true, true); }, @@ -547,7 +535,6 @@ void OnPaste(const CommandContext &context) n->TypeSwitch( [&](WaveTrack *wn){ bPastedSomething = true; - // Note: rely on locker being still be in scope! wn->ClearAndPaste(t0, t1, c, true, true); }, [&](Track *){ @@ -582,11 +569,6 @@ void OnPaste(const CommandContext &context) { const auto wc = *clipboard.GetTracks().Any< const WaveTrack >().rbegin(); - Optional locker; - if (clipboardProject.get() != &project && wc) - // Cause duplication of block files on disk, when copy is - // between projects - locker.emplace(static_cast(wc)); tracks.Any().StartingWith(*pN).Visit( [&](WaveTrack *wt, const Track::Fallthrough &fallthrough) { @@ -598,7 +580,7 @@ void OnPaste(const CommandContext &context) wt->ClearAndPaste(t0, t1, wc, true, true); } else { - auto tmp = wt->EmptyCopy(); + auto tmp = wt->EmptyCopy( pSampleBlockFactory ); tmp->InsertSilence( 0.0, // MJS: Is this correct? clipboard.Duration() );