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

New Feature: Rating from id3 popm frame translates into star ratings. #924

Closed
wants to merge 3 commits into from
Closed

New Feature: Rating from id3 popm frame translates into star ratings. #924

wants to merge 3 commits into from

Conversation

WaylonR
Copy link
Contributor

@WaylonR WaylonR commented Apr 12, 2016

This pull request will pull in the id3 popm or Populimeter frame, and translate its rating numbers into stars, and apply them to the database. For existing files, they will have to be 'Reload from file metadata'ed.

@@ -838,6 +838,29 @@ void readTrackMetadataFromID3v2Tag(TrackMetadata* pTrackMetadata,
parseTrackPeak(pTrackMetadata,
toQString(pTrackPeakFrame->fieldList()[1]));
}
TagLib::ID3v2::FrameList ratingFrame = tag.frameListMap()["POPM"];
Copy link
Member

Choose a reason for hiding this comment

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

// POPM = Popularimeter

@daschuer
Copy link
Member

Thank you very much for working on this feature. It is a good step towards a universal solution.

We have the issue, that Rating from different sourced can mean different things.
If you search the web for "Using Star rating" or similar, you will find some blog with different opinions how to use them.

Mainly we can distinguish at least a DJ-Rating from an Personal rating. Thats why I think we have to support at least two rating columns, but that is an other issue.

Related bugs:
https://bugs.launchpad.net/mixxx/+bug/973136
https://bugs.launchpad.net/mixxx/+bug/973145

Maybe we can setup the rating user as preference option.
How about to store Mixxx rating as mixxx@mixxx.org by default?
Adopting the Traktor rating as Mixxx rating should be an extra action.

It looks like your use case will be work perfectly if you set the rating user to "traktor@native-instruments.de"

@@ -194,6 +202,7 @@ class TrackMetadata {
int m_channels;
int m_duration; // seconds
int m_sampleRate; // Hz
int m_iRating;
Copy link
Contributor

Choose a reason for hiding this comment

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

TrackMetadata stores tags from various file formats in a portable way, both for reading and writing. I don't recommend to implement it in a way that just works for some special cases and loses information when writing tags back to files as Daniel already mentioned.

The int-rating in Mixxx is just the number of stars displayed on the UI. A portable solution needs to provide a bidirectional mapping between the tags from various file formats and the internal representation.

@daschuer
Copy link
Member

@WaylonR:
What is your original use case, that has made you do this work? Which applications and OSs are involved?
Maybe there is a chance to allow it in a way that does not prevent the other things discussed here.

@WaylonR
Copy link
Contributor Author

WaylonR commented Apr 13, 2016

My original cause of doing this is my wife sorting and rating my music (using her personal taste and music charts, with #1's being rated 5 stars), using AudioShell, which uses the Windows Media method of rating. Works well. She uses Windows, I use Mixxx under linux, its my only library organizing software.

@daschuer
Copy link
Member

OK, so it looks like we need a mass import of the Windows Media Player ratings to Mixxx ratings.
Picking the first rating we found might work for the fist version.
This would be normally a context menu feature, but the context menu is already a bit cluttered.
Any other idea?

@WaylonR
Copy link
Contributor Author

WaylonR commented Apr 14, 2016

id suggest making a preference to set 'Import ratings to mixxx star
ratings' with the caution 'overwrites pre-existing star ratings' and let
the metadata reload (an action that a user does, knowing that it
overwrites customized mixxx data) or the new file import update the
ratings. This, for the first version of the feature. There are some dj's
who use external tagging software to tag their files, and mixxx is just
the library to show and search. Thats what I do. The master metadata is
in the id3 tags.

On 14/04/16 03:46, Daniel Schürmann wrote:

OK, so it looks like we need a mass import of the Windows Media Player
ratings to Mixxx ratings.
Picking the first rating we found might work for the fist version.
This would be normally a context menu feature, but the context menu is
already a bit cluttered.
Any other idea?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#924 (comment)

@daschuer
Copy link
Member

This would work for me as a first shot.

This check-box can placed along with "[Library]","WriteAudioTags"
https://github.com/mixxxdj/mixxx/blob/master/src/preferences/dialog/dlgpreflibrary.cpp#L301

And can be evaluated here:

SoundSourceProxy(pTrack).loadTrackMetadata(true);

Do you need some more hints to make it real?

@LindyBalboa
Copy link
Contributor

I'm guessing this PR has kind of died? This is of major interest to me because I am trying to move from Windows to Linux and there is no replacement for MediaMonkey, which stores all ratings as POPM. I plan on forking Mixxx to really make it do what I need to generally, but this is a feature I could definitely do for main branch. I actually already did it mostly for a music player I started designing but scrapped.

I'm kind of low on free time right now, but it is a big deal for me so I will find some time. Should I jump ahead on this?

@daschuer
Copy link
Member

@LindyBalboa It would be great, if you will jump in here helping us to move this into a mere-gable state.

A great benefit of Mixxx is that it is already highly flexible and customizable. However, we have not managed to a add all features required to be a full media player replacement yet (I personally use Clementine as personal media-player for that reason).

I am happy to accept pull requests for upstream, adding these missing features. So there should be no reason to actually fork Mixxx. We have to keep in mind that Mixxx's first purpose is live DJ-ing though.

Some time ago we have discussed to add two modes to Mixxx: Live mode and Prepare mode. This could also be a good approach if you plan to move away from the Live DJ use case.

@daschuer
Copy link
Member

@WaylonR How are your planes here? What is missing to merge this to master? How can @LindyBalboa support you?

In order to merge this to master we need your permission. Please sign:

https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ

and comment here when done. Thank you

@LindyBalboa
Copy link
Contributor

@daschuer There were a few features that I personally would find useful but wasn't sure if they would fit in with the goals of Mixxx. My main use for a music player aside from personal use is to DJ for Swing dancing. This has different needs than club DJs.

Two items I would want to implement are:

  1. Having a pre-queue playlist visible in order to pre-select more than the next song via the other deck. I think this feature is available in Apple DJay.
  2. Built in delay between songs i.e. set 0.5s-3s between the end of the previous track and the next.

There were many more ideas I had, but it is Sunday evening and I am feeling to lazy to hunt down the list. My main concern was if #1 fit in or not. If that is something the main devs would be okay with, I would be happy to try and add that for Mixxx.

@daschuer
Copy link
Member

Swing DJs should belong to Mixxx's first class citizen!

IMHO 1) is already implemented in #991 and will be merged soon.
There you can view the library table and the AutoDJ playlist usable as pre-queue playlist side by side.
Please try it out, we are looking forward to your comments.

  1. If you use AutoDJ, you can set the fade time negative to achieve a pause. However https://bugs.launchpad.net/mixxx/+bug/673515 and https://bugs.launchpad.net/mixxx/+bug/1106813 could be useful to implement

There were many more ideas I had, but it is Sunday evening and I am feeling to lazy to hunt down the list.

Please file a bug for every item on your list at: https://bugs.launchpad.net/mixxx/+filebug

@LindyBalboa
Copy link
Contributor

Super. That is pretty much what my first stab at implementing that was going to be. Will take a look at the pause stuff.

The other big thing I feel is missing is a more advanced search option. I really like how mediamonkey does their search. Is something like that in the works? If not I got pretty far with a clone of it in my last project.

@daschuer
Copy link
Member

Do you know this: https://www.mixxx.org/manual/latest/chapters/library.html#finding-tracks-search
Please file bugs for missing search queries or GUI ideas!

@WaylonR
Copy link
Contributor Author

WaylonR commented Nov 15, 2016

It isn't ready yet, i haven't successfully hooked the UI options in, but
i have got it working, importing the popm info into the database, as the
stars.. so its always on by default. I'll review what ive done, and do
one more push before I ask for merge.

On 14/11/16 04:39, Daniel Schürmann wrote:

@WaylonR https://github.com/WaylonR How are your planes here? What
is missing to merge this to master? How can @LindyBalboa
https://github.com/LindyBalboa support you?

In order to merge this to master we need your permission. Please sign:

https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ

and comment here when done. Thank you


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#924 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AQ5ee6GnVC_D-d3aiOm057LQMafax3qcks5q9y83gaJpZM4IFaj3.

@LindyBalboa
Copy link
Contributor

It looks like this is only implemented for ID3, but not Xiph, APE, or MP4/M4A. I would say those are rather critical pieces to the puzzle especially Xiph and MP4 because of audiophiles and itunes users.

@uklotzde
Copy link
Contributor

This is what I already mentioned when I reviewed this comment sometime ago:
#924 (comment)

The implementation is still incomplete and currently only fits one special
use case.

You may also should consider rebasing this branch on master before working
on it again. There are already some minor merge conflicts. Don't hesitate to
contact me, if you need any help.

On 16.11.2016 11:39, Conner Phillips wrote:

It looks like this is only implemented for ID3, but not Xiph, APE, or
MP4/M4A. I would say those are rather critical pieces to the puzzle
especially Xiph and MP4 because of audiophiles and itunes users.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#924 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABZ-au6Wriuwu58FUdP7f_rar1WawaH9ks5q-t1pgaJpZM4IFaj3.

@illuusio
Copy link
Contributor

Actually is this PR duplicate of more advanced #1089?

@WaylonR
Copy link
Contributor Author

WaylonR commented Apr 26, 2017 via email

@uklotzde
Copy link
Contributor

uklotzde commented Dec 2, 2017

@WaylonR Is it ok for you that we close this PR since there already is a successor and now many merge conflicts? I'm trying to do some housekeeping to get a better overview about what PRs are actually active and need to be reviewed.

@WaylonR
Copy link
Contributor Author

WaylonR commented Dec 2, 2017

Yes, thank you.

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

5 participants