mate-screenshot: Replace fork save with sync save

[vkareh]

Newer versions of gdk-pixbuf use libglycin's asynchronous image save mechanism. This makes the fork-save redundant and has a potential of hanging when saving a screenshot.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2390588

[vkareh]

@raveit65 (in response to #367 (comment))
Can you test this change as a possible fix for https://bugzilla.redhat.com/show_bug.cgi?id=2390588 ?

[raveit65]

@vkareh
Thanks for quick reaction. Commit works fine with upcoming f43. I was able to save screenshots again.
Is this fix backward compatible with older gdk-pixbuf releases?
Means can i use it for f42/41 ?

Edit:
I am guessing new code is only for gdk-pixbuf-2.43.3

[vkareh]

@raveit65 yes, the code is backwards compatible - all it does is removes the fork-save that we used to have to make saving files appear faster. In older/slower systems it might feel sluggish (like, maybe up to 1 second? maybe less?), but I don't think it would make a difference in any computer made in the last decade...

[cwendling]

@vkareh how is this asynchronous? I didn't read GDK Pixbuf's code, but the write ought to have finished by the time gdk_pixbuf_save() returns, so this can't be asynchronous. Maybe it's a lot faster than it used to be, but is it really the case or are computers just usually faster (especially for writing to SSDs without fsync)?

IMO if there's an issue and we want to keep the non-blocking appearance, we should use the actual async APIs, like gdk_pixbuf_save_to_stream_async(), which is the supported way of doing this. Of course, this means we have to wait for the operation to finish before we can exit the process, but that should be fine (and we should actually be doing this already, or we've got a potential bug with the child hitting SIGPIPE or so)

[cwendling]

(after reading the Fedora report) I don't see how https://gitlab.gnome.org/GNOME/glycin/-/issues/175 could be the issue? If the save fails, we should be handling it correctly whichever process does it. Maybe we have bad error handling? or the issue is entirely different and our subprocess has an environment that cause issues with Glycin's subprocess stuff.
However, I'm not quite sure manual fork(), especially without exec() is very safe in a GTK environment.

[raveit65]

@cwendling
Take a look at https://gitlab.gnome.org/GNOME/gdk-pixbuf/-/commits/master?ref_type=HEADS
Only using libglycin is the big change with new gdk-pixbuf-2.43.3
So what other thing can cause the issue?
When using mate-screenshot -a for example a error message pops up complaining about wrong texture values.
I.e.

Texture is only 192813 but was announced differently: Stride size: 1236 Image size: 411 x 256

Because of wrong values the screnshot won't be saved.
Anyway, i will use the fix for f43 because a desktop without working screenshot is a nogo ;-)
f43 i now in beta freeze........

[cwendling]

@raveit65 my point is that calling the same function with the same data from outside a subprocess only changes the context, so:

• either the context is indeed problematic, which means it's not about the data being malformed (or not, your message is indeed from that issue)
• there was a bug in our code
• it won't change anything.

So I don't believe the issue is properly understood.

But anyway I don't like that subprocess either, but I think we should switch to the async variants instead of pretending that the newer GDK Pixubf magically doesn't block sequential calls. And having read a tiny bit more about how Glicyn works and why it's there, it's likely only going to be slower, not faster (it always spawns a sandboxed subprocess and communicates with it through DBus -- but I didn't benchmark anything).

[raveit65]

No problem......
Feel free to install f43 image in a VM to find out the cause of the issue. It's easy to reproduce.
But sorry as distro-maintainer i don't have the time to wait that someone try to reproduce it and dig deeper into the problem.
I am happy to use another fix if there is one.

[vkareh]

@cwendling there are two issues at play here (both caused by the gdk-pixbuf bumping to libglycin-2 in their 2.43 release):

• glycin spawns a sandboxed subprocess that uses dbus to process/save the image. By mate-screenshot forking to a child process, the sandbox hangs waiting for the parent process. Making mate-screenshot not fork the process fixes that mate->pixbuf->glycin communication pipe. However, you have a valid point about using async functions directly - I can give that a try.

• glycin expects a padded rowstride, but mate-screenshot (and gdk-pixbuf) don't add padding. This means that when you're doing an area selection, if you have what glycin considers "the wrong size", then it hangs in processing the image. We could try to fudge the area size, but that would mean reverse-engineering how glycin expects the data and implement that in mate-screenshot. I'd rather wait for the glycin devs to fix that instead (it has an upstream bug reported from the gdk-pixbuf maintainers anyway, since it's a general problem, not a mate specific problem)

[vkareh]

@raveit65 I see that the bz is closed now - the fix didn't need this commit, I assume? If so we can close it without merge (which would be my preference). Let me know.

[raveit65]

OMG.
I added the patches to rawhde/f43 repo. See https://src.fedoraproject.org/rpms/mate-utils/c/8aadfd2f2806c5738f7a9c3e4799a505616f42cc?branch=rawhide
But i forgot to enable them in spec file. :/
https://src.fedoraproject.org/rpms/mate-utils/blob/rawhide/f/mate-utils.spec
I only tested local builds with 'mock' in VM.

Thanks for asking , i will do another build with the commit for fedora rawhide/f43.
Please do not delete this PR.

[raveit65]

For f43 the commit was already in spec file https://src.fedoraproject.org/rpms/mate-utils/blob/f43/f/mate-utils.spec#_12
and
https://kojipkgs.fedoraproject.org//work/tasks/1278/136841278/build.log
For rawhide i fixed my mistake https://src.fedoraproject.org/rpms/mate-utils/c/39bf12d4309ad0e48c3d2c54fec1e417c7a249d9?branch=rawhide

Glycin changes are fully added to fedora. Ie. gdkpixbuff-tumbnailer is now replaced with lbglycin-tumbnailer. So i think eom needs to be updated in general (i don't use this for fedora). No idea when other distros will follow fedora.

[raveit65]

@vkareh
Here is the whole thread about Changes to gdk-pixbuf2 in rawhide and F43 https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/7FTIVJJM5CJSJ6GIX4FV6JDUCT2NW5FY/#G23G7C7OGF6XO3CN7DGCCPWWOCMQV7NQ` from fedora devel-list.
Edit:
It's public, no login is needed.

[raveit65]

@vkareh @cwendling
another problem with thumbnails. Thumbnails of images in caja are broken in released f43 , see https://bugzilla.redhat.com/show_bug.cgi?id=2411809

[raveit65]

@raveit65 I see that the bz is closed now - the fix didn't need this commit, I assume? If so we can close it without merge (which would be my preference). Let me know.

This fix is needed.

[raveit65]

Works for me, patch runs several weeks in fedora 43 without problems

[raveit65]

@vkareh @cwendling another problem with thumbnails. Thumbnails of images in caja are broken in released f43 , see https://bugzilla.redhat.com/show_bug.cgi?id=2411809

@vkareh @cwendling
I could fix that with requiring glycin-thumbnailer in spec file. See https://bugzilla.redhat.com/show_bug.cgi?id=2411809 and https://src.fedoraproject.org/rpms/caja/c/8066b2dbf8409f717c3b6985a691d4ddd074b186?branch=rawhide

[cwendling]

@raveit65 have you had a chance to test #370? It should make things asynchronous again, but in a safe way this time.

@vkareh @cwendling another problem with thumbnails. Thumbnails of images in caja are broken in released f43 , see https://bugzilla.redhat.com/show_bug.cgi?id=2411809

@vkareh @cwendling I could fix that with requiring glycin-thumbnailer in spec file. See https://bugzilla.redhat.com/show_bug.cgi?id=2411809 and https://src.fedoraproject.org/rpms/caja/c/8066b2dbf8409f717c3b6985a691d4ddd074b186?branch=rawhide

OK, cool! But I guess that's not something we have control over upstream, do we? I mean, it looks like it's a question of which (3rd party for us) thumbnailer is available and works, right?