Cleanup and TextureCache changes #798

Merged
merged 38 commits into from Apr 4, 2012

Conversation

Projects
None yet
5 participants
@jmarshallnz
Member

jmarshallnz commented Mar 22, 2012

The first 8 commits here are basically cleanups/refactor to make things a little nicer to read, other than 7d27fbc which fixes a bug.

The 9th commit 857bb8a is a bit of a hack - we're updating items offthread, which basically sets the m_invalidated flag. It's not volatile, and we really shouldn't be doing what we're doing, but it hasn't broken yet. A proper fix would be updating copies of items and passing them via messages, but that's a fix for another day.

The last commits are changes to the TextureCache to prepare for video thumbs moving to the cache. Numerous changes here, and probably some more are required before things are a lot nicer, but more discussion is required, and I want to get this in. :)

Features added:

  • Change thumb:// URL format to image:// URL to allow other uses - in particular allowing a VFS later on for image transformation for the webserver/upnp as well as caching of multiple versions of the same image (mipmap, only load the size the image control needs etc.)
  • Store the original URL in the texture db rather than the cached URL.
  • Add fallbacks to the infomanager GetImage() functions so that if the returned image fails to load it can fallback to another image (eg icon).
  • Don't bother with URL hashing for the texture cache as the db is fast enough anyway.
  • Add exporting functions.
  • Refactor the handling of texture re-caching jobs so that it's not done in Get*(). This also reduces the chance (but doesn't fully eliminate) 2 threads caching the same image at the same time (which could lead to corruption).
  • Refactor texture caching so it uses CJpegIO for the loading stage, and swscale for scaling, plus add orientation support within XBMC so we don't need to hit CxImage as much. Basically we only use CxImage now for decoding non-JPG/DDS and encoding to JPG/PNG and writing to disc.
@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Mar 22, 2012

very nice. can't find anything on a (somewhat brief) look-through.

ghost commented Mar 22, 2012

very nice. can't find anything on a (somewhat brief) look-through.

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 22, 2012

Member

Ok, I've switched to .compare(0, len, "foo") to get rid of the strncmp, and switched to the CDatabase::BeginTransaction() et. al.

Should now be good to go.

Member

jmarshallnz commented Mar 22, 2012

Ok, I've switched to .compare(0, len, "foo") to get rid of the strncmp, and switched to the CDatabase::BeginTransaction() et. al.

Should now be good to go.

@ghost ghost assigned jmarshallnz Mar 23, 2012

@Montellese

This comment has been minimized.

Show comment Hide comment
@Montellese

Montellese Mar 23, 2012

Member

Apart from the %% in the LIKE clause it looks very nice. Well done.

Member

Montellese commented Mar 23, 2012

Apart from the %% in the LIKE clause it looks very nice. Well done.

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 25, 2012

Member

I've noticed a few issues:

  1. With the changes for not caching texture prior to loading them in the large texture manager, some URL formats that we use for the cache are attempted to be loaded by the texture loader, but it doesn't recognise them leading to spurious errors. We instead should resolve the cached name to the actual texture and load that (ideally of the size appropriate for caching so we can just pass the texture buffer across to the cacher and save the whole re-load just to cache).
  2. thumb://foo@bar textures are being cached with the filename portion of the original path (bar) being appended. The extension thereof is then used for the cached texture. So BMP's are being cached as JPG or PNGs (depending on transparancy) but are being named .BMP. Worse, embedded movie thumbs are being cached as f/fa1048a8.mkv etc. (The reason we used .tbn originally was actually valid, who would have guessed?)

I've started work to clean this up, so will modify the pull req with my changes in the next few days for re-review - these existing commits won't change, they'll be new commits on top.

Member

jmarshallnz commented Mar 25, 2012

I've noticed a few issues:

  1. With the changes for not caching texture prior to loading them in the large texture manager, some URL formats that we use for the cache are attempted to be loaded by the texture loader, but it doesn't recognise them leading to spurious errors. We instead should resolve the cached name to the actual texture and load that (ideally of the size appropriate for caching so we can just pass the texture buffer across to the cacher and save the whole re-load just to cache).
  2. thumb://foo@bar textures are being cached with the filename portion of the original path (bar) being appended. The extension thereof is then used for the cached texture. So BMP's are being cached as JPG or PNGs (depending on transparancy) but are being named .BMP. Worse, embedded movie thumbs are being cached as f/fa1048a8.mkv etc. (The reason we used .tbn originally was actually valid, who would have guessed?)

I've started work to clean this up, so will modify the pull req with my changes in the next few days for re-review - these existing commits won't change, they'll be new commits on top.

Jonathan Marshall added some commits Feb 23, 2012

Jonathan Marshall
changed: format thumb urls as image://[type@]<url_encoded_path_to_fil…
…e>?<options> so it can be reused for generic image caching
Jonathan Marshall
use image://picturefolder@ urls for picture folder thumbs, rather tha…
…n storing cached thumb URLs in the path->texture db table
Jonathan Marshall
remove hacks for checking auto-thumbs that fail to cache (0-byte file…
…) for whatever reason, now that the infomanager can return the icon in those instances
@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 31, 2012

Member

A big-ish update, which brings this ready to go I think. Changes:

  1. Minor workaround in 857bb8a for threading issues.
  2. Switch from thumb:// to image://[type@][/transform?options] for texture URLs - allows future VFS and transforms for webserver/upnp etc. as per #822.
  3. The extensions used for saving textures are now correct - previously we'd use the original files' extension, regardless of whether that was correct for the cached image.
  4. Refactored the texture caching so it uses CJpegIO for loading (faster) and swscale + new orientation routines for resizing before cache. Less reliance on CxImage functions.
  5. Refactored texture loading with the background image loader. Before it cached to disk, then loaded the cached version. Now it loads into a texture and copies the texture data into a job for caching, returning directly. Speeds up first time texture loading considerably.

I'll note that it's not perfect. Still TODO is:

  1. Figure out how to cache multiple versions of the same file. Possibilities are to cache using the same file with different post-fixes for sizing/orientation info. ATM we just cache to a different hash as the input is different.
  2. Ensure that the cache jobs that take texture memory are prioritised over disk-based caching jobs - reason is they first of all are chewing up a few MB of RAM, and secondly they're already half done. Need to add support to CJobQueue for overriding previous jobs, plus also ideally adding to the front of a FIFO queue, and perhaps also specifying priority (no point pausing those jobs as otherwise we have a lot of RAM sitting there).
  3. Add support for caching to JPG/PNG in memory, for webserver duty (#822)

Please review a8afce6 (new URL format for textures) as well as the last 13 commits primarily - the others are basically unchanged other than rebasing to bring up to date.

Member

jmarshallnz commented Mar 31, 2012

A big-ish update, which brings this ready to go I think. Changes:

  1. Minor workaround in 857bb8a for threading issues.
  2. Switch from thumb:// to image://[type@][/transform?options] for texture URLs - allows future VFS and transforms for webserver/upnp etc. as per #822.
  3. The extensions used for saving textures are now correct - previously we'd use the original files' extension, regardless of whether that was correct for the cached image.
  4. Refactored the texture caching so it uses CJpegIO for loading (faster) and swscale + new orientation routines for resizing before cache. Less reliance on CxImage functions.
  5. Refactored texture loading with the background image loader. Before it cached to disk, then loaded the cached version. Now it loads into a texture and copies the texture data into a job for caching, returning directly. Speeds up first time texture loading considerably.

I'll note that it's not perfect. Still TODO is:

  1. Figure out how to cache multiple versions of the same file. Possibilities are to cache using the same file with different post-fixes for sizing/orientation info. ATM we just cache to a different hash as the input is different.
  2. Ensure that the cache jobs that take texture memory are prioritised over disk-based caching jobs - reason is they first of all are chewing up a few MB of RAM, and secondly they're already half done. Need to add support to CJobQueue for overriding previous jobs, plus also ideally adding to the front of a FIFO queue, and perhaps also specifying priority (no point pausing those jobs as otherwise we have a lot of RAM sitting there).
  3. Add support for caching to JPG/PNG in memory, for webserver duty (#822)

Please review a8afce6 (new URL format for textures) as well as the last 13 commits primarily - the others are basically unchanged other than rebasing to bring up to date.

Jonathan Marshall added some commits Mar 22, 2012

Jonathan Marshall
instead of having background thumb loaders cache images, do it via jo…
…bs instead, and have the item's thumbnail image be the actual image, not a cached images.
Jonathan Marshall
use CTexture::LoadTexture() for loading images for caching, and CPict…
…ure::CacheTexture() to cache the result. Allows CJpegIO for faster jpeg loading and scaling
Jonathan Marshall
Split CTextureCacheJob::CacheImage() into DecodeImageURL and LoadImag…
…e, with the remainder being done in DoWork()
@theuni

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

this adds a dependence on ffmpeg (technically only swscale, but we treat them all as one atm). IMO we should avoid this for the gui as some platforms will need to be able to run without ffmpeg entirely. Is there some (even if really crappy) fallback we could use?

Collaborator

theuni commented on xbmc/pictures/Picture.cpp in 594f633 Apr 3, 2012

this adds a dependence on ffmpeg (technically only swscale, but we treat them all as one atm). IMO we should avoid this for the gui as some platforms will need to be able to run without ffmpeg entirely. Is there some (even if really crappy) fallback we could use?

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

The alternative ofcourse is to fixup the buildsystem to split swscale and libav*, which wouldn't be a terrible idea as there are several places currently where the removal of ffmpeg would mean the removal of unrelated features (screenshots, as an example).

Collaborator

theuni replied Apr 3, 2012

The alternative ofcourse is to fixup the buildsystem to split swscale and libav*, which wouldn't be a terrible idea as there are several places currently where the removal of ffmpeg would mean the removal of unrelated features (screenshots, as an example).

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

Yeah - that's what I figured - we use swscale + ffmpeg for video thumbs.

It's pretty trivial to write a bilinear scaler if we really want one, but it's gonna be significantly slower than swscale - I'm happy to do it if/when a build is required without swscale support ;)

Owner

jmarshallnz replied Apr 3, 2012

Yeah - that's what I figured - we use swscale + ffmpeg for video thumbs.

It's pretty trivial to write a bilinear scaler if we really want one, but it's gonna be significantly slower than swscale - I'm happy to do it if/when a build is required without swscale support ;)

@theuni

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

Could use !IsArchive so that future archive types are picked up here?

Collaborator

theuni commented on f94c833 Apr 3, 2012

Could use !IsArchive so that future archive types are picked up here?

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

Good idea - will check it out.

Owner

jmarshallnz replied Apr 3, 2012

Good idea - will check it out.

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

IsArchive() doesn't exist yet (we do have IsInArchive) so I'll leave as-is for now.

Owner

jmarshallnz replied Apr 3, 2012

IsArchive() doesn't exist yet (we do have IsInArchive) so I'll leave as-is for now.

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

Hmm, could've sworn we did. Ok, will add that in as it'd be worth having in other places.

Collaborator

theuni replied Apr 3, 2012

Hmm, could've sworn we did. Ok, will add that in as it'd be worth having in other places.

@theuni

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

I think the 2048x1024 here is redundant as it should be filtered out later in the process anyway? I always assumed this was leftover from xbox where max texture size wasn't dynamic.

I think the 2048x1024 here is redundant as it should be filtered out later in the process anyway? I always assumed this was leftover from xbox where max texture size wasn't dynamic.

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

It's using a min, so yeah, this is redundant as most resolutions are less than this anyway (and I'm sure if the resolution is bigger than that, certain users will want to use it all anyway) - will remove.

Owner

jmarshallnz replied Apr 3, 2012

It's using a min, so yeah, this is redundant as most resolutions are less than this anyway (and I'm sure if the resolution is bigger than that, certain users will want to use it all anyway) - will remove.

@theuni

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Collaborator

don't forget xbmc.gui/xbmc.json api bumps here too as (if?) needed, i guess one for the whole shebang is enough.

I would guess that lots of skins have hard-coded hacks for .tbn stuff?

Collaborator

theuni commented on 465164f Apr 3, 2012

don't forget xbmc.gui/xbmc.json api bumps here too as (if?) needed, i guess one for the whole shebang is enough.

I would guess that lots of skins have hard-coded hacks for .tbn stuff?

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

Not as far as I'm aware - skins with hard-coded hacks for .tbn tend to be for picking up local art given the path of the item. This basically changes things from storing a hash to storing the actual path, which as far as skins go are equally useless.

For json, in a later commit, we deliberately run it through the CheckAndCacheImage() so that they're still using the cached version - this will be changed later ofcourse, so I'm fairly sure a bump isn't required.

Owner

jmarshallnz replied Apr 3, 2012

Not as far as I'm aware - skins with hard-coded hacks for .tbn tend to be for picking up local art given the path of the item. This basically changes things from storing a hash to storing the actual path, which as far as skins go are equally useless.

For json, in a later commit, we deliberately run it through the CheckAndCacheImage() so that they're still using the cached version - this will be changed later ofcourse, so I'm fairly sure a bump isn't required.

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Owner

Agreed - it's essentially c&p twice in CCurlFile. Could probably just do a map<string,string> CURL::GetOptionPairs() or some such, though the loops processing these don't quite treat things like a map would (eg all repeated options found might potentially have an effect) so a vector<pair<string, string> > might be more appropriate?

Agreed - it's essentially c&p twice in CCurlFile. Could probably just do a map<string,string> CURL::GetOptionPairs() or some such, though the loops processing these don't quite treat things like a map would (eg all repeated options found might potentially have an effect) so a vector<pair<string, string> > might be more appropriate?

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 3, 2012

Member

Ok, change made. I reckon it's ready to go as it's going to be - quick signoff?

Member

jmarshallnz commented Apr 3, 2012

Ok, change made. I reckon it's ready to go as it's going to be - quick signoff?

@theuni

This comment has been minimized.

Show comment Hide comment
@theuni

theuni Apr 3, 2012

Member

I certainly can't vouch for all of it, but it contains several fixes I have locally and addresses lots of stupids we've been doing for far too long. The whole point of this cycle is to introduce bugs anyway, no ;) ?

Signed-off-by: theuni theuni-nospam-@xbmc.org

Member

theuni commented Apr 3, 2012

I certainly can't vouch for all of it, but it contains several fixes I have locally and addresses lots of stupids we've been doing for far too long. The whole point of this cycle is to introduce bugs anyway, no ;) ?

Signed-off-by: theuni theuni-nospam-@xbmc.org

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 4, 2012

Member

Thanks for the review!

@cptspiff anything else you want looked at, or happy for me to push the big green button :)

Member

jmarshallnz commented Apr 4, 2012

Thanks for the review!

@cptspiff anything else you want looked at, or happy for me to push the big green button :)

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Apr 4, 2012

hit it and see what it does

ghost commented Apr 4, 2012

hit it and see what it does

jmarshallnz added a commit that referenced this pull request Apr 4, 2012

@jmarshallnz jmarshallnz merged commit 031dd48 into xbmc:master Apr 4, 2012

@garbear

This comment has been minimized.

Show comment Hide comment
@garbear

garbear Apr 4, 2012

win32: 'sqrt' : ambiguous call to overloaded function while trying to match the argument list '(unsigned int)'

win32: 'sqrt' : ambiguous call to overloaded function while trying to match the argument list '(unsigned int)'

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 4, 2012

Owner

weird - built fine for me on win32, though it was a while back. Probably needs only a (float) in front of files.size(). Please confirm (or just commit the fix immediately).

Owner

jmarshallnz replied Apr 4, 2012

weird - built fine for me on win32, though it was a while back. Probably needs only a (float) in front of files.size(). Please confirm (or just commit the fix immediately).

This comment has been minimized.

Show comment Hide comment
@garbear

garbear Apr 4, 2012

float fixes it, pushing

float fixes it, pushing

@lemonboy999

This comment has been minimized.

Show comment Hide comment
@lemonboy999

lemonboy999 Apr 5, 2012

There seems to be a mistake in GetCast. On the PrepareSQL statement - there should not be a comma after actors.strThumb and there should be a space before WHERE.

There seems to be a mistake in GetCast. On the PrepareSQL statement - there should not be a comma after actors.strThumb and there should be a space before WHERE.

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 5, 2012

Member

You're right, but notice the later commits where an additional variable is retrieved, thus fixing it up again - this issue is only there due to rebasing out multiple changes that I didn't spot properly (and ofcourse it built just fine)

Member

jmarshallnz replied Apr 5, 2012

You're right, but notice the later commits where an additional variable is retrieved, thus fixing it up again - this issue is only there due to rebasing out multiple changes that I didn't spot properly (and ofcourse it built just fine)

This comment has been minimized.

Show comment Hide comment
@lemonboy999

lemonboy999 Apr 5, 2012

Sorry, I might be missing something, but I just looked at the current file on Github and it is still the same. It builds OK as its just a text string, but if you try to look at cast information for a movie or TV show, there is none and the log shows an SQL error. Making the changes I suggested fixes the problem and the cast info is viewable.

Sorry, I might be missing something, but I just looked at the current file on Github and it is still the same. It builds OK as its just a text string, but if you try to look at cast information for a movie or TV show, there is none and the log shows an SQL error. Making the changes I suggested fixes the problem and the cast info is viewable.

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Apr 5, 2012

Member

Ah - indeed, it's in a later commit that is part of my new changes not in as yet. Will fix - thanks for the heads up.

Member

jmarshallnz replied Apr 5, 2012

Ah - indeed, it's in a later commit that is part of my new changes not in as yet. Will fix - thanks for the heads up.

tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment