Skip to content

Conversation

@sonicdcer
Copy link
Contributor

@sonicdcer sonicdcer commented Jan 1, 2025

This change fixes Vibrato and Portamento issues with the port.

Build Artifacts

Comment on lines 536 to 545
#if 0
// Values for 44100 hz
#define SAMPLES_HIGH 752
#define SAMPLES_LOW 720
#else
// Values for 32000 hz
#define SAMPLES_HIGH 560
#define SAMPLES_LOW 528

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this can we just change

        //AudioMgr_ThreadEntry(&gAudioMgr);
        // 528 and 544 relate to 60 fps at 32 kHz 32000/60 = 533.333..
        // in an ideal world, one third of the calls should use num_samples=544 and two thirds num_samples=528
        //#define SAMPLES_HIGH 560
        //#define SAMPLES_LOW 528
        // PAL values
        //#define SAMPLES_HIGH 656
        //#define SAMPLES_LOW 624

        // 44KHZ values
        #if 0
        // Values for 44100 hz
        #define SAMPLES_HIGH 752
        #define SAMPLES_LOW 720
        #else
        // Values for 32000 hz
        #define SAMPLES_HIGH 560
        #define SAMPLES_LOW 528

        #endif

to

        //AudioMgr_ThreadEntry(&gAudioMgr);
        // 528 and 544 relate to 60 fps at 32 kHz 32000/60 = 533.333..
        // in an ideal world, one third of the calls should use num_samples=544 and two thirds num_samples=528
        #define SAMPLES_HIGH 560
        #define SAMPLES_LOW 528

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you willing to lose values for 44100? You don't rather have this for quick change and testing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the experience we've had so far with 44k, I doubt we'll go back, but if we do, we can look for this PR to see the differences.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with @Malkierian. if someone wants to try to get 44.1k working again they can check the git blame and find them

overlay->SetCurrentFont(CVarGetString(CVAR_GAME_OVERLAY_FONT, "Press Start 2P"));

context->InitAudio({ .SampleRate = 44100, .SampleLength = 1024, .DesiredBuffered = 2480 });
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 2480 });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like DesiredBuffered was changed for 44.1k here

		int SDLAudioPlayer::GetDesiredBuffered(void) {
-            return 1680;
+            return 2480;
		}

interestingly, it seems SampleLength was already 1024 before the switch to 44.1k

i also am not sure where these values are coming from:
32000/1680 = 19.047619048
44100/2480 = 17.782258065

@Kenix3 @NEstelami any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find that. Where is it?

Copy link
Contributor

@briaguya0 briaguya0 Jan 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int SDLAudioPlayer::GetDesiredBuffered(void) {
return 1680;
}

vs

int SDLAudioPlayer::GetDesiredBuffered(void) {
return 2480;
}

and

int WasapiAudioPlayer::GetDesiredBuffered(void) {
return 1680;
}

vs

int WasapiAudioPlayer::GetDesiredBuffered(void) {
return 2480;
}

Copy link
Contributor

@briaguya0 briaguya0 Jan 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sample length, it's used here in LUS (but i don't see it being used in wasapi)

but it was hardcoded to 1024 at the beginning of the repo being public

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, did some digging and figured out stuff about the value for samples

it looks like in SDL3 samples has been removed from AudioSpec
https://wiki.libsdl.org/SDL3/README/migration#sdl_audioh
https://wiki.libsdl.org/SDL2/SDL_AudioSpec
https://wiki.libsdl.org/SDL3/SDL_AudioSpec

from the migration guide:

SDL_AudioSpec has been reduced; now it only holds format, channel, and sample rate. SDL_GetSilenceValueForFormat() can provide the information from the SDL_AudioSpec's removed silence field. SDL3 now manages the removed samples field; apps that want more control over device latency and throughput can force a newly-opened device's sample count with the SDL_HINT_AUDIO_DEVICE_SAMPLE_FRAMES hint, but most apps should not risk messing with the defaults. The other SDL2 SDL_AudioSpec fields aren't relevant anymore.

sdl3 logic for getting the value is here https://github.com/libsdl-org/SDL/blob/7c9f6c631304829cb5d6fc68d873dace47c17152/src/audio/SDL_audio.c#L147-L166

    if (freq <= 22050) {
        return 512;
    } else if (freq <= 48000) {
        return 1024;
    } else if (freq <= 96000) {
        return 2048;
    } else {
        return 4096;
    }

so based on what SDL is doing it seems 1024 is a reasonable value for both 32k and 44.1k

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 2480 });
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 1680 });

see

https://github.com/HarbourMasters/Shipwright/pull/4780/files?diff=unified&w=0#r1900436401
https://github.com/HarbourMasters/Shipwright/pull/4780/files?diff=unified&w=0#r1900475059

overlay->SetCurrentFont(CVarGetString(CVAR_GAME_OVERLAY_FONT, "Press Start 2P"));

context->InitAudio({ .SampleRate = 44100, .SampleLength = 1024, .DesiredBuffered = 2480 });
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 2480 });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 2480 });
context->InitAudio({ .SampleRate = 32000, .SampleLength = 1024, .DesiredBuffered = 1680 });

see

https://github.com/HarbourMasters/Shipwright/pull/4780/files?diff=unified&w=0#r1900436401
https://github.com/HarbourMasters/Shipwright/pull/4780/files?diff=unified&w=0#r1900475059

@Archez Archez merged commit 9455579 into HarbourMasters:develop Jan 8, 2025
5 checks passed
Malkierian added a commit to Malkierian/Shipwright that referenced this pull request Mar 28, 2025
aMannus pushed a commit that referenced this pull request Mar 28, 2025
Malkierian added a commit to Malkierian/Shipwright that referenced this pull request May 17, 2025
…asters#5234)

This reverts commit feea299.

Also applies rupee screech fix LL provided to 2ship.
Malkierian added a commit that referenced this pull request Jun 19, 2025
This reverts commit feea299.

Also applies rupee screech fix LL provided to 2ship.
krazyjakee pushed a commit to krazyjakee/OOT that referenced this pull request Sep 6, 2025
…asters#5234) (HarbourMasters#5508)

This reverts commit feea299.

Also applies rupee screech fix LL provided to 2ship.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants