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

APE Format Support #190

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

APE Format Support #190

wants to merge 9 commits into from

Conversation

Ace-Radom
Copy link

Hey there:
I forked this repo and added supports for ape format. I'm not sure if you're insterested in it.
The things I changed are:

  1. add .ape tag in SUPPORTED_FILE_EXTENSIONS list

    https://github.com/Ace-Radom/tinytag/blob/ef28a83e332a0ab5a51045defd940d72d0b7389d/tinytag/tinytag.py#L87

  2. add .ape pointing to class APE under cls._file_extension_mapping in function _get_parser_for_filename

    https://github.com/Ace-Radom/tinytag/blob/ef28a83e332a0ab5a51045defd940d72d0b7389d/tinytag/tinytag.py#L149

  3. add regex for ape format under cls._magic_bytes_mapping in function _get_parser_for_file_handle

    https://github.com/Ace-Radom/tinytag/blob/ef28a83e332a0ab5a51045defd940d72d0b7389d/tinytag/tinytag.py#L181

  4. then the APE class

I didn't add any new tests because I don't know how to do it. But I tested it with my own samples and it worked as it should be.

The only thing is the keynames of apetag is not defined by the developers of the format. That means: there can be different keynames for one field (e.g. artist). I wrote a map like:

apetag_mapping = {
    'title':        'title',
    'artist':       'artist',
    'album':        'album',
    'album artist': 'albumartist',
    'comment':      'comment',
    'composer':     'composer',
    'disc':         'disc',
    'discnumber':   'disc',
    'genre':        'genre',
    'track':        'track',
    'tracknumber':  'track',
    'year':         'year'
}

This covered all situations I had with the samples created by foobar2k and mp3tag. But like I said, there can be different keynames for one field, and I don't know how efficient this solution is.

@mathiascode
Copy link
Member

Thanks for your contribution! Support for APE files would be great.

You can add sample files here: https://github.com/devsnd/tinytag/tree/master/tinytag/tests/samples

Then add the metadata to the testfiles dictionary: https://github.com/devsnd/tinytag/blob/master/tinytag/tests/test_all.py

Ideally, test files should be as small as possible, and either contain silence, or be truncated to the point that almost no audio data remains. If possible, try to use e.g. Audacity to generate silent audio files, and then add metadata to them.

self.channels = channels_num
self.bitrate = self.filesize / self.duration * 8 / 1000

# start to parse metadatas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, please move any code before this to _determine_duration(), so tags aren't populated when tag parsing is disabled. If it's not possible, please check the _parse_tags and _parse_duration attributes for when to populate tags.

@Ace-Radom
Copy link
Author

Ace-Radom commented Nov 26, 2023

Thanks for your contribution! Support for APE files would be great.

You can add sample files here: https://github.com/devsnd/tinytag/tree/master/tinytag/tests/samples

Then add the metadata to the testfiles dictionary: https://github.com/devsnd/tinytag/blob/master/tinytag/tests/test_all.py

Ideally, test files should be as small as possible, and either contain silence, or be truncated to the point that almost no audio data remains. If possible, try to use e.g. Audacity to generate silent audio files, and then add metadata to them.

Alright I'll try to build some of them.

If possible, please move any code before this to _determine_duration(), so tags aren't populated when tag parsing is disabled. If it's not possible, please check the _parse_tags and _parse_duration attributes for when to populate tags.

Ah okay, I wrote this part of the program with the reference to the Aiff._determine_duration and therefore I put all things into _parse_tag function, but I'll change it this night.

And one further thing, I realize that the checks are failing due to E501 line too long. Is there still something I need to change in the code? (cuz I never see such an error before)

@mathiascode
Copy link
Member

And one further thing, I realize that the checks are failing due to E501 line too long. Is there still something I need to change in the code? (cuz I never see such an error before)

Wrap the long lines so they don't exceed the limit (100 characters per line). Run flake8 locally to verify that everything is fine.

@Ace-Radom
Copy link
Author

Ace-Radom commented Nov 28, 2023

I added one sample to the tests. Sadly I failed to find or build a sample with ape file format lower than version 3.98 (even with the offical encoder released in 2011), it's simply too old.

tinytag/tinytag.py Outdated Show resolved Hide resolved
tinytag/tinytag.py Outdated Show resolved Hide resolved
tinytag/tinytag.py Outdated Show resolved Hide resolved
@mathiascode
Copy link
Member

I added one sample to the tests. Sadly I failed to find or build a sample with ape file format lower than version 3.98 (even with the offical encoder released in 2011), it's simply too old.

No problem, I could see if I can find some sample files. If you fix the remaining linting error, we can get a coverage report showing how much of the code is covered by the tests.

@coveralls
Copy link

Coverage Status

coverage: 95.221% (-1.3%) from 96.565%
when pulling 92ca484 on Ace-Radom:master
into d8a62a2 on devsnd:master.

@Ace-Radom
Copy link
Author

Hm doesn't look really fine (I mean the coverage)
Is there still something I need to do?

'comment': 'comment',
'composer': 'composer',
'disc': 'disc',
'discnumber': 'disc',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add mapping for disctotal -> disc_total

'discnumber': 'disc',
'genre': 'genre',
'track': 'track',
'tracknumber': 'track',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add mapping for tracktotal -> track_total

@mathiascode
Copy link
Member

Here are audio files with both APEv1 and APEv2 tags. Could you add them to the sample files and ensure they are read properly? ape_tracks.zip

I'll try to generate a file for fver < 3980 too.

'title': 'title',
'artist': 'artist',
'album': 'album',
'album artist': 'albumartist',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add albumartist -> albumartists (used by taglib)

@@ -422,6 +422,16 @@
'duration': 2.176, 'filesize': 18532, 'bitrate': 64.0, 'samplerate': 8000, 'bitdepth': 8,
'title': 'song title', 'artist': 'artist 1;artist 2', 'audio_offset': 46}),

# APE
('samples/ape-44100-16-1.ape',
{'extra': {'Copyright': 'nope'}, 'channels': 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert all extra tag keys to lowercase

@mathiascode
Copy link
Member

Please also add a test_invalid_ape_file test (see test_all.py for previous examples), and extend the test_detect_magic_headers test.

Is there still something I need to do?

Ideally, we would have enough sample files so the tests cover every new line of code except self._parse_header(fh).

@Ace-Radom
Copy link
Author

Sorry for the really slow response, I was full buzy with something else in the past two weeks...

Here are audio files with both APEv1 and APEv2 tags. Could you add them to the sample files and ensure they are read properly?

I see, I'll do that tomorrow morning.

Please also add a test_invalid_ape_file test (see test_all.py for previous examples), and extend the test_detect_magic_headers test.

Okay I'll try to do that.

@mathiascode
Copy link
Member

No worries, thank you for working on this. :)

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.

None yet

3 participants