From 1d92504a9223ddc337044602c4c1a9e7bcf1834f Mon Sep 17 00:00:00 2001 From: Casper Jeukendrup <48658420+cbjeukendrup@users.noreply.github.com> Date: Sat, 27 Jul 2024 19:23:38 +0200 Subject: [PATCH] Consistently use seconds instead of milliseconds throughout playback module Resolves https://github.com/musescore/MuseScore/issues/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. --- src/notation/inotationplayback.h | 10 +-- src/notation/internal/notationplayback.cpp | 18 +++--- src/notation/internal/notationplayback.h | 17 +++-- src/playback/internal/playbackcontroller.cpp | 62 +++++++++---------- src/playback/internal/playbackcontroller.h | 12 +--- src/playback/iplaybackcontroller.h | 2 +- src/playback/playbacktypes.h | 24 ++----- .../tests/mocks/playbackcontrollermock.h | 2 +- src/playback/view/playbacktoolbarmodel.cpp | 26 ++++---- src/playback/view/playbacktoolbarmodel.h | 2 +- 10 files changed, 71 insertions(+), 104 deletions(-) diff --git a/src/notation/inotationplayback.h b/src/notation/inotationplayback.h index 5ae320264aad9..ac362b258a1b3 100644 --- a/src/notation/inotationplayback.h +++ b/src/notation/inotationplayback.h @@ -51,12 +51,12 @@ class INotationPlayback virtual muse::async::Channel trackAdded() const = 0; virtual muse::async::Channel trackRemoved() const = 0; - virtual muse::audio::msecs_t totalPlayTime() const = 0; - virtual muse::async::Channel totalPlayTimeChanged() const = 0; + virtual muse::audio::secs_t totalPlayTime() const = 0; + virtual muse::async::Channel 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 playPositionTickByRawTick(muse::midi::tick_t tick) const = 0; virtual muse::RetVal playPositionTickByElement(const EngravingItem* element) const = 0; diff --git a/src/notation/internal/notationplayback.cpp b/src/notation/internal/notationplayback.cpp index ecf05fd790ddf..97f7bcc0d20e9 100644 --- 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 NotationPlayback::totalPlayTimeChanged() const +muse::async::Channel 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 index d5523d7595d6e..87c29a1f07a6c 100644 --- 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 - #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 trackAdded() const override; muse::async::Channel trackRemoved() const override; - muse::audio::msecs_t totalPlayTime() const override; - muse::async::Channel totalPlayTimeChanged() const override; + muse::audio::secs_t totalPlayTime() const override; + muse::async::Channel 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 playPositionTickByRawTick(muse::midi::tick_t tick) const override; muse::RetVal 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 m_totalPlayTimeChanged; + muse::audio::secs_t m_totalPlayTime = 0; + muse::async::Channel m_totalPlayTimeChanged; mutable Tempo m_currentTempo; diff --git a/src/playback/internal/playbackcontroller.cpp b/src/playback/internal/playbackcontroller.cpp index c20523efce127..0fb3db3f0dddb 100644 --- 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(0) : 0; - newPosition = std::clamp(newPosition, startMsecs, endMsecs); + secs_t startSecs = playbackStartSecs(); + secs_t endSecs = playbackEndSecs(); + secs_t newPosition = !args.empty() ? args.arg(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 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 index 7e6c103f25c49..49a3de8df3ffa 100644 --- 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 - #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 index d358734a57a29..6d09620c9151c 100644 --- 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 index 5c233975091b5..c364ac35d9f42 100644 --- a/src/playback/playbacktypes.h +++ b/src/playback/playbacktypes.h @@ -66,30 +66,14 @@ inline QList 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 index e6592025dbca9..ea723e6c79059 100644 --- 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)); diff --git a/src/playback/view/playbacktoolbarmodel.cpp b/src/playback/view/playbacktoolbarmodel.cpp index f157416d8f4bd..10f7b13e88ebf 100644 --- a/src/playback/view/playbacktoolbarmodel.cpp +++ b/src/playback/view/playbacktoolbarmodel.cpp @@ -187,31 +187,31 @@ void PlaybackToolBarModel::setPlayTime(const QDateTime& time) doSetPlayTime(newTime); - msecs_t msec = timeToMilliseconds(newTime); - rewind(msec); + secs_t sec = timeToSeconds(newTime); + rewind(sec); } qreal PlaybackToolBarModel::playPosition() const { QTime totalTime = totalPlayTime(); - msecs_t totalMsecs = timeToMilliseconds(totalTime); + secs_t totalSecs = timeToSeconds(totalTime); - if (totalMsecs == 0) { + if (totalSecs == 0.0) { return 0; } - msecs_t msecsDifference = totalMsecs - m_playTime.msecsTo(totalTime); - qreal position = static_cast(msecsDifference) / static_cast(totalMsecs); + secs_t currentSecs = timeToSeconds(m_playTime); + qreal position = static_cast(currentSecs) / static_cast(totalSecs); return position; } void PlaybackToolBarModel::setPlayPosition(qreal position) { - msecs_t totalPlayTimeMsecs = timeToMilliseconds(totalPlayTime()); - msecs_t playPositionMsecs = totalPlayTimeMsecs * position; + secs_t totalPlayTimeSecs = timeToSeconds(totalPlayTime()); + secs_t playPositionSecs = totalPlayTimeSecs * position; - QTime time = timeFromMilliseconds(playPositionMsecs); + QTime time = timeFromSeconds(playPositionSecs); setPlayTime(QDateTime(QDate::currentDate(), time)); } @@ -252,15 +252,15 @@ void PlaybackToolBarModel::doSetPlayTime(const QTime& time) emit playPositionChanged(); } -void PlaybackToolBarModel::rewind(msecs_t milliseconds) +void PlaybackToolBarModel::rewind(secs_t secs) { - dispatch("rewind", ActionData::make_arg1(milliseconds)); + dispatch("rewind", ActionData::make_arg1(secs)); } void PlaybackToolBarModel::rewindToBeat(const MeasureBeat& beat) { - msecs_t msec = playbackController()->beatToMilliseconds(beat.measureIndex, beat.beatIndex); - rewind(msec); + secs_t secs = playbackController()->beatToSecs(beat.measureIndex, beat.beatIndex); + rewind(secs); } int PlaybackToolBarModel::measureNumber() const diff --git a/src/playback/view/playbacktoolbarmodel.h b/src/playback/view/playbacktoolbarmodel.h index 8c8c2cd74c4c9..4ceaa8a02905f 100644 --- a/src/playback/view/playbacktoolbarmodel.h +++ b/src/playback/view/playbacktoolbarmodel.h @@ -102,7 +102,7 @@ public slots: void updatePlayPosition(muse::audio::secs_t secs); void doSetPlayTime(const QTime& time); - void rewind(muse::audio::msecs_t milliseconds); + void rewind(muse::audio::secs_t secs); void rewindToBeat(const notation::MeasureBeat& beat); bool m_isToolbarFloating = false;