Safely save frames and images

[Baharis]

This PR fixes several deadlocks in Instamatic, including 1) ones resulting from clicking "Save frame" or "Save image" during an ongoing experiment or 2) from repeatedly starting and stopping the alpha wobbler in the controller frame. As a side effect, it replaces save_frame and save_image jobs with new VideostreamFrame.save_frame and .save_image that can be called in the current GUI thread from anywhere. This can result in significant interruptions or lags when run during intensive processes, but it enables FastADT to neatly save its tracking result or to save images annotated via VideostreamFrameProcessor:

A single annotated PNG (~660 KB / image @ 516x516 px)	An animation combining all saved PNG files

Minor changes

• fast_adt/experiment.py: Will now save annotated tracking results to sample_#/tracking/image#_al#.png;
• gui/gui.py: Modified/fixed the MainFrame.__init__ to give VideoStreamFrame access to the AppLoader via .app;
• gui/jobs.py: Removed save_frame and save_image jobs and...;
• gui/videostream_frame.py:VideoStreamFrame: ...added .save_frame/image methods with frame/image and path attrs;

Bugfixes

• camera/__init__.py: Add FakeVideoStream, LiveVideoStream imports to avoid errors at module import;
• gui/ctrl_frame.py: Reworked toggle_alpha_wobbler so that running the wobbler repeatedly no longer crashes the GUI;
• gui/fast_adt_frame.py: Editing tracking/diffraction time no longer produces a ZeroDivisionError in the console;
• gui/fast_adt_frame.py: Fixed the bug starting experiments, adapted the code, removed imports after Multi-tracking in FastADT experiment #145;

Code maintenance

• gui/videostream_processor.py: Removed unused import statement.

[stefsmeets]

As a side effect, it removes save_frame and save_image jobs, replacing them with new VideostreamFrame.save_frame and .save_image that can be called in the current GUI thread from anywhere.

Heh, this is sort of a regression, because it used to be like this a long time ago. It turned out to be a bad idea, because saving images is quite resource intensive and will block or interfere with the measurement loop as well as the GUI. In general, this can be an issue with measurements which require predictable timings, lik CRED.

The reason for the 'job system' is to handle all io-bound tasks on a thread so that they can be scheduled together with the measurement/microscope control. This gives more control over performing the tasks when the measurement thread yields (e.g. during non-blocking tasks, like collecting an image for 0.5 s).

Just a heads-up ;-)

[Baharis]

Heh, this is sort of a regression, because it used to be like this a long time ago. It turned out to be a bad idea, because saving images is quite resource intensive and will block or interfere with the measurement loop as well as the GUI. In general, this can be an issue with measurements which require predictable timings, lik CRED.

The reason for the 'job system' is to handle all io-bound tasks on a thread so that they can be scheduled together with the measurement/microscope control. This gives more control over performing the tasks when the measurement thread yields (e.g. during non-blocking tasks, like collecting an image for 0.5 s).

Just a heads-up ;-)

Thanks for the heads up, I was worried this might have been the case. Still, I would love you to change my mind on the fact that this regression might be better overall:

With this "regression" implemented, if you decide to grab a frame during a job that requires predictable timing like CRED, you might mess it up. Your choice. it may mess the job up, and for the future you will know, but maybe making the photo was important e.g. because you know job will fail but you see something interesting in diffraction.

If you click "get_frame" or "get_image" during ongoing job on the current build, the click does nothing. At least the first one. The second click kills the GUI. You can check that on the simulated job, it is very consistent - since the queue has only space for one element, any blocking put will freeze the GUI. If you are patient, click only once and wait until the job completes, it will work, but only after grab the frame/image after the job is done.

Maybe some behaviors have changed, but I confirmed that is how it is designed to work. All things considered, I think in overall the first behavior is preferable. There is a way to eat this cake and have it, but I do not love it. It involves making two queues - one for large tasks, one for small. Unfortunately this would require another thread and another queue or a system to signal in kwargs whether the task is big or small. I think this is not worth the increase in code complexity over potentially messing timings up, but on the other side I may be wrong because my camera is rather small and the saving times really mostly irrelevant, so I may not see the problem.

[stefsmeets]

The queue is already a bit gimped anyway, because it has a task limit of 1.

instamatic/src/instamatic/gui/gui.py

Line 39 in 9e0fc8d

self.q = queue.LifoQueue(maxsize=1)

This is because otherwise you get multiple actions queued with no way of stopping it (which was the reason for all the reset queue buttons you removed). So the queue is not the best way to handle this, but it has the benefit that it ignores (or overwrites) previous actions when you spam a button.

So yes, the current way of doing it is maybe not ideal, but it has works. It has been there for a long time and everything sort of depends on the implementation.

Even so, it shouldn't crash. I think I'd probably add a timeout to the save image button to avoid this. I wouldn't put much effort in trying to 'fix' this system, because you may break things in other places.

[Baharis]

The current implementation unfortunately does not ignore nor overwrite previous actions but it does freeze the GUI, I am afraid. Adding a timeout on the save_frame would indeed avoid a freeze, but also it does not fix the core problem here. That is why I believe saving images is better done outside the job system. The job system does work phenomenal for experments and I really do not want to change it.

As for the reset button, I did not remove it though? I guess you mean #147 ? I removed the "Clear triggers" button, because it has no function without the trigger existing anymore. The reset queue button, however little use it has, is still there.

I did not plan to modify anything too much because, as you said, it is easy to break things unexpectedly. I was tempted at one point to make a light wrapper around / subclass of the queue to auto-reject put jobs over limit, but I agree better not to fix what is not (too) broken. Well, unless it should protect from spaming buttons, in which case I can patch this in. In fact, the change that I suggest here for the wobbler does exactly that but locally, prevents repeated wobbler use from crashing GUI.

[stefsmeets]

As for the reset button, I did not remove it though? I guess you mean #147 ? I removed the "Clear triggers" button, because it has no function without the trigger existing anymore. The reset queue button, however little use it has, is still there.

You know the code better than I do by now 😅

It feels like one of those two steps forward, one step back situations. If you can make it work, then I have no objections. I wanted to make sure you are aware of the potential pitfalls and lessons learned in the past.

[Baharis]

@stefsmeets Reanimating the discussion. I think the alpha wobbler is just straight up upgrade and fix; I personally do want to suggest the refactor of get_frame/image to VideoStreamFrame not only because it fixes the GUI crash, but also because it allows other modules to easily safe frames and images with annotations without repeated implementation or complex shenanigans.

[stefsmeets]

@stefsmeets Reanimating the discussion. I think the alpha wobbler is just straight up upgrade and fix; I personally do want to suggest the refactor of get_frame/image to VideoStreamFrame not only because it fixes the GUI crash, but also because it allows other modules to easily safe frames and images with annotations without repeated implementation or complex shenanigans.

Sounds good to me!

[Baharis]

If that is the case then I'll be merging this by ~Wednesday unless anyone voices issues.