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

How to add custom field - ID3 Popularimeter #23

Open
wolkenarchitekt opened this issue Nov 27, 2019 · 9 comments · May be fixed by #78
Open

How to add custom field - ID3 Popularimeter #23

wolkenarchitekt opened this issue Nov 27, 2019 · 9 comments · May be fixed by #78

Comments

@wolkenarchitekt
Copy link

wolkenarchitekt commented Nov 27, 2019

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)
[email protected]=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='[email protected]', 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
Copy link
Member

sampsyo commented Nov 27, 2019

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
Copy link
Author

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
Copy link
Member

sampsyo commented Nov 28, 2019

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:

mediafile/mediafile.py

Lines 1417 to 1437 in b6b3520

class QNumberField(MediaField):
"""Access integer-represented Q number fields.
Access a fixed-point fraction as a float. The stored value is shifted by
`fraction_bits` binary digits to the left and then rounded, yielding a
simple integer.
"""
def __init__(self, fraction_bits, *args, **kwargs):
super(QNumberField, self).__init__(out_type=int, *args, **kwargs)
self.__fraction_bits = fraction_bits
def __get__(self, mediafile, owner=None):
q_num = super(QNumberField, self).__get__(mediafile, owner)
if q_num is None:
return None
return q_num / pow(2, self.__fraction_bits)
def __set__(self, mediafile, value):
q_num = round(value * pow(2, self.__fraction_bits))
q_num = int(q_num) # needed for py2.7
super(QNumberField, self).__set__(mediafile, q_num)

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

@wolkenarchitekt
Copy link
Author

wolkenarchitekt commented Nov 29, 2019

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
Copy link
Member

sampsyo commented Nov 29, 2019

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:

'bitdepth': 0,

@wolkenarchitekt
Copy link
Author

wolkenarchitekt commented Nov 30, 2019

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
Copy link
Member

sampsyo commented Dec 1, 2019

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 pushed a commit to wolkenarchitekt/mediafile that referenced this issue Jan 6, 2020
wolkenarchitekt pushed a commit to wolkenarchitekt/mediafile that referenced this issue Jan 6, 2020
wolkenarchitekt pushed a commit to wolkenarchitekt/mediafile that referenced this issue Jan 6, 2020
@wolkenarchitekt
Copy link
Author

wolkenarchitekt commented Jan 7, 2020

I got the basic implementation working within my fork - changes I made: 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 = "[email protected]"
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
Copy link

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

@jmbannon jmbannon linked a pull request Jul 24, 2024 that will close this issue
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 a pull request may close this issue.

3 participants