Skip to content

disable repeated logging of missing coverart image#2805

Merged
uklotzde merged 1 commit intomixxxdj:2.3from
ronso0:coverart-missinng-high-cpu
May 21, 2020
Merged

disable repeated logging of missing coverart image#2805
uklotzde merged 1 commit intomixxxdj:2.3from
ronso0:coverart-missinng-high-cpu

Conversation

@ronso0
Copy link
Copy Markdown
Member

@ronso0 ronso0 commented May 19, 2020

fixes high CPU and xruns when a coverart image is missing in file system and the log warning is printed repeatedly.
https://bugs.launchpad.net/mixxx/+bug/1879160

Is the coverart requested repeatedly with every GUI tick?

The missing cover art is now only noticed by ..well, the missing the cover art in the tracks table and elsewhere.
I guess until anyone comes up with a better fix no one will seriously be harmed as users can re-assign the coverart manually if they miss it.

@uklotzde Any other places where this might happen come to mind?

@ronso0 ronso0 added this to the 2.3.0 milestone May 19, 2020
@ronso0 ronso0 requested a review from uklotzde May 19, 2020 22:17
@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented May 19, 2020

2.2.3 is also affected. Should I rebase?

@uklotzde
Copy link
Copy Markdown
Contributor

2.2.3 is also affected. Should I rebase?

Yes, just in case we need another 2.2.x release.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented May 20, 2020

I was wrong, with 2.2 there's no log spam. Seems I already had 2.4 installed automatically from the ppa without noticing.

So this is ready, except you know of other places where we should fix a similiar issue once I'm at it?

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented May 20, 2020

To print the warning but not enter this infinite loop in the first place, can't we do something like loading a 1x1 px dummy image? Or would that replace the db cover art link?

@uklotzde
Copy link
Copy Markdown
Contributor

uklotzde commented May 20, 2020

To print the warning but not enter this infinite loop in the first place, can't we do something like loading a 1x1 px dummy image? Or would that replace the db cover art link?

This could be an option. It should prevent the warning while the dummy image is cached. Maybe then we don't need to suppress the warning.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented May 20, 2020

Would you mind helping me get this done?
Or do you intend to work on this yourself, which will probably way faster?

Either way, if we reenable the warning we should print the location of the affected audio file so users have a chance to fix this. I searched for the image file mentioned in the warning but that path doesn't exist so I had no chance to fix this manually, and I don't want to mess around with the db.

@uklotzde
Copy link
Copy Markdown
Contributor

I changed my mind. Considering the upcoming changes in #2524 returning a fake image has unintended side-effects. The new hash would be calculated and stored based on this fake image instead of the actual image.

@uklotzde
Copy link
Copy Markdown
Contributor

@ronso0 Implementing a more sophisticated solution that defers logging will conflict with #2524. Let's use this simple workaround for 2.3.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented May 21, 2020

Alright. Ready to merge then?

@uklotzde
Copy link
Copy Markdown
Contributor

LGTM

@uklotzde uklotzde merged commit 24a2ff1 into mixxxdj:2.3 May 21, 2020
@ronso0 ronso0 deleted the coverart-missinng-high-cpu branch May 21, 2020 12:22
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.

2 participants