Created attachment 216802 [details] [review]
Update screenshot handling

The current screenshot handling is pretty bad, doing sync libvirt
calls on the mainloop and constantly doing (sometimes sync) i/o to
disk to access the screenshot data. This is a restructuring of
the screenshot handling with the following features:

* No sync libvirt calls
* Read the screenshots from libvirt directly into a pixbuf
* Save the screenshot caches more rarely
* Only load the screenshot caches on start
* Always get a screenshot of the last frame when closing the
  connection
* Only read the boxes-grid.png file once
* scale down screenshots using gdk-pixbuf (much better quality downscaling)
* Work around bug in libvirt screenshot handling after disconnect

Review of attachment 216802 [details] [review]:

Yeah, I tried to solve things elsewhere for several reasons:

The Boxes code wasn't so much sync. We did a lot of effort to turn Virt streams into async IO. But I never thought if domain.screenshot (stream, 0, 0); should be run in a thread. Are you sure?

The plan was rather to fix the problem at a lower level. Typically, you don't have more than 3-4 VM running, and you rarely just stay in the collection view, but rather connect to one. In which case screenshots were suspended.

However, in collection view, I can just say you reduced (only) x2 the amount of IO unfortunately. My plan by fixing qemu & libvirt, was to reduce by x100 :)

The main problem is that QEMU takes large PPM screenshot to disk, which libvirt reads back (I wonder if it didn't copy it elsewhere a second time even). I proposed a patch to take PNG and avoid writing to disk in QEMU, but use fd passing, socket or similar, which would have the nice effect of benefiting everyone. Then I proposed to augment qemu API to scale the screenshot down, to avoid further IO. Typicially, this would mean, we go from 5Mb picture to something like 60kb.

No process would write to disk until Boxes (which needs to write to disk, as you noticed, although you do it less now). Typically, you would have 60x(3-4)kb / 5 seconds, which I think is very tolerable for a program to manage virtual machines.


I'll continue reviewing your patch

Review of attachment 216802 [details] [review]:

looks good to me

::: src/libvirt-machine.vala
@@ +22,3 @@
+            if (pixbuf != null)
+                set_screenshot (pixbuf, true);
+        } catch (GLib.Error error) {

In remote-machine.vala, you add warning (err.message);

I wonder if this common snippet shouldn't be moved in machine.vala

@@ +363,3 @@
+        yield run_in_thread (()=> {
+            domain.screenshot (stream, 0, 0);
+        });

ok, I think it makes sense since libvirt qemu driver currently does sync IO when taking screenshot, and the caller will not continue until screenshot is done.

::: src/machine.vala
@@ +200,3 @@
+            // There is some kind of bug in libvirt, so the first time we
+            // take a screenshot after displaying the box we get the old
+            // screenshot from before connecting to the box

This is a qxl quirk iirc, alon tried to improve it already. It wasn't easy. Is it so important to have the very latest screenshot?

Review of attachment 216802 [details] [review]:

::: src/libvirt-machine.vala
@@ +22,3 @@
+            if (pixbuf != null)
+                set_screenshot (pixbuf, true);
+        } catch (GLib.Error error) {

The warning was there before, but its true that the two paths are different.

The whole disconnect_display is otherwise identical between the two and could be made a virtual on the baseclass with a default implementation.

@@ +363,3 @@
+        yield run_in_thread (()=> {
+            domain.screenshot (stream, 0, 0);
+        });

I think we should always do libvirt rpc calls like this async, be it an async libvirt-glib call or a (temporary) run_in_thread workaround in boxes.

::: src/machine.vala
@@ +200,3 @@
+            // There is some kind of bug in libvirt, so the first time we
+            // take a screenshot after displaying the box we get the old
+            // screenshot from before connecting to the box

Yes, without this you get the current screenshot when you zoom back, but then after 5 seconds it switches back (for 5 seconds) to something completely different.

Attachment 216802 [details] pushed as a5539fd - Update screenshot handling

We have a few regressions in collection boxes item drawing, see attached screenshots:

- not correctly centered
- extra white borders

Created attachment 217078 [details]
before

Created attachment 217079 [details]
after

Created attachment 217080 [details]
after

Created attachment 217550 [details] [review]
Fix origin of thumbnail drawing

We were not drawing the thumbnails centered.

Created attachment 217551 [details] [review]
Save the non-modified thumnail

We don't want to apply the running state on the saved thumbnail,
because when we reload it the state might be different. So, save
the original pixbuf, not the drawn one.

Created attachment 217552 [details] [review]
Make paused screenshot nicer

We use ADD instead of over so that the rectangle pattern is more visible
on a dark thumbnail too.

Attachment 217550 [details] pushed as ccccf50 - Fix origin of thumbnail drawing
Attachment 217551 [details] pushed as 1052062 - Save the non-modified thumnail
Attachment 217552 [details] pushed as bd09f30 - Make paused screenshot nicer

(In reply to comment #12)
> Created an attachment (id=217552) [details] [review]
> Make paused screenshot nicer
> 
> We use ADD instead of over so that the rectangle pattern is more visible
> on a dark thumbnail too

On a mostly black screen, this makes the thumbnail hard to distinguish from the background.