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

Improved gallery cover lookup #3391

Merged
merged 11 commits into from
Feb 22, 2023
Merged

Improved gallery cover lookup #3391

merged 11 commits into from
Feb 22, 2023

Conversation

Ksrx01
Copy link
Contributor

@Ksrx01 Ksrx01 commented Feb 3, 2023

Small PR that changes how gallery covers are looked up. Which should solve (or alleviate) several open issues, including #3311, #1432, #1856, #2720 and #872

Right now only files ending exactly in cover.jpg are used as covers, any gallery missing such a file uses its first image.
These changes essentially replace CriterionModifierIncludes with CriterionModifierMatchesRegex (performance drop should be negligible), and introduce gallery_cover_regex in config.yml, which defaults to a value of (poster|cover|folder|board)(\.jpg|\.jpeg|\.png)$.

This makes the cover selection function by default much more flexible, but also allows it to be adapted.

Note: First time coding with Go, so I'm unsure if the way the configuration value is passed down to the function isn't total nonsense.

Copy link
Collaborator

@DingDongSoLong4 DingDongSoLong4 left a comment

Choose a reason for hiding this comment

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

Looks good to me, the only addition I would make would be to also add an option on the settings page to avoid having to directly edit the config file. The docs should probably also be updated for these changes as well.

@Ksrx01
Copy link
Contributor Author

Ksrx01 commented Feb 4, 2023

I skipped the UI due to quite limited spare time and the need to familiarize myself a bit more with the project. Figured it couldn't hurt to already commit this as a rudimentary feature for power users. It is however my intention to implement this in a future commit, hopefully in the coming weeks.

Copy link
Collaborator

@bnkai bnkai left a comment

Choose a reason for hiding this comment

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

With latest commit looks and tests ok.

Couldn't notice any performance issues.
Gallery page loading times seem the same and the FindGalleries query doesnt seem to be impacted ( a few ms maybe but the timings arent consistent enough to compare )

@Ksrx01
Copy link
Contributor Author

Ksrx01 commented Feb 10, 2023

It's now possible to change the regex in the settings page. Contrary to what I said previously, I didn't remove the regex compilation check in GetGalleryCoverRegex() (config.go@652). Figure it's better this way, as performance isn't impacted in any meaningful way. Will remove it if deemed better to do so.

@DingDongSoLong4
Copy link
Collaborator

I did some testing and removing the regex check in GetGalleryCoverRegex() actually ends up being disastrous if an invalid regex is passed by directly editing the config file. The current version of stash has a bug (my fix in #3428) where an invalid regex causes all filters to be ignored - which means that the query for the gallery cover returns every single image in the entire database. For my database with >1M images, loading 40 covers on the galleries page literally maxed my CPU and took over 10 seconds.

I also measured the time that only the regexp.Compile call took and for me it was only around 50µs, so there is no real benefit to removing the check even after the bug is fixed.

This also inspired #3424, adding similar validation to all the other regex settings.

@WithoutPants WithoutPants added the improvement Something needed tweaking. label Feb 15, 2023
@WithoutPants WithoutPants merged commit 2d52873 into stashapp:develop Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something needed tweaking.
Projects
None yet
5 participants