Remove N64 RSP DMA alignment constraint from ADPCM/S8 sample loading - #6364
Conversation
|
@fabienr Can you test this on OpenBSD? |
ecd8edc to
aed0321
Compare
It's better as it didn't crash early but later when playing. I didn't try to understand why exactly, there are a few contexts on the crash :
There is a lot of round stuff in the mixer so I'm curious why there are all those constraints on memory alignment in the RSPA at start, do you know ? If that's something we can remove then it will be easier to think about the calculation but (after spending some time there) I think the math is correct and the ADPCM decode loop isn't the reason why the sound is bad on openbsd. Just it needs aligned memory per design otherwise it crash either with read underflow (landingpad, not sure in which mem layout this trigger but a larger buffer isn't enough isn't enough to fix it) or read overflow (round to 16). I fear removing alignment implies rewriting a lot of stuff. The landing pad is also about alignment to copy data in chunks of 16 bytes, aligned to 16 bytes in memory. If the buffer is strictly aligned (from allocation), then it doesn't crash anymore. On top of this memory issue, I also have a very poor quality sound. With this audio record, where do you think I should dig ? I will have limited spare time until mid-april, still any advice is welcome. |
753d51e to
fd274d3
Compare
|
Your initial comment mentions using the actual padding as part of the offset calculation, but from what I can see, |
koplas
changed the title
I force pushed a new commit and forgot to update the pull request description. I am now testing a different approach. @fabienr Can you check if it now works on OpenBSD. If not I need to setup a OpenBSD machine for proper testing. I converted this to a draft until it is confirmed, that it works on OpenBSD. |
Sadly it still crash. If you plan to test on openbsd I'm using this malloc options (more strict) : I have done two tests, on the second one I regen oot.o2r (rm & restart) and remove the HD pack mod.
For example the first crash :
It tries to copy nbytes=54 but the buffer left is 46 (3664 (audioFontSample->size) - 3618 (sampleData - audioFontSample->sampleAddr)). It overflow by 8. In the second crash I got size=3358, offset=3339, nbytes=27, left 19, overflow 8. In the first example, 3664 isn't aligned to codec framesize (9) which mean it have 407.11.. frames which looks strange. I regen and the second crash is on 3358 (/9=373.11..). It looks like the buffser still sized correctly, loopEnd = 6513 / 16 (frames) * 9 (bytes to decode) = 3663.5625. Do you think both size and loopend pos are truncated, are they based from the same initial number ? If math are correct in audio_synthesis the decoding loop isn't designed to stop on a single byte and will always read past the buffer either the one from mem or from dmem buffer. From dmem it will not crash, just the copy from mem to dmem have to copy bytes left and not frames(*9) left. On linux, maybe you could add an overflow check, dump a core, and check if you have the same truncation I have on size/endpos ? |
f98993b to
e66d11c
Compare
The sampleDataStartPad and aligned variables existed solely to satisfy the N64 RSP DMA requirement that source addresses be 16-byte aligned. On PC, aLoadBuffer is a plain memcpy with no such constraint. The alignment dance caused aLoadBuffer to read up to 15 bytes before sampleData and up to 8+ bytes past the end of the sample buffer. On platforms with strict allocator guard pages (e.g. OpenBSD), this triggers a SIGSEGV. A second issue remains after removing the alignment dance: nFramesToDecode is derived from sample counts (loopEnd), but size is not always a multiple of frameSize. loopEnd and size are derived independently during encoding and can disagree on the final partial frame, leaving nFramesToDecode * frameSize exceeding the remaining bytes in the buffer. Remove sampleDataStartPad and aligned entirely. Clamp the load to min(nFramesToDecode * frameSize, audioFontSample->size - sampleDataOffset). The ADPCM decoder operates on DMEM, so a partial last frame in DMEM produces at most a negligible artifact at sound termination.
e66d11c to
7e7e683
Compare
|
I added logging to easier identify the issue. Here is the output when running the opening scene on macOS: |
Same here on OpenBSD, no crash anymore. But on macOS the sound is smooth and pleasant ? Is it a truncation from another bug and maybe this is the reason the sound isn't correct on OpenBSD or is it normal to decode one byte of ADPCM instead of a full frame ? |
|
On macOS the audio is normal for me. Can you apply the following patch and paste the log output here: diff --git a/include/ship/audio/SDLAudioPlayer.h b/include/ship/audio/SDLAudioPlayer.h
index 8d9a08e..e654418 100644
--- a/include/ship/audio/SDLAudioPlayer.h
+++ b/include/ship/audio/SDLAudioPlayer.h
@@ -1,6 +1,7 @@
#pragma once
#include "AudioPlayer.h"
#include <SDL2/SDL.h>
+#include <chrono>
namespace Ship {
class SDLAudioPlayer final : public AudioPlayer {
@@ -19,5 +20,9 @@ class SDLAudioPlayer final : public AudioPlayer {
private:
SDL_AudioDeviceID mDevice = 0;
int32_t mNumChannels = 2;
+ // Diagnostics: track push timing and sample accounting independent of SDL queue size
+ std::chrono::steady_clock::time_point mLastPushTime = {};
+ int mSamplesQueued = 0;
+ uint64_t mFrameCount = 0;
};
} // namespace Ship
diff --git a/src/ship/audio/SDLAudioPlayer.cpp b/src/ship/audio/SDLAudioPlayer.cpp
index d9f1e44..f1158e4 100644
--- a/src/ship/audio/SDLAudioPlayer.cpp
+++ b/src/ship/audio/SDLAudioPlayer.cpp
@@ -43,7 +43,13 @@ bool SDLAudioPlayer::DoInit() {
return false;
}
- SPDLOG_INFO("SDL Audio initialized: {} channels, {} Hz", mNumChannels, this->GetSampleRate());
+ SPDLOG_INFO("SDL Audio initialized: driver={} channels={} freq={} samples={} (wanted {})",
+ SDL_GetCurrentAudioDriver(), (int)have.channels, have.freq, (int)have.samples,
+ (int)want.samples);
+
+ mLastPushTime = std::chrono::steady_clock::now();
+ mSamplesQueued = 0;
+ mFrameCount = 0;
SDL_PauseAudioDevice(mDevice, 0);
return true;
@@ -54,9 +60,39 @@ int SDLAudioPlayer::Buffered() {
}
void SDLAudioPlayer::DoPlay(const uint8_t* buf, size_t len) {
- if (Buffered() < 6000) {
- // Don't fill the audio buffer too much in case this happens
- SDL_QueueAudio(mDevice, buf, len);
+ auto now = std::chrono::steady_clock::now();
+ int sdlBuffered = Buffered();
+ int newSamples = (int)(len / (sizeof(int16_t) * mNumChannels));
+
+ // Track elapsed time since last push to estimate true playback position
+ auto elapsedUs =
+ std::chrono::duration_cast<std::chrono::microseconds>(now - mLastPushTime).count();
+ int drainedSinceLastPush = (int)((elapsedUs / 1e6) * GetSampleRate());
+ mSamplesQueued -= drainedSinceLastPush;
+ if (mSamplesQueued < 0) {
+ mSamplesQueued = 0;
}
+ mLastPushTime = now;
+
+ // Log every 60 frames to identify queue accounting discrepancy without spamming
+ if (mFrameCount % 60 == 0) {
+ SPDLOG_DEBUG(
+ "[SDL audio] frame={} sdlBuffered={} trackedQueued={} elapsedUs={} drainedEst={} desiredBuffered={}",
+ mFrameCount, sdlBuffered, mSamplesQueued, elapsedUs, drainedSinceLastPush,
+ GetDesiredBuffered());
+ }
+
+ if (sdlBuffered < 6000) {
+ if (SDL_QueueAudio(mDevice, buf, len) != 0) {
+ SPDLOG_WARN("[SDL audio] SDL_QueueAudio failed: {}", SDL_GetError());
+ } else {
+ mSamplesQueued += newSamples;
+ }
+ } else {
+ SPDLOG_DEBUG("[SDL audio] frame={} DROPPED: sdlBuffered={} >= 6000", mFrameCount,
+ sdlBuffered);
+ }
+
+ mFrameCount++;
}
} // namespace ShipDoes the game run smoothly? Can you set the frame limit to 60 fps and check if it always hits the target. If not this seems to be more a performance issue, as the frame-lock between audio and rendering can cause stutter issues. You can use this patch to verify that the audio thread is too slow: diff --git a/soh/soh/OTRGlobals.cpp b/soh/soh/OTRGlobals.cpp
index d1b99cb3f..ff62250a3 100644
--- a/soh/soh/OTRGlobals.cpp
+++ b/soh/soh/OTRGlobals.cpp
@@ -1035,6 +1035,9 @@ void OTRAudio_Thread() {
// 3 is the maximum authentic frame divisor.
s16 audio_buffer[SAMPLES_HIGH * NUM_AUDIO_CHANNELS * 3];
+
+ auto t0 = std::chrono::steady_clock::now();
+
for (int i = 0; i < AUDIO_FRAMES_PER_UPDATE; i++) {
AudioMgr_CreateNextAudioBuffer(audio_buffer + i * (num_audio_samples * NUM_AUDIO_CHANNELS),
num_audio_samples);
@@ -1043,6 +1046,12 @@ void OTRAudio_Thread() {
AudioPlayer_Play((u8*)audio_buffer,
num_audio_samples * (sizeof(int16_t) * NUM_AUDIO_CHANNELS * AUDIO_FRAMES_PER_UPDATE));
+ auto t1 = std::chrono::steady_clock::now();
+ auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count();
+ if (ms > 20) {
+ fprintf(stderr, "[audio] OTRAudio_Thread: slow iteration %lldms\n", (long long)ms);
+ }
+
audio.processing = false;
audio.cv_from_thread.notify_one(); |
I notice a deviation from the FPS rate intended. Setting to 20, the minimum, was running at 16.7 then 25 give 20 ... so it continuously deviates while fps rate can increase up to 40 in my actual setup. And for sure continuous underrun on the SDL audio buffer was visible with your debugging but no audio slow iteration. Now that I know the audio rate is linked to framerate ... Thanks so much for the debugging tips :) I don't know a lot about game engine loop in general, but I notice something in gfx_sdl2.cpp ... Same as APPLE, 1ms wasn't enough. I chose 10ms based on : I'm not sure this is the proper fix but seeing APPLE doing something similar makes me think that's ok to do the same on OpenBSD (?). Then we are left with the overread issue. I'm not sure what's the best solution, either aligned memory or remove alignment, but then we need to check bytes left during copy. Still, this looks weird to decode a single byte. Btw we don't care about the garbage because the synthesis will start from a fresh state (loop) and will not consume the other bytes decoded by adpcm in the synthstate ... I think so. |
|
https://man.openbsd.org/OpenBSD-6.8/sysctl.2 we might be able to lookup tick on BSDs @fabienr can you try Kenix3/libultraship#1023 ? wrote just going off docs |
I want to contain this pull request to fix the undefined behavior and crash in the audio decoder. I guess on original hardware it would read parts of the next sample or simply null bytes. But I doubt the difference can be heard. Good to hear that you found a fix for the issue. I would not mind if you also port this overread fix to the other projects. If I am on my computer again I will delete the last commit and mark this pull request as ready. |
7e7e683 to
d69b0f7
Compare
|
I assume, then, this fixes what is intended now and is ready for merge scrutiny? |
Yes, it fixes an issue I tried fixing earlier and I don't want to extend this pull request to contain other fixes. |
4 participants
The sampleDataStartPad and aligned variables existed solely to satisfy
the N64 RSP DMA requirement that source addresses be 16-byte aligned.
On PC, aLoadBuffer is a plain memcpy with no such constraint.
The alignment dance caused aLoadBuffer to read up to 15 bytes before
sampleData and up to 8+ bytes past the end of the sample buffer. On
platforms with strict allocator guard pages (e.g. OpenBSD), this
triggers a SIGSEGV.
A second issue remains after removing the alignment dance: nFramesToDecode
is derived from sample counts (loopEnd), but size is not always a multiple
of frameSize. loopEnd and size are derived independently during encoding
and can disagree on the final partial frame, leaving nFramesToDecode *
frameSize exceeding the remaining bytes in the buffer.
Remove sampleDataStartPad and aligned entirely. Clamp the load to
min(nFramesToDecode * frameSize, audioFontSample->size - sampleDataOffset).
The ADPCM decoder operates on DMEM, so a partial last frame in DMEM
produces at most a negligible artifact at sound termination.
Build Artifacts