Fix 32-bit overflow in AudioStreamInfo::frames_to_microseconds#58
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an overflow bug in AudioStreamInfo::frames_to_microseconds by widening the intermediate arithmetic, ensuring correct timestamp/playtime math when handling larger frame counts (notably in SyncTask’s hard-sync drop/resync paths).
Changes:
- Change
frames_to_microsecondsto use a 64-bit intermediate multiply and returnint64_t. - Remove the milliseconds + remainder decomposition helper and simplify
SyncTaskplaytime computations to a singleframes_to_microseconds()call. - Remove now-unused
gcdhelper and related state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/sync_task.cpp | Simplifies playtime accumulation to rely on the widened frames_to_microseconds() conversion. |
| src/audio_stream_info.h | Updates API signature/docs for frames_to_microseconds() and removes the milliseconds-with-remainder helper + state. |
| src/audio_stream_info.cpp | Implements the 64-bit-safe conversion and removes the old gcd-based decomposition logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
5f1ac4e to
e769ffa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
frames_to_microsecondscomputedframes * US_PER_SECONDas auint32 * uint32product, which overflows above ~4295 frames (~97 ms at 44.1 kHz). Most callers dodged this by pre-reducing the frame count, but the hard-sync drop path inSyncTaskpasses a frame count bounded only by the decode buffer size, so a large drop could silently corruptdecoded_timestampprecisely during resync.Changes
int64_t, making the conversion exact for any frame count.frames_to_milliseconds_with_remainder(and itsgcdhelper and thems_sample_rate_gcd_member).SyncTask(track_sent_audioand the unplayed-frames calc inprocess_playback_progress) to a singleframes_to_microseconds()call with identical numeric results: the decomposition was a pure overflow-dodge, not a precision trick.Performance note (32-bit targets)
The only added cost is a 64-bit/32-bit software divide (
__udivdi3) in place of the previous all-32-bit arithmetic. This runs at audio-chunk granularity (~tens of Hz), not per-sample, so the absolute cost is negligible (well under 0.01% CPU). The genuinely hot hand-optimized path (theclient/timeformatter) is unaffected.Testing
Verified the library and both host examples build cleanly. Behavioral test coverage, a large-frame case pinning this fix, will be a follow-up unit-test-infrastructure PR.