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
59 stars 12 forks source link

Allow archive format for all single-track file types. #74

Closed myQwil closed 4 months ago

myQwil commented 8 months ago

Rather than having RAR archives reserved specifically for SPC/RSN files, archives are now open to any single-track, or listable, file types. The archive's contents are scanned and an emulator is made for the first listable file type found.

Support has been implemented for RAR and ZIP. Both are optional, with RAR support being enabled with the presence of the UnRAR library, and ZIP support being enabled with the presence of the LibArchive library.

sezero commented 8 months ago

Unrar support removal: Oh yes :))

Play directory: IMHO, such features should be in a player, not library: Library should concern itself with format support / emulation and keep itself simple. (But it's just me and I'm not a maintainer here.)

sezero commented 8 months ago

My old compiler (gcc 4.9) emits the following warnings. Maybe you should use explicit initialization like when it was in library code.

In file included from /tmp/libgme/player/Music_Player.cpp:8:0:
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::ArcName' [-Wmissing-field-initializers]
  RARHeaderData head = {};
                        ^
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileName' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::Flags' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::PackSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::UnpSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::HostOS' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileCRC' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileTime' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::UnpVer' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::Method' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileAttr' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtBuf' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtBufSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtState' [-Wmissing-field-initializers]
In file included from /tmp/libgme/player/Archive_Reader.cpp:1:0:
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::ArcName' [-Wmissing-field-initializers]
  RARHeaderData head = {};
                        ^
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileName' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::Flags' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::PackSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::UnpSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::HostOS' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileCRC' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileTime' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::UnpVer' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::Method' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::FileAttr' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtBuf' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtBufSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.h:49:24: warning: missing initializer for member 'RARHeaderData::CmtState' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp: In member function 'virtual const char* Rar_Reader::open(const char*, bool)':
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::ArcName' [-Wmissing-field-initializers]
  RAROpenArchiveData data = {};
                             ^
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::OpenMode' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::OpenResult' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::CmtBuf' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::CmtBufSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::CmtSize' [-Wmissing-field-initializers]
/tmp/libgme/player/Archive_Reader.cpp:31:29: warning: missing initializer for member 'RAROpenArchiveData::CmtState' [-Wmissing-field-initializers]
sezero commented 8 months ago

Configuration fails if SDL2 is not located:

CMake Warning at player/CMakeLists.txt:3 (find_package):
  By not providing "FindSDL2.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "SDL2", but
  CMake did not find one.

  Could not find a package configuration file provided by "SDL2" with any of
  the following names:

    SDL2Config.cmake
    sdl2-config.cmake

  Add the installation prefix of "SDL2" to CMAKE_PREFIX_PATH or set
  "SDL2_DIR" to a directory containing one of the above files.  If "SDL2"
  provides a separate development package or SDK, be sure it has been
  installed.

-- ** SDL library not found, disabling player demo build
-- UnRAR library located, player demo will support the RAR file format
CMake Error at player/CMakeLists.txt:37 (target_compile_definitions):
  Cannot specify compile definitions for target "gme_player" which is not
  built by this project.

CMake Error at player/CMakeLists.txt:39 (target_compile_definitions):
  Cannot specify compile definitions for target "gme_player" which is not
  built by this project.

CMake Error at player/CMakeLists.txt:43 (target_include_directories):
  Cannot specify include directories for target "gme_player" which is not
  built by this project.

CMake Error at player/CMakeLists.txt:44 (target_link_libraries):
  Cannot specify link libraries for target "gme_player" which is not built by
  this project.

-- Configuring incomplete, errors occurred!

You should move unrar thingies within the SDL2_FOUND block, like the following: (indentation changes not included for clarity)

diff --git a/player/CMakeLists.txt b/player/CMakeLists.txt
index de6eae1..4f894ab 100644
--- a/player/CMakeLists.txt
+++ b/player/CMakeLists.txt
@@ -27,9 +27,6 @@ if(SDL2_FOUND)
     set_property(TARGET gme_player PROPERTY CXX_STANDARD 11)
     target_link_libraries(gme_player PRIVATE ${SDL2_LIBRARIES} gme::gme)
     # Is not to be installed though
-else()
-    message(STATUS "** SDL library not found, disabling player demo build")
-endif()

 if(GME_UNRAR)
   if(UNRAR_FOUND)
@@ -54,3 +51,6 @@ else()
   message(STATUS "RAR file format excluded")
 endif()

+else()
+    message(STATUS "** SDL library not found, disabling player demo build")
+endif()
sezero commented 8 months ago

My old compiler (gcc 4.9) emits the following warnings. Maybe you should use explicit initialization like when it was in library code.

The following makes it warning-free for me

diff --git a/player/Archive_Reader.cpp b/player/Archive_Reader.cpp
index 75e5a31..db888d9 100644
--- a/player/Archive_Reader.cpp
+++ b/player/Archive_Reader.cpp
@@ -28,7 +28,7 @@ blargg_err_t Rar_Reader::restart( RAROpenArchiveData* data )
 blargg_err_t Rar_Reader::open( const char* path, bool skip )
 {
    blargg_err_t err;
-   RAROpenArchiveData data = {};
+   RAROpenArchiveData data = { NULL, RAR_OM_LIST, 0, NULL, 0, 0, 0 };
    data.ArcName = (char *)path;

    // determine space needed for the unpacked size and file count.
diff --git a/player/Archive_Reader.h b/player/Archive_Reader.h
index e385f36..bbc0f01 100644
--- a/player/Archive_Reader.h
+++ b/player/Archive_Reader.h
@@ -46,7 +46,7 @@ public:
 #endif

 class Rar_Reader : public Archive_Reader {
-   RARHeaderData head = {};
+   RARHeaderData head;
    void* rar = nullptr;
    void* bp = nullptr;
    blargg_err_t restart( RAROpenArchiveData* );
myQwil commented 8 months ago

I went with memset but other than that, the same changes were applied.

sezero commented 8 months ago

I went with memset but other than that, the same changes were applied.

OK

sezero commented 8 months ago

I'm OK with this (although I didn't valgrind it)

sezero commented 4 months ago

@Wohlstand : Will this get in ?

Wohlstand commented 4 months ago

Going to merge in this evening. I want to take a deeper look first.

Wohlstand commented 4 months ago

I tested out the thing, it works good, but, it can't open VGZ (GZ-compressed) files loaded via the gme_load_tracks call.

But, this is more problem of the gme_load_tracks() call and not of this thing. This thing works flawless! I going to check the code and I'll merge this soon.

EDIT: Okay, I see the gme_load_tracks() is a new thing here. And ye, if you want to read GZipped files like VGZ, you should decompress the memory block by Mem_File_Reader class which is already used here. I did a small attempt to fix the problem and I almost fixed it, but now I feel sleepy, so, I'll try some tomorrow by myself, but if you fix the ZIPped VGZs faster, I'll just test out that.

Wohlstand commented 4 months ago

Oh wait, I actually had an old copy of the PR state, now I refreshed it, and I see the libarchive support has gone :eyes: Anyway, repacking these files into RAR, the problem is the same: VGM files gets played, but VGZ aren't.

Wohlstand commented 4 months ago

Updated tests: test-rar-archives.zip

myQwil commented 4 months ago

Sorry about that. I probably should have updated my first comment saying that some stuff had been taken out. If you got it to work with VGZ files, feel free to put libarchive support back in.

sezero commented 4 months ago

As far as I can see the purpose of this patch is supporting playback of rsn files (actually moving the existing rsn support from library side to player side), and not playback of vgz files embedded in rar archives. The latter should come later in a new patchset, in my opinion.

mmontag commented 4 weeks ago

Do I understand correctly that building RSN/RAR support is optional? Thank you for doing this.

Wohlstand commented 3 weeks ago

Do I understand correctly that building RSN/RAR support is optional? Thank you for doing this.

Absolutely!