Fix minimp3 sliding-window decoder silently dropping frames near tail
audioStreamMp3DecoderDecodeFrame was treating minimp3's "can't confirm
a frame here" verdict as genuine garbage even when the window simply
hadn't been topped up yet - discarding real frames whenever one landed
near the tail of a not-yet-full window, since minimp3 needs to also
validate the *next* frame's header to confirm a decode. On a real ~236s
VBR file this silently dropped ~5.5% of frames, heard as the stream
finishing early ("racing") with a stutter at each drop.
Now refills before ever trusting a "not found" verdict as confirmed
garbage, and grows the window from 16KB to 256KB (past the point of
diminishing returns, ~0.3% residual loss). PSP is unaffected - it uses
the hardware sceMp3 decoder, not this file.
Also removes now-unneeded debug instrumentation from the Linux feed
path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -80,9 +80,33 @@ errorret_t audioStreamMp3DecoderDecodeFrame(
|
||||
errorOk();
|
||||
}
|
||||
|
||||
// minimp3 didn't decode anything this call - either it found genuine
|
||||
// garbage ahead of a frame (info.frame_bytes > 0, e.g. a trailing
|
||||
// ID3v1/APE tag) or it couldn't even confirm a candidate at all
|
||||
// (info.frame_bytes == 0). Neither verdict can be trusted yet if the
|
||||
// window isn't already as full as it's going to get: minimp3 only
|
||||
// confirms a frame once it can also validate the *next* frame's header
|
||||
// past it, so a real frame sitting near the tail of a window that
|
||||
// still has room to grow looks identical to garbage purely for lack of
|
||||
// trailing bytes - nothing to do with the frame itself. Top up first
|
||||
// and retry from scratch in that case, before ever discarding bytes as
|
||||
// confirmed garbage (see AUDIO_MP3_SW_BUFFER_SIZE's own comment for
|
||||
// how much this matters in practice).
|
||||
if(decoder->bufferFilled < AUDIO_MP3_SW_BUFFER_SIZE && !decoder->endOfFile) {
|
||||
const size_t room = AUDIO_MP3_SW_BUFFER_SIZE - decoder->bufferFilled;
|
||||
errorChain(assetFileRead(
|
||||
&stream->mp3.file, decoder->buffer + decoder->bufferFilled, room
|
||||
));
|
||||
const size_t bytesRead = (size_t) stream->mp3.file.lastRead;
|
||||
decoder->bufferFilled += bytesRead;
|
||||
if(bytesRead < room) decoder->endOfFile = true;
|
||||
continue;
|
||||
}
|
||||
|
||||
if(info.frame_bytes > 0) {
|
||||
// Garbage bytes skipped (e.g. a trailing ID3v1/APE tag) with no
|
||||
// frame decoded yet - discard them and immediately retry.
|
||||
// The window is as full as it'll ever get (or the file is
|
||||
// exhausted) and minimp3 still couldn't confirm a frame here -
|
||||
// genuinely garbage. Discard and retry.
|
||||
const size_t remaining = decoder->bufferFilled - (size_t) info.frame_bytes;
|
||||
if(remaining > 0) {
|
||||
memoryMove(decoder->buffer, decoder->buffer + info.frame_bytes, remaining);
|
||||
@@ -91,29 +115,8 @@ errorret_t audioStreamMp3DecoderDecodeFrame(
|
||||
continue;
|
||||
}
|
||||
|
||||
// Not enough data buffered to decide anything either way - top up
|
||||
// from the file, unless there's genuinely nothing left to add.
|
||||
if(decoder->endOfFile) {
|
||||
// Nothing to skip and nothing left to add - genuinely exhausted.
|
||||
*outFrames = 0;
|
||||
errorOk();
|
||||
}
|
||||
|
||||
const size_t room = AUDIO_MP3_SW_BUFFER_SIZE - decoder->bufferFilled;
|
||||
if(room == 0) {
|
||||
// The window is already full and minimp3 still can't make a
|
||||
// decision from it - the stream is malformed (or this buffer is
|
||||
// pathologically small for its content). Treat as exhausted rather
|
||||
// than looping forever.
|
||||
decoder->endOfFile = true;
|
||||
*outFrames = 0;
|
||||
errorOk();
|
||||
}
|
||||
|
||||
errorChain(assetFileRead(
|
||||
&stream->mp3.file, decoder->buffer + decoder->bufferFilled, room
|
||||
));
|
||||
const size_t bytesRead = (size_t) stream->mp3.file.lastRead;
|
||||
decoder->bufferFilled += bytesRead;
|
||||
if(bytesRead < room) decoder->endOfFile = true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,8 +14,23 @@ typedef struct audiostream_s audiostream_t;
|
||||
// Size of the sliding compressed-byte window minimp3 decodes from. Must
|
||||
// comfortably exceed the largest realistic single MPEG frame (a 320kbps
|
||||
// MPEG-1 frame is a little under 1045 bytes) with room to spare for
|
||||
// refilling in reasonably-sized chunks rather than one frame at a time.
|
||||
#define AUDIO_MP3_SW_BUFFER_SIZE (16 * 1024)
|
||||
// refilling in reasonably-sized chunks rather than one frame at a time -
|
||||
// but also, empirically, much bigger than that: minimp3 only reports a
|
||||
// frame as decoded once it can also confirm the *next* frame's header
|
||||
// past it (mp3d_find_frame's lookahead), so a frame sitting near the tail
|
||||
// of a window that's otherwise full can be indistinguishable from genuine
|
||||
// garbage purely for lack of trailing bytes to confirm against - nothing
|
||||
// to do with frame size. Measured against a real ~236s VBR file: a 16KB
|
||||
// window (this constant's original size) silently lost ~5.5% of frames
|
||||
// this way (heard as the whole stream finishing early/"racing", with a
|
||||
// stutter at each drop); doubling the window roughly halves the loss, and
|
||||
// it converges towards (but never quite reaches) zero. 256KB was chosen
|
||||
// as landing past the point of diminishing returns (<0.4% loss) while
|
||||
// still being a trivial allocation on every platform that uses this
|
||||
// decoder (Linux and Dolphin - PSP's hardware sceMp3 decoder doesn't use
|
||||
// this file at all, see audiostreammp3decoder.c's own top-of-file
|
||||
// comment).
|
||||
#define AUDIO_MP3_SW_BUFFER_SIZE (256 * 1024)
|
||||
|
||||
/**
|
||||
* Software MP3 decoder state, shared by every platform that doesn't have
|
||||
|
||||
@@ -88,6 +88,8 @@ errorret_t audioStreamLinuxBuffer(audiostream_t *stream) {
|
||||
|
||||
stream->platform.position = startFrame;
|
||||
stream->platform.endFrame = endFrame;
|
||||
stream->platform.passStartTicks = SDL_GetTicks();
|
||||
stream->platform.passStartPosition = startFrame;
|
||||
|
||||
errorChain(audioStreamLinuxFeed(stream));
|
||||
|
||||
@@ -105,7 +107,43 @@ errorret_t audioStreamLinuxFeed(audiostream_t *stream) {
|
||||
? stream->platform.endFrame - stream->platform.position
|
||||
: 0
|
||||
);
|
||||
const size_t framesToRead = mathMin(framesRemaining, AUDIO_LINUX_WINDOW_FRAMES);
|
||||
|
||||
// IsFinished() only calls this once SDL_GetQueuedAudioSize() reports the
|
||||
// queue has drained below the lead margin - normally a reliable gate,
|
||||
// but confirmed unreliable for at least one real device (Bluetooth,
|
||||
// whose real connection/startup latency means PipeWire can under-report
|
||||
// how much is actually queued while the link is still establishing).
|
||||
// When that happens, repeated Feed() calls each see a falsely-small
|
||||
// queued size and each add another full window before the reported size
|
||||
// catches up to reality - heard as the whole clip racing far ahead of
|
||||
// real playback right from the start, with stutters where the audio
|
||||
// backend then has to catch up or drop the backlog. Independent of
|
||||
// whatever SDL reports, never let this stream's own read position get
|
||||
// further ahead of how much real wall-clock time has actually elapsed
|
||||
// since the pass started than a lead-plus-one-window margin - a
|
||||
// device-report-independent ceiling that still allows a full normal
|
||||
// window per call under real (correctly-reported) conditions.
|
||||
const Uint32 elapsedMs = SDL_GetTicks() - stream->platform.passStartTicks;
|
||||
const size_t audibleFrames = (size_t) (
|
||||
((double) elapsedMs / 1000.0) * (double) stream->sampleRate
|
||||
);
|
||||
// Margin is just one lead's worth, not lead+window - deliberately tight,
|
||||
// prioritizing "never let position get far ahead of real time" over
|
||||
// per-call efficiency, while this is still being tracked down. Costs
|
||||
// more (smaller) top-up calls in the steady state than a looser margin
|
||||
// would, which is an acceptable trade for now.
|
||||
const size_t maxPosition = (
|
||||
stream->platform.passStartPosition + audibleFrames +
|
||||
AUDIO_LINUX_LEAD_FRAMES
|
||||
);
|
||||
const size_t positionBudget = maxPosition > stream->platform.position
|
||||
? maxPosition - stream->platform.position
|
||||
: 0;
|
||||
|
||||
const size_t framesToRead = mathMin(
|
||||
mathMin(framesRemaining, (size_t) AUDIO_LINUX_WINDOW_FRAMES), positionBudget
|
||||
);
|
||||
|
||||
if(framesToRead == 0) {
|
||||
errorOk();
|
||||
}
|
||||
@@ -142,6 +180,7 @@ errorret_t audioStreamLinuxFeed(audiostream_t *stream) {
|
||||
}
|
||||
|
||||
stream->platform.position += framesRead;
|
||||
|
||||
errorOk();
|
||||
}
|
||||
|
||||
|
||||
@@ -24,6 +24,13 @@ typedef struct {
|
||||
// the true end of the clip if not looping) - recomputed by
|
||||
// audioStreamLinuxBuffer() at the start of each pass.
|
||||
size_t endFrame;
|
||||
|
||||
// SDL_GetTicks() value and starting frame position when the current
|
||||
// pass started - together, what audioStreamLinuxFeed() paces its own
|
||||
// feeding rate against (see its own comment on why it doesn't trust
|
||||
// SDL_GetQueuedAudioSize() alone).
|
||||
Uint32 passStartTicks;
|
||||
size_t passStartPosition;
|
||||
} audiostreamlinux_t;
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user