GNOME Bugzilla – Bug 653590
stage: Add clutter_stage_read_pixels_async
Last modified: 2021-06-10 11:32:53 UTC
This deprecates clutter_stage_read_pixels since there are numerous issues with that interface, including lacking of format choice and difficult to support semantics considering read_pixel requests that happen mid-scene. As a replacement this adds clutter_stage_read_pixels_async which serves a similar purpose but instead of synchronously painting the stage it queues a clipped redraw of the region of interest and after the stage is next painted the region is read and a user given callback is called to notify that their pixel data is ready. The new API allows control over the pixel format that pixels are returned in, there is the potential for us to optimize this api to use PBOs in the future to completely avoid blocking the mainloop while waiting for the reads to complete, we avoid redundant scenegraph traversals and avoid tricky semantics when reading mid-scene.
Created attachment 190887 [details] [review] stage: Add clutter_stage_read_pixels_async
*** Bug 653585 has been marked as a duplicate of this bug. ***
Review of attachment 190887 [details] [review]: ::: clutter/clutter-stage.c @@ +2612,3 @@ * to release the resources it has allocated. + * + * Deprecated: 1.8: Use clutter_stage_read_pixels_async you should add () at the end, so that gtk-doc can do proper linking. @@ +4185,3 @@ + * @callback: A #ClutterStageReadPixelsCallback to notify of read back completion + * @user_data: Private data that will be passed to @callback. + * @y: The top-left y coordinate of the region to read as the closure will be removed at a later point and this function will return immediately, we need to add a GDestroyNotify as well. @@ +4190,3 @@ + * stage is next painted it reads back that region to @pointer + * and calls @callback to notify you of completion. + * @height: The height of the region to read missing a Since: 1.8 annotation. @@ +4193,3 @@ +void +clutter_stage_read_pixels_async (ClutterStage *stage, + * @callback: A #ClutterStageReadPixelsCallback to notify of read back completion could you please line up the arguments of the function? ::: clutter/clutter-stage.h @@ +270,3 @@ gboolean clutter_stage_get_motion_events_enabled (ClutterStage *stage); +typedef void (*ClutterStageReadPixelsCallback) (ClutterStage *stage, the documentation is missing, and the arguments should be lined up. @@ +272,3 @@ +typedef void (*ClutterStageReadPixelsCallback) (ClutterStage *stage, + guint8 *pixels, + void *user_data); please, use gpointer. @@ +274,3 @@ + void *user_data); + +void clutter_stage_read_pixels_async (ClutterStage *stage, please, line up the arguments.
btw, thanks for the patch. apart from these couple of issues, it looks good to me.
I wonder if we should add some way to cancel the read pixels callback? Ie, maybe it could return some id that could be passed to clutter_stage_cancel_read_pixels_async. It's really awkward to use async APIs that don't have some way to cancel because usually the result of the operation is tied to the lifetime of some object and you need to be able to cope with the object being destroyed before the results are available. I think for robustness the stage should clean up the async read queue on finalize too. Otherwise if the stage is destroyed before the paint is actually run then the list would leak. I think this makes having the GDestroyNotify callback as ebassi suggested more important because there then would be two ways for the async callback to be removed without calling the callback.
I wonder if we should use the same style as GIO for this function, i.e. use: void clutter_stage_read_pixels_async (ClutterStage *, gint x, gint y, gint width, gint height, CoglPixelFormat format, GCancellable *, GAsyncReadyCallback, gpointer user_data); and: const guint8 *clutter_stage_read_pixels_finish (ClutterStage *, GAsyncResult *, GError **); to be called to access the buffer.
(In reply to comment #6) > I wonder if we should use the same style as GIO for this function, i.e. use: Yes you should. It's more consistent with everything else, and more easily handled by bindings. (In reply to comment #5) > I think for robustness the stage should clean up the async read queue on > finalize too. In gio style, when you create a GSimpleAsyncResult, that will add a ref to the "source object" (in this case, the stage), and so it wouldn't be possible for it to be finalized until after the async operation completed (either successfully or by being cancelled).
Created attachment 198319 [details] [review] Depend on GIO We're going to need the GIO types, as well as exposing them in the API, to implement the asynchronous version of clutter_stage_read_pixels().
Created attachment 198320 [details] [review] stage: Implement an async read pixels method The synchronous clutter_stage_read_pixels() has various issues: it does not offer the choice of pixel format for the resulting buffer; it has hard to support semantics for requests happening mid-scene; it allocates the result buffer. A better implementation of a read pixels operation should take into account all these things, as well as the asynchronous nature of the operation itself. The new clutter_stage_read_pixels_async() function has been modelled on the GIO style of asynchronous functions: the async() method will receive the parameters for the operation, as well as a way to cancel it (through an optional GCancellable object), and the callback to be invoked when the operation should be completed. The finish() method has to be called from the callback to complete the pixels read operation; it will take a pointer to a buffer, and avoid a memory allocation.
Created attachment 198322 [details] [review] stage: Deprecate clutter_stage_read_pixels() We have a better alternative, now. There are some conformance tests that use read_pixels(), though, and will need porting to the new asynchronous API.
Comment on attachment 198320 [details] [review] stage: Implement an async read pixels method >+ * @buffer: (array): a pointer to a buffer capable of holding the >+ * pixels read from the @stage That should be "(out caller-allocates)" as well. Some bindings may have issues with there not being a length argument... (I'm not sure about that.) But anyway, if you force the caller to pass the length, then you can double-check that they got it right rather than just doing a buffer overrun. (And you can document in the function docs what that length is supposed to be.) Anyway, if you want this to be usable from bindings then you'd probably better test that it actually is before the API gets frozen. (It might end up being easier to just provide an additional more-bindings-friendly version that returns an allocated buffer, and using the "Rename to" annotation...) >+ g_return_val_if_fail (G_IS_SIMPLE_ASYNC_RESULT (result), FALSE); >+ >+ simple = G_SIMPLE_ASYNC_RESULT (result); >+ >+ if (g_simple_async_result_propagate_error (simple, error)) >+ return FALSE; >+ >+ g_warn_if_fail (g_simple_async_result_get_source_tag (simple) == clutter_stage_read_pixels_async); g_return_val_if_fail (g_simple_async_result_is_valid (result, G_OBJECT (stage), clutter_stage_read_pixels_async), FALSE); simple = G_SIMPLE_ASYNC_RESULT (result); if (g_simple_async_result_propagate_error (simple, error)) return FALSE; >+ * @cancellable: (allow-none): optional #GCancellable, or %NULL >+ * @callback: (scope async): a function to be called when the read >+ * operation should be completed >+ * @user_data: (closure): data to pass to the @callback function you don't need to specify (allow-none) on GCancellables, or (scope async) on GAsyncReadyCallbacks, or (closure) on "gpointer user_data"s that immediately follow a callback. >+ data->cancellable = cancellable != NULL ? g_object_ref (cancellable) : NULL; You're not currently making any use of this; you need to connect to its "cancelled" signal, and if it triggers, call g_cancellable_set_error_if_cancelled(), g_simple_async_result_take_error(), and g_simple_async_result_complete_in_idle(). (And disconnect from the signal in async_read_pixels_data_free()).
(In reply to comment #11) > (From update of attachment 198320 [details] [review]) > >+ * @buffer: (array): a pointer to a buffer capable of holding the > >+ * pixels read from the @stage > > That should be "(out caller-allocates)" as well. > Some bindings may have issues with there not being a length argument... (I'm > not sure about that.) But anyway, if you force the caller to pass the length, > then you can double-check that they got it right rather than just doing a > buffer overrun. (And you can document in the function docs what that length is > supposed to be.) I think GChecksum has something similar. I even thought of making the function return a pointer to a const guint8* (allocated internally) if the passed buffer is NULL; the length would then become an in-out parameter — but it seemed far too overengineered. the whole point is to try to be lean and avoid multiple, multi-MB allocations, given that the main user (the shell recorder) passed the buffer to GStreamer. anyway, I added a length parameter to the finish() method, and it is used to check that it can at least hold the size of the read_pixels region. > Anyway, if you want this to be usable from bindings then you'd probably better > test that it actually is before the API gets frozen. > > (It might end up being easier to just provide an additional > more-bindings-friendly version that returns an allocated buffer, and using the > "Rename to" annotation...) we can definitely do that, if push come to shove. > >+ g_return_val_if_fail (G_IS_SIMPLE_ASYNC_RESULT (result), FALSE); > >+ > >+ simple = G_SIMPLE_ASYNC_RESULT (result); > >+ > >+ if (g_simple_async_result_propagate_error (simple, error)) > >+ return FALSE; > >+ > >+ g_warn_if_fail (g_simple_async_result_get_source_tag (simple) == clutter_stage_read_pixels_async); > > g_return_val_if_fail (g_simple_async_result_is_valid (result, G_OBJECT > (stage), clutter_stage_read_pixels_async), FALSE); > > simple = G_SIMPLE_ASYNC_RESULT (result); > > if (g_simple_async_result_propagate_error (simple, error)) > return FALSE; fixed. > >+ * @cancellable: (allow-none): optional #GCancellable, or %NULL > >+ * @callback: (scope async): a function to be called when the read > >+ * operation should be completed > >+ * @user_data: (closure): data to pass to the @callback function > > you don't need to specify (allow-none) on GCancellables, or (scope async) on > GAsyncReadyCallbacks, or (closure) on "gpointer user_data"s that immediately > follow a callback. I actually had a look at GIO, and it has the same annotations. given g-i known increasing strictness, I'd rather err on the side of over-annotating stuff. :-) > >+ data->cancellable = cancellable != NULL ? g_object_ref (cancellable) : NULL; > > You're not currently making any use of this I am: I check if the Cancellable is cancelled, and if it is I don't complete the async result — which I guess it's incorrect, now that I look at how it's supposed to work. >; you need to connect to its > "cancelled" signal, and if it triggers, call > g_cancellable_set_error_if_cancelled(), g_simple_async_result_take_error(), and > g_simple_async_result_complete_in_idle(). (And disconnect from the signal in > async_read_pixels_data_free()). fixed.
Created attachment 198432 [details] [review] stage: Implement an async read pixels method version 2.0, with fixes after the review.
Created attachment 198433 [details] [review] stage: Deprecate clutter_stage_read_pixels() the previous patch had a missing test.
Comment on attachment 198432 [details] [review] stage: Implement an async read pixels method >+ * clutter_stage_read_pixels_finish: >+ * @stage: a #ClutterStage >+ * @result: a #GAsyncResult >+ * @buffer: (array): a pointer to a buffer capable of holding the >+ * pixels read from the @stage >+ * @error: return location for a #GError, or %NULL need to add @length, and make @buffer be "(array length=length)" (which looks silly, but means the length of the array is passed in the @length param) oh, and that still needs to be "(out caller-allocates)" as well. (Meaning, this is a buffer that clutter_stage_read_pixels_finish() will write into, as opposed to being data that you're giving to it.) >+ * Asynchronous version of clutter_stage_read_pixels(). It's more "replacement for" given that that's being deprecated otherwise looks plausible to me
Created attachment 198447 [details] [review] stage: Implement an async read pixels method version 3.0 / now with a valid implementation and correct annotations.
(In reply to comment #15) > (From update of attachment 198432 [details] [review]) > >+ * clutter_stage_read_pixels_finish: > >+ * @stage: a #ClutterStage > >+ * @result: a #GAsyncResult > >+ * @buffer: (array): a pointer to a buffer capable of holding the > >+ * pixels read from the @stage > >+ * @error: return location for a #GError, or %NULL > > need to add @length, and make @buffer be "(array length=length)" (which looks > silly, but means the length of the array is passed in the @length param) > > oh, and that still needs to be "(out caller-allocates)" as well. (Meaning, this > is a buffer that clutter_stage_read_pixels_finish() will write into, as opposed > to being data that you're giving to it.) yeah, I noticed after I git-bz attached the patch. :-/ > >+ * Asynchronous version of clutter_stage_read_pixels(). > > It's more "replacement for" given that that's being deprecated I'm not entirely sold on the deprecation, that's why I split the commit. > otherwise looks plausible to me cool. I'll get a review from Robert as soon as he's back.
(In reply to comment #17) > cool. I'll get a review from Robert as soon as he's back. Is he back yet? ;)
would it be possible for you to test the patch, so that at least I know whether the implementation is not insane? :-)
*** Bug 702754 has been marked as a duplicate of this bug. ***
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of clutter, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/clutter/-/issues/ Thank you for your understanding and your help.