GNOME Bugzilla – Bug 790584
Implement async loading without threads
Last modified: 2017-12-30 21:28:11 UTC
This is currently necessary for the GTK4 clipboard implementation, which provides an InputStream.
Created attachment 364018 [details] [review] gdk-pixbuf-io: Implement async loading without threads The new GTK4 clipboard input streams don't like threads very much. So don't use them.
Attachment 364018 [details] pushed as ca6adee - gdk-pixbuf-io: Implement async loading without threads
Review of attachment 364018 [details] [review]: I hear that this patch introduced some CRITICALs in gnome-photos. I haven't had the chance to look very closely, but a quick glance revealed some oddities: ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1560,3 @@ + gdk_pixbuf_loader_close (loader, NULL); + g_task_return_error (task, error); + g_object_unref (task); Aren't we leaking the GBytes here ... @@ +1568,3 @@ + g_task_get_cancellable (task), + load_from_stream_async_cb, + task); ... and here? It might be easier to use a 'goto out' and g_clear_object to avoid this. @@ +1876,3 @@ g_task_get_source_tag (task) == gdk_pixbuf_new_from_stream_at_scale_async); + return g_object_ref (g_task_propagate_pointer (task, error)); Umm... why the g_object_ref? g_task_propagate_pointer will transfer its ownership of the GdkPixbuf to the caller.
Review of attachment 364018 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1837,3 @@ g_return_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable)); + task = g_task_new (NULL, cancellable, callback, user_data); This s/stream/NULL/ causes the CRITICAL. Having the 'stream' passed as the source_object in the GAsyncReadyCallback was a convenient little detail. Since gdk_pixbuf_new_from_stream_at_scale_async continues to do that, maybe it should be restored here too?
Created attachment 365136 [details] [review] Fix up gdk_pixbuf_new_from_stream_async() Add fixes for the various issues brought up in https://bugzilla.gnome.org/show_bug.cgi?id=790584 * g_task_propagate_pointer() is transfer full * GBytes get leaked * always pass the stream as task source object to avoid criticals * don't check the source object in finish()
Created attachment 365145 [details] [review] gdk-pixbuf-io: Simplify code
Created attachment 365146 [details] [review] gdk-pixbuf-io: Task should be acting upon input stream Having the 'stream' passed as the source_object in the GAsyncReadyCallback was a convenient little detail. Absence of this was causing a CRITICAL in gnome-photos. Fallout from ca6adee161ee81784eefe6049491f070938b22ec
Review of attachment 365136 [details] [review]: Thanks for the fix, Benjamin!
Review of attachment 365146 [details] [review]: Nice patch, Umang, except for one small niggle: (Sadly, it's obsoleted by Benjamin's) ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1873,3 @@ + stream = G_INPUT_STREAM (g_task_get_source_object (task)); + + g_return_val_if_fail (g_task_is_valid (async_result, stream), NULL); The g_task_is_valid isn't any different than the simpler G_IS_TASK. The idea is to ensure that the *caller* of _finish method matched the GAsyncResult with the correct (source) object. Supplying the source object inside the _finish negates that part.
Review of attachment 365145 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1579,3 @@ } + +out: Consolidating the return site still seems like a worthwhile endeavour to me. So, maybe you could rebase your patch on top of attachment 365136 [details] [review]? @@ +1629,2 @@ loader = gdk_pixbuf_loader_new (); + g_signal_connect (loader, "size-prepared", Nitpick: this looks like a spurious change. @@ +1631,3 @@ G_CALLBACK (at_scale_size_prepared_cb), data); g_object_set_data_full (G_OBJECT (loader), + "gdk-pixbuf-please-kill-me-later", Ditto.
Created attachment 365227 [details] [review] gdk-pixbuf-io: Simply Code
Comment on attachment 365136 [details] [review] Fix up gdk_pixbuf_new_from_stream_async() Pushed to master.
Review of attachment 365227 [details] [review]: Looks good to me, apart from the missing bug reference. Added it and pushed to master.