sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)
http://mikmod.sourceforge.net/
69 stars 21 forks source link

Unable to play strange.uni in libmikmod 3.3.11 but was able to play it in libmikmod 3.3.10 #68

Closed Baguettedood closed 1 year ago

Baguettedood commented 1 year ago

strange.uni is a music track included in Onlink (the mod to the hacking game Uplink, but the track doesn't seem to be included in the original game or its bonus CD). I've attached a zip of the specific file, which is hopefully permitted. If not then it could be extracted from Onlink's data.dat yourself. strange.zip

This track used to play on earlier versions of libmikmod but broke in later versions. The strange_loader.mod available on modarchive is still playable in newer versions of libmikmod.

The breaking commit appears to be 3a8d364f30153f3d9777cc3a9c5ae44ab92420b5 and the track worked in the previous commit 1c8dfd0870b6026681d212be3c1de6ccfc61d731

sezero commented 1 year ago

Well, the file hits end-of-file during in the middle of sample reading in SL_LoadInternal() at https://github.com/sezero/mikmod/blob/master/libmikmod/playercode/sloader.c#L297

I don't know whether I can do anything about it. @AliceLR: Can you think of any better solution than the existing one?

sezero commented 1 year ago

FWIW, here are the debug lines I added:

diff --git a/libmikmod/playercode/sloader.c b/libmikmod/playercode/sloader.c
index 4c52ab0..e135403 100644
--- a/libmikmod/playercode/sloader.c
+++ b/libmikmod/playercode/sloader.c
@@ -234,7 +234,7 @@ static int SL_LoadInternal(void *buffer,UWORD infmt,UWORD outfmt,int scalefactor
    SBYTE compressionTable[16];
    SWORD adpcmDelta = 0;
    BOOL hasTable = 0;
-
+fprintf(stderr, "SL_LoadInternal: length==%lu (at %ld)\n",length,reader->Tell(reader));
    status.buf = 0;
    status.last = 0;
    status.bufbits = 0;
@@ -282,7 +282,7 @@ static int SL_LoadInternal(void *buffer,UWORD infmt,UWORD outfmt,int scalefactor
            }
        } else {
            if(infmt&SF_16BITS) {
-               if(_mm_eof(reader)) {
+               if(_mm_eof(reader)) {fprintf(stderr, "SL_LoadInternal(%d): EOF, length==%lu at %ld\n",__LINE__,length,reader->Tell(reader));
                    _mm_errno=MMERR_NOT_A_STREAM;/* better error? */
                    return 1;
                }
@@ -294,7 +294,7 @@ static int SL_LoadInternal(void *buffer,UWORD infmt,UWORD outfmt,int scalefactor
                SBYTE *src;
                SWORD *dest;

-               if(_mm_eof(reader)) {
+               if(_mm_eof(reader)) {fprintf(stderr, "SL_LoadInternal(%d): EOF, length==%lu at %ld\n",__LINE__,length,reader->Tell(reader));
                    _mm_errno=MMERR_NOT_A_STREAM;/* better error? */
                    return 1;
                }

... and it reports:

SL_LoadInternal: length==2476 (at 13001)
SL_LoadInternal: length==1060 (at 15477)
SL_LoadInternal: length==2434 (at 16537)
SL_LoadInternal: length==5346 (at 18971)
SL_LoadInternal: length==3470 (at 24317)
SL_LoadInternal: length==4024 (at 27787)
SL_LoadInternal: length==2764 (at 31811)
SL_LoadInternal: length==800 (at 34575)
SL_LoadInternal: length==8906 (at 35375)
SL_LoadInternal(297): EOF, length==714 at 43375

(Worthy of note is that commit 3a8d364f30153f3d9777cc3a9c5ae44ab92420b5 failed to add the same EOF checks to the SF_ADPCM4 case at line 267.)

sezero commented 1 year ago

@AliceLR: Do you have any possible solutions? Or should I close this as wontfix?

Baguettedood commented 1 year ago

Ultimately it seems like the file only played because two bugs cancelled each other out 😅

The Onlink devs are fixing the file in the next version so I guess it won't really be a problem for much longer though.

Thanks for looking into it.

sezero commented 1 year ago

The Onlink devs are fixing the file in the next version so I guess it won't really be a problem for much longer though.

OK then, closing.

Thanks for looking into it.

You're welcome!

AliceLR commented 1 year ago

Sorry I missed this. IMO failed sample loads for these very old formats shouldn't necessarily cause MikMod to reject the file, but I'm not familiar with Unimod. Failed sample loads are one of the most recoverable issues (just disable the sample).

sezero commented 1 year ago

Hearing strange tones from broken files would be preferred? Well, that's a choice too.

I might mess with it, but not a high priority.. Do you have a patch?