From 619b21425a5c01191bd66aeb523f873ff7cd911b Mon Sep 17 00:00:00 2001 From: krcroft Date: Sat, 29 Feb 2020 07:05:29 -0800 Subject: [PATCH] Handle and fix small-block reads from decoder interfaces Context: The codecs implement a read-write callback function (RWops) used to read N bytes from the underlying binary stream into a buffer. In some cases, a codec might only return a subset of the requested bytes and requires subsequent read() requests to get the remaining bytes. Internally, the codec might have to reposition or run second decode sequence to get the bytes. The RWops callbacks for the various codecs were inconsistently implemented: some performed the above mentioned subsequent re-read attempts while others simply accepted whatever we got after the first read attempt. This commit makes them the same by attempting to re-read ("get the requested bytes at all costs") until the underly stream goes EOF. Some of these RWops functions also contained a book-keeping bug from upstream that resulted in over-reading after under-delivering on the first read attempt. The concequence being that too much data would be written to the buffer (writing past its end) and leaving the underlying stream's file position too-far-forward. --- src/libs/decoders/flac.c | 10 ++--- src/libs/decoders/mp3.cpp | 10 ++--- src/libs/decoders/opus.cpp | 76 ++++++++++++++++++++++---------------- src/libs/decoders/wav.c | 10 ++--- 4 files changed, 60 insertions(+), 46 deletions(-) diff --git a/src/libs/decoders/flac.c b/src/libs/decoders/flac.c index a0df9273..00600897 100644 --- a/src/libs/decoders/flac.c +++ b/src/libs/decoders/flac.c @@ -53,20 +53,20 @@ static size_t flac_read(void* pUserData, void* pBufferOut, size_t bytesToRead) Sound_Sample *sample = (Sound_Sample *) pUserData; Sound_SampleInternal *internal = (Sound_SampleInternal *) sample->opaque; SDL_RWops *rwops = internal->rw; - size_t retval = 0; + size_t bytes_read = 0; - while (retval < bytesToRead) + while (bytes_read < bytesToRead) { - const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead); + const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead - bytes_read); if (rc == 0) { sample->flags |= SOUND_SAMPLEFLAG_EOF; break; } /* if */ - retval += rc; + bytes_read += rc; ptr += rc; } /* while */ - return retval; + return bytes_read; } /* flac_read */ static drflac_bool32 flac_seek(void* pUserData, int offset, drflac_seek_origin origin) diff --git a/src/libs/decoders/mp3.cpp b/src/libs/decoders/mp3.cpp index 90d7ca90..7f8002eb 100644 --- a/src/libs/decoders/mp3.cpp +++ b/src/libs/decoders/mp3.cpp @@ -51,20 +51,20 @@ static size_t mp3_read(void* const pUserData, void* const pBufferOut, const size Sound_Sample* const sample = static_cast(pUserData); const Sound_SampleInternal* const internal = static_cast(sample->opaque); SDL_RWops* rwops = internal->rw; - size_t retval = 0; + size_t bytes_read = 0; - while (retval < bytesToRead) + while (bytes_read < bytesToRead) { - const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead); + const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead - bytes_read); if (rc == 0) { sample->flags |= SOUND_SAMPLEFLAG_EOF; break; } /* if */ - retval += rc; + bytes_read += rc; ptr += rc; } /* while */ - return retval; + return bytes_read; } /* mp3_read */ static drmp3_bool32 mp3_seek(void* const pUserData, const Sint32 offset, const drmp3_seek_origin origin) diff --git a/src/libs/decoders/opus.cpp b/src/libs/decoders/opus.cpp index 6f0cc934..c0d6be39 100644 --- a/src/libs/decoders/opus.cpp +++ b/src/libs/decoders/opus.cpp @@ -64,14 +64,29 @@ static void opus_quit(void) * size_t size --> the size of each object to read, in bytes * size_t maxnum --> the maximum number of objects to be read */ -static Sint32 RWops_opus_read(void* stream, unsigned char* ptr, Sint32 nbytes) +static int RWops_opus_read(void * stream, uint8_t * buffer, int32_t requested_bytes) { - const Sint32 bytes_read = SDL_RWread((SDL_RWops*)stream, - (void*)ptr, - sizeof(unsigned char), - (size_t)nbytes); - SNDDBG(("Opus ops read: " - "{wanted: %d, returned: %ld}\n", nbytes, bytes_read)); + // Guard against invalid inputs and the no-op scenario + assertm(stream && buffer, "OPUS: Inputs are not initialized [bug]"); + if (requested_bytes <= 0) + return 0; + + uint8_t *buf_pos = buffer; + int32_t bytes_read = 0; + Sound_Sample *sample = static_cast(stream); + while (bytes_read < requested_bytes) { + const size_t rc = SDL_RWread(static_cast(stream), + static_cast(buf_pos), + 1, + static_cast(requested_bytes - bytes_read)); + + if (rc == 0) { + sample->flags |= SOUND_SAMPLEFLAG_EOF; + break; + } + buf_pos += rc; + bytes_read += rc; + } /* while */ return bytes_read; } /* RWops_opus_read */ @@ -252,7 +267,7 @@ static int32_t opus_open(Sound_Sample * sample, const char * ext) int64_t pcm_result = op_pcm_total(of, -1); // If positive, holds total PCM samples internal->total_time = pcm_result == OP_EINVAL ? -1 : // total milliseconds in the stream static_cast - (ceil_divide(static_cast(pcm_result), OPUS_SAMPLE_RATE_PER_MS)); + (ceil_divide(static_cast(pcm_result), OPUS_SAMPLE_RATE_PER_MS)); return rcode; } /* opus_open */ @@ -266,39 +281,38 @@ static uint32_t opus_read(Sound_Sample * sample, void * buffer, uint32_t request assertm(sample && buffer, "OPUS: Inputs are not initialized [bug]"); if (requested_frames == 0) return 0u; - Sound_SampleInternal* internal = - reinterpret_cast(sample->opaque); - OggOpusFile* of = - reinterpret_cast(internal->decoder_private); - opus_int16 *sample_buffer = - reinterpret_cast(buffer); + Sound_SampleInternal* internal = static_cast(sample->opaque); + OggOpusFile* of = static_cast(internal->decoder_private); const uint32_t channels = sample->actual.channels; - const uint32_t requested_samples = requested_frames * channels; - uint32_t decoded_samples = 0; - int result = 0; - while(decoded_samples < requested_samples) { - result = op_read( - of, // pointer to the opusfile object - sample_buffer + decoded_samples, // buffer offset to save decoded samples - requested_samples - decoded_samples, // remaining samples to decode - nullptr); // link index, which we don't use - // Check the result code + + // Initial state-tracking variables + uint32_t total_decoded_samples = 0; + opus_int16 *buf_pos = static_cast(buffer); + int32_t remaining_samples = static_cast(requested_frames * channels); + + // Start the decode loop, incrementing as we go + while(remaining_samples > 0) { + const int result = op_read(of, buf_pos, remaining_samples, nullptr); if (result == 0) { sample->flags |= SOUND_SAMPLEFLAG_EOF; break; - } else if (result == OP_HOLE) { // hole in the data - continue; - // sample->flags |= SOUND_SAMPLEFLAG_EAGAIN; - // break; + } else if (result == OP_HOLE) { + continue; // hole in the data; keeping going! } else if (result < 0) { sample->flags |= SOUND_SAMPLEFLAG_ERROR; break; } - // If good, result is number of samples decoded per channel (ie: frames) - decoded_samples += static_cast(result) * channels; + // If good, the result contains the number samples decoded per channel (ie: frames) + const uint32_t decoded_samples = static_cast(result) * channels; + buf_pos += decoded_samples; + remaining_samples -= decoded_samples; + total_decoded_samples += decoded_samples; } - return ceil_divide(decoded_samples, channels); + + // Finally, we return the number of frames decoded + const uint32_t decoded_frames = ceil_divide(total_decoded_samples, channels); + return decoded_frames; } /* opus_read */ /* diff --git a/src/libs/decoders/wav.c b/src/libs/decoders/wav.c index b01974df..857f1ca4 100644 --- a/src/libs/decoders/wav.c +++ b/src/libs/decoders/wav.c @@ -40,19 +40,19 @@ static size_t wav_read(void* pUserData, void* pBufferOut, size_t bytesToRead) Sound_Sample *sample = (Sound_Sample *) pUserData; Sound_SampleInternal *internal = (Sound_SampleInternal *) sample->opaque; SDL_RWops *rwops = internal->rw; - size_t retval = 0; + size_t bytes_read = 0; - while (retval < bytesToRead) { - const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead); + while (bytes_read < bytesToRead) { + const size_t rc = SDL_RWread(rwops, ptr, 1, bytesToRead - bytes_read); if (rc == 0) { sample->flags |= SOUND_SAMPLEFLAG_EOF; break; } /* if */ - retval += rc; + bytes_read += rc; ptr += rc; } /* while */ - return retval; + return bytes_read; } /* wav_read */ static drwav_bool32 wav_seek(void* pUserData, int offset, drwav_seek_origin origin)