Merge pull request #4336 from uklotzde/beats

Beats: Use std::make_shared/shared_from_this()

5 files changed, 118 insertions, 58 deletions

diff --git a/src/track/beatgrid.cpp b/src/track/beatgrid.cpp
index e931eb6a36..c52f30b19e 100644
--- a/src/track/beatgrid.cpp
+++ b/src/track/beatgrid.cpp
@@ -46,6 +46,7 @@ class BeatGridIterator : public BeatIterator {
 };
 
 BeatGrid::BeatGrid(
+        MakeSharedTag,
         audio::SampleRate sampleRate,
         const QString& subVersion,
         const mixxx::track::io::BeatGrid& grid,
@@ -54,21 +55,25 @@ BeatGrid::BeatGrid(
           m_sampleRate(sampleRate),
           m_grid(grid),
           m_beatLengthFrames(beatLengthFrames) {
-    // BeatGrid should live in the same thread as the track it is associated
-    // with.
 }
 
-BeatGrid::BeatGrid(const BeatGrid& other,
+BeatGrid::BeatGrid(
+        MakeSharedTag,
+        const BeatGrid& other,
         const mixxx::track::io::BeatGrid& grid,
         audio::FrameDiff_t beatLengthFrames)
-        : m_subVersion(other.m_subVersion),
-          m_sampleRate(other.m_sampleRate),
-          m_grid(grid),
-          m_beatLengthFrames(beatLengthFrames) {
+        : BeatGrid(
+                  MakeSharedTag{},
+                  other.m_sampleRate,
+                  other.m_subVersion,
+                  grid,
+                  beatLengthFrames) {
 }
 
-BeatGrid::BeatGrid(const BeatGrid& other)
-        : BeatGrid(other, other.m_grid, other.m_beatLengthFrames) {
+BeatGrid::BeatGrid(
+        MakeSharedTag,
+        const BeatGrid& other)
+        : BeatGrid(MakeSharedTag{}, other, other.m_grid, other.m_beatLengthFrames) {
 }
 
 // static
@@ -98,7 +103,8 @@ BeatsPointer BeatGrid::makeBeatGrid(
     // Calculate beat length as sample offsets
     const audio::FrameDiff_t beatLengthFrames = 60.0 * sampleRate / bpm.value();
 
-    return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
+    return std::make_shared<BeatGrid>(
+            MakeSharedTag{}, sampleRate, subVersion, grid, beatLengthFrames);
 }
 
 // static
@@ -109,12 +115,16 @@ BeatsPointer BeatGrid::fromByteArray(
     mixxx::track::io::BeatGrid grid;
     if (grid.ParseFromArray(byteArray.constData(), byteArray.length())) {
         const audio::FrameDiff_t beatLengthFrames = (60.0 * sampleRate / grid.bpm().bpm());
-        return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
+        return std::make_shared<BeatGrid>(MakeSharedTag{},
+                sampleRate,
+                subVersion,
+                grid,
+                beatLengthFrames);
     }
 
     // Legacy fallback for BeatGrid-1.0
     if (byteArray.size() != sizeof(BeatGridData)) {
-        return BeatsPointer(new BeatGrid(sampleRate, QString(), grid, 0));
+        return std::make_shared<BeatGrid>(MakeSharedTag{}, sampleRate, QString(), grid, 0);
     }
     const BeatGridData* blob = reinterpret_cast<const BeatGridData*>(byteArray.constData());
     const auto firstBeat = mixxx::audio::FramePos(blob->firstBeat);
@@ -248,27 +258,28 @@ mixxx::Bpm BeatGrid::getBpm() const {
 mixxx::Bpm BeatGrid::getBpmAroundPosition(audio::FramePos position, int n) const {
     Q_UNUSED(position);
     Q_UNUSED(n);
-
-    if (!isValid()) {
-        return {};
-    }
-    return bpm();
+    return getBpm();
 }
 
 BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
-    if (!isValid()) {
-        return BeatsPointer(new BeatGrid(*this));
+    VERIFY_OR_DEBUG_ASSERT(isValid()) {
+        return clonePointer();
     }
+
     mixxx::track::io::BeatGrid grid = m_grid;
     const audio::FramePos newFirstBeatPosition = firstBeatPosition() + offset;
     grid.mutable_first_beat()->set_frame_position(
             static_cast<google::protobuf::int32>(
                     newFirstBeatPosition.toLowerFrameBoundary().value()));
 
-    return BeatsPointer(new BeatGrid(*this, grid, m_beatLengthFrames));
+    return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, m_beatLengthFrames);
 }
 
 BeatsPointer BeatGrid::scale(BpmScale scale) const {
+    VERIFY_OR_DEBUG_ASSERT(isValid()) {
+        return clonePointer();
+    }
+
     mixxx::track::io::BeatGrid grid = m_grid;
     auto bpm = mixxx::Bpm(grid.bpm().bpm());
 
@@ -293,27 +304,28 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
         break;
     default:
         DEBUG_ASSERT(!"scale value invalid");
-        return BeatsPointer(new BeatGrid(*this));
+        return clonePointer();
     }
 
     if (!bpm.isValid()) {
-        return BeatsPointer(new BeatGrid(*this));
+        return clonePointer();
     }
 
     bpm = BeatUtils::roundBpmWithinRange(bpm - kBpmScaleRounding, bpm, bpm + kBpmScaleRounding);
     grid.mutable_bpm()->set_bpm(bpm.value());
     const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * m_sampleRate / bpm.value());
-    return BeatsPointer(new BeatGrid(*this, grid, beatLengthFrames));
+    return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
 }
 
-BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) {
+BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) const {
     VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
-        return nullptr;
+        return clonePointer();
     }
+
     mixxx::track::io::BeatGrid grid = m_grid;
     grid.mutable_bpm()->set_bpm(bpm.value());
     const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * m_sampleRate / bpm.value());
-    return BeatsPointer(new BeatGrid(*this, grid, beatLengthFrames));
+    return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
 }
 
 } // namespace mixxx
diff --git a/src/track/beatgrid.h b/src/track/beatgrid.h
index 0aa7a3b7aa..a2e5b117b1 100644
--- a/src/track/beatgrid.h
+++ b/src/track/beatgrid.h
@@ -64,18 +64,29 @@ class BeatGrid final : public Beats {
 
     BeatsPointer translate(audio::FrameDiff_t offset) const override;
     BeatsPointer scale(BpmScale scale) const override;
-    BeatsPointer setBpm(mixxx::Bpm bpm) override;
+    BeatsPointer setBpm(mixxx::Bpm bpm) const override;
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Hidden constructors
+    ////////////////////////////////////////////////////////////////////////////
 
-  private:
     BeatGrid(
+            MakeSharedTag,
             audio::SampleRate sampleRate,
             const QString& subVersion,
             const mixxx::track::io::BeatGrid& grid,
             double beatLength);
     // Constructor to update the beat grid
-    BeatGrid(const BeatGrid& other, const mixxx::track::io::BeatGrid& grid, double beatLength);
-    BeatGrid(const BeatGrid& other);
+    BeatGrid(
+            MakeSharedTag,
+            const BeatGrid& other,
+            const mixxx::track::io::BeatGrid& grid,
+            double beatLength);
+    BeatGrid(
+            MakeSharedTag,
+            const BeatGrid& other);
 
+  private:
     audio::FramePos firstBeatPosition() const;
     mixxx::Bpm bpm() const;
 
diff --git a/src/track/beatmap.cpp b/src/track/beatmap.cpp
index d91e21cf43..3a51a9f040 100644
--- a/src/track/beatmap.cpp
+++ b/src/track/beatmap.cpp
@@ -178,6 +178,7 @@ class BeatMapIterator : public BeatIterator {
 };
 
 BeatMap::BeatMap(
+        MakeSharedTag,
         audio::SampleRate sampleRate,
         const QString& subVersion,
         BeatList beats,
@@ -188,15 +189,23 @@ BeatMap::BeatMap(
           m_beats(std::move(beats)) {
 }
 
-BeatMap::BeatMap(const BeatMap& other, BeatList beats, mixxx::Bpm nominalBpm)
-        : m_subVersion(other.m_subVersion),
-          m_sampleRate(other.m_sampleRate),
-          m_nominalBpm(nominalBpm),
-          m_beats(std::move(beats)) {
+BeatMap::BeatMap(
+        MakeSharedTag,
+        const BeatMap& other,
+        BeatList beats,
+        mixxx::Bpm nominalBpm)
+        : BeatMap(
+                  MakeSharedTag{},
+                  other.m_sampleRate,
+                  other.m_subVersion,
+                  std::move(beats),
+                  nominalBpm) {
 }
 
-BeatMap::BeatMap(const BeatMap& other)
-        : BeatMap(other, other.m_beats, other.m_nominalBpm) {
+BeatMap::BeatMap(
+        MakeSharedTag,
+        const BeatMap& other)
+        : BeatMap(MakeSharedTag{}, other, other.m_beats, other.m_nominalBpm) {
 }
 
 // static
@@ -218,7 +227,7 @@ BeatsPointer BeatMap::fromByteArray(
         qDebug() << "ERROR: Could not parse BeatMap from QByteArray of size"
                  << byteArray.size();
     }
-    return BeatsPointer(new BeatMap(sampleRate, subVersion, beatList, nominalBpm));
+    return std::make_shared<BeatMap>(MakeSharedTag{}, sampleRate, subVersion, beatList, nominalBpm);
 }
 
 // static
@@ -250,7 +259,7 @@ BeatsPointer BeatMap::makeBeatMap(
         previousBeatPos = beatPos;
     }
     const auto nominalBpm = calculateNominalBpm(beatList, sampleRate);
-    return BeatsPointer(new BeatMap(sampleRate, subVersion, beatList, nominalBpm));
+    return std::make_shared<BeatMap>(MakeSharedTag{}, sampleRate, subVersion, beatList, nominalBpm);
 }
 
 QByteArray BeatMap::toByteArray() const {
@@ -524,9 +533,8 @@ mixxx::Bpm BeatMap::getBpmAroundPosition(audio::FramePos position, int n) const
 }
 
 BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
-    // Converting to frame offset
     if (!isValid()) {
-        return BeatsPointer(new BeatMap(*this));
+        return clonePointer();
     }
 
     BeatList beats = m_beats;
@@ -544,12 +552,12 @@ BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
         }
     }
 
-    return BeatsPointer(new BeatMap(*this, beats, m_nominalBpm));
+    return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, m_nominalBpm);
 }
 
 BeatsPointer BeatMap::scale(BpmScale scale) const {
-    if (!isValid() || m_beats.isEmpty()) {
-        return BeatsPointer(new BeatMap(*this));
+    VERIFY_OR_DEBUG_ASSERT(isValid()) {
+        return clonePointer();
     }
 
     BeatList beats = m_beats;
@@ -588,16 +596,16 @@ BeatsPointer BeatMap::scale(BpmScale scale) const {
         break;
     default:
         DEBUG_ASSERT(!"scale value invalid");
-        return BeatsPointer(new BeatMap(*this));
+        return clonePointer();
     }
 
     mixxx::Bpm bpm = calculateNominalBpm(beats, m_sampleRate);
-    return BeatsPointer(new BeatMap(*this, beats, bpm));
+    return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, bpm);
 }
 
-BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) {
-    if (!isValid()) {
-        return {};
+BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) const {
+    VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
+        return clonePointer();
     }
 
     const auto firstBeatPosition = mixxx::audio::FramePos(m_beats.first().frame_position());
diff --git a/src/track/beatmap.h b/src/track/beatmap.h
index f394e67210..1ad9232dcc 100644
--- a/src/track/beatmap.h
+++ b/src/track/beatmap.h
@@ -20,7 +20,6 @@ namespace mixxx {
 
 class BeatMap final : public Beats {
   public:
-
     ~BeatMap() override = default;
 
     static BeatsPointer fromByteArray(
@@ -67,17 +66,29 @@ class BeatMap final : public Beats {
 
     BeatsPointer translate(audio::FrameDiff_t offset) const override;
     BeatsPointer scale(BpmScale scale) const override;
-    BeatsPointer setBpm(mixxx::Bpm bpm) override;
+    BeatsPointer setBpm(mixxx::Bpm bpm) const override;
 
-  private:
-    BeatMap(audio::SampleRate sampleRate,
+    ////////////////////////////////////////////////////////////////////////////
+    // Hidden constructors
+    ////////////////////////////////////////////////////////////////////////////
+
+    BeatMap(
+            MakeSharedTag,
+            audio::SampleRate sampleRate,
             const QString& subVersion,
             BeatList beats,
             mixxx::Bpm nominalBpm);
     // Constructor to update the beat map
-    BeatMap(const BeatMap& other, BeatList beats, mixxx::Bpm nominalBpm);
-    BeatMap(const BeatMap& other);
+    BeatMap(
+            MakeSharedTag,
+            const BeatMap& other,
+            BeatList beats,
+            mixxx::Bpm nominalBpm);
+    BeatMap(
+            MakeSharedTag,
+            const BeatMap& other);
 
+  private:
     bool isValid() const override;
 
     const QString m_subVersion;
diff --git a/src/track/beats.h b/src/track/beats.h
index 54afb8fb98..085edcb52a 100644
--- a/src/track/beats.h
+++ b/src/track/beats.h
@@ -15,7 +15,7 @@
 namespace mixxx {
 
 class Beats;
-typedef std::shared_ptr<Beats> BeatsPointer;
+typedef std::shared_ptr<const Beats> BeatsPointer;
 
 class BeatIterator {
   public:
@@ -27,10 +27,17 @@ class BeatIterator {
 /// Beats is the base class for BPM and beat management classes. It provides a
 /// specification of all methods a beat-manager class must provide, as well as
 /// a capability model for representing optional features.
-class Beats {
+///
+/// All instances of this class are supposed to be managed by std::shared_ptr!
+class Beats : private std::enable_shared_from_this<Beats> {
   public:
     virtual ~Beats() = default;
 
+    BeatsPointer clonePointer() const {
+        // All instances are immutable and can be shared safely
+        return shared_from_this();
+    }
+
     static mixxx::BeatsPointer fromByteArray(
             mixxx::audio::SampleRate sampleRate,
             const QString& beatsVersion,
@@ -166,10 +173,21 @@ class Beats {
     virtual BeatsPointer scale(BpmScale scale) const = 0;
 
     /// Adjust the beats so the global average BPM matches `bpm`.
-    virtual BeatsPointer setBpm(mixxx::Bpm bpm) = 0;
+    virtual BeatsPointer setBpm(mixxx::Bpm bpm) const = 0;
 
   protected:
+    /// Type tag for making public constructors of derived classes inaccessible.
+    ///
+    /// The constructors must be public for using std::make_shared().
+    struct MakeSharedTag {};
+
+    Beats() = default;
+
     virtual bool isValid() const = 0;
+
+  private:
+    Beats(const Beats&) = delete;
+    Beats(Beats&&) = delete;
 };
 
 } // namespace mixxx