Autres/audacity
1
0
Fork 0
mirror of https://github.com/cookiengineer/audacity synced 2025-08-03 01:19:24 +02:00
Code Issues Releases Wiki Activity

Cross project copy data loss (#604)

Browse Source 
* 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.
This commit is contained in:
Paul Licameli 2020-07-07 16:41:50 -04:00 committed by GitHub
parent 590d8c6d09
commit 875e8e0984
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 78 additions and 152 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/SampleBlock.h
View File

@ -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;

83
src/Sequence.cpp
View File

@ -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> 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> Sequence::Copy( const SampleBlockFactoryPtr &pFactory,
sampleCount s0, sampleCount s1) const
{
auto dest = std::make_unique<Sequence>(mpFactory, mSampleFormat);
// Make a new Sequence object for the specified factory:
auto dest = std::make_unique<Sequence>(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> 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> 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> 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

13
src/Sequence.h
View File

@ -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<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;
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);

37
src/SqliteSampleBlock.cpp
View File

@ -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<ProjectFileIO> 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<SqliteSampleBlock>(&mProject);
auto sb = std::make_shared<SqliteSampleBlock>(*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<SqliteSampleBlock>(&mProject);
auto sb = std::make_shared<SqliteSampleBlock>(*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<SqliteSampleBlock>(&mProject);
auto sb = std::make_shared<SqliteSampleBlock>(*mpIO);
sb->mSampleFormat = srcformat;
int found = 0;
@ -230,17 +226,16 @@ SampleBlockPtr SqliteSampleBlockFactory::DoCreateFromXML(
SampleBlockPtr SqliteSampleBlockFactory::DoGet( SampleBlockID sbid )
{
auto sb = std::make_shared<SqliteSampleBlock>(&mProject);
auto sb = std::make_shared<SqliteSampleBlock>(*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()

3
src/Track.h
View File

@ -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;

16
src/WaveClip.cpp
View File

@ -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<Envelope>(
*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;

7
src/WaveClip.h
View File

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

17
src/WaveTrack.cpp
View File

@ -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) );

26
src/WaveTrack.h
View File

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

26
src/menus/EditMenus.cpp
View File

@ -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<WaveTrack::Locker> 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<WaveTrack::Locker> locker;
n->TypeSwitch(
[&](WaveTrack *wn){
const auto wc = static_cast<const WaveTrack *>(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<WaveTrack::Locker> locker;
if (clipboardProject.get() != &project && wc)
// Cause duplication of block files on disk, when copy is
// between projects
locker.emplace(static_cast<const WaveTrack*>(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() );