From 705a069400c43aab1c2c63e9f23c3f1bf2fa56c5 Mon Sep 17 00:00:00 2001 From: sukibaby <163092272+sukibaby@users.noreply.github.com> Date: Wed, 24 Apr 2024 02:31:03 -0700 Subject: [PATCH] Resolve the SM5 sync drift issue Summary of changes 1) Fix issues in RageTimer and RageSoundPosMap 2) Calculate seconds from microseconds more accurately 3) Replace lrint(x) with static_cast(x+0.5) for better performance 4) Replace C style casting with C++ style casting 5) Make important values 64-bit wide 6) Update RageUtil.cpp (timer conversion RStrings had some math which needed to be fixed after fixing RageTimer's math) 7) Ensure floating point math is done as floating point 8) Improve code commentary all around 9) Improve clarity and efficiency of RageSoundPosMap "return closest position" error logging --- src/RageSoundPosMap.cpp | 117 ++++++++++++++++------------------------ src/RageSoundPosMap.h | 5 +- src/RageTimer.cpp | 79 +++++++++++++-------------- src/RageTimer.h | 18 +++---- src/RageUtil.cpp | 38 ++++++------- 5 files changed, 118 insertions(+), 139 deletions(-) diff --git a/src/RageSoundPosMap.cpp b/src/RageSoundPosMap.cpp index 4cd779ab3e..97746eca08 100644 --- a/src/RageSoundPosMap.cpp +++ b/src/RageSoundPosMap.cpp @@ -9,18 +9,21 @@ #include #include -/* The number of frames we should keep pos_map data for. This being too high - * is mostly harmless; the data is small. */ -const int pos_map_backlog_frames = 100000; +/* The number of frames we should keep pos_map data for. +This comes out to about ~800kb in audio frames, assuming 44.1khz. +File bitrate, metadata, etc will factor in here. If the queue is +TOO big it will make things slow, but 200k frames is no problem. +Making the queue larger than 200k hasn't been tested yet. */ +const int pos_map_backlog_frames = 200000; struct pos_map_t { std::int64_t m_iSourceFrame; std::int64_t m_iDestFrame; - int m_iFrames; - float m_fSourceToDestRatio; + std::int64_t m_iFrames; + double m_fSourceToDestRatio; - pos_map_t() { m_iSourceFrame = 0; m_iDestFrame = 0; m_iFrames = 0; m_fSourceToDestRatio = 1.0f; } + pos_map_t() { m_iSourceFrame = 0; m_iDestFrame = 0; m_iFrames = 0; m_fSourceToDestRatio = 1.0; } }; struct pos_map_impl @@ -54,71 +57,53 @@ pos_map_queue &pos_map_queue::operator=( const pos_map_queue &rhs ) return *this; } -void pos_map_queue::Insert( std::int64_t iSourceFrame, int iFrames, std::int64_t iDestFrame, float fSourceToDestRatio ) +void pos_map_queue::Insert(std::int64_t iSourceFrame, std::int64_t iFrames, std::int64_t iDestFrame, double fSourceToDestRatio) { - if( !m_pImpl->m_Queue.empty() ) + bool merged = false; + if (!m_pImpl->m_Queue.empty()) { - /* Optimization: If the last entry lines up with this new entry, just merge them. */ - pos_map_t &last = m_pImpl->m_Queue.back(); - if( last.m_iSourceFrame + last.m_iFrames == iSourceFrame && - last.m_fSourceToDestRatio == fSourceToDestRatio && - llabs(last.m_iDestFrame + std::lrint(last.m_iFrames * last.m_fSourceToDestRatio) - iDestFrame) <= 1 ) + // Check if the last entry can be merged with the new entry + pos_map_t& last = m_pImpl->m_Queue.back(); + if (last.m_iSourceFrame + last.m_iFrames == iSourceFrame && + last.m_fSourceToDestRatio == fSourceToDestRatio && + + // llabs() is used instead of abs() because abs() would be susceptible to an integer overflow. + llabs(last.m_iDestFrame + static_cast((last.m_iFrames * last.m_fSourceToDestRatio) + 0.5) - iDestFrame) <= 1) + { + // Merge the frames and set the merged flag to true. last.m_iFrames += iFrames; - - /* Make sure that m_Frames doesn't grow too large and overflow an int. */ - if( !m_pImpl->m_Queue.empty() && last.m_iFrames > pos_map_backlog_frames * 2 ) - { - /* - * Split this entry into two smaller entries. This will cause up to one - * sample of rounding error in m_iDestFrame. This will not accumulate; - * if we split again and it becomes two frames of error, the next Insert() - * will be beyond the tolerance of the above iDestFrame check, and new - * data will be added to a new entry. - */ - int iDeleteFrames = last.m_iFrames - pos_map_backlog_frames; - - pos_map_t next(last); - - last.m_iFrames = iDeleteFrames; - - next.m_iSourceFrame += iDeleteFrames; - next.m_iFrames -= iDeleteFrames; - next.m_iDestFrame += std::lrint( iDeleteFrames * next.m_fSourceToDestRatio ); - - m_pImpl->m_Queue.push_back( next ); - } - - m_pImpl->Cleanup(); - - return; + merged = true; } } - m_pImpl->m_Queue.push_back( pos_map_t() ); - pos_map_t &m = m_pImpl->m_Queue.back(); - m.m_iSourceFrame = iSourceFrame; - m.m_iDestFrame = iDestFrame; - m.m_iFrames = iFrames; - m.m_fSourceToDestRatio = fSourceToDestRatio; + if (!merged) + { + m_pImpl->m_Queue.push_back(pos_map_t()); + pos_map_t& m = m_pImpl->m_Queue.back(); + m.m_iSourceFrame = iSourceFrame; + m.m_iDestFrame = iDestFrame; + m.m_iFrames = iFrames; + m.m_fSourceToDestRatio = fSourceToDestRatio; + } m_pImpl->Cleanup(); } void pos_map_impl::Cleanup() { - /* Scan backwards until we have at least pos_map_backlog_frames. */ std::list::iterator it = m_Queue.end(); - int iTotalFrames = 0; - while( iTotalFrames < pos_map_backlog_frames ) + std::int64_t iTotalFrames = 0; + // Scan backwards until we have at least pos_map_backlog_frames. + while (iTotalFrames < pos_map_backlog_frames) { - if( it == m_Queue.begin() ) + if (it == m_Queue.begin()) break; --it; iTotalFrames += it->m_iFrames; } - m_Queue.erase( m_Queue.begin(), it ); + m_Queue.erase(m_Queue.begin(), it); } std::int64_t pos_map_queue::Search( std::int64_t iSourceFrame, bool *bApproximate ) const @@ -133,38 +118,34 @@ std::int64_t pos_map_queue::Search( std::int64_t iSourceFrame, bool *bApproximat return 0; } - /* iSourceFrame is probably in pos_map. Search to figure out what position - * it maps to. */ - std::int64_t iClosestPosition = 0, iClosestPositionDist = INT_MAX; - const pos_map_t *pClosestBlock = &*m_pImpl->m_Queue.begin(); /* print only */ + // iSourceFrame is probably in pos_map. Search to figure out what position it maps to. + std::int64_t iClosestPosition = 0, iClosestPositionDist = std::numeric_limits::max(); for (pos_map_t const &pm : m_pImpl->m_Queue) { + // Loop over the queue until we know generally where iSourceFrame is if( iSourceFrame >= pm.m_iSourceFrame && iSourceFrame < pm.m_iSourceFrame+pm.m_iFrames ) { - /* iSourceFrame lies in this block; it's an exact match. Figure - * out the exact position. */ - int iDiff = int(iSourceFrame - pm.m_iSourceFrame); - iDiff = std::lrint( iDiff * pm.m_fSourceToDestRatio ); + // If we are in the correct block, calculate its current position + std::int64_t iDiff = static_cast(iSourceFrame - pm.m_iSourceFrame); + iDiff = static_cast(( iDiff * pm.m_fSourceToDestRatio) + 0.5 ); return pm.m_iDestFrame + iDiff; } - /* See if the current position is close to the beginning of this block. */ + // See if the current position is close to the beginning of this block. std::int64_t dist = llabs( pm.m_iSourceFrame - iSourceFrame ); if( dist < iClosestPositionDist ) { iClosestPositionDist = dist; - pClosestBlock = ± iClosestPosition = pm.m_iDestFrame; } - /* See if the current position is close to the end of this block. */ + // See if the current position is close to the end of this block. dist = llabs( pm.m_iSourceFrame + pm.m_iFrames - iSourceFrame ); if( dist < iClosestPositionDist ) { iClosestPositionDist = dist; - pClosestBlock = ± - iClosestPosition = pm.m_iDestFrame + std::lrint( pm.m_iFrames * pm.m_fSourceToDestRatio ); + iClosestPosition = pm.m_iDestFrame + static_cast((pm.m_iFrames * pm.m_fSourceToDestRatio) + 0.5 ); } } @@ -181,12 +162,8 @@ std::int64_t pos_map_queue::Search( std::int64_t iSourceFrame, bool *bApproximat static RageTimer last; if( last.PeekDeltaTime() >= 1.0f ) { - last.GetDeltaTime(); - std::stringstream ss; - ss << "Approximate sound time: driver frame " << iSourceFrame << ", m_pImpl->m_Queue frame " - << pClosestBlock->m_iDestFrame << ".." << (pClosestBlock->m_iDestFrame+pClosestBlock->m_iFrames) - << " (dist " << iClosestPositionDist << "), closest position is " << iClosestPosition; - LOG->Trace( "%s", ss.str().c_str() ); + last.Touch(); + LOG->Trace("Audio frame was out of range of the data sent - possible buffer underflow? This is not always an error, however if you see it frequently there could be sound buffer problems."); } if( bApproximate ) diff --git a/src/RageSoundPosMap.h b/src/RageSoundPosMap.h index 98201fab0b..1e5b5effee 100644 --- a/src/RageSoundPosMap.h +++ b/src/RageSoundPosMap.h @@ -14,8 +14,9 @@ public: pos_map_queue( const pos_map_queue &cpy ); pos_map_queue &operator=( const pos_map_queue &rhs ); - /* Insert a mapping from iSourceFrame to iDestFrame, containing iFrames. */ - void Insert( std::int64_t iSourceFrame, int iFrames, std::int64_t iDestFrame, float fSourceToDestRatio = 1.0f ); + /* Insert a mapping from iSourceFrame to iDestFrame, containing iFrames. + * The double type is used to prevent precision loss leading to sync drift the longer the game runs. -sukibaby */ + void Insert( std::int64_t iSourceFrame, std::int64_t iFrames, std::int64_t iDestFrame, double fSourceToDestRatio = 1.0 ); /* Return the iDestFrame for the given iSourceFrame. */ std::int64_t Search( std::int64_t iSourceFrame, bool *bApproximate ) const; diff --git a/src/RageTimer.cpp b/src/RageTimer.cpp index dd7a987464..dd11cf89de 100644 --- a/src/RageTimer.cpp +++ b/src/RageTimer.cpp @@ -38,27 +38,21 @@ static std::uint64_t g_iStartTime = ArchHooks::GetMicrosecondsSinceStart( true ) static std::uint64_t GetTime( bool /* bAccurate */ ) { return ArchHooks::GetMicrosecondsSinceStart( true ); - - /* This isn't threadsafe, and locking it would undo any benefit of not - * calling GetMicrosecondsSinceStart. */ -#if 0 - // if !bAccurate, then don't call ArchHooks to find the current time. Just return the - // last calculated time. GetMicrosecondsSinceStart is slow on some archs. - static std::uint64_t usecs = 0; - if( bAccurate ) - usecs = ArchHooks::GetMicrosecondsSinceStart( true ); - return usecs; -#endif } -float RageTimer::GetTimeSinceStart( bool bAccurate ) +/* The accuracy of RageTimer::GetTimeSinceStart() is directly tied to the + * stability of the clock sync. Maintaining precision here is crucial. Too + * much error here will manifest as a drastic shift in the game's sync, and + * will feel like the global offset suddenly changed. Incorrect math here will + * manifest as a _consistent_ sync offset in game. Resolution mismatches or + * values truncated or rounded when they shouldn't be can cause errors when + * this is calculated and manifest as a _sudden_ drift of sync. Use caution + * and do thorough testing if you change anything here. -sukibaby */ +double RageTimer::GetTimeSinceStart(bool bAccurate) { - std::uint64_t usecs = GetTime( bAccurate ); + std::uint64_t usecs = GetTime(bAccurate); usecs -= g_iStartTime; - /* Avoid using doubles for hardware that doesn't support them. - * This is writing usecs = high*2^32 + low and doing - * usecs/10^6 = high * (2^32/10^6) + low/10^6. */ - return std::uint32_t(usecs>>32) * 4294.967296f + std::uint32_t(usecs)/1000000.f; + return usecs / 1000000.0; } std::uint64_t RageTimer::GetUsecsSinceStart() @@ -70,8 +64,8 @@ void RageTimer::Touch() { std::uint64_t usecs = GetTime( true ); - this->m_secs = unsigned(usecs / 1000000); - this->m_us = unsigned(usecs % 1000000); + this->m_secs = std::uint64_t(usecs / TIMESTAMP_RESOLUTION); + this->m_us = std::uint64_t(usecs % TIMESTAMP_RESOLUTION); } float RageTimer::Ago() const @@ -88,15 +82,14 @@ float RageTimer::GetDeltaTime() return diff; } -/* - * Get a timer representing half of the time ago as this one. This is - * useful for averaging time. For example, - * - * RageTimer tm; - * ... do stuff ... - * RageTimer AverageTime = tm.Half(); - * printf( "Something happened approximately %f seconds ago.\n", tm.Ago() ); - */ +/* Get a timer representing half of the time ago as this one. This is + * useful for averaging time. For example, + * + * RageTimer tm; + * ... do stuff ... + * RageTimer AverageTime = tm.Half(); + * printf( "Something happened approximately %f seconds ago.\n", tm.Ago() ); + * Note this has been reverted to the original SM3.95 function. */ RageTimer RageTimer::Half() const { const float fProbableDelay = Ago() / 2; @@ -121,18 +114,23 @@ bool RageTimer::operator<( const RageTimer &rhs ) const } -RageTimer RageTimer::Sum(const RageTimer &lhs, float tm) +RageTimer RageTimer::Sum(const RageTimer& lhs, float tm) { - /* tm == 5.25 -> secs = 5, us = 5.25 - ( 5) = .25 + /* Calculate the seconds and microseconds from the time: + * tm == 5.25 -> secs = 5, us = 5.25 - ( 5) = .25 * tm == -1.25 -> secs = -2, us = -1.25 - (-2) = .75 */ - int seconds = std::floor(tm); - int us = int( (tm - seconds) * TIMESTAMP_RESOLUTION ); + int64_t seconds = std::floor(tm); + int64_t us = int64_t((tm - seconds) * TIMESTAMP_RESOLUTION); - RageTimer ret(0,0); // Prevent unnecessarily checking the time + // Prevent unnecessarily checking the time + RageTimer ret(0, 0); + + // Calculate the sum of the seconds and microseconds ret.m_secs = seconds + lhs.m_secs; ret.m_us = us + lhs.m_us; - if( ret.m_us >= TIMESTAMP_RESOLUTION ) + // Adjust the seconds and microseconds if microseconds is greater than or equal to TIMESTAMP_RESOLUTION + if (ret.m_us >= TIMESTAMP_RESOLUTION) { ret.m_us -= TIMESTAMP_RESOLUTION; ++ret.m_secs; @@ -141,18 +139,21 @@ RageTimer RageTimer::Sum(const RageTimer &lhs, float tm) return ret; } -float RageTimer::Difference(const RageTimer &lhs, const RageTimer &rhs) +double RageTimer::Difference(const RageTimer& lhs, const RageTimer& rhs) { - int secs = lhs.m_secs - rhs.m_secs; - int us = lhs.m_us - rhs.m_us; + // Calculate the difference in seconds and microseconds respectively + int64_t secs = lhs.m_secs - rhs.m_secs; + int64_t us = lhs.m_us - rhs.m_us; - if( us < 0 ) + // Adjust seconds and microseconds if microseconds is negative + if ( us < 0 ) { us += TIMESTAMP_RESOLUTION; --secs; } - return float(secs) + float(us) / TIMESTAMP_RESOLUTION; + // Return the difference as a double to preserve the fractional part + return static_cast(secs) + static_cast(us) / TIMESTAMP_RESOLUTION; } #include "LuaManager.h" diff --git a/src/RageTimer.h b/src/RageTimer.h index a642a02579..213e256edb 100644 --- a/src/RageTimer.h +++ b/src/RageTimer.h @@ -8,8 +8,9 @@ class RageTimer { public: + /* Initialize the m_secs and m_us values to 0 and then fill them with the current time. */ RageTimer(): m_secs(0), m_us(0) { Touch(); } - RageTimer( int secs, int us ): m_secs(secs), m_us(us) { } + RageTimer( int64_t secs, int64_t us ): m_secs(secs), m_us(us) { } /* Time ago this RageTimer represents. */ float Ago() const; @@ -22,8 +23,7 @@ public: /* (alias) */ float PeekDeltaTime() const { return Ago(); } - /* deprecated: */ - static float GetTimeSinceStart( bool bAccurate = true ); // seconds since the program was started + static double GetTimeSinceStart( bool bAccurate = true ); // seconds since the program was started static float GetTimeSinceStartFast() { return GetTimeSinceStart(false); } static std::uint64_t GetUsecsSinceStart(); @@ -41,15 +41,15 @@ public: bool operator<( const RageTimer &rhs ) const; - /* "float" is bad for a "time since start" RageTimer. If the game is running for - * several days, we'll lose a lot of resolution. I don't want to use double - * everywhere, since it's slow. I'd rather not use double just for RageTimers, since - * it's too easy to get a type wrong and end up with obscure resolution problems. */ - unsigned m_secs, m_us; + /* The following is a "time since start" RageTimer. Splitting the seconds and + * microseconds values into two integers and combining them later allows for + * better precision. Use caution when changing data types, since resolution + * mismatch errors are easy to cause when changing things in RageTimer. */ + std::uint64_t m_secs, m_us; private: static RageTimer Sum( const RageTimer &lhs, float tm ); - static float Difference( const RageTimer &lhs, const RageTimer &rhs ); + static double Difference( const RageTimer &lhs, const RageTimer &rhs ); }; extern const RageTimer RageZeroTimer; diff --git a/src/RageUtil.cpp b/src/RageUtil.cpp index 8478a8bc41..bf1b883967 100644 --- a/src/RageUtil.cpp +++ b/src/RageUtil.cpp @@ -226,53 +226,53 @@ float HHMMSSToSeconds( const RString &sHHMMSS ) return fSeconds; } -RString SecondsToHHMMSS( float fSecs ) +RString SecondsToHHMMSS(float fSecs) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; - RString sReturn = ssprintf( "%02d:%02d:%02d", iMinsDisplay/60, iMinsDisplay%60, iSecsDisplay ); + const int iMinsDisplay = static_cast(fSecs / 60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); + RString sReturn = ssprintf("%02d:%02d:%02d", iMinsDisplay / 60, iMinsDisplay % 60, iSecsDisplay); return sReturn; } -RString SecondsToMMSSMsMs( float fSecs ) +RString SecondsToMMSSMsMs(float fSecs) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; - const int iLeftoverDisplay = (int) ( (fSecs - iMinsDisplay*60 - iSecsDisplay) * 100 ); - RString sReturn = ssprintf( "%02d:%02d.%02d", iMinsDisplay, iSecsDisplay, std::min(99,iLeftoverDisplay) ); + const int iMinsDisplay = static_cast(fSecs / 60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); + const int iLeftoverDisplay = static_cast((fSecs - iMinsDisplay * 60 - iSecsDisplay) * 100); + RString sReturn = ssprintf("%02d:%02d.%02d", iMinsDisplay, iSecsDisplay, std::min(99, iLeftoverDisplay)); return sReturn; } RString SecondsToMSSMsMs( float fSecs ) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; - const int iLeftoverDisplay = (int) ( (fSecs - iMinsDisplay*60 - iSecsDisplay) * 100 ); + const int iMinsDisplay = static_cast(fSecs/60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); + const int iLeftoverDisplay = static_cast((fSecs - iMinsDisplay*60 - iSecsDisplay) * 100 ); RString sReturn = ssprintf( "%01d:%02d.%02d", iMinsDisplay, iSecsDisplay, std::min(99,iLeftoverDisplay) ); return sReturn; } RString SecondsToMMSSMsMsMs( float fSecs ) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; - const int iLeftoverDisplay = (int) ( (fSecs - iMinsDisplay*60 - iSecsDisplay) * 1000 ); + const int iMinsDisplay = static_cast(fSecs/60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); + const int iLeftoverDisplay = static_cast((fSecs - iMinsDisplay*60 - iSecsDisplay) * 1000 ); RString sReturn = ssprintf( "%02d:%02d.%03d", iMinsDisplay, iSecsDisplay, std::min(999,iLeftoverDisplay) ); return sReturn; } RString SecondsToMSS( float fSecs ) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; + const int iMinsDisplay = static_cast(fSecs/60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); RString sReturn = ssprintf( "%01d:%02d", iMinsDisplay, iSecsDisplay); return sReturn; } RString SecondsToMMSS( float fSecs ) { - const int iMinsDisplay = (int)fSecs/60; - const int iSecsDisplay = (int)fSecs - iMinsDisplay*60; + const int iMinsDisplay = static_cast(fSecs/60); + const int iSecsDisplay = static_cast(fmod(fSecs, 60)); RString sReturn = ssprintf( "%02d:%02d", iMinsDisplay, iSecsDisplay); return sReturn; }