Use LRU caching for all pixmaps

[kiddac]

https://github.com/OpenViX/enigma2/commit/df56a9ccc9c3a5fe4cf417c1fc80f52de76ec8b3

You have wrongly made the assumption that all pixmaps are static and never change. This is not the case. This caching of pixmaps has now broken all my skins that bring in cover art for channels as they use the same png name. To fix this I am now providing all my users a copy of the OpenATV LoadPixmap.py.

Can you please undo this LRU caching for all pixmaps, there was nothing wrong with the original LoadPixmap.py Thanks.

[SimonCapewell]

Something I've just noticed is that the c++ side has static caching already for loadPNG with no file updated or LRU refresh logic; you can see the code in epng.cpp pixmapFromTable. loadJPG on the other hand doesn't. @kiddac did you ever try to use PNGs for these refreshing images whilst developing the feature, or were they jpgs from the start?

So my current understanding is that having the caching in the Python side isn't right. PNGs will currently be cached twice in memory. Once in the C++ layer, once in the Python layer which will be using more memory that necessary. However a question mark still hangs over why the graph EPGs 50 pngs are so much quicker to load when cached in static Python variables. My guess would be marshalling between the 2 layers, but it's just a guess.

[kiddac]

My rotating cover images were originally pngs, but they could be reduced alot in size by using jpgs. So I now use jpgs for these images.

I don't fully understand what you are saying, but if I run a job Manager (script runner), to change these images manually. They are currently cached (with old code) until I fully exit out of menus. Once you go back in menus they reload. So there is some sort of caching going on already. Which is fine.

I don't know how or why this pixmap caching has come about, but if its to overcome the hundreds of picons on channel select and epg, why don't you just cache the picons. (if they are not already)

I know cache works differently on linux as it always tries to fill the cache, but through my experience a full cache on these boxes actually slows them down, rahther than speeds things up. Most people even on newer boxes still use cacheflush plugin to constantly keep things buzzing along nicely.

[AbuBaniaz]

The only recommendation for usage of cache flush plugin is for the original duo receivers with leaking memory fault. You won't find any experienced users on the Vix forum recommending usage of cache flush plugin on recent receivers.

[kiddac]

This maybe the case AbuBaniaz, but try using your box with 75% memory full and then running cacheflush plugin to reduce it to about 40% memory and notice the speed difference in response from your box. Most people in your team, me included have a fast box (dual core/ quad core lots of ram etc), but not every joe public does)

[IanSav]

@kiddac It is more than just the picons that benefit from caching.

I still think that verifying the cache contents is the better way to go. The cache can be at the C++ or Python layers. It shouldn't be in both. ;)

[IanSav]

@kiddac: Have you tried using the "cache=False" on your LoadPixmap commands and giving the test code to your OpenViX users reporting problems? That should confirm if the new caching is the cause of the reported OpenViX issues. (This code change should be fully compatible on most, if not all, Enigma2 images.)

[RobvanderDoes]

The only recommendation for usage of cache flush plugin is for the original duo receivers with leaking memory fault. You won't find any experienced users on the Vix forum recommending usage of cache flush plugin on recent receivers.

I wholeheartedly second that. Forced emptying cache really slows down a a box. Even the oldest, smallest and slowest boxes only deteriorate by doing that. It is impossible to improve the very good Linux memory manager by simply and brutally emptying the cache.

[SimonCapewell]

Yes, PC users have been sold memory maximizer snake oil for years, which do little more that clear the cache and then show how much memory is free. Having memory sat there doing nothing isn't making good use of it unless you know you're going to need it for transient spikes of demand. Where cache flushing works is where you're short of memory and the cache is too large for the total memory or is holding onto the wrong data, forcing swapping to disk when these spikes of demand come in.

It's worth pointing out that the cache for PNGs in epng.cpp isn't a proper cache, it's just a static store, from which images are never expired. This is probably why loadJPG doesn't have the same mechanism - it'd be a very bad idea to cache every photograph you view as you browse through the media plugin!

[IanSav]

Are you thinking of improving/fixing the C++ caching and adding the JPG cache or moving the caches to Python (and fixing them there)?

[SimonCapewell]

Architecturally, you'd want the caching to be in the C++ side with expiry and some control over whether or not to cache on a per call basis and to apply to all image types. Unfortunately that's a naieve assumption. It's going to have to be on the Python side, reason being these performance results:

Loading PNGs No Python caching (cache on c++ side only. First load fetches from disk, subsequent loads bypass disk Edit: No they don't. These results ended up being for loading from disk every time due to the c++ caching being a ref count cache)

Image Size	1	2	3	4	5	6
5k	8ms	2	2	1	2	2
50k	35	22	26	28	22	22
500k	576	432	450	477	408	406

Python caching (First load fetches from disk)

Image Size	1	2	3	4	5	6
5k	3ms	1	1	1	1	1
50k	26	1	1	1	1	1
500k	450	1	1	1	1	1

So if the bulk of your images are ~5k, an image loads twice as fast. I also observed that resolveFilename, which is used a lot when setting up a call to loadPixmap, typically takes 1ms per call, so costs as much as fetching the image from the Python cache!

Where things start getting a little odd are with the larger images. Caching Python side becomes much more beneficial as there seems to be quite a cost getting the image data across to the Python side. Someone with some knowledge of how the Python layer communicates with the c++ layer would need to look into why this is.

What we can say is that the Python cache is demonstrably a good idea, however my current thoughts are

• Don't cache jpgs by default, their usage is more likely to be large, infrequently needed images and there are none in the 10 or so skins I have installed (including some of kiddacs installed in Dec 2017). Obviously there are exceptions like kiddac's tiled screen above, but this must've had acceptable load times without any caching
• Cache pngs by default, keeping the current strategy of no update checking, simply because the c++ layer's loadPNG has been doing that for 5 years with no issues
• Allow override of the default behaviour using cached=true or cached=false

[SimonCapewell]

Just to add that looking at the new cache usage, typical usage is around 120 pngs after browsing the EPG, movielist, timelist and main menu for Simple 1080 and Confluence. With Kiddac's Onyx the cache floats at around 175 pngs. Note that these measurements were take with picons loaded using LoadPixmap rather than loadPNG.

[RobvanderDoes]

Can this be closed as fixed by https://github.com/openvix/enigma2/commit/80abf40a6a019bcf9bff50f3921d3af29a1ae733 ?

[davesayers2014]

Sorry is the issue fixed by using jpg rarther than png? As the images still don't change without restarting gui unless I change them to jpg. Using vix 5.2.026

[IanSav]

The solution chosen to address caching problems was to make .png images to be cached and .jpg files to not be cached. You can still override .png caching by nominating "cached=False" in the LoadPixmap call..

[davesayers2014]

Thanks so how do I modify the toppicks.xml?

To

<ePixmap pixmap="cached=False"q-toppicks/toppicks2.jpg" position="806,386" size="246,327" backgroundColor="mid" transparent="0" zPosition="2" />

Will using "cached=False" cause issues with images that don't have this feature?

[IanSav]

Direct access to the cache control is via Python code and not the skin.

Adding cache control to the skin could be an interesting enhancement. :) I would suggest that all images be cached unless a specific request to not cache is made. That is, add an attribute cached="0" to disable caching and cached="1" is an optional no-op attribute that confirms the built in default of caching is enabled.

[davesayers2014]

Ok I'll use jpg rarther than png. These images are brought via script in crontimer as above then the skin displays them.

[SimonCapewell]

Yes, whilst it would have been possible to add a cache flag to ePixmap, it wouldn't have any effect on pngs due to caching in the C++ side of E2. Using jpgs is the way to go at this stage.

[kiddac]

Closed as fixed????? Quite simply dsayers, the issue is not fixed, and they seem unprepared to back track this unnecessary pixmap caching. Using jpgs isn't a solution because it wasn't broken in the first place. More and more people are beginning to complain about it this, but what is the point in spending days trying to explain the problem to be totally ignored.

[ViX-Sicilian]

Commits causing this issue should all now be reverted.

[IanSav]

Rather than going backwards, why not add the skin cache control I suggested above? With the cache controllable in both code and the skin we could then allow caching for all images while allowing coders and skinners to turn off the caching where appropriate or where caching has undesirable effects.

It is interesting that the C++ level caching, that has been present for a long time, was not contributing to the issue. That said, perhaps the C++ level caching should also be removed to make all caching uniform at the Python layer (where performance was shown to be improved).

[RobvanderDoes]

Going backwards is soooo much easier....

[IanSav]

"Easier" rarely equates to "better". ;) Let's not throw the baby out with the bath water.

In this case it has been shown that caching can improve the performance of many Enigma2 skin / screen displays. The users who will benefit from the caching should not be excluded from consideration against the issues that caching has triggered.

A suggestion has been made on how the issue can be resolved. Why can't this suggestion be considered?

[ViX-Sicilian]

I reverted these because no one has been able to provide a working fix. If a solution is found please submit a PR or post the files so that we can test.

[SimonCapewell]

Does the revert provide the desired behaviour. As I mentioned earlier, there's caching in epng.cpp loadPNG which may continue to prevent pngs from refreshing: https://github.com/OpenViX/enigma2/commit/30300b8a686fde636cf21c1f9786c92b23a49d91

Removing the c++ layer caching may help, but there may also be places where the c++ layer is calling loadPNG directly that would be impacted. Careful consideration is needed before changing.

[IanSav]

@Simon, do you have the interest and time to fix this mess? That is, in my opinion:

1. Restore the reverted cache code
2. Remove the C++ PNG image cache code
3. Add Python caching for JPG images as well as PNG images
4. Add a cache="0" attribute to the ePixmap skin code to allow the cache to be disabled on demand for skin based images (in addition to the Python code based LoadPixmap cache control).

[SimonCapewell]

I've probably got the time. What I haven't got is any idea how to do a typical dev cycle on the c++ side. How do I do the build? How do I get my modified source into the build? Where do I edit my branch? Do I need to push it up to my github repo every time I want to do a build?

How to get up and running with a dev environment should really be in the readme or a wiki on the github repo - it'd be much better to have a coherent set of instructions, rather than hints being fragmented amongst various forum posts.

[Huevos]

Simon if you want I can add a feature branch to my enigma repo and build it for you. Then once you are happy with it we can push to dev branch for wider testing.

[SimonCapewell]

Thanks, that might be an option, but the ideal would still be to have an easily maintainable step by step guide though.

So, after a bit of digging around, I've found a comment against the original commit of the c++ code in epng.cpp that mentions the caching there as being ref counted. The code is quite well commented re deadlocks and pitfalls, but doesn't mention this vital fact and to someone not well versed in this area, this isn't obvious. What this means is that PNGs tend to be cached on the c++ side, but only for the lifespan of the screen or component that refers to them. This means that the caching has no benefit for static screens like the main menu where you might have a pretty icon (e.g. Confluence skin). For something like the graph EPG, loads of graphics get loaded every time you open the screen, none of these will be loaded from cache, and as you scroll around, the graphics are referred to from member variables, so they don't use the c++ cache there. So for example if you make a change to the timer icon on the graph EPG cells whilst the graph EPG is open, it isn't reflected until the screen is destroyed and reopened.

A quick test without the Python caching shows this mixed bag. I can hack around with said menu icons and they're loaded on the fly next time I open the screen, however if I hack around with icons related to the volume control, they aren't picked up as this screen is loaded at startup, so retains its reference. Opening and closing the graph EPG picks up any changes too.

So, I'm intrigued by this c++ caching as it appears to have rather limited benefit with my current understanding of the code. The scenarios where it's likely to kick in are:

• Skins where the same png is referred to multiple times within a screen
• Skin frames when opening cascading dialogs (assuming the original dialog remains in memory)
• Code like this, where png2 would be loaded from cache:

  png1 = loadPNG("big_picture.png")
  png2 = loadPNG("big_picture.png")

Doing this would probably load from disk both calls

png1 = loadPNG("big_picture.png")
png1 = None
png2 = loadPNG("big_picture.png")

[AbuBaniaz]

You can define your branch to build from in site.conf. I don't build regularly, but I have a branch called build_branch. Enigam2 is built from there. I would strongly recommend you add the lot of baggage plugins we keep sifting through during a build for no reason into the plugin not wanted.

SCONF_VERSION = "1"
BB_NUMBER_THREADS = "32"
PARALLEL_MAKE = "-j 16"
BUILD_OPTIMIZATION = "-march=native -O2 -pipe"
DL_DIR = "/home/openvix/sources"
INHERIT += "rm_work"
ENIGMA2_URI = "git://github.com/AbuBaniaz/enigma2_OpenViX;protocol=git;branch=build_branch"
DISTRO_FEED_URI = "192.168.0.70/feeds/${DISTRO_NAME}/${DISTRO_TYPE}/${DISTRO_VERSION}/${MACHINE}"

[SimonCapewell]

Thanks. I'll have a go with that later.

Meantime, results from a proper test of the c++ caching show that it's as fast as the Python caching. Sub millisecond time to call LoadPixmap when the image is in the c++ cache regardless of image size.

[ViX-Sicilian]

Simon, you should have access to the guest dev room on our forum, please confirm you can have access.

[SimonCapewell]

Yes

[kiddac]

and you can't add the cache control to the skin, because that would make the skin crash in any image that doesn't have this parameter.

[RobvanderDoes]

The days of one skin fits all are long over, years ago. That's why we have dedicated skins for ViX, that's why I have a skin for ViX, one for PLi and one for ATV. Skinning and maintaining skins does take some effort, and you can't expect an image to be adapted for your skin; that would be the world upside down.

[SimonCapewell]

@kiddac Does that happen in other images? If I add a load of rubbish attributes to the end of an ePixmap element Vix ignores them and outputs [Skin] Attribute not implemented: thisdoesntexist value: whatisthisrubbish to the log.

I'm considering using a file modified check anyway, pending performance tests.

[Huevos]

I'd rather use an md5 signature if possible. Not sure how much overhead there would be for that.

[SimonCapewell]

It'd need to be measured to test its effectiveness. What it would do is cause an IO request for each loadPNG call, which a modified date check won't. The png data may be returned from the filesystem cache which would reduce the impact of the load.

[Huevos]

kiddac alleges that the same issue affects a number of Enigma2 plugins. The allegations have never been investigated and don't seem to have any basis.

kiddac has not been forthcoming with his code (which, in my opinion, is most probably where the underlying problem is).

On the issue of image load problems in the IMDb plugin, it's impossible for changes in LoadPixmap() to affect the loading of movie posters in the plugin, because the IMDb plugin doesn't use LoadPixmap() (or even loadJPG()) to load its poster images, it uses ePicLoad(), which is completely uncached. Almost all issues with information not loading in the IMDb plugin are due to IMDb's content management system changing its generated HTML and breaking the screen scraping regular expressions.

[Huevos]

@SimonCapewell, if filemtime gives the desired effect without the IO request maybe we should probably go with that. We use filemtime in ABM for the caching of providers' files and it works well there. Several seconds are saved not reading those xml files. And when new files with identical filenames are imported the cache updates itself.

[prl001]

I'd rather use an md5 signature if possible. Not sure how much overhead there would be for that.

The architecture of loadPNG() makes it hard to directly access the file bytes to calculate any file checksum. Also, calculating a checksum requires the file to be read, so the caching only saves decode time, and not file I/O time.

[kiddac]

"The days of one skin fits all are long over, years ago.".

.. er no.. for the last 3 years I spent a lot of time making my skins work for as many OE-alliance builds as possible. Yes it is a lot more effort which is why most skinners don't. But it is still rewarding to do so, and then frustrating when 1 image decides to break things.

"you can't expect an image to be adapted for your skin;"

This wasn't broke in the first place until someone decided to change it.

@SimonCapewell I stand corrected as I have just tested this and I thought this would throw up an error and therefore be a problem for anyone that had 'crash on skin error set'. It only throws up a warning, so its not a problem.

@Huevos I initially said this may cause potential problems for plugins. It was made clear to me plugins we can work around in the plugin code. The main concern is skins that do clever eye candy by swapping images. As for not being forthcoming with my code. I explained what my code was doing. For example. I might have 10 images in a folder. tile1.png tile2.png tile3.png

Then via a cron, I run a particular py file which scrapes the new images and then swap these images out above with the new images. Using the PIL library to do some resizing and combing first. This is a jpg example but some I use pngs.

im.save(skin_folder + image_folder + image_prefix + str(x) + ".jpg",quality=85)

But as you have cached these images in my image folder, they will never change unless I restart GUI.

And yes the underlying problem is it does effect all my skins. Hence why I brought up the issue in the first place. And if it was such a minor throw away issue why is this now one of the longest threads on your github. I have raised a very valid point and I am glad people are looking into the issue rather than just being dismissive of it.

No one has actually explained the real benefit of this change. Its 2019, nearly all boxes are duo core or quad core and memory sizes are being ever increased. Is saving a few nano seconds for the load time of a screen of any real noticeable benefit if it's potentially causing other issues. Or is this change to help people with older boxes, in which case why do certain individuals keep saying we need to move forward.

[AbuBaniaz]

We have reverted the changes.

[RobvanderDoes]

"The days of one skin fits all are long over, years ago.".

.. er no.. for the last 3 years I spent a lot of time making my skins work for as many OE-alliance builds as possible. Yes it is a lot more effort which is why most skinners don't. But it is still rewarding to do so, and then frustrating when 1 image decides to break things.

Indeed: when not using all options and innovations it's quite easy to make a 'one skin fits all'. But that easy way out is not what I meant.....

I explained what my code was doing. For example. I might have 10 images in a folder. tile1.png tile2.png tile3.png

Then via a cron, I run a particular py file which......

That's a description, not code. And adding 'a particular py' to a skin is not done; it's changing E2. As I said before: you can't expect an image to be adapted for your skin; that would be the world upside down. And using 'a particular py' even means it's impossible to take a skin in consideration. That's why we have dedicated skins on our feeds for which we do take care.