Fix rule of three for SampleBuffer, SampleTCO (#5727)

* Automatic formatting changes

* Add copy constructor and assignemnt to SampleBuffer

* Add copy constructor to SampleTCO

* Delete SampleTCO copy assignment, initial work on SampleBuffer swap method

* SampleBuffer: Finish(?) swap and use it for copy assignment, lock for read in copy constructor

* Don't forget to unlock in copy assignment!

* Formatting changes

* Lock ordering in swap

* Fix leak and constness

Co-authored-by: Dominic Clark <mrdomclark@gmail.com>

* Fix multiplication style, ensure lock is held when necessary

... by switching from an initializer list to manual assignments.

* Fixes from review

* Avoid more undefined behavior

* Update src/tracks/SampleTrack.cpp

Co-authored-by: Dominic Clark <mrdomclark@gmail.com>

include/SampleBuffer.h

@@ -109,6 +109,10 @@ public:
 	SampleBuffer(const QString & audioFile, bool isBase64Data = false);
 	SampleBuffer(const sampleFrame * data, const f_cnt_t frames);
 	explicit SampleBuffer(const f_cnt_t frames);
+	SampleBuffer(const SampleBuffer & orig);
+
+	friend void swap(SampleBuffer & first, SampleBuffer & second) noexcept;
+	SampleBuffer& operator= (const SampleBuffer that);
 
 	virtual ~SampleBuffer();
 
@@ -311,7 +315,7 @@ private:
 	sampleFrame * m_origData;
 	f_cnt_t m_origFrames;
 	sampleFrame * m_data;
-	QReadWriteLock m_varLock;
+	mutable QReadWriteLock m_varLock;
 	f_cnt_t m_frames;
 	f_cnt_t m_startFrame;
 	f_cnt_t m_endFrame;

include/SampleTrack.h

@@ -50,8 +50,11 @@ class SampleTCO : public TrackContentObject
 	mapPropertyFromModel(bool,isRecord,setRecord,m_recordModel);
 public:
 	SampleTCO( Track * _track );
+	SampleTCO( const SampleTCO& orig );
 	virtual ~SampleTCO();
 
+	SampleTCO& operator=( const SampleTCO& that ) = delete;
+
 	void changeLength( const TimePos & _length ) override;
 	const QString & sampleFile() const;
 

src/core/SampleBuffer.cpp

@@ -130,6 +130,86 @@ SampleBuffer::SampleBuffer(const f_cnt_t frames)
 
 
 
+SampleBuffer::SampleBuffer(const SampleBuffer& orig)
+{
+	orig.m_varLock.lockForRead();
+
+	m_audioFile = orig.m_audioFile;
+	m_origFrames = orig.m_origFrames;
+	m_origData = (m_origFrames > 0) ? MM_ALLOC(sampleFrame, m_origFrames) : nullptr;
+	m_frames = orig.m_frames;
+	m_data = (m_frames > 0) ? MM_ALLOC(sampleFrame, m_frames) : nullptr;
+	m_startFrame = orig.m_startFrame;
+	m_endFrame = orig.m_endFrame;
+	m_loopStartFrame = orig.m_loopStartFrame;
+	m_loopEndFrame = orig.m_loopEndFrame;
+	m_amplification = orig.m_amplification;
+	m_reversed = orig.m_reversed;
+	m_frequency = orig.m_frequency;
+	m_sampleRate = orig.m_sampleRate;
+
+	//Deep copy m_origData and m_data from original
+	const auto origFrameBytes = m_origFrames * BYTES_PER_FRAME;
+	const auto frameBytes = m_frames * BYTES_PER_FRAME;
+	if (orig.m_origData != nullptr && origFrameBytes > 0)
+		{ memcpy(m_origData, orig.m_origData, origFrameBytes); }
+	if (orig.m_data != nullptr && frameBytes > 0)
+		{ memcpy(m_data, orig.m_data, frameBytes); }
+
+	orig.m_varLock.unlock();
+}
+
+
+
+
+void swap(SampleBuffer& first, SampleBuffer& second) noexcept
+{
+	using std::swap;
+
+	// Lock both buffers for writing, with address as lock ordering
+	if (&first == &second) { return; }
+	else if (&first > &second)
+	{
+		first.m_varLock.lockForWrite();
+		second.m_varLock.lockForWrite();
+	}
+	else
+	{
+		second.m_varLock.lockForWrite();
+		first.m_varLock.lockForWrite();
+	}
+
+	first.m_audioFile.swap(second.m_audioFile);
+	swap(first.m_origData, second.m_origData);
+	swap(first.m_data, second.m_data);
+	swap(first.m_origFrames, second.m_origFrames);
+	swap(first.m_frames, second.m_frames);
+	swap(first.m_startFrame, second.m_startFrame);
+	swap(first.m_endFrame, second.m_endFrame);
+	swap(first.m_loopStartFrame, second.m_loopStartFrame);
+	swap(first.m_loopEndFrame, second.m_loopEndFrame);
+	swap(first.m_amplification, second.m_amplification);
+	swap(first.m_frequency, second.m_frequency);
+	swap(first.m_reversed, second.m_reversed);
+	swap(first.m_sampleRate, second.m_sampleRate);
+
+	// Unlock again
+	first.m_varLock.unlock();
+	second.m_varLock.unlock();
+}
+
+
+
+
+SampleBuffer& SampleBuffer::operator=(SampleBuffer that)
+{
+	swap(*this, that);
+	return *this;
+}
+
+
+
+
 SampleBuffer::~SampleBuffer()
 {
 	MM_FREE(m_origData);

src/tracks/SampleTrack.cpp

@@ -109,6 +109,19 @@ SampleTCO::SampleTCO( Track * _track ) :
 
 
 
+SampleTCO::SampleTCO(const SampleTCO& orig) :
+	SampleTCO(orig.getTrack())
+{
+	// TODO: This creates a new SampleBuffer for the new TCO, eating up memory
+	// & eventually causing performance issues. Letting tracks share buffers
+	// when they're identical would fix this, but isn't possible right now.
+	*m_sampleBuffer = *orig.m_sampleBuffer;
+	m_isPlaying = orig.m_isPlaying;
+}
+
+
+
+
 SampleTCO::~SampleTCO()
 {
 	SampleTrack * sampletrack = dynamic_cast<SampleTrack*>( getTrack() );
@@ -309,7 +322,7 @@ void SampleTCO::loadSettings( const QDomElement & _this )
 	if ( _this.hasAttribute( "sample_rate" ) ) {
 		m_sampleBuffer->setSampleRate( _this.attribute( "sample_rate" ).toInt() );
 	}
-	
+
 	if( _this.hasAttribute( "color" ) )
 	{
 		useCustomClipColor( true );
@@ -433,7 +446,7 @@ void SampleTCOView::contextMenuEvent( QContextMenuEvent * _cme )
 			tr( "Set clip color" ), this, SLOT( changeClipColor() ) );
 	contextMenu.addAction( embed::getIconPixmap( "colorize" ),
 			tr( "Use track color" ), this, SLOT( useTrackColor() ) );
-	
+
 	constructContextMenu( &contextMenu );
 
 	contextMenu.exec( QCursor::pos() );