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

Complete Rewrite of PictureFrame2020 in PictureFrame2021. #57

Closed
wants to merge 1 commit into from

Conversation

cornim
Copy link

@cornim cornim commented Jan 27, 2021

Hi there,

I used your PictureFrame2020 on my Raspberry and I liked it a lot. There were only two things that bothered me:

  1. I wanted a filter based on Exif ratings
  2. Because my pictures are pretty large, there were always hic-ups in the fading transitions when something loaded in the background. Probably because my Pi 3B+ doesn't have that much CPU capacity.

The first issue was easy to fix but for the second it was necessary to set up preloading of the next image while there was no transition activity.
Given the structure of PictureFrame2020.py (e.g. few functions, no classes, lots of global variables) restructuring everything would have been quite messy.
I therefore rewrote the entire application (as PictureFrame2021) to be more pythonic, which is the content of this pull request.
Some features are still broken (e.g. mqtt support, keyboard support, geo location etc.) and dual portrait pictures I dropped all-together as it was incompatible with lazy loading the pictures.

All in all I believe the application is now much better structured and therefore easier to extend but is currently lacking some features and (as the original) does not have sufficient documentation.

If you are interested in merging this, I can repair/improve on some of these aspects but as I don't use/need them and therefore won't do it if there is no interest in merging this on your side.

Best have a look and let me know what you think.

Main feature:
 - Preloading of next picture and text when there is no activity to
   improve performance on older Raspis with larger pictures.

Dropped some other features like mqtt support or dual portrait pictures
mainly because they were too much work to reimplement (might do this
later).

New structure has no more global variables and a clear program flow
using functions/objects where sensible.

Geolocation is currently still broken (but will be fixed soon).
@paddywwoof
Copy link
Member

Cornelius. Thank you very much for this. I will look at the code tomorrow. I'm actually rewriting things now so your input will be really useful. See the discussion on this repo and on GitHub helgeerbe

@paddywwoof
Copy link
Member

Cornelius. I've had a bit longer to look at your code this morning and I have to say it's much tidier than I would have done - even knowing all the extra functionality that's been added over the years. I would be more than happy to merge your mod but it can't break existing setups too badly!

At a trivial level I have tried to keep all the pi3d and pi3d_demos code python2 compatible. Although we are now over a year past the overdue death of p2, and I would dearly like to dump it, in debian systems when you type python or pip at the command line you get python2. Which means that a lot of people new to python get stuck down this blind alley.

More tricky is implementation of MQTT and specifically the back and delete functions which I think would be difficult to implement using the filter as a generator and lazy exif_data in Picture. While looking at using sqlite as a possible way to effectively hold the picture_list I persuaded myself that the only sensible way to do this was with a python list. With your system it might be possible to do something along the lines of making a previous_pictures list as a byproduct of pic_nums_to_display though I can see from your code that you (rightly) prefer non-byproduct functions.

If you look at the discussion here and #55 here helgeerbe/picframe#5 you will see that the hope was to get round the delayed exif reading as part of the filtering by keeping a "database" of the info on the SD card. The question is should that be done as a one-off job when the picture frame starts (it could take a few seconds the first time, or tens of minutes if there many thousands of pictures to catalogue) or should it be done only when the image is being read (like your system but written to a database for subsequent occasions) or (probably my preferred) should it be done when the image is being read and in a background thread or subprocess (which would have to a communication pipe to pause it during transitions)

Anyway, your input would be greatly appreciated.

Paddy

@cornim
Copy link
Author

cornim commented Jan 28, 2021

Hi Paddy,

thanks for having a look and thanks for creating the original setup. I know very little of openGL programming and could not have developed it from scratch.

First of, for it to break no existing setups, was the main reason why I implemented in a completely separate set of files. That way, people can decide when they want to switch over (assuming missing functionality will be added in time).

Regarding the python2 compatibility: I guess the easiest way to deal with that would be to move the entire thing into a PiPy package. That way necessary requirements could be listed an checked via pip at install time.

Back and forth functionality should be easy to implement by collecting shown pictures into a list (as you have said). The other mqtt functionalities should also be fairly easy to implement based on the fact the all loop variables are now grouped in a separate class. I don't see any major hurdles there other than the time it takes to do the work.

I read through parts of the discussion you mentioned and here are some of my thoughts.

  • Reading everything upfront is most likely prohibitively expensive timewise. If people have tens of thousands of pictures it could take hours.
  • Building a cache only makes sense if reading the pictures is what really takes the time. Has anybody ever measured this? Naively I would assume the resizing and blurring are much more time intensive, but then again hard disk access is always slow. So I don't know. Especially since reading from SQLite would also read from disk.
  • I purposely put all loading and transforming actions into the part were no transition is happening to avoid any artifacts which I think is essential for an application like this.

Hence, if I were to extend/improve the loading functionality I would approach it like this:

  1. To not interfere with showing the pictures/transitions all loading would need to happen in a different process as threading is not really a sensible option due to the GIL.
  2. As inter process communication is a pain one would have to build a mechanism either based on queue's or a database with proper transaction (not sure if SQLite is up to the task). This is especially important as race conditions are otherwise easy to introduce and very hard to debug.
  3. Having set up reliable inter process communication one could set up a process for each reading picture and exif data, acquiring geo locations, display the pictures.

I would assume that setting something like this up to work reliably is easily as much work as the entire application was so far and I have to admit that I'm not an expert in inter-process communications in python. My approach would probably be to build it based on queue first (the python stdlib seems to have a good implementation) and introduce caching if performance is still an issue then.

@paddywwoof
Copy link
Member

paddywwoof commented Jan 28, 2021

Cornelius, The main driver for using a cache is as more selection filters are added. You have identified one additional filter but others have suggested location as well as different EXIF data. If the full 10,000 images are being filtered lazily and the date is very specific (Christmas day 2014, or just mistyped, or any other filter) then the picture frame grinds to a halt.

The other reason for having a cache is that it can allow functionality such as the portrait pairs, as you have found that is difficult to do with a sequential system. I'm not sure what other things people will think of in the future but sorting by the date the image was taken or by the average RGB value etc become feasible.

I agree that reading upfront might take an annoying length of time but maybe if the process was wrapped up in a way that could be run stand-alone or started as a background process as part of picture frame it could potentially get the best of both worlds. Sqlite isn't multi-user (like mysql or progresql) but it has table locking and I'm pretty sure that multiple process can access it safely. I have some (but not a great deal) of experience with the multiprocessing module but I have found that it doesn't seem to speed up IO intensive activities as the operating system automatically optimizes those (yes even though python supposedly suffers from GIL) often threading is just as fast. I imagine that the picture frame display/viewer/controller would only read from the cache and all writing would be done by the background process/thread. By default it would just work its way through a directory listing but there would be a queue allowing messages from the viewer: 'stop while I do a transition', 'start again', 'add details to the cache for xxx'. The exif info can be entered as fast as possible but the http requests to nominatim OSM must be kept well below 1 per second so that might need its own process/thread.

@cornim
Copy link
Author

cornim commented Jan 29, 2021

Your case regarding very specific filters makes sense. I just wanted to point out that caching, incl. delta updates and invalidation is a work intensive topic. Could be slightly easier here as file mtime could be used.

My suggestion regarding processes in favor of threading was because threading uses only one cpu in python whereas processes would run on a different cores and therefore alleviate the issue of stopping the per-processing while transitioning from one picture to the next.

I read up a little on multi-process access to SQLite and the answer is: It is supported, but... (see question 5 here).
At the very least a robust retry logic would have to be build and the DB would have to be queried for every picture update to not miss out on (possibly frequently occurring) updates to the DB by other processes. In case of multiple updating threads there would also have to be some kind of completion tracking.

@cornim
Copy link
Author

cornim commented Feb 8, 2021

Paddy,
as you are apparently not planning to move forward with this pull request in the short term, would you be ok with me publishing my rewrite (nothing else of course) as a GPL licensed PyPi package?
Cornelius

@paddywwoof
Copy link
Member

paddywwoof commented Feb 8, 2021

Cornelius, of course not. It's all supposed to be open source for everyone to use without restriction. I'm sorry I haven't corresponded more but I've been concentrating on the re-write based on @helgeerbe's re-write. There are disadvantages with that structure too (personally I wouldn't have used as many underscores, setters and getters) but I thought there would be less things to add in later (MQTT, portait pairs, home-assistant communication, geolocation etc)

The sqlite image cache system seems to run pretty fast - I will maybe add a couple more tweaks to make it even less obtrusive on the very first run. The logic isn't too complicated at all really with just one threaded loop https://github.com/paddywwoof/picture_frame/blob/develop/picframe/image_cache.py#L36

On the subject of threading in python: people often dismiss them because they're not proper pthreads but I have often found them to be no slower than more complicated systems using multiprocessing, which only really seems to give a speed advantage where there are quite long-running CPU intensive jobs to be farmed out.

Anyway, keep in touch if you have any questions - or good ideas!

Paddy

@cornim
Copy link
Author

cornim commented Feb 11, 2021

Thanks for the quick reply. I'll let you know once I published something.

@cornim
Copy link
Author

cornim commented Feb 22, 2021

Paddy,
just wanted to let you know that I published a version on Pypi.
I added a feature which allowed me to adjust the probability of a picture to be shown based on the number of times it has been shown in the past. Therefore I had to build something which stores all pictures. I used SqlAlchemy so any kind of db can be used. Prefilling of db runs in a background process.
Feel free to have a look and let me know should you have any questions.
Cornelius

@cornim cornim closed this Feb 22, 2021
@paddywwoof
Copy link
Member

Cornelius, thanks for that, I will have a look later. I noticed you have the libatlas BLAS libraries as a dependency for numpy - are they not already there? I always assumed they were, at least on the standard raspberrypios. pi3d uses numpy a lot so that might make a difference to general animation performance (where there are lots of Shapes happening)
Paddy

@cornim
Copy link
Author

cornim commented Feb 22, 2021

On my Pi3B+ with a clean installation of raspbian buster libatlas was missing which caused an exception when automatically trying to install numpy with pip. This is why I added it to the installation instructions.

@paddywwoof
Copy link
Member

Ah, were you starting from raspbian lite (I'm sure numpy is already on the full-fat version)? Mind you I've done that many times and never had to install the BLAS libraries - just numpy. Maybe it's something that's been split out on the latest ISO to increase liteness. Time to burn a new SD card and double check my FAQ page.

@cornim
Copy link
Author

cornim commented Feb 22, 2021

I used this version which is called Raspberry Pi OS with desktop (not the Raspberry Pi OS with desktop and recommended software version).
I guess it is also important to note that I installed numpy via pip not apt-get.
One question from my side: Is numpy an optional or a mandatory requirement of pi3d? I'm asking as it is not listed as a requirement for the pi3d pypi package.

@paddywwoof
Copy link
Member

Cornelius, well that is odd. I've not made an image from that update but certainly on all previous (and I think on the more recent lite images) numpy was already installed. I will check that out now.

Yes numpy is a requirement now and it should go into the setup.py and on pypi, well spotted. Originally pi3d used python and ctypes to do the matrix multiplication and array stuff but numpy is much faster.

@paddywwoof
Copy link
Member

Cornelius, following up. I burnt a new SD with the 2021-Jan-12 desktop without extra junk version and it has numpy already on. When I check what /usr/lib/python3/dist-packages/numpy/core/_multiarray.....so was built with it shows libblas.so.3 (and others) so I think everything required for numpy is also there.

pip3 install numpy says it's already installed.

So it does seem strange that it was missing on your earlier download.

Anyway I will add the requirement to pi3d, so thanks for pointing that out.

@cornim
Copy link
Author

cornim commented Feb 23, 2021

That is indeed strange. Thanks for letting me know though. I'll remove the requirement from the installation instructions then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants