[Amarok] a0c4a6c: Add embedded cover art support
Bart Cerneels
bart.cerneels at kde.org
Fri Feb 26 20:33:44 CET 2010
Looking at the commit at first glance I would say Dan is mostly right.
Technically this patch still needs a lot of work, if not outright
redesign. I'm thinking generic artwork providers that can be separate
from any collection negating the need to link taglib to sqlmeta.
I'm afraid markey, you merged this way to quick. We're past feature
freeze and adding completely new stuff can surely wait 2 weeks. Those
people waiting on embedded cover art support can hold out until 2.3.1
for sure. Don;t mistake a few vocal commenters for our complete
userbase.
Dan: what makes you think loading image data from an APIC frame is
that much slower then loading it from a jpeg-file on the same disk?
Consider the hot cache effect on recently touched mp3's as well.
On Fri, Feb 26, 2010 at 18:39, Dan Meltzer <parallelgrapefruit at gmail.com> wrote:
> The more I think about this, the more I object to it's being merged.
> Not only does it have some blatently obvious (and therefore easily
> fixable) problems, it also has a few questions that are currently in
> discussion about it's implementation. In addition, we are deep into
> feature freeze right now. This is not going to get the testing it
> deserves (and requires) to ensure we are releasing a quality product.
> This should have waited to be merged until 2.3.1. We've seen enough
> regressions in image handling code in the past to not be afraid that
> this might do something unexpected, and the TRUNK testers are not a
> wide enough audience to give us the feedback we need. I strongly feel
> this needs to be reverted until after the 2.3.0 release. People have
> waited for it for a few years now, a few more months won't make a big
> difference.
>
> On Fri, Feb 26, 2010 at 12:33 AM, ecroy <ecroy at gmx.net> wrote:
>> commit a0c4a6c706d6f4f6cf22740bfa4aec768a7c4b87
>> Author: ecroy <ecroy at gmx.net>
>> Date: Fri Feb 26 01:41:55 2010 +0100
>>
>> Add embedded cover art support
>>
>> diff --git a/src/collection/sqlcollection/ScanManager.cpp b/src/collection/sqlcollection/ScanManager.cpp
>> index a21c6e5..8fb4a3f 100644
>> --- a/src/collection/sqlcollection/ScanManager.cpp
>> +++ b/src/collection/sqlcollection/ScanManager.cpp
>> @@ -799,6 +799,15 @@ XmlParseJob::run()
>> directoryData.append( data );
>> currentDir = url.directory();
>> }
>> +
>> + //the boolean attribute 'apic' indicates that the file contains an embedded cover-art image
>> + //we therefore add the path of the audio file to the list of images for this album
>> + if( !attrs.value( "apic" ).isEmpty() )
>> + {
>> + QList<QPair<QString, QString> > covers;
>> + covers += qMakePair( attrs.value( "artist" ).toString(), attrs.value( "album" ).toString() );
>> + processor.addImage( attrs.value( "path" ).toString(), covers );
>> + }
>> }
>> else if( localname == "folder" )
>> {
>> diff --git a/src/collection/sqlcollection/ScanResultProcessor.cpp b/src/collection/sqlcollection/ScanResultProcessor.cpp
>> index ce12d48..b9bd7b6 100644
>> --- a/src/collection/sqlcollection/ScanResultProcessor.cpp
>> +++ b/src/collection/sqlcollection/ScanResultProcessor.cpp
>> @@ -188,6 +188,10 @@ ScanResultProcessor::findBestImagePath( const QList<QString> &paths )
>> QString goodPath;
>> foreach( const QString &path, paths )
>> {
>> + // skip embedded images
>> + if( SqlAlbum::isEmbeddedImage( path ) )
>> + continue;
>> +
>> QString file = QFileInfo( path ).fileName();
>>
>> //prioritize "front"
>> @@ -234,21 +238,26 @@ ScanResultProcessor::findBestImagePath( const QList<QString> &paths )
>> if( !goodPath.isEmpty() )
>> return goodPath;
>>
>> - //finally: pick largest image -- often a high-quality blowup of the front
>> + //next: pick largest non-embedded image -- often a high-quality blowup of the front
>> //so that people can print it out
>> - qint64 size = 0;
>> + qint64 size = -1;
>> QString current;
>> foreach( const QString &path, paths )
>> {
>> QFileInfo info( path );
>> - if( info.size() > size )
>> + if( info.size() > size &&
>> + ! SqlAlbum::isEmbeddedImage( path ) )
>> {
>> size = info.size();
>> current = path;
>> }
>> }
>> - return current;
>>
>> + //finally: if all available images are embedded, simply pick the first one
>> + if( size == -1 )
>> + return paths.first();
>> +
>> + return current;
>> }
>>
>> void
>> diff --git a/src/collection/sqlcollection/SqlMeta.cpp b/src/collection/sqlcollection/SqlMeta.cpp
>> index dacce04..96b9d44 100644
>> --- a/src/collection/sqlcollection/SqlMeta.cpp
>> +++ b/src/collection/sqlcollection/SqlMeta.cpp
>> @@ -46,6 +46,11 @@
>> #include <KLocale>
>> #include <KSharedPtr>
>>
>> +//Taglib:
>> +#include <mpegfile.h>
>> +#include <id3v2tag.h>
>> +#include <attachedpictureframe.h>
>> +
>> using namespace Meta;
>>
>> QString
>> @@ -1113,7 +1118,12 @@ SqlAlbum::image( int size )
>>
>> //FIXME this cache doesn't differentiate between shadowed/unshadowed
>> if( m_images.contains( size ) )
>> - return QPixmap( m_images.value( size ) );
>> + {
>> + if (isEmbeddedImage( m_images.value( size ) ) )
>> + return QPixmap::fromImage( loadImageFromTag( m_images.value( size ) ) );
>> + else
>> + return QPixmap( m_images.value( size ) );
>> + }
>>
>> QString result;
>>
>> @@ -1152,7 +1162,10 @@ SqlAlbum::image( int size )
>> {
>> m_hasImage = true;
>> m_images.insert( size, result );
>> - return QPixmap( result );
>> + if (isEmbeddedImage( m_images.value( size ) ) )
>> + return QPixmap::fromImage( loadImageFromTag( result ) );
>> + else
>> + return QPixmap( result );
>> }
>>
>> // Cover fetching runs in another thread. If there is a retrieved cover
>> @@ -1411,8 +1424,13 @@ SqlAlbum::createScaledImage( QString path, int size ) const
>> // Don't overwrite if it already exists
>> if( !QFile::exists( cachedImagePath ) )
>> {
>> - QImage img( path );
>> - if( img.isNull() )
>> + QImage img;
>> + if ( isEmbeddedImage( path ) )
>> + img = loadImageFromTag( path );
>> + else
>> + img.load( path );
>> +
>> + if( img.isNull() )
>> return QString();
>>
>> // resize and save the image
>> @@ -1424,6 +1442,49 @@ SqlAlbum::createScaledImage( QString path, int size ) const
>> return QString();
>> }
>>
>> +QImage
>> +SqlAlbum::loadImageFromTag( const QString path ) const
>> +{
>> +#ifdef COMPLEX_TAGLIB_FILENAME
>> + const wchar_t * encodedName = reinterpret_cast<const wchar_t *>(path.utf16());
>> +#else
>> + QByteArray fileName = QFile::encodeName( path );
>> + const char * encodedName = fileName.constData(); // valid as long as fileName exists
>> +#endif
>> +
>> + TagLib::FileRef fileref;
>> + TagLib::Tag *tag = 0;
>> + fileref = TagLib::FileRef( encodedName, true );
>> +
>> + if( !fileref.isNull() )
>> + {
>> + tag = fileref.tag();
>> + if ( tag )
>> + {
>> + if ( TagLib::MPEG::File *file = dynamic_cast<TagLib::MPEG::File *>( fileref.file() ) )
>> + {
>> + if ( !file->ID3v2Tag()->frameListMap()["APIC"].isEmpty() )
>> + {
>> + TagLib::ID3v2::FrameList apicList = file->ID3v2Tag()->frameListMap()["APIC"];
>> + for ( TagLib::ID3v2::FrameList::ConstIterator it = apicList.begin(), end = apicList.end(); it != end; ++it )
>> + {
>> + TagLib::ID3v2::AttachedPictureFrame *apicFrame = (TagLib::ID3v2::AttachedPictureFrame *)(*it);
>> + // take first APIC frame which is a FrontCover
>> + if ( apicFrame->type() == TagLib::ID3v2::AttachedPictureFrame::FrontCover)
>> + {
>> + return QImage::fromData((uchar*)(apicFrame->picture().data()), apicFrame->picture().size());
>> + }
>> + }
>> + // if there was no FrontCover, just take the first picture available
>> + TagLib::ID3v2::AttachedPictureFrame *apicFrame = (TagLib::ID3v2::AttachedPictureFrame *)(apicList[0]);
>> + return QImage::fromData((uchar*)(apicFrame->picture().data()), apicFrame->picture().size());
>> + }
>> + }
>> + }
>> + }
>> + return QImage::QImage();
>> +}
>> +
>> QString
>> SqlAlbum::findImage( int size )
>> {
>> @@ -1673,6 +1734,20 @@ SqlAlbum::createCapabilityInterface( Meta::Capability::Type type )
>> return ( m_delegate ? m_delegate->createCapabilityInterface( type, this ) : 0 );
>> }
>>
>> +bool
>> +SqlAlbum::isEmbeddedImage( const QString& path )
>> +{
>> + //Entries in the images table are currently either image files or audio files with embedded cover-art images.
>> + //This function simply checks the file extension and determines whether it is an audio file or not, using the
>> + //same suffix criteria as in ScanResultProcessor::addTrack for now.
>> + //Future versions could extend the database scheme to include image type information.
>> + //No additional checks are made - we trust that the entry was put into the images table for a reason.
>> +
>> + QStringList audioFiletypes;
>> + audioFiletypes << "mp3" << "ogg" << "oga" << "flac" << "wma" << "m4a" << "m4b";
>> + return audioFiletypes.contains( QFileInfo( path ).suffix().toLower() );
>> +}
>> +
>> //---------------SqlComposer---------------------------------
>>
>> SqlComposer::SqlComposer( SqlCollection* collection, int id, const QString &name ) : Composer()
>> @@ -1838,4 +1913,3 @@ SqlYear::tracks()
>> return TrackList();
>> }
>>
>> -
>> diff --git a/src/collection/sqlcollection/SqlMeta.h b/src/collection/sqlcollection/SqlMeta.h
>> index 9c4d9cd..82edd9f 100644
>> --- a/src/collection/sqlcollection/SqlMeta.h
>> +++ b/src/collection/sqlcollection/SqlMeta.h
>> @@ -292,6 +292,8 @@ class SqlAlbum : public Meta::Album
>>
>> virtual Meta::Capability* createCapabilityInterface( Meta::Capability::Type type );
>>
>> + static bool isEmbeddedImage( const QString& path );
>> +
>> //SQL specific methods
>> int id() const { return m_id; }
>>
>> @@ -309,6 +311,7 @@ class SqlAlbum : public Meta::Album
>> void updateImage( const QString path ) const; // Updates the database to ensure the album has the correct path
>> // Finds or creates a magic value in the database which tells Amarok not to auto fetch an image since it has been explicitly unset.
>> int unsetImageId() const;
>> + QImage loadImageFromTag( const QString path ) const;
>>
>> private:
>> SqlCollection* m_collection;
>> diff --git a/utilities/collectionscanner/CollectionScanner.cpp b/utilities/collectionscanner/CollectionScanner.cpp
>> index 9299ccf..5488b1e 100644
>> --- a/utilities/collectionscanner/CollectionScanner.cpp
>> +++ b/utilities/collectionscanner/CollectionScanner.cpp
>> @@ -43,6 +43,7 @@
>> #include <QTextStream>
>> #include <QTime>
>> #include <QTimer>
>> +#include <qpixmap.h>
>>
>> //Taglib:
>> #include <apetag.h>
>> @@ -61,6 +62,8 @@
>> #include <tlist.h>
>> #include <tstring.h>
>> #include <vorbisfile.h>
>> +#include <attachedpictureframe.h>
>> +#include <tbytevector.h>
>>
>> #include <audiblefiletyperesolver.h>
>> #include <realmediafiletyperesolver.h>
>> @@ -455,8 +458,6 @@ CollectionScanner::scanFiles( const QStringList& entries )
>>
>> else
>> {
>> - //FIXME: PORT 2.0
>> -// QList<EmbeddedImage> images;
>> const AttributeHash attributes = readTags( path );
>>
>> if( !attributes.empty() )
>> @@ -467,24 +468,6 @@ CollectionScanner::scanFiles( const QStringList& entries )
>>
>> if( !covers.contains( cover ) )
>> covers += cover;
>> -
>> - //FIXME: PORT 2.0
>> -// foreach( EmbeddedImage image, images )
>> -// {
>> -// AttributeHash attributes;
>> -// if( m_batch && !m_rpath.isEmpty() )
>> -// {
>> -// QString rpath = path;
>> -// rpath.remove( QDir::cleanPath( QDir::currentPath() ) );
>> -// rpath.prepend( QDir::cleanPath( m_rpath + '/' ) );
>> -// attributes["path"] = rpath;
>> -// }
>> -// else
>> -// attributes["path"] = path;
>> -// attributes["hash"] = image.hash();
>> -// attributes["description"] = image.description();
>> -// writeElement( "embed", attributes );
>> -// }
>> }
>> }
>>
>> @@ -621,9 +604,8 @@ CollectionScanner::readTags( const QString &path, TagLib::AudioProperties::ReadS
>> if ( !file->ID3v2Tag()->frameListMap()["TCMP"].isEmpty() )
>> compilation = TStringToQString( file->ID3v2Tag()->frameListMap()["TCMP"].front()->toString() ).trimmed();
>>
>> - //FIXME: Port 2.0
>> -// if( images )
>> -// loadImagesFromTag( *file->ID3v2Tag(), *images );
>> + if ( !file->ID3v2Tag()->frameListMap()["APIC"].isEmpty() )
>> + attributes["apic"] = QString("true");
>> }
>> // HACK: charset-detector disabled, so all tags assumed utf-8
>> // TODO: fix charset-detector to detect encoding with higher accuracy
>>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
More information about the Amarok-devel
mailing list