search

libgme / game-music-emu

Blargg's video game music emulation library, which allows audio applications to easily add playback support for the music of many classic video game consoles.
GNU Lesser General Public License v2.1
72 stars 14 forks source link

Sms_Apu.cpp: Assert of "end_time >= last_time" fails with one VGM file #32

Closed Wohlstand closed 5 years ago

Wohlstand commented 5 years ago

Original report by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).

I have got a file that makes a crash of the library in the void Sms_Apu::run_until( blip_time_t end_time ) call:

Song didn't began to play: it caused a crash on attempt to start_track().

Wohlstand commented 5 years ago

Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).

Okay, I have researched a problem, and I have found that this VGM has too high PSG clock value ( 3224297472 which is 00 E0 2E C0 in raw bytes). I made an ugly workaround in Vgm_Emu.cpp to avoid the crash:

I think, here it must have a better fix than this ugly crap…

Wohlstand commented 5 years ago

Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).

Do you think it's possible that the most-significant byte (which I'm assuming is C0 here) is meant to be masked to zero by actual hardware? 00 2E E0 00 is equal to 3072000 which is very close indeed to the 3079545 you have here in your workaround.

Wohlstand commented 5 years ago

Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).

diff --git a/gme/Vgm_Emu.cpp b/gme/Vgm_Emu.cpp
index 8f19b7d..33f8633 100644
--- a/gme/Vgm_Emu.cpp
+++ b/gme/Vgm_Emu.cpp
@@ -303,7 +303,7 @@ blargg_err_t Vgm_Emu::load_mem_( byte const* new_data, long new_size )
        check( get_le32( h.version ) <= 0x150 );

        // psg rate
-       psg_rate = get_le32( h.psg_rate );
+       psg_rate = get_le32( h.psg_rate ) & 0x00FFFFFFu;
        if ( !psg_rate )
                psg_rate = 3579545;
        blip_buf.clock_rate( psg_rate );

This patch seems to fix the file you’ve provided without breaking the only other VGM file I’m personally familiar with.

Wohlstand commented 5 years ago

Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).

Then, it’s a question: what greatest byte means here? :thinking:
Anyway, I see the note here: https://vgmrips.net/wiki/VGM_Specification

Bit 31 (0x80000000) is used on combination with the dual-chip-bit to indicate that this is a T6W28. (PSG variant used in Neo Geo Pocket)

So, yeah, these two last bits ast should be cut-off, and we need to use something to give correct T6W28 thing as yeah, this VGM is the song from a Neo Geo Pocket game. So, I guess, a mask would be 0x3FFFFFFFu, but, yeah, keep this 0x00FFFFFFu as it safer and should never lead a crash, otherwise, I’ll try to set `3FFFFFFF` clock value for a crash test, will it happen or not :thinking:

Wohlstand commented 5 years ago

Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).

Ok, 3F makes crash, however, 0F (`0x0FFFFFFF`) doesn't, so, feel free to use 0x0FFFFFFF mask :fox:

Wohlstand commented 5 years ago

Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).

So, this issue: #33/vgm-dual-chips-support and it’s related pull-request does fixes this.

Wohlstand commented 5 years ago

Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).

OK, so I’ll track this together with your PR for dual chips support and close this once we land that.

Wohlstand commented 5 years ago

Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).

Dual chip support has been merged and addresses this issue, closing