[Bug 789843] API: GstPromise - object with promise/future-like semantics

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=789843

--- Comment #4 from Matthew Waters (ystreet00) <ystreet00 at gmail.com> ---
(In reply to Olivier Crête from comment #2)
> Review of attachment 362870 [details] [review]:
> 
> The API seems relatively sane. The only thing I'm not so happy about is the
> change function being called synchronously with the mutex held, is that
> really required? I'd feel better having it called from another thread, doing
> something like gst_element_call_async(), it could even be using the same
> thread pool.

Yea, see I'm very hesitant to add another thread into the mix.

Removing the change_func callback outside the mutex also introduces a race
whereby gst_promise_set_change_callback() can race with calling the change_func
and access freed data.

i.e. before the change_func is called and after the state has changed, it's
possible to 1. change the function but not the data, 2. notify (and therefore
free) the data that will be used in the state change.  Both of these are pretty
severe memory issues :).  The only way I can see to mitigate these is some
extra checking to e.g. seal the state change function once set and passed for
reply and not allow that scenario to occur.

> It may also be nice to have a vaarg version of gst_promise_reply_...() which
> calls gst_structure_new_valist() internally to save a line.

Can be done.

> ::: gst/gstpromise.c
> @@ +40,3 @@
> + *
> + * A #GstPromise can be created with gst_promise_new(), replied to
> + * with gst_promise_reply(), interrupted with gst_promise_reply() and
> 
> I guess you mean "interrupted with gst_promise_interrupt()" ?

Yea

> @@ +157,3 @@
> +    GST_LOG ("%p replied", promise);
> +
> +    promise->promise = s;
> 
> Maybe you want to also do gst_structure_set_parent_refcount() ? Or does it
> make no sense at all?

It sort of makes sense to do that however the idea is that once the structure
is set on the promise, it is immutable which is not quite what
gst_structure_set_parent_refcount() ensures.

> ::: gst/gstpromise.h
> @@ +69,3 @@
> +  GstStructure         *promise;
> +
> +  /*< private >*/
> 
> Shouldn't this be in a hidden struct part of the struct? Maybe using the
> GstXXXImpl trick used by GstBuffer. I would hide all of the implementation
> details, including the structure and the result and only allow access to the
> accessor function.

Yea, I'll look at that.

(In reply to Tim-Philipp Müller from comment #3)
> Is the padding in the GstPromise struct needed? It's a mini object, so can't
> be subclassed anyway, can it?

GstMiniObject's can be subclassed and embedded in other structs.  See GstMemory
and all the memory subclasses :).  In this case I don't think I envision
anybody attempting to subclass GstPromise.  Adding fields to the GstStructure
is enough for user-side extensibility and with the GstXXXImpl we get
implementation-side extensibility as well.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.