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

Fix unnecessary painting in CoverArtDelegate #13715

Merged
merged 9 commits into from
Nov 3, 2024
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 1, 2024

I have created this during my my first attempt to fix the priority inversion #11128 when scrolling with cover arts column enabled. This is a bit of a speed up, but does not fix the actual problem. This will be part of the next PR #13719

@ronso0
Copy link
Member

ronso0 commented Oct 4, 2024

FWIW I'm not noticing a difference in performance between 2.4 and this.
With both branches, for covers that are skipped while scrolling fast (only the color is painted) the images are loaded only when I scroll back to the skipped rows. Ie. I don't see any effect of c36e566/6c841bd4b2320f6dd4ebf0599c4f3d6697caf9e4 (IIUC how those are supposed to work).

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

Just some small findings, I'll take a closer look soonish.

src/library/coverartdelegate.cpp Outdated Show resolved Hide resolved
src/library/coverartdelegate.cpp Outdated Show resolved Hide resolved
src/library/coverartdelegate.cpp Outdated Show resolved Hide resolved
@daschuer
Copy link
Member Author

daschuer commented Oct 4, 2024

The actual but small speed up happens once you stop scrolling. In this case the cover pictures appears faster. In the original version the uni-color replacements are first painted again after scroll stop and than the original covers are fetched and pained.

The other optimizations has only a negligible speed up. In 2.4 the chache grows up to 1000 and more, which requires repeated memory allocations. This branch reuses the memory once it is allocated for the number of all visible rows.

@daschuer
Copy link
Member Author

daschuer commented Oct 4, 2024

Done

@daschuer
Copy link
Member Author

daschuer commented Oct 6, 2024

Done

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

LGTM, but let's polish that comment

src/library/coverartdelegate.cpp Outdated Show resolved Hide resolved
Co-authored-by: ronso0 <[email protected]>
@daschuer
Copy link
Member Author

daschuer commented Nov 3, 2024

Done

@ronso0
Copy link
Member

ronso0 commented Nov 3, 2024

Thank you!

@ronso0 ronso0 merged commit 3d0d098 into mixxxdj:2.4 Nov 3, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants