From 4e3a9b382c686c590e5e3d2c96bd9a6decbe3196 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 21 May 2026 12:03:24 -0400 Subject: [PATCH 1/4] Fix 32-bit overflow in AudioStreamInfo::frames_to_microseconds frames * US_PER_SECOND was computed as a uint32 product, which overflows above ~4295 frames (~97 ms at 44.1 kHz). The hard-sync drop path in SyncTask passes a frame count bounded only by the decode buffer size, so a large drop could silently corrupt decoded_timestamp precisely during resync. Widen the multiply to 64-bit and return int64_t, making the conversion exact for any frame count. This removes the need for the two-step remainder decomposition that callers used purely to dodge the overflow: frames_to_milliseconds_with_remainder (along with its gcd helper and the ms_sample_rate_gcd_ member) is deleted, and the two call sites in SyncTask collapse to a single frames_to_microseconds() call with identical results. --- src/audio_stream_info.cpp | 33 +++++++-------------------------- src/audio_stream_info.h | 14 +++++--------- src/sync_task.cpp | 13 ++----------- 3 files changed, 14 insertions(+), 46 deletions(-) diff --git a/src/audio_stream_info.cpp b/src/audio_stream_info.cpp index 1c393fa..35a2e96 100644 --- a/src/audio_stream_info.cpp +++ b/src/audio_stream_info.cpp @@ -16,26 +16,12 @@ namespace sendspin { -// ============================================================================ -// Static helpers -// ============================================================================ - -static uint32_t gcd(uint32_t a, uint32_t b) { - while (b != 0) { - uint32_t t = b; - b = a % b; - a = t; - } - return a; -} - // ============================================================================ // Constructor / Destructor // ============================================================================ AudioStreamInfo::AudioStreamInfo(uint8_t bits_per_sample, uint8_t channels, uint32_t sample_rate) : sample_rate_(sample_rate), bits_per_sample_(bits_per_sample), channels_(channels) { - this->ms_sample_rate_gcd_ = gcd(MS_PER_SECOND, this->sample_rate_); this->bytes_per_sample_ = (this->bits_per_sample_ + 7) / 8; } @@ -43,18 +29,13 @@ AudioStreamInfo::AudioStreamInfo(uint8_t bits_per_sample, uint8_t channels, uint // Public API // ============================================================================ -uint32_t AudioStreamInfo::frames_to_microseconds(uint32_t frames) const { - return (frames * US_PER_SECOND + (this->sample_rate_ >> 1)) / this->sample_rate_; -} - -uint32_t AudioStreamInfo::frames_to_milliseconds_with_remainder(uint32_t* total_frames) const { - uint32_t unprocessable_frames = - *total_frames % (this->sample_rate_ / this->ms_sample_rate_gcd_); - uint32_t frames_for_ms_calculation = *total_frames - unprocessable_frames; - - uint32_t playback_ms = (frames_for_ms_calculation * MS_PER_SECOND) / this->sample_rate_; - *total_frames = unprocessable_frames; - return playback_ms; +int64_t AudioStreamInfo::frames_to_microseconds(uint32_t frames) const { + // The product is widened to 64-bit before the multiply so it cannot overflow for any frame + // count (a 32-bit product overflows above ~4295 frames, i.e. ~97 ms at 44.1 kHz). The + // single rounded divide is exact for the whole input, which removes the need to peel off + // whole milliseconds separately before converting the sub-millisecond remainder. + return (static_cast(frames) * US_PER_SECOND + (this->sample_rate_ >> 1)) / + this->sample_rate_; } bool AudioStreamInfo::operator==(const AudioStreamInfo& rhs) const { diff --git a/src/audio_stream_info.h b/src/audio_stream_info.h index a237610..709aaa6 100644 --- a/src/audio_stream_info.h +++ b/src/audio_stream_info.h @@ -46,7 +46,7 @@ static constexpr uint32_t DEFAULT_SAMPLE_RATE_HZ = 16000U; * * size_t bytes_for_100ms = info.ms_to_bytes(100); * uint32_t frames = info.bytes_to_frames(bytes_for_100ms); - * uint32_t duration_us = info.frames_to_microseconds(frames); + * int64_t duration_us = info.frames_to_microseconds(frames); * @endcode */ class AudioStreamInfo { @@ -115,13 +115,10 @@ class AudioStreamInfo { /// @brief Converts a frame count to microseconds /// @param frames Number of audio frames. /// @return Duration in microseconds. - uint32_t frames_to_microseconds(uint32_t frames) const; - - /// @brief Converts frames to milliseconds, updating frames with the remainder - /// @param[out] frames Pointer to the frame count; updated in place with the leftover frames - /// that could not be converted to a whole millisecond. - /// @return Whole milliseconds represented by the converted frames. - uint32_t frames_to_milliseconds_with_remainder(uint32_t* frames) const; + /// @note The intermediate frames * US_PER_SECOND product is computed in 64-bit, so this is + /// exact for any frame count (no 32-bit overflow). The result is returned as int64_t to + /// match the playtime accumulators that consume it. + int64_t frames_to_microseconds(uint32_t frames) const; /// @brief Returns true if both AudioStreamInfo objects describe the same format /// @param rhs The other AudioStreamInfo to compare against. @@ -140,7 +137,6 @@ class AudioStreamInfo { size_t bytes_per_sample_; // 32-bit fields - uint32_t ms_sample_rate_gcd_; uint32_t sample_rate_; // 8-bit fields diff --git a/src/sync_task.cpp b/src/sync_task.cpp index eeba48e..ea46cb6 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -303,11 +303,8 @@ SyncTaskState SyncTask::handle_transfer_audio(SyncContext& sync_context) { void SyncTask::track_sent_audio(SyncContext& sync_context, size_t bytes_sent) { uint32_t frames_sent = sync_context.current_stream_info.bytes_to_frames(bytes_sent); sync_context.buffered_frames += frames_sent; - uint32_t remainder = frames_sent; - int64_t ms = sync_context.current_stream_info.frames_to_milliseconds_with_remainder(&remainder); sync_context.new_audio_client_playtime += - US_PER_MS * ms + - static_cast(sync_context.current_stream_info.frames_to_microseconds(remainder)); + sync_context.current_stream_info.frames_to_microseconds(frames_sent); } void SyncTask::send_pending_silence(SyncContext& sync_context) { @@ -692,14 +689,8 @@ void SyncTask::process_playback_progress(SyncContext& sync_context) { sync_context.buffered_frames -= frames_played; } - uint32_t unplayed_frames = sync_context.buffered_frames; - int64_t unplayed_ms = - sync_context.current_stream_info.frames_to_milliseconds_with_remainder( - &unplayed_frames); int64_t unplayed_us = - US_PER_MS * unplayed_ms + - static_cast( - sync_context.current_stream_info.frames_to_microseconds(unplayed_frames)); + sync_context.current_stream_info.frames_to_microseconds(sync_context.buffered_frames); sync_context.new_audio_client_playtime = playback_progress.finish_timestamp + unplayed_us; } } From 9367654f4a3df9227d13ff1fbd4f2044d9d0a316 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 21 May 2026 12:41:47 -0400 Subject: [PATCH 2/4] Make comment about the objective state of the current code --- src/audio_stream_info.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/audio_stream_info.cpp b/src/audio_stream_info.cpp index 35a2e96..bdb4551 100644 --- a/src/audio_stream_info.cpp +++ b/src/audio_stream_info.cpp @@ -31,9 +31,7 @@ AudioStreamInfo::AudioStreamInfo(uint8_t bits_per_sample, uint8_t channels, uint int64_t AudioStreamInfo::frames_to_microseconds(uint32_t frames) const { // The product is widened to 64-bit before the multiply so it cannot overflow for any frame - // count (a 32-bit product overflows above ~4295 frames, i.e. ~97 ms at 44.1 kHz). The - // single rounded divide is exact for the whole input, which removes the need to peel off - // whole milliseconds separately before converting the sub-millisecond remainder. + // count (a 32-bit product overflows above ~4295 frames, i.e. ~97 ms at 44.1 kHz). return (static_cast(frames) * US_PER_SECOND + (this->sample_rate_ >> 1)) / this->sample_rate_; } From 369c76d23024d99317efa7a8eee3c71fbd1b05f9 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 21 May 2026 12:44:15 -0400 Subject: [PATCH 3/4] Avoid absolute language in comment --- src/audio_stream_info.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/audio_stream_info.cpp b/src/audio_stream_info.cpp index bdb4551..2887a53 100644 --- a/src/audio_stream_info.cpp +++ b/src/audio_stream_info.cpp @@ -30,8 +30,8 @@ AudioStreamInfo::AudioStreamInfo(uint8_t bits_per_sample, uint8_t channels, uint // ============================================================================ int64_t AudioStreamInfo::frames_to_microseconds(uint32_t frames) const { - // The product is widened to 64-bit before the multiply so it cannot overflow for any frame - // count (a 32-bit product overflows above ~4295 frames, i.e. ~97 ms at 44.1 kHz). + // The product is widened to 64-bit before the multiply so it cannot overflow for any reasonable + // frame count. return (static_cast(frames) * US_PER_SECOND + (this->sample_rate_ >> 1)) / this->sample_rate_; } From e769ffae1738e6ece7ab46352e47ecb86b04a46e Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 21 May 2026 12:45:41 -0400 Subject: [PATCH 4/4] Address another comment precision issue --- src/audio_stream_info.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/audio_stream_info.h b/src/audio_stream_info.h index 709aaa6..e4c9806 100644 --- a/src/audio_stream_info.h +++ b/src/audio_stream_info.h @@ -116,8 +116,8 @@ class AudioStreamInfo { /// @param frames Number of audio frames. /// @return Duration in microseconds. /// @note The intermediate frames * US_PER_SECOND product is computed in 64-bit, so this is - /// exact for any frame count (no 32-bit overflow). The result is returned as int64_t to - /// match the playtime accumulators that consume it. + /// avoids overflow for any reasonable frame count. The result is returned as int64_t + /// to match the playtime accumulators that consume it. int64_t frames_to_microseconds(uint32_t frames) const; /// @brief Returns true if both AudioStreamInfo objects describe the same format