Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tagfetcher: cache fetched covers #12301

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 14, 2023

Closes #11084

A quick test to cache the fetched cover images with the global QPixmapCache a QMap.
Cache keys are "DlgTagftecher" + the MusicBrainz release id.
This greatly improves UX when comparing covers of MusicBrainz results.

Works good, though removing the covers from the cache is a bit dirty I think.
To discuss:
* really use global QPixmapCache which is also used by CoverArtCache?
* shall we increase the QPixmapCache cache limit when DlgTagFetcher is constructed to avoid clearing cached covers by adding large fetched covers?

I didn't check the sizes of fetched covers, yet, we need to estimate a reasonable limit.

ToDo

  • clear cache when loading another track
  • preferred image size vs. cached images:
    currently the preference is read from config each time image urls are received. With the current implementation this may result in a mismatch of current size preference / cached images.
    We could
    a) read from config once in the constructor to avoid the issue in the first place
    b) read the preference each time urls are received and use the size preference (int) for the cache key
  • remove/disable debug output

based on #12300

@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch from 23e8213 to f4e046c Compare November 14, 2023 15:02
@ronso0 ronso0 added this to the 2.4.0 milestone Nov 14, 2023
@ronso0 ronso0 linked an issue Nov 14, 2023 that may be closed by this pull request
@ronso0 ronso0 marked this pull request as draft November 14, 2023 21:59
@ronso0
Copy link
Member Author

ronso0 commented Nov 15, 2023

Rebased, I adjusted & simplified related code a bit, e.g. loading a cover to DlgCoverArtFullsize.
It's working fine now.

However, (ab)using QPixmapCache is not the way to go: it's capped to 20 MB, and fetching some 1200px covers would flush all covers cached by the library table.

So a QCache is unavoidable. (that simplifies clearing the flushing the fetched covers... a lot)

@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch from f4e046c to cf6c689 Compare November 15, 2023 12:45
@github-actions github-actions bot added the ui label Nov 15, 2023
@ronso0
Copy link
Member Author

ronso0 commented Nov 15, 2023

Now the covers are cached in a QMap. I didn't set a cache limit, yet. Let's figure out a reasonable size, i.e. for how many high-res covers the size would become a problem.

I couldn't get it to work with QCache, I always got segfaults when retrieving pixmaps. Must be something with QCache taking ownership of the objects and/or with QPixmap being one of the implicitly shared classes, but I couldn't figure it out. Same when I stored the fetched QByteArray, it was always nulled for some reason. If someone wants to take a look, the experimental QCache commits are in https://github.com/ronso0/mixxx/tree/tagfetcher-cache-fetched-covers_qcache.

@ronso0 ronso0 marked this pull request as ready for review November 15, 2023 12:52
@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch from cf6c689 to cd1e4e1 Compare November 15, 2023 14:43
src/library/dlgcoverartfullsize.cpp Outdated Show resolved Hide resolved
src/library/dlgcoverartfullsize.h Outdated Show resolved Hide resolved
@@ -107,5 +111,7 @@ class DlgTagFetcher : public QDialog, public Ui::DlgTagFetcher {

QByteArray m_fetchedCoverArtByteArrays;

QMap<QString, QPixmap> m_coverCache;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider to extend use the exiting cover cache here?
I have not looked into it, but I can imagine that this way we can save one cover access.

An other option is to use QPixmapCache or the general purpose QCache container.

Copy link
Member Author

@ronso0 ronso0 Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the comments above I explained why QPixmapCache is unsuitable (e.g. it requires manual cleanup), and that I didn't get it to work with QCache. QPixmapCache is derived from QCache).

Any idea how to fix the QCache implementation is welcome, even though the only benefits seem to be slightly faster lookup and automatic cleanup (limit).

@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch 2 times, most recently from ab23b90 to 973d012 Compare November 16, 2023 10:17
@ronso0 ronso0 modified the milestones: 2.4.0, 2.4.1 Nov 30, 2023
@ronso0 ronso0 changed the title Tagfetcher cache fetched covers Tagfetcher: cache fetched covers Dec 27, 2023
@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch from 973d012 to 15fb353 Compare February 11, 2024 22:49
@ronso0 ronso0 modified the milestones: 2.4.1, 2.4.2 Apr 13, 2024
@daschuer
Copy link
Member

It looks like this is mergable after you have rebased out the debug commit.

@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

Oh great, will do.

@ronso0 ronso0 modified the milestones: 2.4.2, 2.4.1 Apr 14, 2024
@ronso0 ronso0 force-pushed the tagfetcher-cache-fetched-covers branch from 15fb353 to 6e5082a Compare April 14, 2024 12:04
@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

Done.
For TODO #2 I filed #13099

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and works good! Thank you.

@daschuer daschuer merged commit 74aa041 into mixxxdj:2.4 Apr 15, 2024
14 checks passed
@ronso0 ronso0 deleted the tagfetcher-cache-fetched-covers branch April 15, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover Art Fetcher: cache fetched images per track
2 participants