[Fixed] [SDL Mixer X] AIFF files

Description: It's archive, posting new is here http://wohlsoft.ru/forum/viewforum.php?f=11

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #1by StefG » 10 Jan 2019, 18:45

Hello again,

In the official SDL_mixer, the code for AIFF files in LoadMUS is awful. I doubt anyone even tried to play an AIFF file with it.

Yours is much better. But still a few problems are present, or they are kind of corrected but not really for the good reason.

(I will not always annoy you so much as I do now, it is just because I have spent the last 2 weeks on this part of SDL_mixer code, so I've learned a couple of things and they are still fresh in memory.)




If I take the current music_wav.c, around lines 1023-1025 :

Code: Select all

        /* Paranoia to avoid infinite loops */
        if (chunk_type == 0 && chunk_length == 0)
            break;


OK, this is a slightly improved version coming from the official SDL_mixer which would exit when it encountered an empty chunk, which was a very wrong behaviour.

But the thing is that this "paranoia" piece of code is not needed at all in neither codebase.

Why? Because just before it, we always perform:

Code: Select all

        chunk_type      = SDL_ReadLE32(src);
        chunk_length    = SDL_ReadBE32(src);


And that means that we always advance by at least 2x4=8 bytes. An infinite stationary loop is not possible. (But at the end of the message we will see that it was still somehow useful).




Now let's skip to the bottom of the loop:

Code: Select all

        default:
            /* For cases of inter-chunk junk bytes, deep scan is needed. For example,
             * some AIFF files are have zero byte that follows end of NAME chunk. */
            deep_scan = SDL_TRUE;
            break;
        }

        if (deep_scan) {
            /* Scan at every next byte for a well known chunk */
            next_chunk = (++prev_chunk);
        } else {
            /* Jump to end of well known chunk */
            prev_chunk = next_chunk;
        }
 


Well, that kinda does the job, but not exactly for the right right reason, ans sometimes it won't do the job.

Code: Select all

            /* For cases of inter-chunk junk bytes, deep scan is needed. For example,
             * some AIFF files are have zero byte that follows end of NAME chunk. */


That's not really what happens. Let's look at AIFF specification http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/AIFF/Docs/AIFF-1.3.pdf .

On page 4, it says "If the data is and odd number of bytes in length, a zero pad byte must be added at the end. the pad byte is not included in ckSize."

Almost all types of chunks are aligned on even lengths, the only ones which are not are the 4 text chunks: Name, Author, Copyright, Annotation. That's because ckSize represents the exact length of the text in those cases.




Then there is another problem, with your deep_scan strategy. It seems to work, but it can fail. Once it is triggered, it will scan every single byte for a chunk name.

Mix that behaviour with the fact that deep_scan is triggered by unknown chunks. That means that you are going to look for chunk types markers inside the content of an unknown chunk. And that's bad :)

I have attached a small ZIP file with 2 AIFF files: one will play OK, the second should but won't. That's because I placed the letters "SSND" inside a text comment (an ANNO chunk). ANNO is considered unknown in your code, so it triggers deep_scan, then it its the text string "SSND" and believe it is the start of a chunk, so it read the following bytes as a chunk size, and then, well, it can only go wrong :biggrin:

The fix is not to add ANNO to the supported chunk. To be robust with any future unsupported type of chunk (like the ID3 didn't exist in the specification), you just need to read their length and skip them.




So the fix for the previous 2 problems is in fact super simple : when chunk_length is odd, just add 1 to it or to next_chunk if you like it better.

Thus the beginning of the loop becomes simply:

Code: Select all

    do {
        chunk_type      = SDL_ReadLE32(src);
        chunk_length    = SDL_ReadBE32(src);
        next_chunk      = SDL_RWtell(src) + chunk_length;

   if(chunk_length % 2) next_chunk++;

        switch (chunk_type) {


and the end is even simpler:

Code: Select all

        default:
            /* Unknown/unsupported chunk: we just skip over*/
            break;
        }

    } while ((!found_SSND || !found_COMM || !found_ID3 || (is_AIFC && !found_FVER))
         && SDL_RWseek(src, next_chunk, RW_SEEK_SET) != -1);


Now we have a new problem, SDL_RWseek() never returns -1, because the underlying fseek() always returns 0. That's quite a surprise for me, but you can fseek() to any offset and it is considered an error. :( Even worse: ftell() will report the fantasy offset you have "seeked" to. :crazy:

So we never get out of the loop. We used to get out of the loop via the "paranoia" bit (with the ReadBE32 functions happening to return both 0 for undocumented reasons).

So we can either bring back the "paranoia" snippet (but that means relying on not-so-great assumptions), or do something like this:

in the beginning of the function :

Code: Select all

    SInt64 file_length;
    /* ... */
    file_length = SDL_RWsize(src);


and for the loop condition:

Code: Select all

    } while ((!found_SSND || !found_COMM || !found_ID3 || (is_AIFC && !found_FVER))
         && next_chunk<file_length && SDL_RWseek(src, next_chunk, RW_SEEK_SET) != -1);



Then I am not sure of the interest of the first part of the loop condition. I mean, in original SDL_mixer code, it was understandable, they just supported one single case: a file with one COMM and one SSND, so they exited as soon as they got one of each.

But now... to get the first part FALSE, you need to have found_SSND and found_COMM and found_ID3 and the AIFC/FVER condition at the same time. So it stays TRUE for most files all the time. And in the rare case it becomes FALSE, the benefit of exiting before the end of file is not great.

I think the condition could be simplified to:

Code: Select all

    } while (next_chunk<file_length && SDL_RWseek(src, next_chunk, RW_SEEK_SET) != -1);


And then we can perform the wanted checks after the loop, if needed as it is already done there:

Code: Select all

    if (!found_SSND) {
        Mix_SetError("Bad AIFF file (no SSND chunk)");
        return SDL_FALSE;
    }

    if (!found_COMM) {
        Mix_SetError("Bad AIFF file (no COMM chunk)");
        return SDL_FALSE;
    }


It think it would be safer than this big not-very-useful condition.

I guess the only required check we have to add would be:

Code: Select all

    if (!found_SSND) {
        Mix_SetError("Bad AIFF/AIFF-C file (no SSND chunk)");
        return SDL_FALSE;
    }

    if (!found_COMM) {
        Mix_SetError("Bad AIFF/AIFF-C file (no COMM chunk)");
        return SDL_FALSE;
    }
   
    if(is_AIFC && !found_FVER) {
        Mix_SetError("Bad AIFF-C file (no FVER chunk)");
        return SDL_FALSE;
    }





OK, now that's a lot of long remarks on this topic, I'll let you take your time to reflect on it :music: there is zero hurry, you do what you want with it (including nothing). You can also ask me, or tell me where you think I am wrong (I am no expert on the subject, so it is very possible I made mistakes in the reasoning or the implementation).

I also also attached the whole set of modifications off music_wav.c as they are on my system at the moment in a zipped patch.
Attachments
music_wav.c.svn_diff_git__AIFF.patch.zip
(1.21 KiB) Downloaded 200 times
AIFF_test_files_chunktype_in_unknown_chunk.zip
(41.91 KiB) Downloaded 172 times

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 31
Reputation: 507
Posts: 1825
Joined: 15 Feb 2014
English Pronouns: he/him
Location: Moscow, Russia
Website Youtube channel URL Skype Tencent QQ

Post #2by Wohlstand » 11 Jan 2019, 3:19

The reason why I added the `deep_scan` thing are some weird AIFF files are invalid, but, VLC plays them... If I have kept them, I'll try to share them to you for analyze ;-)

I gonna to review this stuff more accurate, for now I'm tired (at me is 3:01 AM, UTC+3) and I gonna to sleep. I have quickly checked out your article and still not completely "baked" the sense in my mind yet... So, Good day/night ;-)

Added after 12 minutes 1 second:
I made the issue as the note: https://github.com/WohlSoft/SDL-Mixer-X/issues/27
Image

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #3by StefG » 11 Jan 2019, 13:42

Wohlstand wrote:The reason why I added the `deep_scan` thing are some weird AIFF files are invalid, but, VLC plays them... If I have kept them, I'll try to share them to you for analyze ;-)

Yes, please.


Wohlstand wrote:I gonna to review this stuff more accurate, for now I'm tired (at me is 3:01 AM, UTC+3) and I gonna to sleep. I have quickly checked out your article and still not completely "baked" the sense in my mind yet... So, Good day/night ;-)

Take your time, it doesn't matter if you answer after 5 days instead of 5 hours. :)

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 31
Reputation: 507
Posts: 1825
Joined: 15 Feb 2014
English Pronouns: he/him
Location: Moscow, Russia
Website Youtube channel URL Skype Tencent QQ

Post #4by Wohlstand » 14 Jan 2019, 3:29

Just now I have tested the patch and I have took it!
https://github.com/WohlSoft/SDL-Mixer-X/commit/fd ... f604dcce35f4b83ff2c7cfca8f2caa

Also, I have fixed some other bugs like inability to play OPUS inside of OGG files. ;-)
Image

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #5by StefG » 14 Jan 2019, 18:12

:cool: I hope we didn't break anything :angel:




At the moment, I am working on improving seeking and skipping through audio files. The original SDL function Mix_MusicSetPosition() is an abomination of an API: sometimes it performs absolute seeking, sometimes relative skipping, sometimes it is about time, sometimes about blocks/patterns, sometimes it does nothing; so I would like to have extra clear functions like Mix_SeekMusicStreamSeconds() (going to an absolute time position) and Mix_SkipMusicStreamSeconds() (fast forwarding/backwarding relatively to the current time position) which do only one clearly defined thing (and possibly another set of separate clearly defined functions for those who really want to seek/skip patterns).

Also, since we implemented the song duration calculation (I mean, you implemented it in SDL Mixer X, but that's the one thing I had done on my side too before I knew your library :) ), for some music codecs that means that we have to parse the full file beforehand anyway in order to calculate this duration, so we can take advantage of this action to gather at the same time information to make seeking/skipping more efficient later (I think of the MAD mp3 codec, which which I started).

Anyway, this is not at all 'showable' yet and I will open another thread when it is showable (if it is, one day...) to discuss it.

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 31
Reputation: 507
Posts: 1825
Joined: 15 Feb 2014
English Pronouns: he/him
Location: Moscow, Russia
Website Youtube channel URL Skype Tencent QQ

Post #6by Wohlstand » 14 Jan 2019, 19:00

About `Mix_MusicSetPosition`, in the fact it's always used for absolute position and nothing relative, and always in seconds unit. Even on the side of official SDL Mixer this function works for absolute seconds position. Maybe, in the past it had some crap, but on the moment when I have picked it up initially, it had the sense "absolute seconds position". MAD MP3 hadn't seekability originally, I have implemented my own seeker which does two algorithms in dependence from seek the direction (when position is going before, drop position into begin and seek frames forward; but when it going after, just skip some frames) Yeah, MP3 is a mess to seek, but it's possible.

MPG123 has own built-in seeker which is fast and good, yeah. We can just make something similar to that. Anyway, about MPG123, I need to provide the full CMake build of MPG123 at AudioCodecs, then, I would prefer MPG123 than MAD due better support (the libMAD is ancient and last update was in 2004'th year, but with exception of few minor security pathches are was applied by community). However, the proc of libMAD is simpleness, compactness, and the best decoding quality as I know.

Anyway, let's begin new thread to discuss future things, as the topic of this (AIFF related) was solved and applied ;-)
Image


Return to “Troubleshooting”

Who is online (over the past 5 minutes)

Users browsing this forum: 5 guests