player: add zip implementation and improve rar implementation by myQwil · Pull Request #101 · libgme/game-music-emu

myQwil · 2024-11-02T14:50:36Z

Improvements to rar reader:

better and more specific error handling
add arc_entry_t to reduce the amount of virtual functions
use std::unique_ptr so our reader always cleans up on scope exit

The zip implementation requires libarchive. Zlib is an optional requirement for decompressing individually gzipped files within a zip. The zip reader should still work somewhat without zlib, just not in as many scenarios.

EDIT: One more change was made. Some zips contain one multi-track file like .kss along with several .m3u's, so the reader now accounts for that scenario.

Wohlstand · 2024-11-02T14:55:34Z

I'll try to take a look on this probably tomorrow, this week is pretty busy for me.

sezero · 2024-11-02T16:25:55Z

player/Archive_Reader.cpp: In member function 'const char* Zip_Reader::open_zip(const char*)':
player/Archive_Reader.cpp:108:44: error: 'archive_read_support_filter_all' was not declared in this scope
  if ( archive_read_support_filter_all( zip ) != ARCHIVE_OK
                                            ^
player/Archive_Reader.cpp: In member function 'virtual const char* Zip_Reader::open(const char*)':
player/Archive_Reader.cpp:145:52: error: 'archive_read_free' was not declared in this scope
  if ( res != ARCHIVE_EOF || archive_read_free( zip ) != ARCHIVE_OK )
                                                    ^
player/Archive_Reader.cpp: In member function 'virtual const char* Zip_Reader::next(void*, arc_entry_t*)':
player/Archive_Reader.cpp:157:2: error: 'la_int64_t' was not declared in this scope
  la_int64_t size = archive_entry_size( head );
  ^
player/Archive_Reader.cpp:158:2: error: 'la_ssize_t' was not declared in this scope
  la_ssize_t pos = 0;
  ^
player/Archive_Reader.cpp:160:2: error: 'pos' was not declared in this scope
  pos += archive_read_data( zip, bp, 3 );
  ^
player/Archive_Reader.cpp:166:32: error: invalid use of non-static member function
   if ( (err = buf.resize( size )) )
                                ^
player/Archive_Reader.cpp:168:28: error: 'memcpy' was not declared in this scope
   memcpy( &buf[0], bp, pos );
                            ^
player/Archive_Reader.cpp:171:8: error: invalid use of member function (did you forget the '()' ?)
   size = BLARGG_4CHAR(b[3], b[2], b[1], b[0]);
        ^
player/Archive_Reader.cpp:174:37: error: 'memset' was not declared in this scope
   memset( &stream, 0, sizeof stream );
                                     ^
player/Archive_Reader.cpp:178:20: error: cannot convert 'Archive_Reader::size' from type 'long int (Archive_Reader::)() const' to type 'uInt {aka unsigned int}'
   stream.avail_out = size;
                    ^
player/Archive_Reader.cpp:199:14: error: cannot convert 'Archive_Reader::size' from type 'long int (Archive_Reader::)() const' to type 'size_t {aka unsigned int}'
  entry->size = size;
              ^
player/Archive_Reader.cpp: In destructor 'virtual Zip_Reader::~Zip_Reader()':
player/Archive_Reader.cpp:205:25: error: 'archive_read_free' was not declared in this scope
  archive_read_free( zip );
                         ^

This was with gcc4.9

If I enabled unrar, two was not declared in this scope errors for memcpy and memset did not appear, so you at least need this:

--- a/player/Archive_Reader.cpp
+++ b/player/Archive_Reader.cpp
@@ -3,8 +3,8 @@
 blargg_err_t const arc_eof = "Archive: End of file";
 
-#ifdef RARDLL
-
 #include <string.h>
 
+#ifdef RARDLL
+
 static const int erar_offset = 10;
 static blargg_err_t const erar_handle = "Failed to instantiate RAR handle";

As for rest of the errors, I guess you need some new version of libarchive?? If so, you should specify it in cmake'ry (my libarchive is 2.8.3)

Wohlstand · 2024-11-02T16:35:11Z

If so, you should specify it in cmake'ry (my libarchive is 2.8.3)

Or better, polish the code to ensure it will build with older version too to maintain compatibility with older distros environments (even some of supported LTS distro versions will have a similiar older version).

myQwil · 2024-11-03T00:49:14Z

That should hopefully take care of the unrecognized type and memset/memcpy issues.

But it sounds like there are still some functions being used which either didn't exist before or had their name changed. Also, i'm using libarchive 3.7.7-1.

sezero · 2024-11-03T01:09:24Z

archive_read_free can be replaced with archive_read_finish if ARCHIVE_VERSION_NUMBER < 3000000

Don't know what to do with archive_read_support_filter_all

myQwil · 2024-11-03T02:08:23Z

From what I can tell, a filtering feature hadn't been added yet by that version.

I think we can just leave that function call out. "filter all" seems to be the default behavior anyway, and it still works on my end when I take it out.

sezero · 2024-11-03T02:09:37Z

Don't know what to do with archive_read_support_filter_all

Ah, obviously, archive_read_support_filter_all can be replaced with archive_read_support_compression_all, again if ARCHIVE_VERSION_NUMBER < 3000000

So, the following patch makes it build for me:

diff --git a/player/Archive_Reader.cpp b/player/Archive_Reader.cpp
index 6423d18..a272523 100644
--- a/player/Archive_Reader.cpp
+++ b/player/Archive_Reader.cpp
@@ -102,6 +102,16 @@ static blargg_err_t const zerrs[] = {
 };
 #endif // HAVE_ZLIB_H
 
+#if (ARCHIVE_VERSION_NUMBER < 3000000)
+static inline int archive_read_free(struct archive *a) {
+	return archive_read_finish(a);
+}
+
+static inline int archive_read_support_filter_all(struct archive *a) {
+	return archive_read_support_compression_all(a);
+}
+#endif
+
 blargg_err_t Zip_Reader::open_zip( const char* path ) {
 	if ( !(zip = archive_read_new()) )
 		return zip_err_struct;

sezero · 2024-11-03T02:13:57Z

I think we can just leave that function call out. "filter all" seems to be the default behavior anyway, and it still works on my end when I take it out.

That'd be OK too, I guess.. (not tried though)

myQwil · 2024-11-03T02:14:13Z

Actually, I think you're right. Looks like "filter" was just a name change from "compression"

sezero · 2024-11-03T09:39:04Z

Latest rev builds cleanly for me.

sezero · 2025-02-25T12:42:57Z

player/Audio_Scope.h

-#include "SDL.h"
 #include "gme/gme.h"
-
+#include <SDL_render.h>


Recommended include method for SDL2 headers is quotes instead of brackets: replace <SDL_xxx.h> with "SDL_xxx.h"

sezero · 2025-02-25T12:43:21Z

player/Music_Player.cpp

 #include <string.h>
-#include <ctype.h>
-#include "SDL_rwops.h"
+#include <SDL_rwops.h>


same as above

sezero · 2025-02-25T12:44:26Z

player/Music_Player.cpp

 // Sound output driver using SDL

-#include "SDL.h"
+#include <SDL_audio.h>


same as above

sezero · 2025-02-25T12:48:12Z

player/Archive_Reader.h

+
+// GME_4CHAR('a','b','c','d') = 'abcd' (four character integer constant)
+#define GME_4CHAR( a, b, c, d ) \
+	((a&0xFF)*0x1000000L + (b&0xFF)*0x10000L + (c&0xFF)*0x100L + (d&0xFF))


Now that you replaced long with uint32_t as the sinature type, you should remove L suffixes from the multipliers

(Possibly all long type usage should be replaced with fixed 32 bit types, e.g. int32_t as was suggested in #117, but that's another story...)

i'll see if i can get rid of all the longs in player-related code

i'll see if i can get rid of all the longs in player-related code

It's not limited to player code, but that'd certainly be a start

And that shouldn't be in the way of merging this particular PR, either (Hint: @Wohlstand )

gme/gme.h

sezero · 2025-02-25T18:16:04Z

player/Archive_Reader.cpp

+
+gme_err_t Zip_Reader::next( void* buf_ptr, arc_entry_t* entry )
+{
+	ssize_t res;


ssize_t may not be universally available, e.g. MSVC. (Although the CI MSVC runs seem to have succeeded...)

what about ptrdiff_t?

ptrdiff_t is available to many, yes.

- better and more specific error handling - signatures placed in derived archive classes - add arc_entry_t to make fewer virtual funcs necessary - use std::unique_ptr so it always deletes on scope exit

Requires libarchive. Zlib is an optional requirement for decompressing individually gzipped files within a zip. The zip reader should still work somewhat without zlib, just not in as many scenarios.

changed to size_t for sizes of memory chunks. changed to uint32_t for counts, time durations, and sample rates. also changed ssize_t to the more widely supported ptrdiff_t

myQwil force-pushed the listable-zip branch from 6f8cc75 to 7a1d811 Compare November 2, 2024 14:54

myQwil force-pushed the listable-zip branch from 7a1d811 to ffdfd78 Compare November 2, 2024 15:12

myQwil force-pushed the listable-zip branch from ffdfd78 to 3949a95 Compare November 3, 2024 00:46

myQwil force-pushed the listable-zip branch 2 times, most recently from 8927766 to 1848cff Compare November 3, 2024 01:05

myQwil force-pushed the listable-zip branch from 1848cff to f92365e Compare November 3, 2024 02:09

myQwil force-pushed the listable-zip branch from f92365e to 94b88a8 Compare November 3, 2024 02:19

myQwil force-pushed the listable-zip branch from fa1c043 to eb989d8 Compare February 19, 2025 08:54

sezero mentioned this pull request Feb 25, 2025

demos, player: do not use private headers and apis. #121

Merged

myQwil force-pushed the listable-zip branch from eb989d8 to c82ba04 Compare February 25, 2025 12:34

sezero reviewed Feb 25, 2025

View reviewed changes

myQwil force-pushed the listable-zip branch from c82ba04 to 617b6c9 Compare February 25, 2025 12:59

sezero reviewed Feb 25, 2025

View reviewed changes

gme/gme.h Outdated Show resolved Hide resolved

myQwil force-pushed the listable-zip branch from c59529f to e62c7f3 Compare February 25, 2025 15:35

sezero reviewed Feb 25, 2025

View reviewed changes

myQwil force-pushed the listable-zip branch 2 times, most recently from 5049952 to 0678df5 Compare February 26, 2025 08:00

myQwil added 4 commits March 22, 2025 23:05

player: improve current rar implementation

4358180

- better and more specific error handling - signatures placed in derived archive classes - add arc_entry_t to make fewer virtual funcs necessary - use std::unique_ptr so it always deletes on scope exit

player: add zip implementation

7e1ba2d

Requires libarchive. Zlib is an optional requirement for decompressing individually gzipped files within a zip. The zip reader should still work somewhat without zlib, just not in as many scenarios.

player: allow archives that contain a single multi-track file

967e795

player: change long types to size_t or uint32_t

4b1adee

changed to size_t for sizes of memory chunks. changed to uint32_t for counts, time durations, and sample rates. also changed ssize_t to the more widely supported ptrdiff_t

myQwil force-pushed the listable-zip branch from 0678df5 to 4b1adee Compare March 23, 2025 03:06

Conversation

myQwil commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wohlstand commented Nov 2, 2024

Uh oh!

sezero commented Nov 2, 2024

Uh oh!

Wohlstand commented Nov 2, 2024

Uh oh!

myQwil commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sezero commented Nov 3, 2024

Uh oh!

myQwil commented Nov 3, 2024

Uh oh!

sezero commented Nov 3, 2024

Uh oh!

sezero commented Nov 3, 2024

Uh oh!

myQwil commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sezero commented Nov 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myQwil commented Nov 2, 2024 •

edited

Loading

myQwil commented Nov 3, 2024 •

edited

Loading

myQwil commented Nov 3, 2024 •

edited

Loading