Consistently use seconds instead of milliseconds throughout playback …

…module Resolves #23793: the time input field in the playback toolbar was not working properly, because the amount of milliseconds resulting from the entered time was accidentally interpreted as a number of ticks, because the wrong overload of `seek` was used. This problem is avoided by eliminating the usage of the msecs_t type completely.
musescore · Jul 27, 2024 · 1d92504 · 1d92504
1 parent 73cc000
commit 1d92504
Show file tree

Hide file tree

Showing 10 changed files with 71 additions and 104 deletions.
diff --git a/src/notation/inotationplayback.h b/src/notation/inotationplayback.h
@@ -51,12 +51,12 @@ class INotationPlayback
     virtual muse::async::Channel<engraving::InstrumentTrackId> trackAdded() const = 0;
     virtual muse::async::Channel<engraving::InstrumentTrackId> trackRemoved() const = 0;
 
-    virtual muse::audio::msecs_t totalPlayTime() const = 0;
-    virtual muse::async::Channel<muse::audio::msecs_t> totalPlayTimeChanged() const = 0;
+    virtual muse::audio::secs_t totalPlayTime() const = 0;
+    virtual muse::async::Channel<muse::audio::secs_t> totalPlayTimeChanged() const = 0;
 
-    virtual float playedTickToSec(muse::midi::tick_t tick) const = 0;
-    virtual muse::midi::tick_t secToPlayedTick(float sec) const = 0;
-    virtual muse::midi::tick_t secToTick(float sec) const = 0;
+    virtual muse::audio::secs_t playedTickToSec(muse::midi::tick_t tick) const = 0;
+    virtual muse::midi::tick_t secToPlayedTick(muse::audio::secs_t sec) const = 0;
+    virtual muse::midi::tick_t secToTick(muse::audio::secs_t sec) const = 0;
 
     virtual muse::RetVal<muse::midi::tick_t> playPositionTickByRawTick(muse::midi::tick_t tick) const = 0;
     virtual muse::RetVal<muse::midi::tick_t> playPositionTickByElement(const EngravingItem* element) const = 0;

diff --git a/src/notation/internal/notationplayback.cpp b/src/notation/internal/notationplayback.cpp
@@ -51,7 +51,7 @@ using namespace muse;
 using namespace muse::midi;
 using namespace muse::async;
 
-static constexpr int PLAYBACK_TAIL_SECS = 3;
+static constexpr double PLAYBACK_TAIL_SECS = 3;
 
 NotationPlayback::NotationPlayback(IGetScore* getScore,
                                    muse::async::Notification notationChanged)
@@ -170,10 +170,8 @@ void NotationPlayback::updateTotalPlayTime()
     }
 
     int lastTick = score->repeatList(m_playbackModel.isPlayRepeatsEnabled()).ticks();
-    qreal secs = score->utick2utime(lastTick);
-    secs += PLAYBACK_TAIL_SECS;
-
-    muse::audio::msecs_t newPlayTime = secs * 1000.f;
+    audio::secs_t newPlayTime = score->utick2utime(lastTick);
+    newPlayTime += PLAYBACK_TAIL_SECS;
 
     if (m_totalPlayTime == newPlayTime) {
         return;
@@ -183,22 +181,22 @@ void NotationPlayback::updateTotalPlayTime()
     m_totalPlayTimeChanged.send(m_totalPlayTime);
 }
 
-muse::audio::msecs_t NotationPlayback::totalPlayTime() const
+muse::audio::secs_t NotationPlayback::totalPlayTime() const
 {
     return m_totalPlayTime;
 }
 
-muse::async::Channel<muse::audio::msecs_t> NotationPlayback::totalPlayTimeChanged() const
+muse::async::Channel<muse::audio::secs_t> NotationPlayback::totalPlayTimeChanged() const
 {
     return m_totalPlayTimeChanged;
 }
 
-float NotationPlayback::playedTickToSec(tick_t tick) const
+muse::audio::secs_t NotationPlayback::playedTickToSec(tick_t tick) const
 {
     return score() ? score()->utick2utime(tick) : 0.0;
 }
 
-tick_t NotationPlayback::secToPlayedTick(float sec) const
+tick_t NotationPlayback::secToPlayedTick(muse::audio::secs_t sec) const
 {
     if (!score()) {
         return 0;
@@ -207,7 +205,7 @@ tick_t NotationPlayback::secToPlayedTick(float sec) const
     return score()->utime2utick(sec);
 }
 
-tick_t NotationPlayback::secToTick(float sec) const
+tick_t NotationPlayback::secToTick(muse::audio::secs_t sec) const
 {
     if (!score()) {
         return 0;

diff --git a/src/notation/internal/notationplayback.h b/src/notation/internal/notationplayback.h
@@ -22,15 +22,12 @@
 #ifndef MU_NOTATION_NOTATIONPLAYBACK_H
 #define MU_NOTATION_NOTATIONPLAYBACK_H
 
-#include <memory>
-
 #include "modularity/ioc.h"
 #include "async/asyncable.h"
 #include "engraving/playback/playbackmodel.h"
 
 #include "../inotationplayback.h"
 #include "igetscore.h"
-#include "inotationundostack.h"
 #include "inotationconfiguration.h"
 
 namespace mu::engraving {
@@ -59,12 +56,12 @@ class NotationPlayback : public INotationPlayback, public muse::async::Asyncable
     muse::async::Channel<engraving::InstrumentTrackId> trackAdded() const override;
     muse::async::Channel<engraving::InstrumentTrackId> trackRemoved() const override;
 
-    muse::audio::msecs_t totalPlayTime() const override;
-    muse::async::Channel<muse::audio::msecs_t> totalPlayTimeChanged() const override;
+    muse::audio::secs_t totalPlayTime() const override;
+    muse::async::Channel<muse::audio::secs_t> totalPlayTimeChanged() const override;
 
-    float playedTickToSec(muse::midi::tick_t tick) const override;
-    muse::midi::tick_t secToPlayedTick(float sec) const override;
-    muse::midi::tick_t secToTick(float sec) const override;
+    muse::audio::secs_t playedTickToSec(muse::midi::tick_t tick) const override;
+    muse::midi::tick_t secToPlayedTick(muse::audio::secs_t sec) const override;
+    muse::midi::tick_t secToTick(muse::audio::secs_t sec) const override;
 
     muse::RetVal<muse::midi::tick_t> playPositionTickByRawTick(muse::midi::tick_t tick) const override;
     muse::RetVal<muse::midi::tick_t> playPositionTickByElement(const EngravingItem* element) const override;
@@ -107,8 +104,8 @@ class NotationPlayback : public INotationPlayback, public muse::async::Asyncable
     LoopBoundaries m_loopBoundaries;
     muse::async::Notification m_loopBoundariesChanged;
 
-    muse::audio::msecs_t m_totalPlayTime = 0;
-    muse::async::Channel<muse::audio::msecs_t> m_totalPlayTimeChanged;
+    muse::audio::secs_t m_totalPlayTime = 0;
+    muse::async::Channel<muse::audio::secs_t> m_totalPlayTimeChanged;
 
     mutable Tempo m_currentTempo;
 

diff --git a/src/playback/internal/playbackcontroller.cpp b/src/playback/internal/playbackcontroller.cpp
@@ -28,19 +28,21 @@
 
 #include "audio/audioutils.h"
 #include "audio/devtools/inputlag.h"
+#include "audio/iaudiooutput.h"
+#include "audio/itracks.h"
 
 #include "containers.h"
 #include "defer.h"
 #include "log.h"
 
 using namespace muse;
-using namespace mu::playback;
-using namespace muse::midi;
-using namespace mu::notation;
+using namespace muse::actions;
 using namespace muse::async;
 using namespace muse::audio;
-using namespace muse::actions;
+using namespace muse::midi;
 using namespace mu::engraving;
+using namespace mu::notation;
+using namespace mu::playback;
 
 static const ActionCode PLAY_CODE("play");
 static const ActionCode STOP_CODE("stop");
@@ -558,9 +560,9 @@ void PlaybackController::togglePlay()
     } else if (isPaused()) {
         if (currentPlayer()) {
             secs_t pos = currentPlayer()->playbackPosition();
-            secs_t endSecs = milisecsToSecs(playbackEndMsecs());
+            secs_t endSecs = playbackEndSecs();
             if (pos == endSecs) {
-                secs_t startSecs = milisecsToSecs(playbackStartMsecs());
+                secs_t startSecs = playbackStartSecs();
                 seek(startSecs);
             }
 
@@ -578,7 +580,7 @@ void PlaybackController::play()
     }
 
     if (isLoopEnabled()) {
-        secs_t startSecs = milisecsToSecs(playbackStartMsecs());
+        secs_t startSecs = playbackStartSecs();
         seek(startSecs);
     }
 
@@ -587,10 +589,10 @@ void PlaybackController::play()
 
 void PlaybackController::rewind(const ActionData& args)
 {
-    msecs_t startMsecs = playbackStartMsecs();
-    msecs_t endMsecs = playbackEndMsecs();
-    msecs_t newPosition = !args.empty() ? args.arg<msecs_t>(0) : 0;
-    newPosition = std::clamp(newPosition, startMsecs, endMsecs);
+    secs_t startSecs = playbackStartSecs();
+    secs_t endSecs = playbackEndSecs();
+    secs_t newPosition = !args.empty() ? args.arg<secs_t>(0) : secs_t{ 0 };
+    newPosition = std::clamp(newPosition, startSecs, endSecs);
 
     seek(newPosition);
 }
@@ -622,7 +624,7 @@ void PlaybackController::resume()
     currentPlayer()->resume();
 }
 
-msecs_t PlaybackController::playbackStartMsecs() const
+secs_t PlaybackController::playbackStartSecs() const
 {
     if (!m_notation) {
         return 0;
@@ -635,15 +637,15 @@ msecs_t PlaybackController::playbackStartMsecs() const
         if (!startTick.ret) {
             return 0;
         }
-        return tickToMsecs(startTick.val);
+        return tickToSecs(startTick.val);
     }
 
     return 0;
 }
 
-msecs_t PlaybackController::playbackEndMsecs() const
+secs_t PlaybackController::playbackEndSecs() const
 {
-    return notationPlayback() ? notationPlayback()->totalPlayTime() : 0;
+    return notationPlayback() ? notationPlayback()->totalPlayTime() : secs_t { 0 };
 }
 
 InstrumentTrackIdSet PlaybackController::instrumentTrackIdSetForRangePlayback() const
@@ -797,9 +799,9 @@ void PlaybackController::updateLoop()
         return;
     }
 
-    msecs_t fromMsecs = tickToMsecs(playbackTickFrom.val);
-    msecs_t toMsecs = tickToMsecs(playbackTickTo.val);
-    currentPlayer()->setLoop(fromMsecs, toMsecs);
+    secs_t fromSecs = tickToSecs(playbackTickFrom.val);
+    secs_t toSecs = tickToSecs(playbackTickTo.val);
+    currentPlayer()->setLoop(secsToMilisecs(fromSecs), secsToMilisecs(toSecs));
 
     enableLoop();
 
@@ -1266,7 +1268,7 @@ void PlaybackController::setupSequencePlayer()
 
         updateCurrentTempo();
 
-        secs_t endSecs = milisecsToSecs(playbackEndMsecs());
+        secs_t endSecs = playbackEndSecs();
         if (pos == endSecs) {
             stop();
         }
@@ -1276,10 +1278,10 @@ void PlaybackController::setupSequencePlayer()
         m_isPlayingChanged.notify();
     });
 
-    currentPlayer()->setDuration(notationPlayback()->totalPlayTime());
+    currentPlayer()->setDuration(secsToMilisecs(notationPlayback()->totalPlayTime()));
 
-    notationPlayback()->totalPlayTimeChanged().onReceive(this, [this](const audio::msecs_t totalPlaybackTime) {
-        currentPlayer()->setDuration(totalPlaybackTime);
+    notationPlayback()->totalPlayTimeChanged().onReceive(this, [this](const audio::secs_t totalPlaybackTime) {
+        currentPlayer()->setDuration(secsToMilisecs(totalPlaybackTime));
         m_totalPlayTimeChanged.notify();
     });
 }
@@ -1403,13 +1405,11 @@ Channel<ActionCode> PlaybackController::actionCheckedChanged() const
 
 QTime PlaybackController::totalPlayTime() const
 {
-    QTime result = ZERO_TIME;
-
     if (!notationPlayback()) {
-        return result;
+        return ZERO_TIME;
     }
 
-    return result.addMSecs(notationPlayback()->totalPlayTime());
+    return timeFromSeconds(notationPlayback()->totalPlayTime());
 }
 
 Notification PlaybackController::totalPlayTimeChanged() const
@@ -1432,9 +1432,9 @@ MeasureBeat PlaybackController::currentBeat() const
     return notationPlayback() ? notationPlayback()->beat(m_currentTick) : MeasureBeat();
 }
 
-msecs_t PlaybackController::beatToMilliseconds(int measureIndex, int beatIndex) const
+secs_t PlaybackController::beatToSecs(int measureIndex, int beatIndex) const
 {
-    return notationPlayback() ? tickToMsecs(notationPlayback()->beatToTick(measureIndex, beatIndex)) : 0;
+    return notationPlayback() ? tickToSecs(notationPlayback()->beatToTick(measureIndex, beatIndex)) : secs_t { 0 };
 }
 
 double PlaybackController::tempoMultiplier() const
@@ -1576,12 +1576,6 @@ bool PlaybackController::canReceiveAction(const ActionCode&) const
     return m_masterNotation != nullptr && m_masterNotation->hasParts();
 }
 
-msecs_t PlaybackController::tickToMsecs(int tick) const
-{
-    float sec = notationPlayback()->playedTickToSec(tick);
-    return secondsToMilliseconds(sec);
-}
-
 muse::audio::secs_t PlaybackController::tickToSecs(int tick) const
 {
     return secs_t(notationPlayback()->playedTickToSec(tick));

diff --git a/src/playback/internal/playbackcontroller.h b/src/playback/internal/playbackcontroller.h
@@ -22,10 +22,7 @@
 #ifndef MU_PLAYBACK_PLAYBACKCONTROLLER_H
 #define MU_PLAYBACK_PLAYBACKCONTROLLER_H
 
-#include <unordered_map>
-
 #include "modularity/ioc.h"
-#include "types/retval.h"
 #include "async/asyncable.h"
 #include "actions/iactionsdispatcher.h"
 #include "actions/actionable.h"
@@ -35,8 +32,6 @@
 #include "notation/inotationconfiguration.h"
 #include "notation/inotationplayback.h"
 #include "audio/iplayer.h"
-#include "audio/itracks.h"
-#include "audio/iaudiooutput.h"
 #include "audio/iplayback.h"
 #include "audio/audiotypes.h"
 #include "iinteractive.h"
@@ -103,7 +98,7 @@ class PlaybackController : public IPlaybackController, public muse::actions::Act
     muse::async::Notification currentTempoChanged() const override;
 
     notation::MeasureBeat currentBeat() const override;
-    muse::audio::msecs_t beatToMilliseconds(int measureIndex, int beatIndex) const override;
+    muse::audio::secs_t beatToSecs(int measureIndex, int beatIndex) const override;
 
     double tempoMultiplier() const override;
     void setTempoMultiplier(double multiplier) override;
@@ -158,8 +153,8 @@ class PlaybackController : public IPlaybackController, public muse::actions::Act
     void stop();
     void resume();
 
-    muse::audio::msecs_t playbackStartMsecs() const;
-    muse::audio::msecs_t playbackEndMsecs() const;
+    muse::audio::secs_t playbackStartSecs() const;
+    muse::audio::secs_t playbackEndSecs() const;
 
     notation::InstrumentTrackIdSet instrumentTrackIdSetForRangePlayback() const;
 
@@ -207,7 +202,6 @@ class PlaybackController : public IPlaybackController, public muse::actions::Act
     void removeNonExistingTracks();
     void removeTrack(const engraving::InstrumentTrackId& instrumentTrackId);
 
-    muse::audio::msecs_t tickToMsecs(int tick) const;
     muse::audio::secs_t tickToSecs(int tick) const;
 
     notation::INotationPtr m_notation;

diff --git a/src/playback/iplaybackcontroller.h b/src/playback/iplaybackcontroller.h
@@ -89,7 +89,7 @@ class IPlaybackController : MODULE_EXPORT_INTERFACE
     virtual muse::async::Notification currentTempoChanged() const = 0;
 
     virtual notation::MeasureBeat currentBeat() const = 0;
-    virtual muse::audio::msecs_t beatToMilliseconds(int measureIndex, int beatIndex) const = 0;
+    virtual muse::audio::secs_t beatToSecs(int measureIndex, int beatIndex) const = 0;
 
     virtual double tempoMultiplier() const = 0;
     virtual void setTempoMultiplier(double multiplier) = 0;

diff --git a/src/playback/playbacktypes.h b/src/playback/playbacktypes.h
@@ -66,30 +66,14 @@ inline QList<MixerSectionType> allMixerSectionTypes()
 
 static const QTime ZERO_TIME(0, 0, 0, 0);
 
-inline muse::audio::msecs_t secondsToMilliseconds(float seconds)
+inline QTime timeFromSeconds(muse::audio::secs_t seconds)
 {
-    return seconds * 1000;
+    return ZERO_TIME.addMSecs(muse::audio::secsToMilisecs(seconds));
 }
 
-inline float secondsFromMilliseconds(muse::audio::msecs_t milliseconds)
+inline muse::audio::msecs_t timeToSeconds(const QTime& time)
 {
-    return milliseconds / 1000.f;
-}
-
-inline QTime timeFromMilliseconds(muse::audio::msecs_t milliseconds)
-{
-    return ZERO_TIME.addMSecs(milliseconds);
-}
-
-inline QTime timeFromSeconds(float seconds)
-{
-    muse::audio::msecs_t milliseconds = secondsToMilliseconds(seconds);
-    return timeFromMilliseconds(milliseconds);
-}
-
-inline muse::audio::msecs_t timeToMilliseconds(const QTime& time)
-{
-    return ZERO_TIME.msecsTo(time);
+    return muse::audio::milisecsToSecs(ZERO_TIME.msecsTo(time));
 }
 
 enum class SoundProfileType {

diff --git a/src/playback/tests/mocks/playbackcontrollermock.h b/src/playback/tests/mocks/playbackcontrollermock.h
@@ -75,7 +75,7 @@ class PlaybackControllerMock : public IPlaybackController
     MOCK_METHOD(muse::async::Notification, currentTempoChanged, (), (const, override));
 
     MOCK_METHOD(notation::MeasureBeat, currentBeat, (), (const, override));
-    MOCK_METHOD(muse::audio::msecs_t, beatToMilliseconds, (int, int), (const, override));
+    MOCK_METHOD(muse::audio::secs_t, beatToSecs, (int, int), (const, override));
 
     MOCK_METHOD(double, tempoMultiplier, (), (const, override));
     MOCK_METHOD(void, setTempoMultiplier, (double), (override));