[Amarok] a0c4a6c: Add embedded cover art support

Dan Meltzer parallelgrapefruit at gmail.com
Fri Feb 26 18:39:01 CET 2010


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
>


More information about the Amarok-devel mailing list