Skip to content

Commit

Permalink
SoundSource - Fixing TString convertion
Browse files Browse the repository at this point in the history 
- Try converting a TString from a metadata directly to QString can produce segfaults.
- We must check if it is valid before convert it.
  • Loading branch information
@cardinot
cardinot committed Jul 16, 2014
1 parent 128dd1b commit 76ebcd4
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 42 deletions.
90 changes: 49 additions & 41 deletions src/soundsource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,26 +232,34 @@ void SoundSource::setKey(QString key){
m_sKey = key;
}

QString SoundSource::getQString(TagLib::String TString) {
if (TString != TagLib::String::null) {

This comment has been minimized.

Sign in to view

Copy link
@kain88-de

kain88-de Jul 22, 2014

This does not compile under windows

src\soundsource.cpp(236) : error C2678: binary '!=' : no operator found which ta
kes a left-hand operand of type 'TagLib::String' (or there is no acceptable conv
ersion)

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot Jul 22, 2014

Author Owner

We are using a old taglib version in "winlib" (are you getting it from there?)
https://github.com/mixxxdj/winlib/blob/1.12-msvc10-static/x86/taglib/tstring.h

This comment has been minimized.

Sign in to view

Copy link
@kain88-de

kain88-de Jul 23, 2014

yeah I'm using the prebuild dependencies. so what version is needed?

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot via email Jul 23, 2014

Author Owner
return TStringToQString(TString);
}
return QString();
}

bool SoundSource::processTaglibFile(TagLib::File& f) {
if (s_bDebugMetadata)
qDebug() << "Parsing" << getFilename();

if (f.isValid()) {
TagLib::Tag *tag = f.tag();
if (tag) {
QString title = TStringToQString(tag->title());

QString title = getQString(tag->title());
setTitle(title);

QString artist = TStringToQString(tag->artist());
QString artist = getQString(tag->artist());
setArtist(artist);

QString album = TStringToQString(tag->album());
QString album = getQString(tag->album());
setAlbum(album);

QString comment = TStringToQString(tag->comment());
QString comment = getQString(tag->comment());
setComment(comment);

QString genre = TStringToQString(tag->genre());
QString genre = getQString(tag->genre());
setGenre(genre);

int iYear = tag->year();
Expand Down Expand Up @@ -322,19 +330,19 @@ bool SoundSource::processID3v2Tag(TagLib::ID3v2::Tag* id3v2) {
TagLib::ID3v2::FrameList::ConstIterator it = id3v2->frameList().begin();
for(; it != id3v2->frameList().end(); it++) {
qDebug() << "ID3V2" << (*it)->frameID().data() << "-"
<< TStringToQString((*it)->toString());
<< getQString((*it)->toString());
}
}

TagLib::ID3v2::FrameList bpmFrame = id3v2->frameListMap()["TBPM"];
if (!bpmFrame.isEmpty()) {
QString sBpm = TStringToQString(bpmFrame.front()->toString());
QString sBpm = getQString(bpmFrame.front()->toString());
processBpmString("ID3v2", sBpm);
}

TagLib::ID3v2::FrameList keyFrame = id3v2->frameListMap()["TKEY"];
if (!keyFrame.isEmpty()) {
QString sKey = TStringToQString(keyFrame.front()->toString());
QString sKey = getQString(keyFrame.front()->toString());
setKey(sKey);
}
// Foobar2000-style ID3v2.3.0 tags
Expand All @@ -345,31 +353,31 @@ bool SoundSource::processID3v2Tag(TagLib::ID3v2::Tag* id3v2) {
dynamic_cast<TagLib::ID3v2::UserTextIdentificationFrame*>( *it );
if ( ReplayGainframe && ReplayGainframe->fieldList().size() >= 2 )
{
QString desc = TStringToQString( ReplayGainframe->description() ).toLower();
QString desc = getQString( ReplayGainframe->description() ).toLower();
if ( desc == "replaygain_album_gain" ){
QString sReplayGain = TStringToQString( ReplayGainframe->fieldList()[1]);
QString sReplayGain = getQString( ReplayGainframe->fieldList()[1]);
parseReplayGainString(sReplayGain);
}
if ( desc == "replaygain_track_gain" ){
QString sReplayGain = TStringToQString( ReplayGainframe->fieldList()[1]);
QString sReplayGain = getQString( ReplayGainframe->fieldList()[1]);
parseReplayGainString(sReplayGain);
}
}
}

TagLib::ID3v2::FrameList albumArtistFrame = id3v2->frameListMap()["TPE2"];
if (!albumArtistFrame.isEmpty()) {
QString sAlbumArtist = TStringToQString(albumArtistFrame.front()->toString());
QString sAlbumArtist = getQString(albumArtistFrame.front()->toString());
setAlbumArtist(sAlbumArtist);
}
TagLib::ID3v2::FrameList composerFrame = id3v2->frameListMap()["TCOM"];
if (!composerFrame.isEmpty()) {
QString sComposer = TStringToQString(composerFrame.front()->toString());
QString sComposer = getQString(composerFrame.front()->toString());
setComposer(sComposer);
}
TagLib::ID3v2::FrameList groupingFrame = id3v2->frameListMap()["TIT1"];
if (!groupingFrame.isEmpty()) {
QString sGrouping = TStringToQString(groupingFrame.front()->toString());
QString sGrouping = getQString(groupingFrame.front()->toString());
setGrouping(sGrouping);
}

Expand All @@ -392,7 +400,7 @@ bool SoundSource::processAPETag(TagLib::APE::Tag* ape) {
if (s_bDebugMetadata) {
for(TagLib::APE::ItemListMap::ConstIterator it = ape->itemListMap().begin();
it != ape->itemListMap().end(); ++it) {
qDebug() << "APE" << TStringToQString((*it).first) << "-" << TStringToQString((*it).second.toString());
qDebug() << "APE" << getQString((*it).first) << "-" << getQString((*it).second.toString());
}
}

Expand All @@ -412,31 +420,31 @@ bool SoundSource::processAPETag(TagLib::APE::Tag* ape) {
}

if (ape->itemListMap().contains("BPM")) {
QString sBpm = TStringToQString(ape->itemListMap()["BPM"].toString());
QString sBpm = getQString(ape->itemListMap()["BPM"].toString());
processBpmString("APE", sBpm);
}

if (ape->itemListMap().contains("REPLAYGAIN_ALBUM_GAIN")) {
QString sReplayGain = TStringToQString(ape->itemListMap()["REPLAYGAIN_ALBUM_GAIN"].toString());
QString sReplayGain = getQString(ape->itemListMap()["REPLAYGAIN_ALBUM_GAIN"].toString());
parseReplayGainString(sReplayGain);
}

//Prefer track gain over album gain.
if (ape->itemListMap().contains("REPLAYGAIN_TRACK_GAIN")) {
QString sReplayGain = TStringToQString(ape->itemListMap()["REPLAYGAIN_TRACK_GAIN"].toString());
QString sReplayGain = getQString(ape->itemListMap()["REPLAYGAIN_TRACK_GAIN"].toString());
parseReplayGainString(sReplayGain);
}

if (ape->itemListMap().contains("Album Artist")) {
m_sAlbumArtist = TStringToQString(ape->itemListMap()["Album Artist"].toString());
m_sAlbumArtist = getQString(ape->itemListMap()["Album Artist"].toString());
}

if (ape->itemListMap().contains("Composer")) {
m_sComposer = TStringToQString(ape->itemListMap()["Composer"].toString());
m_sComposer = getQString(ape->itemListMap()["Composer"].toString());
}

if (ape->itemListMap().contains("Grouping")) {
m_sGrouping = TStringToQString(ape->itemListMap()["Grouping"].toString());
m_sGrouping = getQString(ape->itemListMap()["Grouping"].toString());
}

return true;
Expand All @@ -446,7 +454,7 @@ bool SoundSource::processXiphComment(TagLib::Ogg::XiphComment* xiph) {
if (s_bDebugMetadata) {
for (TagLib::Ogg::FieldListMap::ConstIterator it = xiph->fieldListMap().begin();
it != xiph->fieldListMap().end(); ++it) {
qDebug() << "XIPH" << TStringToQString((*it).first) << "-" << TStringToQString((*it).second.toString());
qDebug() << "XIPH" << getQString((*it).first) << "-" << getQString((*it).second.toString());
}
}

Expand All @@ -463,26 +471,26 @@ bool SoundSource::processXiphComment(TagLib::Ogg::XiphComment* xiph) {
// Some tags use "BPM" so check for that.
if (xiph->fieldListMap().contains("BPM")) {
TagLib::StringList bpmString = xiph->fieldListMap()["BPM"];
QString sBpm = TStringToQString(bpmString.toString());
QString sBpm = getQString(bpmString.toString());
processBpmString("XIPH-BPM", sBpm);
}

// Give preference to the "TEMPO" tag which seems to be more standard
if (xiph->fieldListMap().contains("TEMPO")) {
TagLib::StringList bpmString = xiph->fieldListMap()["TEMPO"];
QString sBpm = TStringToQString(bpmString.toString());
QString sBpm = getQString(bpmString.toString());
processBpmString("XIPH-TEMPO", sBpm);
}

if (xiph->fieldListMap().contains("REPLAYGAIN_ALBUM_GAIN")) {
TagLib::StringList rgainString = xiph->fieldListMap()["REPLAYGAIN_ALBUM_GAIN"];
QString sReplayGain = TStringToQString(rgainString.toString());
QString sReplayGain = getQString(rgainString.toString());
parseReplayGainString(sReplayGain);
}

if (xiph->fieldListMap().contains("REPLAYGAIN_TRACK_GAIN")) {
TagLib::StringList rgainString = xiph->fieldListMap()["REPLAYGAIN_TRACK_GAIN"];
QString sReplayGain = TStringToQString(rgainString.toString());
QString sReplayGain = getQString(rgainString.toString());
parseReplayGainString(sReplayGain);
}

Expand All @@ -496,34 +504,34 @@ bool SoundSource::processXiphComment(TagLib::Ogg::XiphComment* xiph) {
*/
if (xiph->fieldListMap().contains("KEY")) {
TagLib::StringList keyStr = xiph->fieldListMap()["KEY"];
QString key = TStringToQString(keyStr.toString());
QString key = getQString(keyStr.toString());
setKey(key);
}
if (getKey().isEmpty() && xiph->fieldListMap().contains("INITIALKEY")) {
TagLib::StringList keyStr = xiph->fieldListMap()["INITIALKEY"];
QString key = TStringToQString(keyStr.toString());
QString key = getQString(keyStr.toString());
setKey(key);
}

if (xiph->fieldListMap().contains("ALBUMARTIST")) {
TagLib::StringList albumArtistString = xiph->fieldListMap()["ALBUMARTIST"];
m_sAlbumArtist = TStringToQString(albumArtistString.toString());
m_sAlbumArtist = getQString(albumArtistString.toString());
} else {
// try alternative field name
if (xiph->fieldListMap().contains("ALBUM_ARTIST")) {
TagLib::StringList albumArtistString = xiph->fieldListMap()["ALBUM_ARTIST"];
m_sAlbumArtist = TStringToQString(albumArtistString.toString());
m_sAlbumArtist = getQString(albumArtistString.toString());
}
}

if (xiph->fieldListMap().contains("COMPOSER")) {
TagLib::StringList composerString = xiph->fieldListMap()["COMPOSER"];
m_sComposer = TStringToQString(composerString.toString());
m_sComposer = getQString(composerString.toString());
}

if (xiph->fieldListMap().contains("GROUPING")) {
TagLib::StringList groupingString = xiph->fieldListMap()["GROUPING"];
m_sGrouping = TStringToQString(groupingString.toString());
m_sGrouping = getQString(groupingString.toString());
}

return true;
Expand All @@ -533,8 +541,8 @@ bool SoundSource::processMP4Tag(TagLib::MP4::Tag* mp4) {
if (s_bDebugMetadata) {
for(TagLib::MP4::ItemListMap::ConstIterator it = mp4->itemListMap().begin();
it != mp4->itemListMap().end(); ++it) {
qDebug() << "MP4" << TStringToQString((*it).first) << "-"
<< TStringToQString((*it).second.toStringList().toString());
qDebug() << "MP4" << getQString((*it).first) << "-"
<< getQString((*it).second.toStringList().toString());
}
}

Expand All @@ -551,20 +559,20 @@ bool SoundSource::processMP4Tag(TagLib::MP4::Tag* mp4) {

// Get BPM
if (mp4->itemListMap().contains("tmpo")) {
QString sBpm = TStringToQString(
QString sBpm = getQString(
mp4->itemListMap()["tmpo"].toStringList().toString());
processBpmString("MP4", sBpm);
} else if (mp4->itemListMap().contains("----:com.apple.iTunes:BPM")) {
// This is an alternate way of storing BPM.
QString sBpm = TStringToQString(mp4->itemListMap()[
QString sBpm = getQString(mp4->itemListMap()[
"----:com.apple.iTunes:BPM"].toStringList().toString());
processBpmString("MP4", sBpm);
}

// Get Album Artist
if (mp4->itemListMap().contains("aART")) {
// this is technically a list of values -> concatenate into single string
QString albumArtist = TStringToQString(
QString albumArtist = getQString(
mp4->itemListMap()["aART"].toStringList().toString());
setAlbumArtist(albumArtist);
}
Expand All @@ -573,21 +581,21 @@ bool SoundSource::processMP4Tag(TagLib::MP4::Tag* mp4) {
if (mp4->itemListMap().contains("\251wrt")) {
// rryan 1/2012 I believe this is technically a list of composers. We
// don't support multiple composers in Mixxx, so just use them joined.
QString composer = TStringToQString(
QString composer = getQString(
mp4->itemListMap()["\251wrt"].toStringList().toString());
setComposer(composer);
}

// Get Grouping
if (mp4->itemListMap().contains("\251grp")) {
QString grouping = TStringToQString(
QString grouping = getQString(
mp4->itemListMap()["\251grp"].toStringList().toString());
setGrouping(grouping);
}

// Get KEY (conforms to Rapid Evolution)
if (mp4->itemListMap().contains("----:com.apple.iTunes:KEY")) {
QString key = TStringToQString(
QString key = getQString(
mp4->itemListMap()["----:com.apple.iTunes:KEY"].toStringList().toString());
setKey(key);
}
Expand All @@ -598,7 +606,7 @@ bool SoundSource::processMP4Tag(TagLib::MP4::Tag* mp4) {
// TODO(XXX) find tracks with this property and check what it looks
// like.

//QString replaygain = TStringToQString(mp4->itemListMap()["----:com.apple.iTunes:replaygain_track_gain"].toStringList().toString());
//QString replaygain = getQString(mp4->itemListMap()["----:com.apple.iTunes:replaygain_track_gain"].toStringList().toString());
}

return true;
Expand Down
4 changes: 3 additions & 1 deletion src/soundsource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ class SoundSource
virtual void setSampleRate(unsigned int);
virtual void setChannels(int);
virtual void setCoverArt(QImage);
protected:

protected:
// Automatically collects generic data from a TagLib File: title, artist,
// album, comment, genre, year, tracknumber, duration, bitrate, samplerate,
// and channels.
Expand All @@ -125,6 +125,8 @@ class SoundSource
void processBpmString(QString tagName, QString sBpm);
void parseReplayGainString(QString sReplayGain);

QString getQString(TagLib::String TString);

This comment has been minimized.

Sign in to view

Copy link
@ywwg

ywwg Jul 22, 2014

maybe call it "toQString". mark function const. Also can you make the parameter lowercase, "tstring"? And please add a comment here why this function is needed, like: "// Taglib strings can be null, so create an empty QString if so"

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot Jul 22, 2014

Author Owner

nice points! I'll do that!
Thanks =D

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot Jul 22, 2014

Author Owner

Done!

This comment has been minimized.

Sign in to view

Copy link
@ywwg

ywwg Jul 22, 2014

I can't see the changes, maybe create a pull request?

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot Jul 22, 2014

Author Owner

Oh, it is because you are commenting direct on a specif commit...
It is fixed here:
c0f83a6
It's part of the CoverArt PR. However, as it affect directly 1.12 and the CoverArt project will be merged just on 1.13... I could do a new PR with this discussion (would you like it?)

This comment has been minimized.

Sign in to view

Copy link
@kain88-de

kain88-de Jul 22, 2014

yes please. Changes like this should go into 1.12 and pushing the fix into upstream early will reduce the scop of the cover art PR

This comment has been minimized.

Sign in to view

Copy link
@cardinot

cardinot Jul 22, 2014

Author Owner

Done! mixxxdj#296

/** File name */
QString m_qFilename;

Expand Down

0 comments on commit 76ebcd4

Please sign in to comment.