Restore invalid cover image hashes #2508

uklotzde · 2020-02-21T09:40:08Z

Invalid cover image hashes with the value 0 are not handled correctly and wrong cover images are displayed. Instead those hashes need to be fixed and updated on-the-fly when the images are loaded the next time by CoverArtCache.

The updating is done in CoverArtDelegate that has access to TrackModel. CoverArtCache does not have direct access to Track objects or the library and I don't want to introduce such a dependency. Updating the hashes when tracks are loaded is too late, because first the images are loaded and displayed in the track table view without instantiating any Track objects. Not ideal but a clean solution that fits into our current design.

Follow-up

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

uklotzde · 2020-02-22T13:15:07Z

Needs to be rebased on master after #2511 has been merged to fix the spurious test errors.

src/library/coverart.cpp

daschuer

Thank you for the PR. I have left some comments, the manual test is pending.

daschuer · 2020-02-22T21:54:23Z

src/library/coverart.cpp

+                << *this;
+        // Preserve the track location, i.e. slicing during
+        // assignment is intended here!
+        static_cast<CoverInfoRelative&>(*this) = CoverInfoRelative();


I would prefer a separate function to reset the cover info. I can imagine that even with this comment, the code cannot be understand by a less experienced contributor.

uklotzde · 2020-02-24T22:40:16Z

Let's see if the tests will succeed after the database issues have been fixed.

uklotzde · 2020-02-25T07:57:49Z

Still segfaults with ctest on Travis

uklotzde · 2020-02-28T14:19:26Z

After a long search for the spurious segfaults during tests I have rebuilt the whole branch and reorganized the commits into small, correlated work packages. Hopefully all tests will pass now.

uklotzde · 2020-02-28T16:40:18Z

The remaining CI failures are now really unrelated.

uklotzde · 2020-02-29T12:06:04Z

@daschuer: Ready, finally.

The concurrent guessing of cover art was poorly documented and designed with lots of implicit assumptions and dependencies. It worked more or less by chance. This investigation and fix has cost me a LOT of time!

#2525 contains a separate fix for the application, independent of any test setup.

daschuer

The API is still a bit cluttered. It would be nice to clean up.
The manual test was successful. So it is already merge-able.

src/library/coverartutils.cpp

src/library/coverart.cpp

daschuer · 2020-03-01T18:07:43Z

src/library/coverartcache.cpp

-CoverArtCache::~CoverArtCache() {
-    qDebug() << "~CoverArtCache()";
+//static
+void CoverArtCache::requestCover(


Request cover implies to respond with a cover. Is this always the case here?
What happens if the cover is returned instantly?
I think the called tryLoadCover() name needs to be adjusted as well. ... Or split into two functions.

I've just tried to preserve the original naming that used "request" for asynchronous invocations.

request is always called with a pRequestor that expects to receive the result as a signal. What's wrong with this? Returning a QFuture from tryLoad would be nice, but it doesn't allow to return immediate results. Aynway, I'm not planning to rewrite this.

daschuer · 2020-03-01T19:45:57Z

src/library/coverartcache.cpp

-                 << requestInfo << pRequestor <<
-                desiredWidth << onlyCached << signalWhenDone;
+//static
+void CoverArtCache::requestTrackCover(


This is the request that unlike all direct calls to requestCover also updates the Track.

I think we should get rid of the default null track parameter in requestCover and call pCache->tryLoadCover expicite will pTrack or TrackPointer()

The request functions are static and return nothing. Both (public) variants boil down to a single, special-case invocation of tryLoad. The tryLoad function is non-static and may return something immediately, later, or never.

The use cases are already split. How and why does this need to be split even further?

daschuer · 2020-03-01T20:12:51Z

src/library/coverartdelegate.cpp

+            kLogger.info()
+                    << "Updating cover info of track"
+                    << coverInfo.trackLocation;
+            pTrack->setCoverInfo(coverInfo);


This is redundant if we pass the track into the request.
Can't we do it at one place only?

No, we don't. The CoverArtDelegate accesses query results directly. Track objects are only loaded if the CoverArtCache signals that the cover info has been updated.

Only support two distinct use cases, either with or without a Track object at hand.

daschuer · 2020-03-02T06:59:38Z

Ok, no need to overengineer this. It is already a good step forward.
Unfortunately build fails:

/home/appveyor/projects/mixxx/src/library/coverart.cpp:127:21: error: no match for ‘operator<<’ (operand types are ‘QDebug’ and ‘QFileInfo’)
             kLogger.warning()
             ~~~~~~~~~~~~~~~~~
                     << "loadImage"
                     ~~~~~~~~~~~~~~
                     << type
                     ~~~~~~~
                     << "cover does not exist:"
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~
                     << coverFile;
                     ^~~~~~~~~~~~

uklotzde · 2020-03-02T07:22:18Z

Ok, no need to overengineer this. It is already a good step forward.
Unfortunately build fails:

/home/appveyor/projects/mixxx/src/library/coverart.cpp:127:21: error: no match for ‘operator<<’ (operand types are ‘QDebug’ and ‘QFileInfo’)
             kLogger.warning()
             ~~~~~~~~~~~~~~~~~
                     << "loadImage"
                     ~~~~~~~~~~~~~~
                     << type
                     ~~~~~~~
                     << "cover does not exist:"
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~
                     << coverFile;
                     ^~~~~~~~~~~~

Backward compatibility with legacy Qt versions strikes once again.

uklotzde · 2020-03-05T07:26:07Z

Anything else to do? CI failures are unrelated.

uklotzde · 2020-03-06T09:55:54Z

@daschuer Ping.

I have more changes ready on top of this PR that will allow to share UI code between the internal track collection and external track collections like aoide. Mainly additional base classes and a signal/slot optimization for the CoverArtDelegate. I already experienced inconsistencies due to redundant code that I want to prevent in the future.

daschuer · 2020-03-09T06:34:23Z

No issues found during manual tests. LGTM. Thank you.

uklotzde added this to the 2.3.0 milestone Feb 21, 2020

uklotzde added library minor bug labels Feb 21, 2020

uklotzde changed the title ~~Restore invalid cover image hashes~~ [WiP] Restore invalid cover image hashes Feb 22, 2020

uklotzde mentioned this pull request Feb 22, 2020

Use URI path for in-memory database #2511

Merged

daschuer reviewed Feb 22, 2020

View reviewed changes

src/library/coverart.cpp Outdated Show resolved Hide resolved

daschuer requested changes Feb 22, 2020

View reviewed changes

uklotzde mentioned this pull request Feb 24, 2020

Fix runtime dependencies of LibraryScanner #2515

Merged

uklotzde changed the title ~~[WiP] Restore invalid cover image hashes~~ Restore invalid cover image hashes Feb 24, 2020

uklotzde changed the title ~~Restore invalid cover image hashes~~ [WiP] Restore invalid cover image hashes Feb 25, 2020

uklotzde added 6 commits February 28, 2020 15:16

Extract guessing of cover art from CoverArtCache

63565b0

Fix spurious segfaults in library tests

60b03bc

Prepare CoverArtCache for migration to new hash function

3af3644

Get tracks by TrackRef from TableModel

86b5732

Refresh cover art hashes when showing track table view

77458ee

Refresh cover art hashes when loading tracks

7eb21c3

uklotzde changed the title ~~[WiP] Restore invalid cover image hashes~~ Restore invalid cover image hashes Feb 28, 2020

uklotzde mentioned this pull request Feb 28, 2020

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

Merged

uklotzde added 4 commits February 28, 2020 19:54

Merge branch 'master' into coverimagehash

9d4f056

Merge branch 'master' into coverimagehash

ee515ca

Fix more spurious test failures due caused by concurrent tasks

35b96e5

Disable concurrent guessing of track cover art during tests

9069f44

uklotzde mentioned this pull request Feb 29, 2020

Fix potential race conditions and crashes during shutdown #2525

Closed

daschuer requested changes Mar 1, 2020

View reviewed changes

uklotzde added 3 commits March 1, 2020 23:38

Trim public API of CoverArtCache

7562e38

Only support two distinct use cases, either with or without a Track object at hand.

Handle absolute/relative cover locations including edge cases

7b5b16e

Merge branch 'master' into coverimagehash

6f8cfc8

uklotzde added 2 commits March 2, 2020 08:53

Fix compatbility issues with legacy Qt version

fa33206

Merge branch 'master' into coverimagehash

43c5167

Merge branch 'master' into coverimagehash

323d894

Merge branch 'master' into coverimagehash

c65632b

uklotzde mentioned this pull request Mar 8, 2020

Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate #2538

Merged

daschuer merged commit 7a3095d into mixxxdj:master Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore invalid cover image hashes #2508

Restore invalid cover image hashes #2508

uklotzde commented Feb 21, 2020 •

edited

Loading

uklotzde commented Feb 22, 2020 •

edited

Loading

daschuer left a comment

daschuer Feb 22, 2020

uklotzde Feb 22, 2020

uklotzde commented Feb 24, 2020

uklotzde commented Feb 25, 2020

uklotzde commented Feb 28, 2020

uklotzde commented Feb 28, 2020

uklotzde commented Feb 29, 2020

daschuer left a comment

daschuer Mar 1, 2020

uklotzde Mar 1, 2020

uklotzde Mar 1, 2020

daschuer Mar 1, 2020

uklotzde Mar 1, 2020

uklotzde Mar 1, 2020

uklotzde Mar 1, 2020

daschuer Mar 1, 2020

uklotzde Mar 1, 2020

daschuer commented Mar 2, 2020

uklotzde commented Mar 2, 2020

uklotzde commented Mar 5, 2020

uklotzde commented Mar 6, 2020

daschuer commented Mar 9, 2020

Restore invalid cover image hashes #2508

Restore invalid cover image hashes #2508

Conversation

uklotzde commented Feb 21, 2020 • edited Loading

Follow-up

uklotzde commented Feb 22, 2020 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Feb 24, 2020

uklotzde commented Feb 25, 2020

uklotzde commented Feb 28, 2020

uklotzde commented Feb 28, 2020

uklotzde commented Feb 29, 2020

daschuer left a 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

Choose a reason for hiding this comment

daschuer commented Mar 2, 2020

uklotzde commented Mar 2, 2020

uklotzde commented Mar 5, 2020

uklotzde commented Mar 6, 2020

daschuer commented Mar 9, 2020

uklotzde commented Feb 21, 2020 •

edited

Loading

uklotzde commented Feb 22, 2020 •

edited

Loading