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 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. 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 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.