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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5917a6
New Feature: Rating from id3 popm frame translates into star ratings.
WaylonR Apr 12, 2016
120e151
Merge remote-tracking branch 'upstream/master' into id3ratings_to_stars
WaylonR Apr 17, 2016
f13b110
Attempt #1 to include updating ratings from mp3's as a preference in …
WaylonR May 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/preferences/dialog/dlgpreflibrary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ void DlgPrefLibrary::slotExtraPlugins() {
void DlgPrefLibrary::slotResetToDefaults() {
checkBox_library_scan->setChecked(false);
checkbox_ID3_sync->setChecked(false);
checkBox_ID3_rating_sync->setChecked(false);
checkBox_use_relative_path->setChecked(false);
checkBox_show_rhythmbox->setChecked(true);
checkBox_show_banshee->setChecked(true);
Expand All @@ -167,6 +168,8 @@ void DlgPrefLibrary::slotUpdate() {
ConfigKey("[Library]","RescanOnStartup")).toInt());
checkbox_ID3_sync->setChecked((bool)m_pconfig->getValueString(
ConfigKey("[Library]","WriteAudioTags")).toInt());
checkBox_ID3_rating_sync->setChecked((bool)m_pconfig->getValueString(
ConfigKey("[Library]","ID3RatingSync")).toInt());
checkBox_use_relative_path->setChecked((bool)m_pconfig->getValueString(
ConfigKey("[Library]","UseRelativePathOnExport")).toInt());
checkBox_show_rhythmbox->setChecked((bool)m_pconfig->getValueString(
Expand Down Expand Up @@ -300,6 +303,8 @@ void DlgPrefLibrary::slotApply() {
ConfigValue((int)checkBox_library_scan->isChecked()));
m_pconfig->set(ConfigKey("[Library]","WriteAudioTags"),
ConfigValue((int)checkbox_ID3_sync->isChecked()));
m_pconfig->set(ConfigKey("[Library]","ID3RatingSync"),
ConfigValue((int)checkBox_ID3_rating_sync->isChecked()));
m_pconfig->set(ConfigKey("[Library]","UseRelativePathOnExport"),
ConfigValue((int)checkBox_use_relative_path->isChecked()));
m_pconfig->set(ConfigKey("[Library]","ShowRhythmboxLibrary"),
Expand Down
10 changes: 10 additions & 0 deletions src/preferences/dialog/dlgpreflibrarydlg.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,16 @@
</property>
</widget>
</item>
<item row="1" column="0" colspan="2">
<widget class="QCheckBox" name="checkBox_ID3_rating_sync">
<property name="enabled">
<bool>true</bool>
</property>
<property name="text">
<string>Synchronize mixxx star ratings with ID3 ratings (caution: overwrites mixxx ratings)</string>
</property>
</widget>
</item>
<item row="2" column="0" colspan="2">
<widget class="QCheckBox" name="checkBox_use_relative_path">
<property name="text">
Expand Down
2 changes: 2 additions & 0 deletions src/preferences/settingsmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ void SettingsManager::initializeDefaults() {
// For safety reasons, we deactivate this feature.
m_pSettings->set(ConfigKey("[Library]","WriteAudioTags"), ConfigValue(0));

m_pSettings->set(ConfigKey("[Library]","ID3RatingSync"),ConfigValue(0));;

// Intialize default BPM system values.
// NOTE(rryan): These should be in a better place but they've always been in
// MixxxMainWindow.
Expand Down
3 changes: 2 additions & 1 deletion src/track/trackmetadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ bool operator==(const TrackMetadata& lhs, const TrackMetadata& rhs) {
(lhs.getBitrate() == rhs.getBitrate()) &&
(lhs.getDuration() == rhs.getDuration()) &&
(lhs.getBpm() == rhs.getBpm()) &&
(lhs.getReplayGain() == rhs.getReplayGain());
(lhs.getReplayGain() == rhs.getReplayGain()) &&
(lhs.getRating() == rhs.getRating());
}

} //namespace Mixxx
9 changes: 9 additions & 0 deletions src/track/trackmetadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,14 @@ class TrackMetadata {
m_replayGain = ReplayGain();
}

int getRating() const {
return m_iRating;
}

void setRating(int rating) {
m_iRating = rating;
}

// Parse an format date/time values according to ISO 8601
static QDate parseDate(QString str) {
return QDate::fromString(str.trimmed().replace(" ", ""), Qt::ISODate);
Expand Down Expand Up @@ -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.

};

bool operator==(const TrackMetadata& lhs, const TrackMetadata& rhs);
Expand Down
27 changes: 27 additions & 0 deletions src/track/trackmetadatataglib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ static_assert(sizeof(wchar_t) == sizeof(QChar), "wchar_t is not the same size th
#include <taglib/textidentificationframe.h>
#include <taglib/attachedpictureframe.h>
#include <taglib/flacpicture.h>
#include "controlobject.h"

namespace Mixxx {

Expand Down Expand Up @@ -838,6 +839,32 @@ 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

int rating = 0;

// const int ratingbool = ControlObject::getControl(ConfigKey("[Library]","ID3RatingSync"));
//qDebug() << "ID3 " << ratingbool;
if(!ratingFrame.isEmpty()) {
// RatingString "traktor@native-instruments.de rating=255 counter=2"

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the tractor format does not apply to the POPM standard:
http://id3.org/id3v2.4.0-frames
According this, the Traktor values have to be:
"traktor@native-instruments.de\0\xff\x00000002"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

drat, forgot about that example RatingString. Eh, it works on Windows Media player rated mp3's, at least.

QString sRating = TStringToQString(ratingFrame.front()->toString());
sRating = sRating.section("=",1,2).left(3);
float fRating = sRating.toInt();
rating = ceil(fRating/51);
Copy link
Member

Choose a reason for hiding this comment

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

Since we want to write back the metadadata one day, it is Important not to loose information during the round trip in Mixxx. So we should do the mapping to stars only when we display the data.

Here, I think a data structure like this will help:

class Popularity {
  public:
      unsigned char rating; 
      double counter;
}
...
QMap<QString, Popularity> m_popularity 
...
TrackMetadata::addPoplarity(const QString& email, const Popularity& popularity) {
    m_popularity.insert(email, popularity) 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QMap? Why create a hash, here? also doesn't seem to be associating with the current track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With you idea of not connecting ratings with stars automatically, but only if flagged, we'd need to have a new column in the track table called iRating or similar? this wouldn't work so well though, as searching for rating "rating:>3" would pull mixxx's internal only star rating, wouldn't pick up the id3 rating.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because you are able to store multiple independent ratings. Each rating belongs to an e-mail address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, this is a problem then. How does the code decide which rating to use for its star rating display? and as far as i know, metatags are only read in on a forced reload or new file.. a library scanner didn't even refresh the rating. How do you store a Qmap/hash in the database? how would you use the library search to search for rated files in stars, when its not stored in stars?

Copy link
Member

Choose a reason for hiding this comment

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

I would keep it simple: The Mixxx database should keep track of the Mixxx ratings.
These can be optionally stored into the file at "mixxx@mixxx.org"

Adopting foreign ratings need an explicit user action like:

  • "Adopt Traktor ratings as Mixxx ratings for unrated tracks"

This can be simplified with a second read only column that just displays the mean value of any external rating found.

Or we implement both.


// Calc rating
//
// NI - Rating
// 255 = 5 | 204 = 4 | 153 = 3 | 102 = 2 | 51 = 1 | 0 = 0
//
// Banshee - Rating
// 255 = 5 | 192 = 4 | 128 = 3 | 64 = 2 | 1 = 1
//
// ==> Rating = ceil ( X / 51 )
Copy link
Contributor

Choose a reason for hiding this comment

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

This has mapping has to be justified by more examples. If it works for 2 applications it may fail for the 3rd.

Ratings are a really complicated topic if you study the various file formats. Not taking into account the various interpretations and implementation of the standards that can be found in applications. This is the reason why I kept the Mixxx-specific rating in TrackInfoObject.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should just linear map the rating to a double 0.0 - 5.0 and change the Mixxx DB field to double.
The double to star conversion should be moved to the display.

Copy link
Contributor

Choose a reason for hiding this comment

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

When using an internal floating-point representation I would prefer normalized internal ranges like [0.0, 1.0] for asysmmetric (e.g. "rating") and [-1.0, 1.0] for symmetric values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i failed at maths at school. Have no idea how that would work.
And daschuer is right, it should be a double.

Copy link
Member

Choose a reason for hiding this comment

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

Normally I would prefer the [0.0, 1.0] as well. If we keep the [0.0, 5.0] range, we can keep the library column, because sqlite does not distinguish between int and double internal anyway.
For a [0.0, 1.0] solution we need to migrate to a new column.


qDebug() << "Final int() Rating" << rating;
pTrackMetadata->setRating(rating);
}
}

void readTrackMetadataFromAPETag(TrackMetadata* pTrackMetadata, const TagLib::APE::Tag& tag) {
Expand Down
2 changes: 0 additions & 2 deletions src/track/trackmetadatataglib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include <taglib/id3v2tag.h>
#include <taglib/xiphcomment.h>
#include <taglib/mp4tag.h>

#include <QImage>

#include "track/trackmetadata.h"
Expand All @@ -32,7 +31,6 @@ bool writeTrackMetadataIntoAPETag(TagLib::APE::Tag* pTag, const TrackMetadata& t
bool writeTrackMetadataIntoXiphComment(TagLib::Ogg::XiphComment* pTag,
const TrackMetadata& trackMetadata);
bool writeTrackMetadataIntoMP4Tag(TagLib::MP4::Tag* pTag, const TrackMetadata& trackMetadata);

} //namespace Mixxx

#endif
7 changes: 4 additions & 3 deletions src/trackinfoobject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,13 +783,14 @@ bool TrackInfoObject::isDirty() {

int TrackInfoObject::getRating() const {
QMutexLocker lock(&m_qMutex);
return m_iRating;
return m_metadata.getRating();
Copy link
Member

Choose a reason for hiding this comment

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

Mixxx rating can be != Traktor rating for some users or files.
We have to avoid that we loose the Mixxx rating when we reload the meta data from the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, daschuer, can you make the necessary changes? Not sure how to go about them.
And are you suggesting a 'sync rating to stars' option in interface or something?

Copy link
Member

Choose a reason for hiding this comment

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

I have other projects on my todo list. Currently I am working on the native Jack interface.

Such a 'sync rating to stars' option seam to work. I am only unsure how we have to deal with the email address. Do we need to expose it to the user?
@uklotzde any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, display of the email address is unnecessary in this case. With Windows media player, and some taggers, they don't even save a email address in that field, but a string about the app. Windows media player puts "Windows Media 9 Series rating=128 counter=0" for its 3 star rating.

I need some traktor rated files, anyone able to provide?

Copy link
Contributor

Choose a reason for hiding this comment

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

If Mixxx silently ignores and discards this application-specific string, users may lose their ratings in those applications after tags have been written back by Mixxx.

Copy link
Member

Choose a reason for hiding this comment

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

How does Windows media player behave if there is an embedded rating with a foreign email field?

Banshee ignored the rating if I change "Banshee" to "Mixxx ", which is correct in the first place, but is not what the user might expect. He might want to use the ID3 tag to see or even synchronize ratings among several media players.

In my personal use case, Mixxx is not my main media player. I use Clementine together with my family.
It would be nice, If I be able to see the Clementine ratings in Mixxx but in a separate column, without destroying the DJ rating.

An other DJ has a second mixing app along with Mixxx (I can't think of a reason why) he might want to keep the ratings in sync. If we want to do that, I think there is now way around exposing the Email field to the user.

}

void TrackInfoObject::setRating (int rating) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(&m_iRating, rating)) {
markDirtyAndUnlock(&lock);
if (m_metadata.getRating() != rating ) {
m_metadata.setRating(rating);
markDirtyAndUnlock(&lock);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/trackinfoobject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class TrackInfoObject : public QObject {
Q_PROPERTY(QString key READ getKeyText WRITE setKeyText)
Q_PROPERTY(int duration READ getDuration WRITE setDuration)
Q_PROPERTY(QString durationFormatted READ getDurationText STORED false)
Q_PROPERTY(int rating READ getRating WRITE setRating)

TrackId getId() const;

Expand Down