Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VGM import doesn't determine start of data correctly #22

Open
OPNA2608 opened this issue Feb 6, 2023 · 2 comments
Open

VGM import doesn't determine start of data correctly #22

OPNA2608 opened this issue Feb 6, 2023 · 2 comments

Comments

@OPNA2608
Copy link

OPNA2608 commented Feb 6, 2023

The VGM data stream is currently hardcoded to be expected at 0x100:

size_t csr = 0x100;
for (bool flag = true; flag;) {
switch (uint8_t com = container.readUint8(csr++)) {

This is a bad assumption. The VGM specification says:
image

So the correct flow for getting the start of the data should be (excluding read checks):

uint32_t version = container.readUint32(0x08);
size_t csr = 0x40;

if (version >= 0x150)
    csr += container.readUint32(0x34);

Some examples of VGM files that don't get read correctly:

YU-NO: Imperial City
VGM version is 0x170, VGM data offset is 0xAC. Real start of VGM data is at 0xE0.
A YM2608 port 0 write (command 0x56) starts at 0xFE, 0x100 has the last argument to that command
-> 0x70, which is being interpreted as a 1-sample wait. All the parsing afterwards is likely very wrong.

Briganty: Coda "BRIGANTY" (Ending)
VGM version is 0x170, VGM data offset is 0xAC. Real start of VGM data is at 0xE0.
A YM2608 port 0 write (command 0x56) starts at 0xFE, 0x100 has the last argument to that command
-> 0x00, which causes the VGM parser to abort due to an unknown command

@rerrahkr
Copy link
Owner

Thank you for your report. VGM Data Offset is the number of bytes from 0x34, so I will fix this as:

diff --git a/src/io/vgm_io.cpp b/src/io/vgm_io.cpp
index 0667a59..980adfd 100644
--- a/src/io/vgm_io.cpp
+++ b/src/io/vgm_io.cpp
@@ -11,7 +11,7 @@ VgmIo::VgmIo() : AbstractSongFileIo("vgm", "VGM file") {}
 
 std::vector<TonePtr> VgmIo::load(BinaryContainer& container) const
 {
-    if (container.size() < 256) throw FileCorruptionError(io::FileType::SongFile);
+	if (container.size() < 0x40) throw FileCorruptionError(io::FileType::SongFile);
 
     std::string ident = container.readString(0, 4);
     if (ident != "Vgm ") throw FileCorruptionError(io::FileType::SongFile);
@@ -20,9 +20,12 @@ std::vector<TonePtr> VgmIo::load(BinaryContainer& container) const
     uint32_t eof = container.readUint32(4);
     if (container.size() - 4 != eof) throw FileCorruptionError(io::FileType::SongFile);
 
+	uint32_t version = container.readUint32(0x08);
+	size_t csr = (version < 0x150) ? 0x40 : (0x34 + container.readUint32(0x34));
+	if (container.size() <= csr) throw FileCorruptionError(io::FileType::SongFile);
+
 	RegisterRecorder rec(44100);
 
-    size_t csr = 0x100;
 	for (bool flag = true; flag;) {
 		switch (uint8_t com = container.readUint8(csr++)) {
 		case 0x52:	// YM2612 Port A

@rerrahkr
Copy link
Owner

I fixed this in ee95aad.

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

No branches or pull requests

2 participants