beetbox / mediafile

elegant audio file tagging
http://mediafile.readthedocs.io/
MIT License
100 stars 26 forks source link

How to add custom field - ID3 Popularimeter #23

Open wolkenarchitekt opened 5 years ago

wolkenarchitekt commented 5 years ago

I need to read/write the ID3 Popularimeter tag within my app, specifically in the way that Native Instruments Traktor writes them.

This is a sample file containing the tag as written by Traktor: https://github.com/ifischer/django-tracks-api/raw/master/tracks_api/tests/fixtures/popm.mp3

This is how mutagen-inspect shows it on command line:

$ mutagen-inspect tracks_api/tests/fixtures/popm.mp3
-- tracks_api/tests/fixtures/popm.mp3
- MPEG 1 layer 3, 32678 bps (VBR), 44100 Hz, 1 chn, 5.04 seconds (audio/mp3)
POPM=traktor@native-instruments.de=0 102/255
TSSE=Lavf57.83.100

So far I'm using pure mutagen in my app to do it:

from mutagen.id3 import ID3
from mutagen.id3 import POPM

ID3_POPM = 'POPM'

class SimpleID3(ID3):
    def __init__(self, filename):
        super().__init__(filename)

    @property
    def rating(self):
        rating = self.getall(ID3_POPM)
        if rating:
            rating = rating[0].rating
            return int(rating / 51)
        return None

    @rating.setter
    def rating(self, rating: int):
        popm = POPM(email='traktor@native-instruments.de', rating=rating * 51, count=0)
        self[ID3_POPM] = popm

How can I achieve something similar with mediafile, adding the popularimeter tag as a custom field?

sampsyo commented 5 years ago

Hi! There is not a built-in extensibility mechanism in MediaFile, but it would be cool to add this field if you're interested. Have you tried extending MediaFile directly to add one more MediaField for the titled rating or similar?

wolkenarchitekt commented 5 years ago

Hi! There is not a built-in extensibility mechanism in MediaFile, but it would be cool to add this field if you're interested.

Yes it probably makes sense to add the field to the library as it's a standard ID3 field and also used by other Software. I'm happy to help sending a MR when I got it fully working.

Have you tried extending MediaFile directly to add one more MediaField for the titled rating or similar?

From what I understand I have to add a new StorageStyle to wrap the Mutagen-access and then define and add a new MediaField field using that style, right? Will try to get it working and report back - in the meantime I appreciate any hints ;)

sampsyo commented 4 years ago

Yep! I see that the field seems to store a proportional value as a denormalized integer. A good place to start might be this existing storage style that deals with fixed-point storage: https://github.com/beetbox/mediafile/blob/b6b3520b3f54ccb0a95a10c18366ef857517439d/mediafile.py#L1417-L1437

Presumably, a similar thing for normalizing an integer should be similar but simpler.

wolkenarchitekt commented 4 years ago

I think I know how to implement this now and just had a look how the mediafile tests are setup, to prepare a new PR with popularimeter-feature. I created some convenience methods for reliable and reproducible test setups using docker, see my fork: https://github.com/ifischer/mediafile/blob/master/Dockerfile https://github.com/ifischer/mediafile/blob/master/Makefile

Unfortunately, running the tests in Docker (and also on bare metal OSX), one test from current master branch fails with all Python versions:

======================================================================
FAIL: test_read_audio_properties (test.test_mediafile.AIFFTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/app/test/test_mediafile.py", line 441, in test_read_audio_properties
    self.assertEqual(getattr(mediafile, key), value)
AssertionError: 16 != 0

The test fails while checking "bitdepth" attribute. Do you have any idea why?

To reproduce:

git clone https://github.com/ifischer/mediafile 
cd mediafile
make build test
sampsyo commented 4 years ago

Aha; nice catch! This actually has nothing to do with your Docker setup; it's a dependency version thing.

The root cause is a new feature in the latest version of Mutagen. The symptom is that the bitdepth field now returns a useful value for AIFF files as of Mutagen 1.43.0, as a result of https://github.com/quodlibet/mutagen/pull/403/files, which defines bits_per_sample for that format.

We should probably just bump our required Mutagen version and then change this line to give a value of 16 instead of 0 for that field: https://github.com/beetbox/mediafile/blob/b6b3520b3f54ccb0a95a10c18366ef857517439d/test/test_mediafile.py#L933

wolkenarchitekt commented 4 years ago

Alright, good that I asked 😉 I can take care of that. What do you think about adding a requirements.txt to pin versions more strictly and explicit? This would also speed up the docker build.

sampsyo commented 4 years ago

Good question---I worry a little that pinning dependency versions for (e.g.) CI will just move the problem somewhere else, when a dependent project notices breakage. (Because of the way Python manages dependencies, it’s nice to keep the “real” library dependency specs in setup.py as permissive as possible rather than pinning a specific version, as npm makes it safe to do.) But happy to add a requirements.txt just for faster Docker builds or whatever if you’re interested.

wolkenarchitekt commented 4 years ago

I got the basic implementation working within my fork - changes I made: https://github.com/beetbox/mediafile/compare/master...ifischer:master What makes it a bit trickier is that Popularimeter is basically a dictionary, and I wanted to keep this behavior when accessing it via mediafile - as from id3v2.3.0 docs:

There may be more than one "POPM" frame in each tag, but only one with the same email address.

This is how I can currently access the Popularimeter with my fork:

from mediafile import MediaFile

email = "test@foobar.com"
filename = 'test/rsrc/popm.mp3'
mf = MediaFile(filename)
mf.popm = {
    email: {'rating': 1 'count': 1}
}
mf.save()
assert mf.popm == {email: {'rating': 1, 'count': 1}}

Let me know what you think of a behavior like this. In the meantime I'll continue testing and open a PR.

soulstyle commented 3 years ago

This looks very interesting! @wolkenarchitekt Did you have time to take this any furter?