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

CoverArt: SHA-256 digest and (background) color #2524

Merged
merged 17 commits into from
Jun 18, 2020
Merged

CoverArt: SHA-256 digest and (background) color #2524

merged 17 commits into from
Jun 18, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Feb 28, 2020

Includes a database schema migration

The next evolution of cover art display:

  • New SHA-256 digest with 64-bit in-memory cache keys to reliably prevent wrong or swapping cover art display: https://bugs.launchpad.net/mixxx/+bug/1607097
  • Optional background color that is calculated from scaled-down images (1x1 pixel) and displayed immediately as a preview while the actual cover art is still loading
  • Completely backwards compatible and therefore maybe a valid candidate for 2.3:
    • you can seamlessly switch between Mixxx versions without any data inconsistencies
    • the old hash is still calculated and stored in the database for backwards compatibility with previous Mixxx versions
    • the new table columns digest and color are optional, initialized with default values (NULL), and populated lazily on demand when first needed
  • Fixes https://bugs.launchpad.net/mixxx/+bug/1879160 by logging image load failures only once fixed in lp1879160: Log warning about cover load failure only once #2815 and merged into 2.3

The only drawback is that displaying tracks in the track table view for the first time will take more time until all tracks have been updated with the additional data. This is mainly triggered by CoverArtDelegate, because usually tracks a first displayed in the track table view before they are actually loaded by TrackDAO.

This PR finally enables cover art display for the track table view in #2282.

The following image shows a side-by-side comparison of two subsequent screenshots. On the left side the covers have only partially been loaded while on the right side loading of all covers has finished:
mixxx_coverart_preview

@uklotzde uklotzde changed the title [Preview] CoverArt: SHA-256 digest and background color [Preview] CoverArt: SHA-256 digest and (background) color Feb 28, 2020
@uklotzde uklotzde added this to the 2.3.0 milestone Feb 29, 2020
@uklotzde
Copy link
Contributor Author

I consider this ready for 2.3.0. Remove the label if you disagree.

@Be-ing Be-ing modified the milestones: 2.3.0, 2.4.0 Feb 29, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Feb 29, 2020

Thank you for working on this, but this looks like a pretty substantial change and I am not comfortable rushing it for 2.3. We already rushing the changes for Serato metadata import.

@uklotzde uklotzde changed the title [Preview] CoverArt: SHA-256 digest and (background) color CoverArt: SHA-256 digest and (background) color Apr 7, 2020
@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 7, 2020

After the prerequisite PRs have been merged into master this feature is considered done.

I will rebase on master from time to time for the time being until review and merge are considered.

@uklotzde uklotzde mentioned this pull request Apr 12, 2020
11 tasks
@uklotzde uklotzde changed the title CoverArt: SHA-256 digest and (background) color [Suspended] CoverArt: SHA-256 digest and (background) color May 7, 2020
@Be-ing Be-ing requested a review from daschuer May 7, 2020 12:52
@uklotzde uklotzde changed the title [Suspended] CoverArt: SHA-256 digest and (background) color CoverArt: SHA-256 digest and (background) color May 7, 2020
@uklotzde
Copy link
Contributor Author

uklotzde commented May 7, 2020

Back on track :)

@Holzhaus
Copy link
Member

Holzhaus commented May 7, 2020

Can you add a change log entry? Going through hundreds of old PRs before release is cumbersome. Let's just do it as we go.

@Holzhaus
Copy link
Member

Ready to merge?

@uklotzde uklotzde changed the title CoverArt: SHA-256 digest and (background) color [WiP] CoverArt: SHA-256 digest and (background) color May 22, 2020
@uklotzde
Copy link
Contributor Author

Ready to merge?

I decided to first prepare a backport for 2.3 fixing https://bugs.launchpad.net/mixxx/+bug/1879160. PR will follow. This requires another rebase of this PR.

@ronso0
Copy link
Member

ronso0 commented May 30, 2020

Unfortunately this doesn't solve the high CPU either.
(Same testing procedure as with #2815 )

  • pick a track, make sure it doesn't have coverart embedded
  • assign any cover image from file system
  • delete/move cover image in file system
    = cover error is logged once
    = idle CPU stays very high (~105% here)
  • restore cover image
    = idle CPU load is back to normal (~18% here)

src/library/coverart.h Outdated Show resolved Hide resolved
src/library/basecoverartdelegate.cpp Show resolved Hide resolved
src/library/basecoverartdelegate.cpp Show resolved Hide resolved
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. I'll hold back merge until @ronso0 confirms that this works fine for him, too.

@uklotzde
Copy link
Contributor Author

Anything else to do? I would like to get rid of this long pending PR.

@ronso0
Copy link
Member

ronso0 commented Jun 12, 2020

I won't be at my pc until sunday. So if this can't wait until then basically anyone else could do the simple test I described above #2524 (comment)

@ronso0 ronso0 closed this Jun 12, 2020
@ronso0 ronso0 reopened this Jun 12, 2020
@ronso0
Copy link
Member

ronso0 commented Jun 12, 2020

(sry for closing, but weirder stuff happened when my phone slipped out of my hand... ;)

@uklotzde uklotzde mentioned this pull request Jun 13, 2020
7 tasks
@Be-ing
Copy link
Contributor

Be-ing commented Jun 18, 2020

ping @ronso0

@ronso0
Copy link
Member

ronso0 commented Jun 18, 2020

Yes, CPU issue is solved. Thanks!

@Holzhaus
Copy link
Member

Alright, let's merge then.

@Holzhaus Holzhaus merged commit eef9cec into mixxxdj:master Jun 18, 2020
@uklotzde uklotzde deleted the coverart_color_and_digest branch June 19, 2020 09:50
@uklotzde
Copy link
Contributor Author

@Holzhaus Thank you :)

daschuer added a commit to daschuer/mixxx that referenced this pull request Aug 16, 2021
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants