Don't copy DetuningHelper in Note copy constructor (#7888)

Reworks how note detuning copying works so as not to perform a clip duplication and allocation by default in the constructors

---------

Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>

include/InlineAutomation.h

@@ -27,7 +27,6 @@
 
 #include "AutomationNode.h"
 #include "AutomationClip.h"
-#include "shared_object.h"
 
 namespace lmms
 {

include/Note.h

@@ -104,12 +104,15 @@ public:
 		int key = DefaultKey,
 		volume_t volume = DefaultVolume,
 		panning_t panning = DefaultPanning,
-		DetuningHelper * detuning = nullptr );
+		std::shared_ptr<DetuningHelper> detuning = nullptr);
 	Note( const Note & note );
 	~Note() override;
 
 	Note& operator=(const Note& note);
 
+	//! Performs a deep copy and returns an owning raw pointer
+	Note* clone() const;
+
 	// Note types
 	enum class Type
 	{
@@ -237,10 +240,8 @@ public:
 
 	static TimePos quantized( const TimePos & m, const int qGrid );
 
-	DetuningHelper * detuning() const
-	{
-		return m_detuning.get();
-	}
+	const std::shared_ptr<DetuningHelper>& detuning() const { return m_detuning; }
+
 	bool hasDetuningInfo() const;
 	bool withinRange(int tickStart, int tickEnd) const;
 
@@ -265,7 +266,7 @@ private:
 	panning_t m_panning;
 	TimePos m_length;
 	TimePos m_pos;
-	std::unique_ptr<DetuningHelper> m_detuning;
+	std::shared_ptr<DetuningHelper> m_detuning;
 
 	Type m_type = Type::Regular;
 };

include/shared_object.h

@@ -1,89 +0,0 @@
-/*
- * shared_object.h - class sharedObject for use among other objects
- *
- * Copyright (c) 2006-2007 Javier Serrano Polo <jasp00/at/users.sourceforge.net>
- * Copyright (c) 2008-2014 Tobias Doerffel <tobydox/at/users.sourceforge.net>
- *
- * This file is part of LMMS - https://lmms.io
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public
- * License along with this program (see COPYING); if not, write to the
- * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301 USA.
- *
- */
-
-#ifndef LMMS_SHARED_OBJECT_H
-#define LMMS_SHARED_OBJECT_H
-
-#include <atomic>
-
-namespace lmms
-{
-
-class sharedObject
-{
-public:
-	sharedObject() :
-		m_referenceCount(1)
-	{
-	}
-
-	virtual ~sharedObject() = default;
-
-	template<class T>
-	static T* ref( T* object )
-	{
-		// Incrementing an atomic reference count can be relaxed since no action
-		// is ever taken as a result of increasing the count.
-		// Other loads and stores can be reordered around this without consequence.
-		object->m_referenceCount.fetch_add(1, std::memory_order_relaxed);
-		return object;
-	}
-
-	template<class T>
-	static void unref( T* object )
-	{
-		// When decrementing an atomic reference count, we need to provide
-		// two ordering guarantees:
-		// 1. All reads and writes to the referenced object occur before
-		//    the count reaches zero.
-		// 2. Deletion occurs after the count reaches zero.
-		//
-		// To accomplish this, each decrement must be store-released,
-		// and the final thread (which is deleting the referenced data)
-		// must load-acquire those stores.
-		// The simplest way to do this to give the decrement acquire-release
-		// semantics.
-		//
-		// See https://www.boost.org/doc/libs/1_67_0/doc/html/atomic/usage_examples.html
-		// for further discussion, along with a slightly more complicated
-		// (but possibly more performant on weakly-ordered hardware like ARM)
-		// approach.
-		const bool deleteObject =
-			object->m_referenceCount.fetch_sub(1, std::memory_order_acq_rel) == 1;
-
-		if ( deleteObject )
-		{
-			object->deleteLater();
-		}
-	}
-
-private:
-	std::atomic_int m_referenceCount;
-} ;
-
-
-} // namespace lmms
-
-#endif // LMMS_SHARED_OBJECT_H

src/core/Note.cpp

@@ -36,7 +36,7 @@ namespace lmms
 
 Note::Note( const TimePos & length, const TimePos & pos,
 		int key, volume_t volume, panning_t panning,
-						DetuningHelper * detuning ) :
+						std::shared_ptr<DetuningHelper> detuning ) :
 	m_selected( false ),
 	m_oldKey(std::clamp(key, 0, NumKeys)),
 	m_oldPos( pos ),
@@ -46,13 +46,10 @@ Note::Note( const TimePos & length, const TimePos & pos,
 	m_volume(std::clamp(volume, MinVolume, MaxVolume)),
 	m_panning(std::clamp(panning, PanningLeft, PanningRight)),
 	m_length( length ),
-	m_pos( pos )
+	m_pos(pos),
+	m_detuning(std::move(detuning))
 {
-	if (detuning)
-	{
-		m_detuning = std::make_unique<DetuningHelper>(*detuning);
-	}
-	else
+	if (!detuning)
 	{
 		createDetuning();
 	}
@@ -73,12 +70,9 @@ Note::Note( const Note & note ) :
 	m_panning( note.m_panning ),
 	m_length( note.m_length ),
 	m_pos( note.m_pos ),
+	m_detuning(note.m_detuning),
 	m_type(note.m_type)
 {
-	if (note.m_detuning)
-	{
-		m_detuning = std::make_unique<DetuningHelper>(*note.m_detuning);
-	}
 }
 
 Note& Note::operator=(const Note& note)
@@ -94,16 +88,19 @@ Note& Note::operator=(const Note& note)
 	m_length = note.m_length;
 	m_pos = note.m_pos;
 	m_type = note.m_type;
-
-	if (note.m_detuning)
-	{
-		m_detuning = std::make_unique<DetuningHelper>(*note.m_detuning);
-	}
+	m_detuning = note.m_detuning;
 
 	return *this;
 }
 
 
+Note* Note::clone() const
+{
+	Note* newNote = new Note(*this);
+	newNote->m_detuning = std::make_shared<DetuningHelper>(*newNote->m_detuning);
+	return newNote;
+}
+
 
 
 Note::~Note()
@@ -234,7 +231,7 @@ void Note::createDetuning()
 {
 	if( m_detuning == nullptr )
 	{
-		m_detuning = std::make_unique<DetuningHelper>();
+		m_detuning = std::make_shared<DetuningHelper>();
 		(void) m_detuning->automationClip();
 		m_detuning->setRange( -MaxDetuning, MaxDetuning, 0.5f );
 		m_detuning->automationClip()->setProgressionType( AutomationClip::ProgressionType::Linear );

src/core/NotePlayHandle.cpp

@@ -54,7 +54,7 @@ NotePlayHandle::NotePlayHandle( InstrumentTrack* instrumentTrack,
 								int midiEventChannel,
 								Origin origin ) :
 	PlayHandle( PlayHandle::Type::NotePlayHandle, _offset ),
-	Note( n.length(), n.pos(), n.key(), n.getVolume(), n.getPanning(), n.detuning() ),
+	Note(n),
 	m_pluginData( nullptr ),
 	m_instrumentTrack( instrumentTrack ),
 	m_frames( 0 ),
@@ -84,7 +84,7 @@ NotePlayHandle::NotePlayHandle( InstrumentTrack* instrumentTrack,
 	lock();
 	if( hasParent() == false )
 	{
-		m_baseDetuning = new BaseDetuning( detuning() );
+		m_baseDetuning = new BaseDetuning(detuning().get());
 		m_instrumentTrack->m_processHandles.push_back( this );
 	}
 	else

src/tracks/MidiClip.cpp

@@ -68,7 +68,7 @@ MidiClip::MidiClip( const MidiClip& other ) :
 {
 	for (const auto& note : other.m_notes)
 	{
-		m_notes.push_back(new Note(*note));
+		m_notes.push_back(note->clone());
 	}
 
 	init();
@@ -197,7 +197,7 @@ TimePos MidiClip::beatClipLength() const
 
 Note * MidiClip::addNote( const Note & _new_note, const bool _quant_pos )
 {
-	auto new_note = new Note(_new_note);
+	auto new_note = _new_note.clone();
 	if (_quant_pos && gui::getGUI()->pianoRoll())
 	{
 		new_note->quantizePos(gui::getGUI()->pianoRoll()->quantization());

tests/src/tracks/AutomationTrackTest.cpp

@@ -163,8 +163,7 @@ private slots:
 		Note* note = midiClip.addNote(Note(TimePos(4, 0)), false);
 		note->createDetuning();
 
-		DetuningHelper* dh = note->detuning();
-		auto clip = dh->automationClip();
+		auto clip = note->detuning()->automationClip();
 		clip->setProgressionType( AutomationClip::ProgressionType::Linear );
 		clip->putValue(TimePos(0, 0), 0.0);
 		clip->putValue(TimePos(4, 0), 1.0);