Add reading of rating tag for MP3, MP4, and FLAC#1089
Add reading of rating tag for MP3, MP4, and FLAC#1089LindyBalboa wants to merge 4 commits intomixxxdj:masterfrom
Conversation
LindyBalboa
force-pushed
the
ratings
branch
4 times, most recently
from
b10f42f to
724b873
Compare
|
Please replace all tabs with 4 spaces: |
| return isBpmValid; | ||
| } | ||
|
|
||
| void parseRating(TrackMetadata *pTrackMetadata, QString sRating, FileType filetype = FileType::UNKNOWN) { |
There was a problem hiding this comment.
The context (i.e. file/tag type) is known at compile time when this function is invoked. Please split it into 3 functions.
|
Tag writing: Simply add the conversion in the opposite direction from iRating into tag values to the specialized write functions in trackmetadatataglib.cpp. Writing tags into files is currently disabled: |
| rating = QString::number(dynamic_cast<TagLib::ID3v2::PopularimeterFrame*>(popmRatingFrameList.front())->rating()); | ||
| for (TagLib::ID3v2::FrameList::ConstIterator it(popmRatingFrameList.begin()); | ||
| it != popmRatingFrameList.end(); it++){ | ||
| auto popmFrame = dynamic_cast<TagLib::ID3v2::PopularimeterFrame*>(*it); |
There was a problem hiding this comment.
The result of a dynamic_cast should always be checked! See various examples in this file
| QString rating; | ||
| const TagLib::ID3v2::FrameList popmRatingFrameList(tag.frameListMap()["POPM"]); | ||
| if (!popmRatingFrameList.isEmpty()) { | ||
| rating = QString::number(dynamic_cast<TagLib::ID3v2::PopularimeterFrame*>(popmRatingFrameList.front())->rating()); |
There was a problem hiding this comment.
This code duplicates part of the loop body. Initialize the rating with -1 and inside the loop body check if either of these conditions is met: (rating == -1) || (email == ...)
There was a problem hiding this comment.
The point for the duplicaton was to take the first rating found and then check all of them if there is a mixxx specific frame. Since this is only truly a problem for MP3, an alternative would be to forget having the mixxx specific email and just reading writing the 1st frame.
That is really a decision that should be made by people more familiar with the trajectory of the project.
| it != popmRatingFrameList.end(); it++){ | ||
| auto popmFrame = dynamic_cast<TagLib::ID3v2::PopularimeterFrame*>(*it); | ||
| if (toQString(popmFrame->email()) == "mixxx@mixxx.org") { | ||
| rating = QString::number(popmFrame->rating()); |
There was a problem hiding this comment.
You should exit the loop after you found the first rating with a matching email.
| m_bpm.resetValue(); | ||
| } | ||
|
|
||
| // rating |
There was a problem hiding this comment.
The comment "// rating" does not add any value.
| for (TagLib::ID3v2::FrameList::ConstIterator it(popmRatingFrameList.begin()); | ||
| it != popmRatingFrameList.end(); it++){ | ||
| auto popmFrame = dynamic_cast<TagLib::ID3v2::PopularimeterFrame*>(*it); | ||
| if (toQString(popmFrame->email()) == "mixxx@mixxx.org") { |
There was a problem hiding this comment.
The e-mail address should be stored in a named constant. Simply define one in the anonymous namespace in the .cpp file.
| return isBpmValid; | ||
| } | ||
|
|
||
| void parseRating(TrackMetadata *pTrackMetadata, QString sRating, FileType filetype = FileType::UNKNOWN) { |
There was a problem hiding this comment.
You can omit the pTrackMetadata argument if you simply return the parsed rating and let the caller decide what to do with this result. Currently this function does two different things: parsing + setting. But the name is only "parse".
There was a problem hiding this comment.
Always prefer to write functions with a result and no side effects over functions with no result but with side effects.
|
|
||
| QString rating; | ||
| if (readMP4Atom(tag, "rate", &rating)) { | ||
| parseRating(pTrackMetadata, rating, FileType::MP4); |
There was a problem hiding this comment.
As already mentioned, the FileType is always known at compile time and does not need to be passed as an additional argument.
LindyBalboa
force-pushed
the
ratings
branch
2 times, most recently
from
09a159d to
1ce1dad
Compare
|
I think this should have a default off operation and only be turned on by choice. It has the potential to overwrite the current database-only ratings. Once writing metadata is implemented, it should also have the option "write all nonzero ratings to file before reading ratings from files". Thoughts? |
|
From a usability viewpoint this decision is far too complex for a user
without background knowledge. It is the application developer's
responsibility to properly design workflows that work as expected and
doesn't expose any implementation details.
Make a decision table and write the desired outcome with any possible side
effects and how they can be solved. Then decide how this can be implemented
in code and database schema without asking the user to make any decisions.
Example: In previous versions Mixxx didn't read the total number of tracks
(track total). For some formats this number is written together with the
number of tracks in the same field. We decided to initialize the new
database field with an (invalid) default value '//'. Whenever we encounter
such a track we re-read track number + total (ignoring any other tags) and
replace the default value in the Mixxx database with the actual values, e.g.
'3/11'. With this strategy all missing values will be read eventually and
(important!) before overwriting any existing tags. Otherwise the track total
tag in the track's file might get lost when writing tags.
…On 19.03.2017 21:42, Conner Phillips wrote:
I think this should have a default off operation and only be turned on by
choice. It has the potential to overwrite the current database-only
ratings. Once writing metadata is implemented, it should also have the
option "write all nonzero ratings to file before reading ratings from files".
Thoughts?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1089 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZ-akV94s56c2884Kl6q13XEwkOQYD-ks5rnZM9gaJpZM4LUxHN>.
|
|
What is this current situation? Should this work and is there test case for this implemented so I can run this on test pack? |
|
This does work. There is no testing implemented. It is basically bare bones minimum working example as of yet. I've really been tied up with school lately and haven't had time to sit down with this. However, I am trying to keep it up to date with master. I am completely open to suggestions on how to continue with this. |
|
Little testing advise would be nice. How to add these in MP3, M4A or OGG so I can test this and what would I expect as return. |
|
I'm not sure what you are asking? This is built directly in to the standard "read tags from file" operations. Nothing else is required at the moment beyond scanning the library. Currently Mixxx does not write any tags to file, it only stores them in the database. As @uklotzde wrote, #728 is concerned with mix writing ratings. So this will only pick up ratings generated by other programs. I'm not sure if iTunes is writing ratings to file yet, or if it is all still stored in their XML file. I personally have used MediaMonkey while on Windows and have written my ratings using that up until now. So basically, rate some files in another program that we know works. Amarok or Banshee maybe for linux? WMP on Windows I think. Then scan them into Mixxx's library and see if they result is as expected. Half star ratings from programs like MediaMonkey should be rounded up to the next full star. |
|
Sorry not being clear about my thoughts! I have to investigate which app I can use to test this. I haven't yet read code very hard but on mp3 it would read ID3v2 POPM tag and on Ogg Vorbis it would be RATING comment and m4a RATE tag. Just wondering which tags to use for testing this out. |
|
You might enrich our test files in src/test/id3-test-data with those tags
and add the corresponding test cases.
…On 07.04.2017 14:18, Tuukka Pasanen wrote:
Sorry not being clear about my thoughts! I have to investigate which app I
can use to test this. I haven't yet read code very hard but on mp3 it
would read ID3v2 *POPM* tag and on Ogg Vorbis it would be *RATING* comment
and m4a *RATE* tag. Just wondering which tags to use for testing this out.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1089 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZ-akoEYD0WgN4oT2Zcji_CAvtDfyNpks5rtil9gaJpZM4LUxHN>.
|
|
@uklotzde I can definitely add some tests there. A few questions.
|
|
First there should be test files with and without stars and maybe different files have different staring or something like that. Added thirty test files sounds awful lot to me. |
|
Ok. i'll give this a shot and check how this is working. Still should have some test cases. |
parseRating has been split into separate functions for MP3, FLAC, and MP4 filetypes since they are already known. parse*TYPE*Rating now only returns the value, without side effects. Now check when casting a generic TagLib frame to a popularimeter frame.
For MP4 and Flac, the convenience functions to read tag items assumes all fields are strings. Previously conversion was done in the parsing functions, it has been switched to write after reading, before the parsers are called.
|
Just to let you guys know: Your work is very appreciated (i do 👍 ) and there are enough scenarios out there, in which the rating is nescessary/helpful. In my case, I have a bigger collection of files (5000 or so) from http://remix.kwed.org with complex sounds, that I have to hear in another audio player/on the go. So i need ratings to filter out, what needs a second listening later and i can mark some hymns (5 stars), which are always possible for a dance crowd. 3 stars is good, but only for private listening and rarely for dj sets. And so son. I deeply hope, your contribution will make in the mixxx code! Thx and greetings! |
|
@LindyBalboa merge conflicts have developed since #728 was merged. |
|
This pull request has not been updated in over a year and merge conflicts have developed, so I am closing it. @LindyBalboa if you fix the merge conflicts and would like to continue working on this, please reopen this PR. |
|
Is this still being developed @LindyBalboa? I can take over this PR if possible |
|
The last update before your post was that Be closed this because there has not been any progress for over a year. If you want to take over feel free to do so. Though, in order to avoid repetition (and maybe frustration), I recommend you read the entire conversation here, and probably also in #9477 and #924. |
|
@ronso0 thank you for the quick reply! I already went through those links (that's how I got to this PR), so I'll probably just create a new issue as a tabula rasa for this problem and mention implementation details, issue/PR mentions and additional info. |
|
Hi Mixxx team, First of all, thank you for building Mixxx — it’s by far the best option I’ve found on Linux for DJing with BPM-aware libraries, and I really appreciate the work you all put into it. I’m a swing dance DJ and until recently I was using iTunes 12, mainly because of its Auto DJ function. My library has about 1000 MP3s, each tagged with gain, start/end markers (to skip silence or clapping), ratings, BPM, and comments. In iTunes this was very easy to manage: search worked across comments, BPM, ratings, etc., and the setup was portable — all I needed was the library file and the correct iTunes version. After moving to Linux I tried Clementine, Rhythmbox, ncmpcpp and others, but none of them could combine BPM support with good searching and rating management. Mixxx finally gave me BPM + search in one app, which is great. That said, I’ve run into a couple of inconveniences where I’d really appreciate some advice or clarification:
If there’s already documentation on these topics that I’ve missed, I’d be very grateful for a link. Thanks again for all your work — Mixxx has already made my Linux DJ life much easier, and I’m excited to keep learning how to get the most out of it. |
|
Hej @CaqKa Let's break it down:
|
7 participants
So this is partly addressed in the open PR #924 but it seems to have stagnated so I have decided to jump on it. Current state is that for mp3 the rating is taken from a POPM tag with email "mixxx@mixxx.org" or the first available if there is none from mixxx. MP4 and FLAC both use a plain 0-100 integer scale.
Values are then parsed into a 0-5 value before being added to the metadata object. For POPM, the same standard as Windows Media Player/Explorer is followed.
To do:
-add support for APE tags I'm not familiar with it and don't use it. I can't seem to find the key for the rating tag. Does anyone know this off hand?
-add writing from database to file. Need some discussion here. As MP4, FLAC, and APE do no use POPM, there isn't the email feature. So I feel it would actually make little sense to use a designated email for mixxx. Some users claim that they need different rating option for different applications/people, but honestly I use a mixed library; format isn't so important to me. Like I said, discussion needed.
-add the option to not overwrite existing ratings in the database
-add the option to turn writing the rating to the file tag on or off
I'm not quite sure where in the code the writing part would be controlled. If someone could point me in the right direction I'm sure I can figure it out, but if someone wants to contribute that, that would be great too.