Cover art support #278

cardinot · 2014-06-17T19:04:19Z

More information about the project:
http://mixxx.org/wiki/doku.php/cover_art_support

Skin for tests:

Deere1280x800-WXGA

Supported features:

analysis
autodj
crates
library
history
playlist

First Part

Second Part
cardinot#1

redesign dlgtrackinfo to show covers
load cover in dlgtrackinfo
edit cover location in dlgtrackinfo
unset cover (load default)
reload cover from metadata
warning box issue
icon for each cover_art action
copy cover from external dir to track dir (if the user changes the cover)

Third Part
cardinot#2

add cover_art column in Library feature
add cover_art column in AutoDJ feature
add cover_art column in Analyze feature
add cover_art column in Playlists feature
add cover_art column in Crates feature
adjust cover art size
delay cover loading when the user is scrolling fast
batch updates (avoiding doing many writes in a short time)

kain88-de · 2014-06-17T19:21:52Z

src/library/dao/coverartdao.cpp

+    );
+    query.bindValue(":id", trackId);
+
+    if (query.exec()) {


a bit style nitpicking. Can you please change this to our default style of

if (!query.exec()) { LOG_FAILED_QUERY(query); return coverArtInfo(); } // handle query results

kain88-de · 2014-06-17T19:33:21Z

from a first rough scanning I have 2 points that you still need to deal with

add wcoverart for all features
- hidden
- missing
- playlists
- crates
- analysis
- autodj
- library
add unit-tests

kain88-de · 2014-06-17T19:43:42Z

@rryan can we cut a 1.12 branch before we merge this? Currently this is still incomplete because there is no way for a user to change the cover (that would in the next PR) and concentrates on how covers are handled internally. Also this has likely still bugs and definitely is against our feature freeze.

cardinot · 2014-06-18T02:58:16Z

-add wcoverart for all features

hidden

missing

Done! 2be0d8b

playlists

crates

It's already working, isn't it?

daschuer · 2014-06-18T06:26:51Z

src/dlganalysis.cpp

@@ -27,6 +27,9 @@ DlgAnalysis::DlgAnalysis(QWidget* parent,
    connect(m_pAnalysisLibraryTableView, SIGNAL(loadTrackToPlayer(TrackPointer, QString)),
            this, SIGNAL(loadTrackToPlayer(TrackPointer, QString)));

+    connect(m_pAnalysisLibraryTableView, SIGNAL(loadCoverArt(QString, int)),
+            this, SIGNAL(loadCoverArt(QString, int)));


Can you add a comment why this is required?

Moving to focusedTrackChanged() would clarify this as well.

The parent class we use for library models is pretty stupid. This signal is needed so that we can inform wcoverart that a different track is being selected. 'focusedTrackChanged' is more descriptive so we could change it

daschuer · 2014-06-18T08:00:34Z

Hi @cardinot

Well done! It looks really nice.

However, I have already commented some code with ideas to make it more streamline.
I will continue my review tonight. And do some tests as well.

Kind regards,

Daniel

kain88-de · 2014-06-18T14:35:05Z

src/library/coverartcache.cpp

+        return result;
+    }
+
+    CoverArtDAO::coverArtInfo coverInfo = m_pCoverArtDAO->getCoverArtInfo(trackId);


This is not good. You are calling a functions across threads and pass data between them. This could end that you potentially do really a LOT of parallel queries. I don't know how sqlite deals with that

Oh, that's true... but we could do it outside this thread (into requestPixmap) and send coverInfo as a parameter of loadImage...

then you would to a query everytime you want to load a non cached picture. I thought the goal was to reduce db queries for normal loading

We could use QFile::exists(coverLocation) and just fill a CoverArtDAO::coverArtInfo from DB if it not exists...

It means, something like:

CoverArtDAO::coverArtInfo coverInfo; if (!QFile::exists(coverLocation)) { coverInfo = m_pCoverArtDAO->getCoverArtInfo(trackId); }

As all covers that were loaded correctly once would be stored in our disk-cache, it would return true most of times and we would be doing the db query just when it would be useful for loadImage()...

no Db queries outside of the UI-Thread! Especially with the highly parallel asynchronous Qt:Concurrent system. It is too easy for this to introduce race-condition bugs. I'd rather have that we avoid them when we can.

no Db queries outside of the UI-Thread!

But my suggestion is move it to requestPixmap() to run in the main thread... so, the code that I suggested was to be placed to run in the main thread...

Hey, just keep in mint that there is a pending pull request from last year, that moves all db quires into a dedicated thread.
I hope we can merge it in the 1.13 release, so there is no need to worry that much here to use the DB from the GUI thread.

And yes, db queries can involve HD access. I just wander if it is required to cache all our date in ram, rather than using sqlite cache facilities, but that is an other topic. (I am no sqlit expert)

ah ok sorry I got you wrong. Yeah inside of the UI-Thread it is ok

Hey, just keep in mint that there is a pending pull request from last year, that moves all db quires into a dedicated thread.

But regardless of that the code will still be easier to understand if we don't do everything in 'loadImage' and we would benefit from it either way.

daschuer · 2014-06-18T21:06:58Z

just added the cover arts to developer_skins/Shade_dev to be able to test on my netbook.

daschuer · 2014-06-18T21:08:58Z

The header for the cover location in "26". It seams a proper text is missing.

cardinot · 2014-06-18T21:14:24Z

The header for the cover location in "26". It seams a proper text is missing.

sorry, but I didn't get your point...

kain88-de · 2014-06-18T22:19:59Z

What I had in mind was more along these lines. For each function I can see from the name what it is supposed to do. The control flow in requestPixmap is easy to understand.

void requestPixmap(id, loc) {
  // check if we are already looking for this tracks cover

  if loc.isEmpty():
    // get track information from coverArtDAO
    QtConcurrent::run(searchForImage, coverInfo)
    // set up future watcher and connect finished signal to 'imageFound'
  else:
    QtConcurrent::run(loadImage, loc, id)
    // set up future watcher and connect finished signal to 'imageLoaded'
}

coverTuple loadImage(loc, id) {
  img = QImage(loc)
  return id, img
}

coverTuple searchForImage(coverInfo) {
  // look in disk-cache
  // look in id3tags
  // look in track directory

  // save image in disk-cache
  return id, img, loc
}

void imageLoaded(id, img) {
  // extract result from sender
  // add to cache
  // remove id from runningIDs
  emit pixmapFound(id, pixmap)
}

void imageFound(id, img, loc) {
  // extract result from sender
  // add image to cover_art table
  // update library table
  // add to cache
  // remove id from runningIDs
  emit pixmapFound(id, pixmap)
}

cardinot · 2014-06-19T00:04:15Z

Hello @kain88-de, thanks for your approach. =D

  if loc.isEmpty():
    // get track information from coverArtDAO
    QtConcurrent::run(searchForImage, coverInfo)
    // set up future watcher and connect finished signal to 'imageFound'
  else:
    QtConcurrent::run(loadImage, loc, id)
    // set up future watcher and connect finished signal to 'imageLoaded'

What about files which are externally removed?
The inicial coverLocation could be notEmpty and it not means that the cover would be loaded. That's the reason why I suggested check if the file exists...

coverTuple searchForImage(coverInfo) {
  // look in disk-cache
  // look in id3tags
  // look in track directory

  // save image in disk-cache
  return id, img, loc
}

Actually we'd be doing it:

coverTuple searchForImage(coverInfo) {
  // look in disk-cache  +  QImage(coverInfo.coverLocation);  [1]
  // look in id3tags  +  return QImage  [2]
  // look in track directory  +  QImage(coverLocation);  [3]

  // save image in disk-cache
  return id, img, loc
}

Note that we will be handling and loading QImages anyway during the search and other VERY important point is that the first step (look in disk cache) is exactly the same thing that you would be doing in the another thread (loadImage(loc, id))... So, IMHO there's no advantage in split it...

I also noticed that the only difference between imageFound and imageLoaded is the db_updating stuff, but it's also very easy to know if we have or not to update it...

Have you seen my last refactoring around this stuff?

daschuer · 2014-06-19T05:01:07Z

The header for the cover location in "26". It seams a proper text is missing.

sorry, but I didn't get your point...

The header in the library table for the new cover location column-

kain88-de · 2014-06-19T06:38:57Z

The header in the library table for the new cover location column-

Oh that should be hidden from the user. @cardinot you can change this by adding the column to the isColumnInternal function of the tablemodels.

kain88-de · 2014-06-19T07:02:49Z

@cardinot I have looked at your recent changes and I know that my approach has a little bit duplicate code. But I have to make just one decision where as in your code you have to make the same decision 3 times. Here is a little ascii-art to show you what I mean.

               Mine                                                    Yours
                   |                                                           |
                  /\                                                          /\
                /    \                                                      /    \ 
                |     |                                                         |
                |     |                                                        /\
                |     |                                                      /    \
                |     |                                                         |
                |     |                                                        /\
                |     |                                                      /    \

Maybe I'm a bit biased but 6 months from now I could understand my control-flow easier and faster then yours. Each function has one well defined task and the decision is happening just once. We've already been clever by using QtConcurrent, now we have to think what would be the easiest maintainable version of this code. @daschuer any comment on this?

Another plus side to this approach is that it is easily testable. If you have a function that behaves differently on different input you have to test all possible input combinations, which is a combinatorial
problem that grows fast.

To check of we need to search for the cover you can also use loc.isEmpty() && !QFile::exists(loc), the extra check for the empty string is not really necessary but the verbosity of the statement is nice. You could also add another function that accepts an id and pixmap which will add the pixmap to the cache and emit the pixmapFound signal to avoid some code duplication.

cardinot · 2014-06-19T16:08:56Z

The header in the library table for the new cover location column-

Oh that should be hidden from the user. @cardinot you can change this by adding the column to the isColumnInternal function of the tablemodels.

Thanks @kain88-de and @daschuer
I pushed it here: 1e2b71e. Please, have a look if it's enough to sort it out! =D

Now only loads a cover once for rows that share the same hash.

Sorts by the hash. It works well except for the case where cover art is similar but not exactly the same. I think it's good enough to include, even if some users may be confused why visually similar cover art doesn't sort together.

Improves scrolling performance when scrolling via keyboard.

interaction timeout.

rryan · 2014-10-30T02:44:17Z

Note, there is discussion about this happening in: cardinot#22

(People subscribing to mixxxdj/mixxx won't see it since it's in @cardinot's fork)

In DlgCoverArtFullSize, max across all top level windows to get the appropriate size for the cover art. Maybe this fixes the original issue?

* Adds CoverArtCache::guessCover(s) methods for calling CoverArtUtils::guessCoverArt in a background thread. * Removes processing of reset menu option from WCoverArtMenu and instead sends a signal that the owner of the menu should handle.

ControlObjectThread is deprecated all new CO's should be of type ControlObjectSlave

Github Issues migration

kain88-de reviewed Jun 17, 2014
View reviewed changes

daschuer reviewed Jun 18, 2014
View reviewed changes

kain88-de reviewed Jun 18, 2014
View reviewed changes

rryan added 8 commits October 29, 2014 16:21

Remove unnecessary use of CoverArt.

06b1952

Fix UTF-8 test.

0c48c06

Change CoverInfo hash to an integer.

320a803

Use cover hash as request reference in CoverArtDelegate.

dbe9811

Now only loads a cover once for rows that share the same hash.

Support sorting by cover art.

f9e9148

Sorts by the hash. It works well except for the case where cover art is similar but not exactly the same. I think it's good enough to include, even if some users may be confused why visually similar cover art doesn't sort together.

Delay trackSelected signal until the user interaction timeout.

343f7c0

Improves scrolling performance when scrolling via keyboard.

Only repaint cells that were cache misses when we hit the user

07fd51f

interaction timeout.

Double-check that we are not overwriting USER_SELECTED covers.

0425bc4

rryan and others added 17 commits October 29, 2014 23:06

Delete unused method.

3d0f864

Switch CoverArt use to CoverInfo.

fa683d8

Revert DlgTrackInfo parentage change.

3468571

In DlgCoverArtFullSize, max across all top level windows to get the appropriate size for the cover art. Maybe this fixes the original issue?

Call activateWindow on DlgCoverArtFullSize.

8dea56a

Compare potential cover filenames case-insensitively.

e394457

Handle COVER_NONE covers in WCoverArtMenu::slotChange.

dddeb3b

Show cover art progress on the library scanner dialog.

cf7c539

Update BaseTrackCache once cover art processing is done.

a7329e9

Only pick non-matching cover files if there is only one option.

4ad6b5d

Create a security token for files chosen via WCoverArtMenu.

f3e2c5c

Acquire sandbox tokens in various places for cover art.

24874c5

Only save images in TIO if it is not NULL

b95c452

Reduce code duplication in finding possible Covers

09b16a3

Move CoverArtUtil tests to own class

46fbd24

Codying style clean up coverartcache

82085f3

Use COSlave instead of COThread

76113d0

ControlObjectThread is deprecated all new CO's should be of type ControlObjectSlave

kain88-de merged commit 76113d0 into mixxxdj:master Nov 5, 2014

esbrandt mentioned this pull request Mar 25, 2017

Cover art UX improvements #1207

Merged

mixxxbot mentioned this pull request Aug 24, 2022

add an option for a non chopped cover art to library table #7635

Closed

m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024

Merge pull request mixxxdj#278 from ronso0/gh-issues-migration

66c2168

Github Issues migration

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover art support #278

Cover art support #278

cardinot commented Jun 17, 2014

kain88-de Jun 17, 2014

kain88-de commented Jun 17, 2014

kain88-de commented Jun 17, 2014

cardinot commented Jun 18, 2014

daschuer Jun 18, 2014

daschuer Jun 18, 2014

kain88-de Jun 18, 2014

daschuer commented Jun 18, 2014

kain88-de Jun 18, 2014

cardinot Jun 18, 2014

kain88-de Jun 18, 2014

cardinot Jun 18, 2014

cardinot Jun 18, 2014

kain88-de Jun 18, 2014

cardinot Jun 18, 2014

daschuer Jun 18, 2014

kain88-de Jun 18, 2014

kain88-de Jun 18, 2014

daschuer commented Jun 18, 2014

daschuer commented Jun 18, 2014

cardinot commented Jun 18, 2014

kain88-de commented Jun 18, 2014

cardinot commented Jun 19, 2014

daschuer commented Jun 19, 2014

kain88-de commented Jun 19, 2014

kain88-de commented Jun 19, 2014

cardinot commented Jun 19, 2014

rryan commented Oct 30, 2014

Cover art support #278

Cover art support #278

Conversation

cardinot commented Jun 17, 2014

Choose a reason for hiding this comment

kain88-de commented Jun 17, 2014

kain88-de commented Jun 17, 2014

cardinot commented Jun 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 18, 2014

daschuer commented Jun 18, 2014

cardinot commented Jun 18, 2014

kain88-de commented Jun 18, 2014

cardinot commented Jun 19, 2014

daschuer commented Jun 19, 2014

kain88-de commented Jun 19, 2014

kain88-de commented Jun 19, 2014

cardinot commented Jun 19, 2014

rryan commented Oct 30, 2014