GNOME Bugzilla – Bug 757350
Add possibility to export map to cairo
Last modified: 2015-11-17 22:35:57 UTC
It would be nice to be able to export the current view to a cairo surface. For instance if we wanted to print the map or export to PNG.
Created attachment 314448 [details] [review] ChamplainTile: Add 'surface' property The surface property is a cairo_surface_t that, if set, represents the content of the tile.
Created attachment 314449 [details] [review] Renderers: Set surface on tiles
Created attachment 314450 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 314451 [details] [review] demos/launcher-gtk: Add export to png buton
Created attachment 314458 [details] [review] ChamplainTile: Add 'surface' property The surface property is a cairo_surface_t that, if set, represents the content of the tile.
Created attachment 314459 [details] [review] Renderers: Set surface on tiles
Created attachment 314460 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 314461 [details] [review] demos/launcher-gtk: Add export to png buton
Thanks Jonas for the patches. I just went through them and in general they look good. However, I'm wondering, isn't there a better way? Isn't there a way to export the whole actor (including the contained actors) to png? If this was possible we could export map including markers if needed. Maybe looking at what gnome shell does to take a screenshot could help (haven't had time to check myself).
(In reply to Jiri Techet from comment #9) > Thanks Jonas for the patches. I just went through them and in general they > look good. > > However, I'm wondering, isn't there a better way? Isn't there a way to > export the whole actor (including the contained actors) to png? If this was > possible we could export map including markers if needed. Maybe looking at > what gnome shell does to take a screenshot could help (haven't had time to > check myself). Hi! Well, exporting the whole actor would be great! But It would require some discipline maybe? I think we need to be sure that all actors are created the same way, with a ClutterCanvas, so that we know how to export them. And since we allow user defined actors as markser it might be tricky? Maybe we could have some kind of property that tells us if they contain a cairo surface representation of themselve, or something similar. And if they do we export them? Maybe a ChamplainExportable interface? I will cc ebassi, maybe he will know how to save us!
(In reply to Jiri Techet from comment #9) > Thanks Jonas for the patches. I just went through them and in general they > look good. > > However, I'm wondering, isn't there a better way? Isn't there a way to > export the whole actor (including the contained actors) to png? No, there isn't such a function — and I do not intend to write it, as it would be impressively wrong, considering how GPUs work. If you start reading back from the GPU you need to think about synchronization with the paint and screen refresh cycle; you need to think about performance in reading back from the GPU using an internal pixel format that will undoubtedly need to be converted; you need to think about the visible area that you submitted and the printable area, and what to do when they do not match; you need to think about threaded printing to avoid blocking the main loop of the application — while simultaneously only allowing *one* thread to access the data on the GPU. In short: it would be a mistake. > If this was > possible we could export map including markers if needed. Maybe looking at > what gnome shell does to take a screenshot could help (haven't had time to > check myself). Don't do that. It's a waste of time. You're already submitting data you locally have to the GPU in form of textures; you should not read back from the GPU to create new textures. Instead, you should use the client-side (i.e. app side) data you are also submitting to the GPU, and "print" that (i.e. save it to a file). Do *not* do a round trip from client-side to GPU, and then back. Additionally, if you're always operating client-side, you can decide which layers should be printable, or how to print without blocking the main thread.
Review of attachment 314458 [details] [review]: ::: champlain/champlain-tile.c @@ +371,3 @@ + g_object_class_install_property (object_class, + PROP_SURFACE, + * Use g_param_spec_boxed() with a CAIRO_GOBJECT_TYPE_SURFACE instead, so that GObject can do memory management for you. @@ +579,3 @@ + * champlain_tile_set_surface: + * @self: the #ChamplainTile + * @surface: (transfer full): the #cairo_surface_t This is a fairly bad idea, especially in non-managed environments… @@ +591,3 @@ + g_return_if_fail (CHAMPLAIN_TILE (self)); + + * Since: 0.12.12 … because if something creates the surface and then wants to keep around a pointer, it won't be notified of a change. It's also a fairly alien way of handling properties; the idiomatic way in the GNOME platform is that setters acquire references — i.e. (transfer full) arguments are odd, and usually an indicator of something wrong. Use: priv->surface = cairo_surface_reference (surface); instead, and make this common setter. @@ +592,3 @@ + + cairo_surface_destroy (self->priv->surface); + */ You need to notify the property, otherwise callers of the setter function won't receive a notification.
Review of attachment 314459 [details] [review]: ::: champlain/champlain-image-renderer.c @@ +189,3 @@ + clutter_canvas_set_size (CLUTTER_CANVAS (content), width, height); + g_signal_connect (content, "draw", G_CALLBACK (image_tile_draw_cb), + I'd probably pass the tile, instead of the image surface, here, for future proofing the implementation. @@ +208,2 @@ g_signal_emit_by_name (tile, "render-complete", data->data, data->size, error); You're leaking the pixbuf.
Review of attachment 314461 [details] [review]: ::: demos/launcher-gtk.c @@ +222,3 @@ + surface = champlain_view_to_surface (view); + if (surface) + cairo_surface_write_to_png() is pretty much a debugging method in Cairo. It's also fully synchronous, so it'll block until it finishes writing the data. I'd strongly encourage you to write the export-to-png function using GdkPixbuf's async API, or use a GTask to send off the image data write to a thread.
Created attachment 315230 [details] [review] ChamplainTile: Add 'surface' property The surface property is a cairo_surface_t that, if set, represents the content of the tile.
Created attachment 315231 [details] [review] Renderers: Set surface on tiles
Created attachment 315232 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 315233 [details] [review] demos/launcher-gtk: Add export to png buton
Review of attachment 315230 [details] [review]: ::: champlain/champlain-tile.c @@ +210,3 @@ + if (priv->surface) + g_clear_pointer (&priv->surface, cairo_surface_destroy); g_clear_pointer() is NULL-safe — it's one of the reasons why we added g_clear_pointer() in the first place. :-) You can always safely do: g_clear_pointer (&pointer, free_func); unconditionally. @@ +420,3 @@ priv->fade_in = FALSE; priv->content_displayed = FALSE; + priv->surface = NULL; All this initialization is really unnecessary: the instance data, including the instance private data structure, is zero-filled when it's created. @@ +487,3 @@ + * map content of this tile. + * + * Returns: a #cairo_surface_t of this tile Missing annotation `(transfer none)` on the return value. @@ +591,3 @@ + cairo_surface_t *surface) +{ + * If surface cannot be NULL then you need to add the precondition check: g_return_if_fail (surface != NULL); as well. You should also add: if (self->priv->surface == surface) return; Otherwise you may end up in the case of: 1. champlain_tile_set_surface(t,a); 2. cairo_surface_destroy(a); ^^^^^^^^^^^^^^^^^^^^^ perfectly valid, the tile owns the surface 3. champlain_tile_set_surface(t,a); 3a. cairo_surface_destroy(a) 3b. cairo_surface_reference(a) ^^^^^^^^^^^^^^^^^^^^^^^ reference invalid memory, segfault
Review of attachment 315231 [details] [review]: ::: champlain/champlain-memphis-renderer.c @@ +376,3 @@ if (pixbuf) g_object_unref (pixbuf); + Don't you need to destroy the surface here as well? The tile owns a reference, now.
Thanks Ebassi!
Created attachment 315249 [details] [review] ChamplainTile: Add 'surface' property The surface property is a cairo_surface_t that, if set, represents the content of the tile.
Created attachment 315250 [details] [review] Renderers: Set surface on tiles
Created attachment 315251 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 315252 [details] [review] demos/launcher-gtk: Add export to png buton
Created attachment 315319 [details] [review] Add ChamplainExportable interface
Created attachment 315320 [details] [review] ChamplainTile: Implement ChamplainExportable
Created attachment 315321 [details] [review] ChamplainPathLayer: Implement ChamplainExportable
Created attachment 315322 [details] [review] Renderers: Set surface on tiles
Created attachment 315323 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 315324 [details] [review] demos/launcher-gtk: Add export to png buton
So I am not sold on the name. But what do you think? This would allow users to implement layers that implements the exportable interface, and they will get exported along with the tile.
Created attachment 315325 [details] [review] ChamplainPathLayer: Implement ChamplainExportable
Review of attachment 315322 [details] [review]: I think in champlain-error-tile-renderer.c image_rendered_cb() the cr should be destroyed with cairo_destroy(). In champlain-error-tile-renderer.c the surface seems to be over-reffed - it is reffed in ChamplainTile already so the ref here seems to be extra.
Review of attachment 315320 [details] [review]: The g_clear_pointer() should be in dispose(), not in finalize().
One more general note - please make sure you use spaces instead of tabs for formatting to make the indentation consistent with the rest. Otherwise looks good to me. @ebassi Thanks a lot for your clarification about how clutter works and also for your review!
Created attachment 315666 [details] [review] Add ChamplainExportable interface
Created attachment 315667 [details] [review] ChamplainTile: Implement ChamplainExportable
Created attachment 315668 [details] [review] ChamplainPathLayer: Implement ChamplainExportable
Created attachment 315669 [details] [review] Renderers: Set surface on tiles
Created attachment 315670 [details] [review] ChamplainView: Add champlain_view_to_surface This function will export the current map view to a cairo_surface_t surface.
Created attachment 315671 [details] [review] demos/launcher-gtk: Add export to png buton
The patches look fine to me so I've just pushed them to master. Would be nice to have the export for the remaining layer types but since they don't draw using cairo now, it would be more work and it can possibly be done later. Thanks Jonas!
Created attachment 315789 [details] [review] Fixes for cairo surface export Return NULL for surface when PathLayer is not visible. And do not try to export layers where surface is NULL.
Created attachment 315790 [details] [review] Fixes for cairo surface export Return NULL for surface when PathLayer is not visible. And do not try to export layers where surface is NULL. Also honor the tile opacity.
Review of attachment 315790 [details] [review]: LGTM.