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

[guilib][vfs][imagecache] Load special images into texture cache when viewed, like standard images #23197

Merged
merged 3 commits into from
May 8, 2023

Conversation

rmrector
Copy link
Contributor

Description

Build a new interface to "pull" special images, like generated video thumbs image://video@nfs...., into the texture cache when displayed, like regular images and embedded music covers.

I moved the embedded music and video images that already behave like this to the new interface.

Adds a new top-level folder imagefiles and namespace IMAGE_FILES, that I hope to populate with a lot of the TextureCache stuff that exists now as I push and pull it around. I chose not to use the term texture because that seems more useful as a skin / GUI / rendering term, while this is more general. This is also used heavily for remote interfaces - web like Chorus2 and remote apps like Kore. Name borrowed from CImageFile, the handler for image://... VFS file paths - I tried imagecache for awhile, but this is also more than just caching.

The special images this is meant for are video, still image thumbnails generated from videos; picturefolder, which are generated grid images for folders viewed in the picture browser; pvr, which are generated grid images for channel groups in PVR; and video chapter thumbnails. They each have some specific behavior to deal with so I'll work with them in separate PRs. I've also seen hints of epg but I don't know where that is generated yet.

This is a trimmed down and commit-organized version of #23134 to be more reviewable, as requested by lrusak.

Motivation and context

Consistent behavior: all images load the same way, whether they are already cached or not. Remote interfaces can show these images even if they haven't been viewed in Kodi yet. Kodi doesn't have to pre-cache a potentially massive list of images when browsing folders, just in case they are viewed, saving both storage size in the cache and taking queue time for images actually displayed in the GUI. These special images can now be preloaded the same as other images.

Easier maintenance and development: this provides a clear interface to load more special images, avoids the complication of preloading a big list for this pull-based functionality, loading for view and preloading work just like other images.

How has this been tested?

I've been dogfooding this as part of #23134 for a month.

What is the effect on users?

None, this is trimmed down to just a new interface and moving code that already functioned exactly like it.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Copy link
Contributor

@lrusak lrusak 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. Much easier to follow. I left a few comments, some are relevant in multiple places but I don't want to spam the PR.

xbmc/imagefiles/SpecialImageFileLoader.h Outdated Show resolved Hide resolved
xbmc/imagefiles/SpecialImageFileLoader.h Outdated Show resolved Hide resolved
xbmc/imagefiles/SpecialImageLoaderFactory.h Outdated Show resolved Hide resolved
xbmc/music/MusicEmbeddedImageFileLoader.cpp Outdated Show resolved Hide resolved
xbmc/music/MusicEmbeddedImageFileLoader.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoEmbeddedImageFileLoader.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoEmbeddedImageFileLoader.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoEmbeddedImageFileLoader.cpp Show resolved Hide resolved
xbmc/imagefiles/SpecialImageLoaderFactory.cpp Outdated Show resolved Hide resolved
xbmc/music/MusicEmbeddedImageFileLoader.h Show resolved Hide resolved
Copy link
Contributor

@lrusak lrusak left a comment

Choose a reason for hiding this comment

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

Just a couple minors.

I also noticed that you may be having some issues with the formatting. I recommend using this git hook (if possible on your platform). https://github.com/barisione/clang-format-hooks

xbmc/imagefiles/SpecialImageFileLoader.h Outdated Show resolved Hide resolved
xbmc/video/VideoEmbeddedImageFileLoader.h Outdated Show resolved Hide resolved
xbmc/music/MusicEmbeddedImageFileLoader.h Outdated Show resolved Hide resolved
@rmrector
Copy link
Contributor Author

Updated.

Yup, I'm gonna need a git hook.

Copy link
Contributor

@lrusak lrusak 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. Thanks for sticking with a few rounds. Feel free to rebase and squash the fixup after a successful build.

@rmrector
Copy link
Contributor Author

For sure, thanks for working with me.

A factory around them, and the expected consumer. This commit adds no
functional change, just a bit of wiring.
to the new loader interface and factory, posters and fanart. This is
just moving some code around, this is still the same functionality
called at the same point during execution.
to the new loader interface and factory. This is just moving some code
around, this is still the same functionality called at the same point
during execution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: FileSystem Filesystem Component: GUI engine Type: Feature non-breaking change which adds functionality v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants