Differential D16671 Diff 44868 src/embeddedimagedata.cpp

Changeset View

Standalone View

View Options

src/embeddedimagedata.cpp

Show All 39 Lines
40 40​
41using namespace KFileMetaData; 41​using namespace KFileMetaData;
42 42​
43class Q_DECL_HIDDEN EmbeddedImageData::Private 43​class Q_DECL_HIDDEN EmbeddedImageData::Private
44{ 44​{
45public: 45​public:
46 QMimeDatabase mMimeDatabase; 46​ QMimeDatabase mMimeDatabase;
47 QByteArray getFrontCover(const QString &fileUrl, const QString &mimeType) const; 47​ QByteArray getFrontCover(const QString &fileUrl, const QString &mimeType) const;
48​ QByteArray getFrontCoverFromID3(TagLib::ID3v2::Tag* Id3Tags) const;
49​ QByteArray getFrontCoverFromFlacPicture(TagLib::List<TagLib::FLAC::Picture *> lstPic) const;
50​ QByteArray getFrontCoverFromMp4(TagLib::MP4::Tag* mp4Tags) const;
51​ QByteArray getFrontCoverFromApe(TagLib::APE::Tag* apeTags) const;
48 static const QStringList mMimetypes; 52​ static const QStringList mMimetypes;
49}; 53​};
50 54​
51const QStringList EmbeddedImageData::Private::mMimetypes = 55​const QStringList EmbeddedImageData::Private::mMimetypes =
52{ 56​{
53 QStringLiteral("audio/flac"), 57​ QStringLiteral("audio/flac"),
54 QStringLiteral("audio/mp4"), 58​ QStringLiteral("audio/mp4"),
55 QStringLiteral("audio/mpeg"), 59​ QStringLiteral("audio/mpeg"),
▲ Show 20 Lines • Show All 45 Lines • ▼ Show 20 Line(s)103 if (!stream.isOpen()) {
101 return QByteArray(); 105​ return QByteArray();
102 } 106​ }
103 if ((mimeType == QLatin1String("audio/mpeg")) 107​ if ((mimeType == QLatin1String("audio/mpeg"))
104 || (mimeType == QLatin1String("audio/mpeg3")) 108​ || (mimeType == QLatin1String("audio/mpeg3"))
105 || (mimeType == QLatin1String("audio/x-mpeg"))) { 109​ || (mimeType == QLatin1String("audio/x-mpeg"))) {
106 110​
107 // Handling multiple tags in mpeg files. 111​ // Handling multiple tags in mpeg files.
108 TagLib::MPEG::File mpegFile(&stream, TagLib::ID3v2::FrameFactory::instance(), true); 112​ TagLib::MPEG::File mpegFile(&stream, TagLib::ID3v2::FrameFactory::instance(), true);
109 if (!mpegFile.ID3v2Tag() || mpegFile.ID3v2Tag()->isEmpty()) { 113​ if (mpegFile.ID3v2Tag()) {
110 return QByteArray(); 114​ return getFrontCoverFromID3(mpegFile.ID3v2Tag());
111 } ​
112 ​
113 TagLib::ID3v2::FrameList lstID3v2; ​
114 // Attached Front Picture. ​
115 lstID3v2 = mpegFile.ID3v2Tag()->frameListMap()["APIC"]; ​
116 if (!lstID3v2.isEmpty()) { ​
117 for (TagLib::ID3v2::FrameList::ConstIterator it = lstID3v2.begin(); it != lstID3v2.end(); ++it) { ​
118 auto *frontCoverFrame = static_cast<TagLib::ID3v2::AttachedPictureFrame *>(*it); ​
119 if (frontCoverFrame->type() == frontCoverFrame->FrontCover) { ​
120 return QByteArray(frontCoverFrame->picture().data(), frontCoverFrame->picture().size()); ​
121 } ​
122 } ​
123 } 115​ }
124 116​
125 } else if (mimeType == QLatin1String("audio/mp4")) { 117​ } else if (mimeType == QLatin1String("audio/mp4")) {
126 118​
127 TagLib::MP4::File mp4File(&stream, true); 119​ TagLib::MP4::File mp4File(&stream, true);
128 if (!mp4File.tag() || mp4File.tag()->isEmpty()) { 120​ if (mp4File.tag()) {
129 return QByteArray(); 121​ return getFrontCoverFromMp4(mp4File.tag());
130 } ​
131 TagLib::MP4::Item coverArtItem = mp4File.tag()->item("covr"); ​
132 if (coverArtItem.isValid()) ​
133 { ​
134 TagLib::MP4::CoverArtList coverArtList = coverArtItem.toCoverArtList(); ​
135 TagLib::MP4::CoverArt& frontCover = coverArtList.front(); ​
136 return QByteArray(frontCover.data().data(), frontCover.data().size()); ​
137 } 122​ }
138 123​
139 } else if (mimeType == QLatin1String("audio/x-musepack")) { 124​ } else if (mimeType == QLatin1String("audio/x-musepack")) {
140 125​
141 TagLib::MPC::File mpcFile(&stream, true); 126​ TagLib::MPC::File mpcFile(&stream, true);
142 if (!mpcFile.tag() || mpcFile.tag()->isEmpty()) { 127​ if (mpcFile.APETag()) {
143 return QByteArray(); 128​ return getFrontCoverFromApe(mpcFile.APETag());
144 } ​
145 ​
146 TagLib::APE::ItemListMap lstMusepack = mpcFile.APETag()->itemListMap(); ​
147 TagLib::APE::ItemListMap::ConstIterator itMPC; ​
148 ​
149 /* The cover art tag for APEv2 tags starts with the filename as string ​
150 * with zero termination followed by the actual picture data */ ​
151 itMPC = lstMusepack.find("COVER ART (FRONT)"); ​
152 if (itMPC != lstMusepack.end()) { ​
153 TagLib::ByteVector pictureData = (*itMPC).second.binaryData(); ​
154 int dataPosition = pictureData.find('\0') + 1; ​
155 return QByteArray(pictureData.data() + dataPosition, pictureData.size() - dataPosition); ​
156 } 129​ }
157 130​
158 } else if (mimeType == QLatin1String("audio/flac")) { 131​ } else if (mimeType == QLatin1String("audio/flac")) {
159 132​
160 TagLib::FLAC::File flacFile(&stream, TagLib::ID3v2::FrameFactory::instance(), true); 133​ TagLib::FLAC::File flacFile(&stream, TagLib::ID3v2::FrameFactory::instance(), true);
161 TagLib::List<TagLib::FLAC::Picture *> lstPic = flacFile.pictureList(); 134​ return getFrontCoverFromFlacPicture(flacFile.pictureList());
brunsUnsubmitted
Not Done

Does Taglib signal an error when it fails to parse the file? Or is calling pictureList() always safe?

bruns: Does Taglib signal an error when it fails to parse the file? Or is calling pictureList() always…
astippichAuthorUnsubmitted
Not Done

Docs don't say, so I fed a musepack file into this codepath and it still ran without issues (no cover extracted of course). Good enough?

astippich: Docs don't say, so I fed a musepack file into this codepath and it still ran without issues (no…
brunsUnsubmitted
Not Done

Yes, thanks.

bruns: Yes, thanks.
162 ​
163 if (!lstPic.isEmpty()) { ​
164 for (TagLib::List<TagLib::FLAC::Picture *>::Iterator it = lstPic.begin(); it != lstPic.end(); ++it) { ​
165 TagLib::FLAC::Picture *picture = *it; ​
166 if (picture->type() == picture->FrontCover) { ​
167 return QByteArray(picture->data().data(), picture->data().size()); ​
168 } ​
169 } ​
170 } ​
171 135​
172 } else { 136​ } else {
173 137​
174 TagLib::List<TagLib::FLAC::Picture *> lstPic; 138​ TagLib::List<TagLib::FLAC::Picture *> lstPic;
175 if ((mimeType == QLatin1String("audio/ogg")) 139​ if ((mimeType == QLatin1String("audio/ogg"))
176 || (mimeType == QLatin1String("audio/x-vorbis+ogg"))) { 140​ || (mimeType == QLatin1String("audio/x-vorbis+ogg"))) {
177 TagLib::Ogg::Vorbis::File oggFile(&stream, true); 141​ TagLib::Ogg::Vorbis::File oggFile(&stream, true);
178 if (oggFile.tag() && !oggFile.tag()->isEmpty()) { 142​ if (oggFile.tag()) {
179 lstPic = oggFile.tag()->pictureList(); 143​ return getFrontCoverFromFlacPicture(oggFile.tag()->pictureList());
180 } 144​ }
181 } 145​ }
brunsUnsubmitted
Not Done

Not sure if this is an issue, but the old code only called tag()->pictureList() after !tag()->isEmpty() - is this safe without?
Dito for oggFile above.

bruns: Not sure if this is an issue, but the old code only called `tag()->pictureList()` after `!tag()…
astippichAuthorUnsubmitted
Not Done

Yes, the list will be empty.

astippich: Yes, the list will be empty.
182 if ((mimeType == QLatin1String("audio/opus")) 146​ if ((mimeType == QLatin1String("audio/opus"))
183 || (mimeType == QLatin1String("audio/x-opus+ogg"))) { 147​ || (mimeType == QLatin1String("audio/x-opus+ogg"))) {
184 TagLib::Ogg::Opus::File opusFile(&stream, true); 148​ TagLib::Ogg::Opus::File opusFile(&stream, true);
185 if (opusFile.tag() && !opusFile.tag()->isEmpty()) { 149​ if (opusFile.tag()) {
186 lstPic = opusFile.tag()->pictureList(); 150​ return getFrontCoverFromFlacPicture(opusFile.tag()->pictureList());
151​ }
152​ }
153​ }
154​ return QByteArray();
187 } 155​}
156​
157​QByteArray
158​EmbeddedImageData::Private::getFrontCoverFromID3(TagLib::ID3v2::Tag* Id3Tags) const
159​{
160​ TagLib::ID3v2::FrameList lstID3v2;
161​ // Attached Front Picture.
162​ lstID3v2 = Id3Tags->frameListMap()["APIC"];
163​ for (const auto& frame : qAsConst(lstID3v2))
164​ {
165​ const auto *frontCoverFrame = static_cast<TagLib::ID3v2::AttachedPictureFrame *>(frame);
166​ if (frontCoverFrame->type() == frontCoverFrame->FrontCover) {
167​ return QByteArray(frontCoverFrame->picture().data(), frontCoverFrame->picture().size());
188 } 168​ }
189 if (!lstPic.isEmpty()) { 169​ }
190 for (TagLib::List<TagLib::FLAC::Picture *>::Iterator it = lstPic.begin(); it != lstPic.end(); ++it) { 170​ return QByteArray();
191 TagLib::FLAC::Picture *picture = *it; 171​}
172​
brunsUnsubmitted
Done

Nitpick - add an empty line in-between functions

bruns: Nitpick - add an empty line in-between functions
173​QByteArray
174​EmbeddedImageData::Private::getFrontCoverFromFlacPicture(TagLib::List<TagLib::FLAC::Picture *> lstPic) const
175​{
176​ for (const auto &picture : qAsConst(lstPic)) {
192 if (picture->type() == picture->FrontCover) { 177​ if (picture->type() == picture->FrontCover) {
193 return QByteArray(picture->data().data(), picture->data().size()); 178​ return QByteArray(picture->data().data(), picture->data().size());
194 } 179​ }
195 } 180​ }
181​ return QByteArray();
196 } 182​}
brunsUnsubmitted
Not Done

You can check for >= 0 and let the -1 case fall through ...

bruns: You can check for `>= 0` and let the `-1` case fall through ...
183​QByteArray
184​EmbeddedImageData::Private::getFrontCoverFromMp4(TagLib::MP4::Tag* mp4Tags) const
185​{
186​ TagLib::MP4::Item coverArtItem = mp4Tags->item("covr");
187​ if (coverArtItem.isValid())
188​ {
189​ TagLib::MP4::CoverArtList coverArtList = coverArtItem.toCoverArtList();
190​ TagLib::MP4::CoverArt& frontCover = coverArtList.front();
191​ return QByteArray(frontCover.data().data(), frontCover.data().size());
192​ }
193​ return QByteArray();
194​}
195​QByteArray
brunsUnsubmitted
Not Done

I know you have only moved this code here ...

TagLib::ByteVector::find returns -1 on not found, and we add 1, so this is safe here. But maybe we should return QByteArray() instead?

bruns: I know you have only moved this code here ... `TagLib::ByteVector::find` returns `-1` on not…
astippichAuthorUnsubmitted
Not Done

The code also works if only the picture data is there without the name.
I could return an empty QByteArray if dataPosition == -1 and pictureData.size == 0.
Do you prefer if I do that separately?

astippich: The code also works if only the picture data is there without the name. I could return an empty…
brunsUnsubmitted
Done

Is it safe to call find on an empty ByteVector - most likely yes ...
Then return QByteArray on dataPosition == -1, so it is obvious the error case is handled, and keep the rest as is.

bruns: Is it safe to call find on an empty ByteVector - most likely yes ... Then return QByteArray on…
196​EmbeddedImageData::Private::getFrontCoverFromApe(TagLib::APE::Tag* apeTags) const
197​{
198​ TagLib::APE::ItemListMap lstApe = apeTags->itemListMap();
199​ TagLib::APE::ItemListMap::ConstIterator itApe;
200​
201​ /* The cover art tag for APEv2 tags starts with the filename as string
202​ * with zero termination followed by the actual picture data */
203​ itApe = lstApe.find("COVER ART (FRONT)");
204​ if (itApe != lstApe.end()) {
205​ TagLib::ByteVector pictureData = (*itApe).second.binaryData();
206​ int dataPosition = pictureData.find('\0') + 1;
207​ return QByteArray(pictureData.data() + dataPosition, pictureData.size() - dataPosition);
197 } 208​ }
198 return QByteArray(); 209​ return QByteArray();
199} 210​}