Cover Files Extraction Branch

[Alexander Stippich]

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.

Best regards,
Alexander


> > Best regards,
> > Alexander
> > 
> > > > Future things I would like to look at:
> > > > -add a QQuickImageProvider for the cover manager (opinions?)
> > > > -add functions to create artist pictures from the album covers (like
> > > > in
> > > > https://community.kde.org/KDE_Visual_Design_Group/Music_Player)
> > > > -make maximum cover size configurable
> > > > 
> > > > I would appreciate any feedback. I _think_ currently it is necessary
> > > > to
> > > > delete the old database in order to get the new images, but this is an
> > > > issue unrelated to this branch.
> > > > 
> > > > Best regards,
> > > > Alexander
> > > > 
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Elisa mailing list
> > > > Elisa at kde.org
> > > > https://mail.kde.org/mailman/listinfo/elisa
> > > 
> > > Best regards
> > > 
> > > --
> > > Matthieu Gallien
> > 
> > _______________________________________________
> > Elisa mailing list
> > Elisa at kde.org
> > https://mail.kde.org/mailman/listinfo/elisa
> 
> Best regards
> 
> --
> Matthieu Gallien