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 788534 - FIXME: cairo-node-serialization: Adjust bytes when width bytes != stride
FIXME: cairo-node-serialization: Adjust bytes when width bytes != stride
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Scene Graph
3.91.x
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2017-10-04 20:53 UTC by Umang Jain
Modified: 2017-10-08 12:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsk: Fix serialiaztion of cairo node (1.85 KB, patch)
2017-10-04 21:02 UTC, Umang Jain
none Details | Review
gsk: Fix serialization of cairo node (1.89 KB, patch)
2017-10-06 18:32 UTC, Umang Jain
accepted-commit_now Details | Review
gsk: Move gsk_cairo_node_new_for_surface into public API (1.82 KB, patch)
2017-10-06 18:33 UTC, Umang Jain
accepted-commit_now Details | Review
Create tests for cairo node (2.31 KB, patch)
2017-10-06 18:33 UTC, Umang Jain
reviewed Details | Review

Description Umang Jain 2017-10-04 20:53:29 UTC
A patch for FIXME in gsk_cairo_node_serialize
Comment 1 Umang Jain 2017-10-04 21:02:51 UTC
Created attachment 360929 [details] [review]
gsk: Fix serialiaztion of cairo node
Comment 2 Matthias Clasen 2017-10-05 19:13:07 UTC
Review of attachment 360929 [details] [review]:

::: gsk/gskrendernodeimpl.c
@@ +1761,3 @@
+      {
+        memcpy (mem_surface + i * width * 4, data + i * stride, width * 4);
+      }

We generally omit {} for single-statement bodies like this. But if you wanted to keep them, they should be indented by 2, like this:

  for (...)
    {
      memcpy...
    }

@@ +1771,3 @@
+                                                       mem_surface,
+                                                       width * height,
+                                                       sizeof (guint32)));

As we discussed on telegram, g_variant_new_fixed_array copies the data, so we will have to free mem_surface to avoid leaking it, since we allocated it ourselves.
Comment 3 Umang Jain 2017-10-06 18:32:57 UTC
Created attachment 361058 [details] [review]
gsk: Fix serialization of cairo node
Comment 4 Umang Jain 2017-10-06 18:33:23 UTC
Created attachment 361059 [details] [review]
gsk: Move gsk_cairo_node_new_for_surface into public API
Comment 5 Umang Jain 2017-10-06 18:33:51 UTC
Created attachment 361060 [details] [review]
Create tests for cairo node
Comment 6 Matthias Clasen 2017-10-07 19:31:46 UTC
Review of attachment 361058 [details] [review]:

other than that, looks good. Feel free to push with that change

::: gsk/gskrendernodeimpl.c
@@ +1756,3 @@
+      data = cairo_image_surface_get_data (self->surface);
+
+      mem_surface = (guchar *) g_slice_alloc (width * height * 4);

I would just use g_malloc/g_free here. g_slice is really for small repetitive allocations, not for big one-off ones.
Comment 7 Matthias Clasen 2017-10-07 19:33:11 UTC
Review of attachment 361059 [details] [review]:

sure
Comment 8 Matthias Clasen 2017-10-07 19:38:45 UTC
Review of attachment 361060 [details] [review]:

::: tests/rendernode-create-tests.c
@@ +445,3 @@
+
+      height = g_random_int_range (0, 100);
+      width = g_random_int_range (0, 100);

Not sure if width == 0 or height == 0 is a good case to test for. Does that actually work ?
Comment 9 Umang Jain 2017-10-08 12:59:04 UTC
(In reply to Matthias Clasen from comment #8)
> Review of attachment 361060 [details] [review] [review]:
> 
> ::: tests/rendernode-create-tests.c
> @@ +445,3 @@
> +
> +      height = g_random_int_range (0, 100);
> +      width = g_random_int_range (0, 100);
> 
> Not sure if width == 0 or height == 0 is a good case to test for. Does that
> actually work ?

No. That's wasn't a good idea. I changed it to (1,100) range and pushed.

74a677a79 - gsk: Fix serialization of cairo node
a933c7c4b - gsk: Move gsk_cairo_node_new_for_surface into public API
74f8fc80d - Create tests for cairo node 

Thanks Matthias for all your help and reviews.