Skip to content

lp1879160: Log warning about cover load failure only once#2815

Merged
daschuer merged 10 commits intomixxxdj:2.3from
uklotzde:coverartcache
Jun 5, 2020
Merged

lp1879160: Log warning about cover load failure only once#2815
daschuer merged 10 commits intomixxxdj:2.3from
uklotzde:coverartcache

Conversation

@uklotzde
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde commented May 22, 2020

Fixes https://bugs.launchpad.net/mixxx/+bug/1879160 without suppressing warnings. The actual issues are already fixed, but currently we don't notice when loading a cover image fails.

Solution: Forward the results of loading cover images to CoverArtCache instead of logging failures directly. In CoverArtCache we are able to detect failed attempts and log the warning for each file path only once per session.

@Holzhaus This PR causes merge conflicts with #2524 which needs to be rebased afterwards. I have already resolved most of them by cherry-picking the initial commit from this PR.

@uklotzde uklotzde added this to the 2.3.0 milestone May 22, 2020
@uklotzde uklotzde requested review from Holzhaus and ronso0 May 22, 2020 13:20
@ronso0
Copy link
Copy Markdown
Member

ronso0 commented May 25, 2020

I'll test this with the tracks I tested earlier when I'm back next days.

Comment thread src/library/coverart.h Outdated
};

LoadedImage()
: result(Result::ErrorUnknown) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this correct that the initial state is considered as errror?
How about "NoResult" or "NotLoaded" or something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is correct, because it will never happen. See the DEBUG_ASSERT in the loadImage() function.

I will hide the default constructor, because LoadedImage should only be created by the loadImage() function.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented May 30, 2020

Sorry, seems CPU issue is not solved at all.

I tested like this:

  • 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)

Comment thread src/library/coverart.cpp
@uklotzde
Copy link
Copy Markdown
Contributor Author

@ronso0 I can confirm the high CPU load. But I guess it is still present in the current version after we have removed just the log message. Is this correct?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented May 30, 2020

Yes, it's back in 2.3 eventhough I was testing for exactly that when disabling the log message in #2805

For now, to avoid CPU issues until there's an ultimate solution I use https://github.com/ronso0/mixxx/tree/cover-highCPU-hack which inserts a pink checkered dummy image EDIt and prints the track location to the log.
I don't care if the previous cover is overwritten as to me it seems unlikely the previous image would show up again once it disappeared.
image

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Jun 1, 2020

@ronso0 I found a simpler solution without additional book keeping that logs the warning once per cached pixmap and works nicely with the background color that will be introduced in #2524.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Jun 3, 2020

I'll test this the next days.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Jun 4, 2020

Yup, can confirm the high CPU issue is solved, and it shows the average color from the previous cover image.
Nice!

Copy link
Copy Markdown
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.

LGTM

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Jun 5, 2020

OK, Thank you. So we can finally merge.

@daschuer daschuer merged commit fa942b8 into mixxxdj:2.3 Jun 5, 2020
@uklotzde uklotzde deleted the coverartcache branch June 5, 2020 19:21
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.

3 participants