lukexor / tetanes

A cross-platform NES emulator written in Rust using wgpu
https://lukeworks.tech/tetanes-part-1
Apache License 2.0
166 stars 18 forks source link

Pal audio stream not downsampled correctly #218

Closed tedsteen closed 6 months ago

tedsteen commented 6 months ago

When running pal the audio sample rate seems to be a bit too low. Ntsc is correct afaik, Dendy I haven't tried.

Note I am using the engine rework branch

lukexor commented 6 months ago

Do you happen to know any more details? The audio sample rate is set to 44.1khz and the only thing that affects the audio when switching regions is the clock rate - as the APU in PAL clocks at 50hz instead of 60hz and has a different frame counter. So in general audio sounds slower on PAL than NTSC.

I can't see anything specifically wrong with it comparing to Mesen and playing a PAL game like Aladdin and manually switching between PAL and NTSC you can notice the speed change, where the slower PAL speed sounds more accurate.

If you have a specific game or sampling rate expectation for comparison let me know.

tedsteen commented 6 months ago

You can ignore this, more info in the next comment

F.ex when running this https://wowroms.com/en/roms/nintendo-entertainment-system/download-super-mario-bros.-3/23768.html

I start audio with

AudioSpec { freq: 44100, format: F32LSB, channels: 1, silence: 0, samples: 733, size: 2932 }

and I start tetanes with

Config { dir: "/Users/tedsteen/Library/Application Support/Darkbits.NES-Bundler-Demo", filter: Pixellate, sample_rate: 44100.0, region: Pal, ram_state: Random, four_player: Disabled, zapper: false, genie_codes: [], save_slot: 1, load_on_start: true, save_on_exit: true, concurrent_dpad: false, channels_enabled: [true, true, true, true, true] }

I clock the emulation using the audio (pushing audio as fast as possible to a bounded queue which the audio thread is consuming) This results in a slightly pitched up tune and a framerate of ~55.

The same result if I load the ROM in tetanes native emulator from engine-rework

tedsteen commented 6 months ago

Sorry, I just pulled down the latest changes and adapted to the new API and now it works as expected! I didn't take the clock rate into account when changing the sample rate. Now I do and it behaves correct.

Before I did this:

pub fn start_rom(rom: &[u8]) -> Result<Self> {
        let region = Bundle::current().config.nes_region.to_tetanes_region();
        let config = Config {
            dir: Bundle::current().settings_path.clone(),
            filter: VideoFilter::Pixellate,
            sample_rate: Settings::current().audio.sample_rate as f32,
            region,
            ram_state: RamState::Random,
            four_player: FourPlayer::Disabled,
            zapper: false,
            genie_codes: vec![],
            load_on_start: true,
            save_on_exit: true,
            save_slot: 1,
            concurrent_dpad: false,
            channels_enabled: [true; 5],
        };

        let mut control_deck = ControlDeck::with_config(config);
        let _ =
            control_deck.load_rom(Bundle::current().config.name.clone(), &mut Cursor::new(rom))?;
        control_deck.set_region(region);

        Ok(Self {
            control_deck,
            speed: 1.0,
        })
    }

Now I do this:

    pub fn start_rom(rom: &[u8]) -> Result<Self> {
        let region = Bundle::current().config.nes_region.to_tetanes_region();
        let config = Config {
            filter: VideoFilter::Pixellate,
            region,
            ram_state: RamState::Random,
            four_player: FourPlayer::Disabled,
            zapper: false,
            genie_codes: vec![],
            concurrent_dpad: false,
            channels_enabled: [true; 5],
        };
        log::debug!("Starting ROM with configuration {config:?}");
        let mut control_deck = ControlDeck::with_config(config);

        control_deck.load_rom(Bundle::current().config.name.clone(), &mut Cursor::new(rom))?;

        control_deck.set_region(region);

        let sample_rate = Settings::current().audio.sample_rate as f32;
        let apu = &mut control_deck.cpu_mut().bus.apu;
        apu.filter_chain = FilterChain::new(region, sample_rate);
        apu.sample_period = Cpu::region_clock_rate(region) / sample_rate;

        Ok(Self {
            control_deck,
            speed: 1.0,
        })
    }
}

(It would be nice with a set_sample_rate(...) 🙂)

Still the tetanes native emulator in the engine-rework-branch is suffering from this problem.

lukexor commented 6 months ago

Hm. Glad to see you got it working.

I specifically removed setting a custom sample rate for now unless it's a higher requested feature - The idea being that the APU runs at a native 1.79Mhz (NTSC) or 1.66Mhz (PAL) and ultimately it doesn't matter what your sound card sample rate is as most modern sound cards support 44.1khz and there's little to be gained in quality to changing it. Higher sample rates just cost more CPU.

Internally the APU downsamples to an intermediate sample rate 88200 (2x 44.1khz) for filtering - and then the APU sample period is set to decimate that down to 44.1khz by taking ~every other sample based on the CPUs clock rate. The clock rate is the only source of real "wall time" the APU has access to, so the end sampling rate for both NTSC and PAL should still be ~44.1khz even though the sampling period is different.

I do see the bug there though - The APU set_region method is forgetting to change the apu.sample_period based on the changed clock rate so thanks!

Do you have a strong need for a different output sampling rate other than 44.1khz?

tedsteen commented 6 months ago

So my use case is a bit involved..

I set the nes sample rate to be the same as the audio interface (probably always 44100). I then push audio to a bounded blocking queue with a small length which is being consumed by the audio device thread. Essentially driving the emulator using the audio clock.

If netplay is active there might be a situation where we need to run the emulator a tiny bit slower to let clients catch up. I do this by altering the sample rate.

lukexor commented 6 months ago

I see. Doesn't that result in noticable pitch shifting? I wonder with a high enough audio latency you'd have room to clock more or less and still not underrun audio while keeping the sample rate constant.

Generally modifying the output sampling rate would mean interpolating or decimating the samples, which wouldn't change the clock rate so doing it this way would seemingly affect the pitch.

I previously explored dynamic rate control but had trouble getting it working correctly. I may need to revisit it.

https://bsnes.org/articles/dynamic-rate-control/#:~:text=When%20running%20a%20native%20game,result%20is%20a%20seamless%20experience.

tedsteen commented 6 months ago

I see. Doesn't that result in noticable pitch shifting? I wonder with a high enough audio latency you'd have room to clock more or less and still not underrun audio while keeping the sample rate constant.

When I'm one frame ahead I slow down very little, when two I do a bit more etc. Most of the time clients are in sync, but there is a drift, and sometimes there are hickups and things get out of sync.

Generally modifying the output sampling rate would mean interpolating or decimating the samples, which wouldn't change the clock rate so doing it this way would seemingly affect the pitch.

Yes, it does effect the pitch

I previously explored dynamic rate control but had trouble getting it working correctly. I may need to revisit it.

https://bsnes.org/articles/dynamic-rate-control/#:~:text=When%20running%20a%20native%20game,result%20is%20a%20seamless%20experience.

That would basically do the same no? they also stretch the sound (which effect the pitch) I just do by changing the sample rate and since you already do the downsampling I get two birds in one stone!

You should ofc keep the API the way you want it, if the intention is that 44100 is the default sample rate then it makes total sense.

I would be happy if you kept the back door open tho so I can access the apu.filter_chain and apu.sample_period

lukexor commented 6 months ago

Yeah dynamic rate control does change the pitch but it's usually kept low enough with a min/max that it's not noticeable.

Okay. Yeah the fields will remain public, primarily to allow other developers and TetaNES itself to add debugging features in the future that may need to inspect or modify the internals separately from the standard API for intended behavior

lukexor commented 6 months ago

That's great! Happy you're making good use of the core. No stress for me, I'm trying to wrap up a few features before doing a release also and the core isn't likely to change much but will also get a separate crates version published as well.

After that I'd like to plan for a stabilizing release of core so there may be some breaking changes until then. If you have any thoughts on preferred API changes lmk!

On Sun, Mar 31, 2024, 11:55 Ted Steen @.***> wrote:

Hey, I just wanted to let you know that I plan to merge this one soonish: https://github.com/tedsteen/nes-bundler/pull/109 (today or tomorrow) and then perhaps do a release. It contains a switch over to tetanes in favour of rusticnes-core. I know I am on an active refactoring branch of yours but I like the fact that the core is extracted and I can depend on it as a single crate. I don't persist any state from the nes except the sram.

At least I made a fork with my own branch so I can slowly pull in your changes as they come. I hope I'm not stressing you out too much by doing this :)

Den lör 30 mars 2024 kl 19:41 skrev Luke @.***>:

Yeah dynamic rate control does change the pitch but it's usually kept low enough with a min/max that it's not noticeable.

Okay. Yeah the fields will remain public, primarily to allow other developers and TetaNES itself to add debugging features in the future that may need to inspect or modify the internals separately from the standard API for intended behavior

— Reply to this email directly, view it on GitHub https://github.com/lukexor/tetanes/issues/218#issuecomment-2028360038,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAGERKXSSM5NXDKHOIYPV4LY232NVAVCNFSM6AAAAABFOBGLSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYGM3DAMBTHA>

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/lukexor/tetanes/issues/218#issuecomment-2028874089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNH2PUK3XZA5CPEW7VPYDY3BL2TAVCNFSM6AAAAABFOBGLSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYHA3TIMBYHE . You are receiving this because you modified the open/close state.Message ID: @.***>