After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 575900 - Async version of gdk_pixbuf_new_from_stream
Async version of gdk_pixbuf_new_from_stream
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2009-03-18 23:51 UTC by Philip Withnall
Modified: 2010-12-13 13:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add async gdk_pixbuf_new_from_stream methods (13.08 KB, patch)
2009-03-18 23:53 UTC, Philip Withnall
none Details | Review
Add async gdk_pixbuf_new_from_stream methods (updated) (13.40 KB, patch)
2009-04-04 14:28 UTC, Philip Withnall
needs-work Details | Review
Async version of gdk_pixbuf_new_from_stream (13.77 KB, patch)
2010-12-10 17:14 UTC, Bastien Nocera
reviewed Details | Review
Async version of gdk_pixbuf_new_from_stream (13.76 KB, patch)
2010-12-10 18:23 UTC, Bastien Nocera
committed Details | Review

Description Philip Withnall 2009-03-18 23:51:47 UTC
It would be nice to have an async version of gdk_pixbuf_new_from_stream and gdk_pixbuf_new_from_stream_at_scale. Patch coming up.
Comment 1 Philip Withnall 2009-03-18 23:53:34 UTC
Created attachment 130929 [details] [review]
Add async gdk_pixbuf_new_from_stream methods

Adds gdk_pixbuf_new_from_stream[_at_scale]_async, gdk_pixbuf_new_from_stream_finish, as well as gdk_pixbuf_save_to_stream_async and gdk_pixbuf_save_to_stream_finish.
Comment 2 Christian Dywan 2009-04-03 16:20:36 UTC
A few style related comments, no in-depth review:

+static void
+new_from_stream_thread (GSimpleAsyncResult *result,
+			GInputStream       *stream,
+			GCancellable       *cancellable)
+{
+	GdkPixbuf *pixbuf;
+	AtScaleData *data;

Please use 2 spaces for intendation.


+	/* Set the new pixbuf as the result, or error out */
+	if (pixbuf == NULL) {
+		g_simple_async_result_set_from_error (result, error);
+		g_error_free (error);
+	} else {
+		g_simple_async_result_set_op_res_gpointer (result, pixbuf, g_object_unref);
+	}

The "if" blocks should look like so:

if (pixbuf == NULL)
  {
    ...
  }
else
  {
    ...
  }

+ * Since: 2.18

Too late for 2.18 unfortunately, so this should be 2.20.

gdk_pixbuf_new_from_stream_at_scale_async is missing a g_return_if_fail (!cancellable || G_IS_CANCELLABLE ()).

gdk_pixbuf_new_from_stream_finish should have g_return_val_if_fail (!error || (error && !*error)).
Comment 3 Matthias Clasen 2009-04-03 16:33:21 UTC
> Please use 2 spaces for intendation.

No, this it gdk-pixbuf, which for historic reasons uses a different style.

> Too late for 2.18 unfortunately, so this should be 2.20.

No, we just released 2.16. 2.18 is still a possibility
Comment 4 Christian Dywan 2009-04-03 16:56:13 UTC
(In reply to comment #3)
> > Please use 2 spaces for intendation.
> 
> No, this it gdk-pixbuf, which for historic reasons uses a different style.
> 
> > Too late for 2.18 unfortunately, so this should be 2.20.
> 
> No, we just released 2.16. 2.18 is still a possibility

Yikes, was somehow thinking of Glib there, thanks for the correction. And yes, GdkPixbuf's special style apparentely still confuses me.
Comment 5 Philip Withnall 2009-04-04 14:28:55 UTC
Created attachment 132077 [details] [review]
Add async gdk_pixbuf_new_from_stream methods (updated)

This adds the missing checks to all relevant new functions.
Comment 6 Philip Withnall 2009-04-26 18:35:48 UTC
Ping? This is needed for Totem 2.27.
Comment 7 Matthias Clasen 2009-04-26 18:42:05 UTC
There is nothing in this that you cannot do in totem itself. 
Comment 8 Philip Withnall 2009-04-26 20:45:24 UTC
That is true, but at the moment we just have this code copied-and-pasted. Perhaps I should've said "this would be nice for Totem 2.27". :)
Comment 9 Bastien Nocera 2010-06-16 12:09:33 UTC
Would be nice to have in GTK+ 3.x

Any comments?
Comment 10 Javier Jardón (IRC: jjardon) 2010-06-16 18:02:43 UTC
Comment on attachment 132077 [details] [review]
Add async gdk_pixbuf_new_from_stream methods (updated)

Some little comments: From a style point of view the patch should be reworked: Do not use tabs and use 2 spaces for indentation.
Also, update the Since tag to 'Since: 3.0'
Comment 11 Bastien Nocera 2010-06-16 18:13:12 UTC
(In reply to comment #10)
> (From update of attachment 132077 [details] [review])
> Some little comments: From a style point of view the patch should be reworked:
> Do not use tabs and use 2 spaces for indentation.

See comment 3.

> Also, update the Since tag to 'Since: 3.0'

Right, easy fix.
Comment 12 Bastien Nocera 2010-12-10 17:14:09 UTC
Created attachment 176192 [details] [review]
Async version of gdk_pixbuf_new_from_stream

Add async versions of gdk_pixbuf_new_from_stream
and gdk_pixbuf_new_from_stream_at_scale.
Comment 13 Emmanuele Bassi (:ebassi) 2010-12-10 17:29:17 UTC
Review of attachment 176192 [details] [review]:

looks generally good to me.

::: gdk-pixbuf/gdk-pixbuf-io.c
@@ +1621,3 @@
+		return NULL;
+
+	pixbuf = GDK_PIXBUF (g_simple_async_result_get_op_res_gpointer (result));

it's friday, and I might be on crack, but doesn't:

  object = SAFE_TYPE_CAST (function_that_might_return_null ());

will warn/assert if null is actually returned?

since this line is followed by a NULL check and there's a GError involved as well, I'd probably skip the safe cast. also, the return value for get_op_res_gpointer is a gpointer, there's no actual need for a cast (and a type-safe case at that).
Comment 14 Bastien Nocera 2010-12-10 18:23:27 UTC
Created attachment 176196 [details] [review]
Async version of gdk_pixbuf_new_from_stream

Add async versions of gdk_pixbuf_new_from_stream
and gdk_pixbuf_new_from_stream_at_scale.
Comment 15 Philip Withnall 2010-12-13 09:44:36 UTC
Review of attachment 176196 [details] [review]:

It's amazing what problems one finds when looking at one's own code again a while later. If you don't want to do another iteration, I will.

::: gdk-pixbuf/gdk-pixbuf-io.c
@@ +1462,3 @@
+		pixbuf = gdk_pixbuf_new_from_stream (stream, cancellable, &error);
+
+	g_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when setting a new value over the top */

That's not true any more; this was originally to work around a bug in GSimpleAsyncResult.

@@ +1470,3 @@
+		g_error_free (error);
+	} else {
+		g_simple_async_result_set_op_res_gpointer (result, pixbuf, g_object_unref);

Wouldn't it be tidier to g_object_ref(pixbuf) here instead of in gdk_pixbuf_new_from_stream_finish()? That would mean the ref/unref are next to each other, and gdk_pixbuf_new_from_stream_finish() would return the original reference created by gdk_pixbuf_new_from_stream().

@@ +2610,3 @@
+	g_strfreev (data->values);
+	g_free (data->type);
+	g_free (data);

Shouldn't data->stream be unreffed here too?

@@ +2632,3 @@
+					       &error);
+
+	save_to_stream_async_data_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when setting a new value over the top */

Again, this shouldn't be called.

@@ +2688,3 @@
+	va_end (args);
+
+	data = g_new (SaveToStreamAsyncData, 1);

Use g_slice_new()?
Comment 16 Bastien Nocera 2010-12-13 12:12:02 UTC
Attachment 176196 [details] pushed as 92e27ca - Async version of gdk_pixbuf_new_from_stream
Comment 17 Bastien Nocera 2010-12-13 12:27:01 UTC
(In reply to comment #15)
> Review of attachment 176196 [details] [review]:
> 
> It's amazing what problems one finds when looking at one's own code again a
> while later. If you don't want to do another iteration, I will.
> 
> ::: gdk-pixbuf/gdk-pixbuf-io.c
> @@ +1462,3 @@
> +        pixbuf = gdk_pixbuf_new_from_stream (stream, cancellable, &error);
> +
> +    g_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when
> setting a new value over the top */
> 
> That's not true any more; this was originally to work around a bug in
> GSimpleAsyncResult.

Removed.

> @@ +1470,3 @@
> +        g_error_free (error);
> +    } else {
> +        g_simple_async_result_set_op_res_gpointer (result, pixbuf,
> g_object_unref);
> 
> Wouldn't it be tidier to g_object_ref(pixbuf) here instead of in
> gdk_pixbuf_new_from_stream_finish()? That would mean the ref/unref are next to
> each other, and gdk_pixbuf_new_from_stream_finish() would return the original
> reference created by gdk_pixbuf_new_from_stream().

Done.

> @@ +2610,3 @@
> +    g_strfreev (data->values);
> +    g_free (data->type);
> +    g_free (data);
> 
> Shouldn't data->stream be unreffed here too?

Done.

> @@ +2632,3 @@
> +                           &error);
> +
> +    save_to_stream_async_data_free (data); /* GSimpleAsyncResult doesn't
> destroy result pointers when setting a new value over the top */
> 
> Again, this shouldn't be called.

Done.

> @@ +2688,3 @@
> +    va_end (args);
> +
> +    data = g_new (SaveToStreamAsyncData, 1);
> 
> Use g_slice_new()?

Done.

commit 8f4cf69dbc6e07289d18dd1e516b39ce178617d5
Author: Bastien Nocera <hadess@hadess.net>
Date:   Mon Dec 13 12:26:13 2010 +0000

    Fixes from review for new async functions
    
    Correct review comments by Philip Withnall in:
    https://bugzilla.gnome.org/show_bug.cgi?id=575900
Comment 18 Christian Persch 2010-12-13 12:58:02 UTC
+ g_simple_async_result_set_from_error (result, error);
+ g_error_free (error);

There's now g_simple_async_result_take_error() for this construct.
Comment 19 Bastien Nocera 2010-12-13 13:26:23 UTC
(In reply to comment #18)
> + g_simple_async_result_set_from_error (result, error);
> + g_error_free (error);
> 
> There's now g_simple_async_result_take_error() for this construct.

Done.