Cover Files Extraction Branch

Sun Nov 25 21:48:43 GMT 2018

Hello Alexander,

On samedi 24 novembre 2018 20:37:45 CET Alexander Stippich wrote:
> Hello Matthieu,
> 
> On Montag, 19. November 2018 21:18:25 CET you wrote:
> > On dimanche 18 novembre 2018 12:25:11 CET Alexander Stippich wrote:
> > > Hello Matthieu,
> > > 
> > > On Sonntag, 18. November 2018 10:56:01 CET you wrote:
> > > > Hello Alexander,
> > > > 
> > > > Thanks for your work on this.
> > > > 
> > > > On samedi 17 novembre 2018 18:01:18 CET Alexander Stippich wrote:
> > > > > Hello everyone,
> > > > > 
> > > > > I've pushed a branch called cover_manager.
> > > > > It contains a working solution for the extraction of cover images
> > > > > embedded
> > > > > in the files, but also does some more stuff. So here is a list:
> > > > > -reorder all platform-agnostic scanning files to a single folder
> > > > > -factors out the current code for cover searching into own class
> > > > > called
> > > > > cover manager
> > > > > -add tests and simple benchmarks
> > > > > -optimizations for cover searching
> > > > > -extract the cover art from music files
> > > > > -resize when image is too large (512x512 current maximum)
> > > > > -save all covers to standard data location, e.g. the same location
> > > > > where
> > > > > the database is located
> > > > 
> > > > I have done a quick review of the branch.
> > > > There are several blocking points.
> > > > 
> > > > I do not like that files and code unrelated to embedded cover images
> > > > are
> > > > modified in the same branch. Please separate each topic you want to
> > > > work
> > > > on.
> > > 
> > > This is really just the first commit. I will cherry-pick it for separate
> > > review and will rebase afterwards.
> > > 
> > > > As far as I understand what you did, you are saving all cover images
> > > > you
> > > > found (in directories or in metadata to a directory dedicated to store
> > > > application data).
> > > > Is my understanding correct ? In case yes, this is also a blocking
> > > > point.
> > > > We should not store cover images in this type of directory. They are
> > > > not
> > > > application data. We should also not duplicate existing files.
> > > 
> > > Your understanding is correct. The location of the data can easily be
> > > changed. I checked where Cantata is saving the files, and it uses the
> > > /.cache location. That's probably the best location, since after all
> > > they
> > > are used as a cache. Is that okay for you?
> > 
> > Qt provides a set of standard directory locations. Please have a look at
> > QStandardPaths (QStandardPaths::CacheLocation is probably what you are
> > looking for).
> 
> I'm already using QStandardPaths. I was just stating which path Cantata is
> using, not that I'm going to hardcode it.
> 
> > > Regarding the duplication of existing cover files in the folders, I also
> > > wasn't sure about this. But there are some good arguments in favor of it
> > > imho. They are scaled down automatically to the maximum size of 512x512
> > > currently (which may still be too large). Thus they will serve as a fast
> > > cache for the covers and loading of possibly large files is avoided.
> > > Having
> > > all cover files loaded from Elisa in one single place makes future
> > > features
> > > imho more easier to implement. This should greatly ease the
> > > implementation
> > > of a
> > > QQuickImageProvider later.
> > > And in case Elisa will download cover art automatically, this will also
> > > be
> > > the location.
> > 
> > Duplication local covers is a no go. This is not in the best interest of
> > our users.
> > Covers embedded in metadata can be fed to Image qml elements without
> > needing to be saved as a real file. In this case, creating such a file is
> > also a no go.
> > You would need to manage their lifetime and that will require some amount
> > of propertly tested code. This is a big work.
> > 
> > Without that, we will create files in the home directory of our users that
> > will just accumulate for cases that could be done without them.
> > 
> > Please have a look at the following page, the described approach is simple
> > and efficient (
> > https://forum.qt.io/topic/69209/how-to-display-a-qpixmap-qimage-in-qml/4
> > ).
> > 
> > Another solution could be to use a QQuickAsyncImageProvider derived class.
> 
> I don't understand that argument. Applying this to all other metadata, there
> would be no need for a local database, since it just duplicates information
> that is already stored in the files, and Elisa would load everything at
> startup.
> Do you want to open each file and extract the album cover on startup? This
> would pretty much contradict your current work of improving startup
> performance and memory consumption.
> 
> Also, if loading on demand is so complicated, why not go with the simple
> solution? Having all cover files in a single directory makes it so much
> easier. The implementation is also encapsulated in a single class, so it is
> easy to change later if ever needed.

As I said, I will refuse to have extracted covers being saved in home 
directory of users of Elisa without any way for them to know it and manage the 
disk space needed for them.

I plainly refuse to duplicate existing cover files. This is just waste and not 
needed at all.

I have worked on an alternative implementation based on 
QQuickAsyncImageProvider.

The diff https://phabricator.kde.org/D17163 is now open for review.

Best regards

[...]