Add useCache option to python ControlImage.setImage function

[jdieter]

Using the current official slideshow screensaver addon, it seems that sometimes the slideshow pictures are cached - at low quality - in the texture cache. I've gone ahead and added a useCache option to the setImage function for images. The default is true (which means all current plugins work exactly the same), but if you set it to false, the texture cache will be completely bypassed when loading the picture.

If you apply this patch, and then change the following line in the slideshow addon, XBMC will no longer try to cache the pictures.

-                cur_img.setImage(img[0])
+                cur_img.setImage(img[0], False)

Most of the work is passing the useCache option from the python interpreter to the code that actually loads the image.

Also, I'm not sure if it's intended, but only one of the two different image loading paths (LargeImage) is caching textures. It seems that regular Images are already bypassing the texture cache.

[jmarshallnz]

First off, let's find the real issue: If it's caching images, why is it caching low resolution copies? Is it due to the size of the images not satisfying the fanart size rule, thus being cached at thumbsize? Please take a look at the sizes of the originals and the sizes of the cached versions in your texture cache.

Secondly, is it a problem with the sizing, or is there a problem with the caching beyond this? Seems to me that if the image is going to be displayed more than once, we probably want to cache.

Thirdly, I wonder whether this might be better associated with the control, rather than specified externally. After all, this is basically what the cache is designed for: displaying images within the skin fast. For a slideshow, you probably prefer the original image, right?

Fourthly, you'll notice the other (non-caching) path won't apply to images that aren't already local (skin images primarily).

[jdieter]

Ok, before I fix the problems you mentioned in the code, I'll answer your questions about the real issue and see if it's still worth getting this patch merged.

1. I wasn't aware that there was a fanart size rule (though it makes sense). The pictures being cached are mainly my extra-high resolution (at least compared with my other photos from 2006) wedding photos. The originals are around 9Megapixels or roughly 3-4MB in size. The versions in the texture cache all have a height of 810 pixels, whether portrait or landscape, with the width being scaled proportionally, and are also pretty low quality JPEGs (I'm guessing 60-70% quality).
2. I don't think there's much point in caching images in a slide show. They may or may not be shown more than once, but they shouldn't be shown twice before all the other photos have been shown at least once, which means you need as much space to cache the photos as you do to store them in the first place, unless you're going to reduce their quality (which goes against the whole point of a slideshow screensaver, I think).
3. I'm not sure I understand what you're suggesting. Are you suggesting a new image control like LargeNoCacheImage (ugly name, I'm sure we could do better) that combines the background loading of the LargeImage control with the lack of caching of the Image control? If so, I really don't mind implementing that instead. I just thought a flag for caching would be simpler.
4. That makes sense, at least from the internal xbmc point of view. At least from an addon's point of view, it might make sense to document that an Image control's picture won't ever be cached, while a LargeImage control's picture might be.

So, in summary, even if there is a problem with the caching logic in (1), I think it still makes sense to bypass the cache completely for a slideshow screensaver, which means either providing a flag or a new control.

[jmarshallnz]

1. There's no difference at all in terms of controls. i.e. it's completely decided internally as to which loader to use, and thus whether things should be cached or not: Anything that's not a skin image is cached. The skinner or programmer has no control over this - i.e. there's a single image control.

There's another way this could be done: It could be done via a special URL construct. You'll notice you're just passing the boolean alongside the path all the way through.

I'm not sure what should be in control of the caching. Should it be the control, or the code that sets images? I don't see a reason why you'd want to stop caching selectively for a particular control, but on the other hand, I'm not sure that should be something the skinner would wish to control (though perhaps it could be done programatically on the control via the python/internal API).

[MartijnKaijser]

I think the Python /API would be the one who needs to be in control. It shouldn't be the skinner.

[jdieter]

Re: 4. One of the things that helped me realize that there was some kind of selective caching going on was the fact that if I changed the control type from "largeimage" to "image" in the screensaver.xbmc.slideshow resources, the caching was always disabled, so there's at least some kind of link between which image control is used and whether the image may be stored in the texture cache, at least for local files.

Yes, we could do a URL construct if that was preferable to passing a boolean all the way through. Does xbmc mind passing parameters as part of the URL? Something like file:///foo/bar?no-cache=1?

I really don't know enough about skinning and XBMC in general to have an opinion on what should be in control of the caching.

[dersphere]

Another good example where caching makes no sense or is even counterproductive is showing pictures from a webcam.

1. caching makes it impossible to show multiple images from the same URL (http://webcam/live.jpg).
2. these pictures won't never be shown again

@jdieter: XBMC already has some kind of internal URL Parameters, e.g. http://host/path/file?get_param1=get_value&get_param2=get_value2|xbmc_param1=value&xbmc_param2=value

[MartijnKaijser]

@jdieter
can you update the PR according to the done comments. Consensus is the control should decide to cache or not.

[jdieter]

Ok, I think I've addressed the minor coding issues raised by jmarshallnz. Are there any architectural changes you'd like me to make?

P.S. I've rebased my patch to current master, not sure if that's appropriate or if anyone cares.

[jmarshallnz]

Looking good. Fix up the last bits then squash down into a single commit. Will go in the November merge window.

[jdieter]

Ok, I've cleaned up the last two items you listed and have squashed the four commits down into one. Let me know if there's anything else. Thanks for reviewing this.

[jmarshallnz]

Looks good. Queued for Nov merge window. By the looks it might need some rebasing around that time. I'll ping you when the time comes (or just do it myself if I have a spare 10).

[jdieter]

Ok, sounds good. Thanks again.

[MartijnKaijser]

@jdieter
could you rebase your PR?

[jdieter]

It's now rebased.

[MartijnKaijser]

jenkins build this please

[jmarshallnz]

@MartijnKaijser will need a python API bump, right?

[MartijnKaijser]

Correct. Will do tonight