Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for ALAC encoded audio files #295

Merged
merged 3 commits into from
Jun 2, 2013

Conversation

simonluijk
Copy link
Contributor

Both AAC and ALAC are normally embedded in an .m4a container. For
this reason I have split the m4a type into aac and alac types.

To distinguish between the two encodings I have taken advantage
of the fact that, in my collection, ALAC don't have a sample_rate
set. I could not find verification if this is always the case
or not. Would bitrate be a more reliable determining factor?
e.g. if bitrate > 320000: type = 'alac'

Signed-off-by: Simon Luijk simon@simonluijk.com

Initial issue #142

Both AAC and ALAC are normally embedded in an .m4a container. For
this reason I have split the m4a type into aac and alac types.

To distinguish between the two encodings I have taken advantage
of the fact that, in my collection, ALAC don't have a sample_rate
set. I could not find verification if this is always the case
or not. Would bitrate be a more reliable determining factor?
e.g. if bitrate > 320000: type = 'alac'

Signed-off-by: Simon Luijk <simon@simonluijk.com>
@sampsyo
Copy link
Member

sampsyo commented May 29, 2013

I see—looking through the Mutagen source, I also conclude that it currently doesn't expose the audio format inside an ALAC container. It would be great if we could find a more principled way to inspect the MPEG-4 container to read its codec (some cursory googling did not make it obvious where this is stored in the header), but barring that, this seems like the right approach as long as we clearly label it as a hack in the source code.

As a very minor style note: I'd like to define MP4_TYPES = ('aac', 'alac') (or somesuch) at the top of the file and use that in place of the tuple for readability. (But I can take care of this post-merge.)

Signed-off-by: Simon Luijk <simon@simonluijk.com>
Signed-off-by: Simon Luijk <simon@simonluijk.com>
@simonluijk
Copy link
Contributor Author

Looking though the Mutagen source on line 677 of mp4.py it checks the stsd atom contains "mp4a" before setting channels, bits_per_sample, sample_rate and bitrate. If it is an ALAC this is equal to "alac".

https://code.google.com/p/mutagen/source/browse/mutagen/mp4.py#677

But the stsd atoms are optional, according to line 701. While in my collection AAC and ALAC both have stsd atoms and identified correctly. There is the possibility for false positives for AAC's without a stsd atom.

@sampsyo
Copy link
Member

sampsyo commented May 30, 2013

Aha! Thanks for the detective work.

It sounds like the current implementation (checking the sample rate) lets us piggy-back on the check that Mutagen already does. Unless it seems really easy to check that atom for "mp4a" vs. "alac", let's stick with the current approach.

And false positives seem inevitable here. Let's not worry about it too much—at this point, anything is better than calling every file AAC.

@simonluijk
Copy link
Contributor Author

I looked at reading the atom. But it would mean copying a large part of
mutagen. It would involve: Reopening the file, finding the first track,
reading the atom length then we can access the format information.

A simple patch upstream, to attach the format to the info object, would
make it possible to detect the format reliably. The only thing I'm not sure
about is what to name it. info.format, info.container_format...

sampsyo added a commit that referenced this pull request Jun 2, 2013
Initial support for ALAC encoded audio files
@sampsyo sampsyo merged commit 3e95c52 into beetbox:master Jun 2, 2013
sampsyo added a commit that referenced this pull request Jun 2, 2013
sampsyo added a commit that referenced this pull request Jun 2, 2013
@sampsyo
Copy link
Member

sampsyo commented Jun 2, 2013

Great. Thanks again for investigating that—and, even more importantly, coming up with an implementation that Just Works.™ This is awesome to have. ✨ 👾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants