[Bug 733187] integrating the tracer branch

[GStreamer (GNOME Bugzilla)]

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

--- Comment #35 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> ---
(In reply to Tim-Philipp Müller from comment #34)
> Sorry if I'm late to the party, just a few questions/comments/observations:
> 
> - is gsttracerutils.h really supposed to be an installed header? (contains
> lots of internal / priv_gst_* stuff) Are the dispatch macros supposed to be
> public? they can only be used internally in GStreamer.

commit 1af0a3ce6f99bdfe7b07791bf54f2ca36892932a
Author: Stefan Sauer <ensonic at users.sf.net>
Date:   Wed Jan 6 11:35:46 2016 +0100

    tracerutils: move header to noinst section

    This is internal code, that is only to be used in core.

> 
> - gst_tracer_log_trace() takes a GstStructure, it might be nice to have a
> variant that takes a vararg function so tracers don't have to do the
> gst_structure_new(..) dance. The other reason the vararg approach might be
> nice is that it gives us more options later to stuff the contents into
> something else or process/serialise them more efficiently. Additionally, I
> wonder if it'd make sense to specify explicitly what types are accepted (you
> seem to use GST_TYPE_STRUCTURE a lot for nesting, which seems unfortunate in
> this context to make it generic). GVariant would be perfect for this, only
> that unlike GstStructure it doesn't make fieldnames mandatory, although we
> could just require GVariant with key/value tuples like we do in
> GstStructure. I guess we can always add some other format in future if we
> want to.

Can we open a new discussion regarding gvariant? I can try to add a varargs
version. Can you open a new ticket for that and link it here.

> - gst_tracer_log_trace() is not annotated/documented. Is it supposed to be
> *the* standard function by which tracers communicate results back, or just
> something simple for now that always dumps the data into the debug log?

commit efa316d66671c314aac820aee00e447a14ff34a0
Author: Stefan Sauer <ensonic at users.sf.net>
Date:   Wed Jan 6 12:47:26 2016 +0100

    docs: add the tracer to the docs

    Add GstTracer and GstTracerFactory to the core docs.

> 
> - does gst_tracing_register_hook_id() make sense as public API?
>     - it's not performance-critical, so just passing strings is fine
>     - quark table is not exported (good thing too really?)
>     - hook id quarks are run-time generated and basically random instead of
> constant, so an enum might be better if one really wants something like this
> (like the old GstTracerHookId which was removed again)
>     - tracer hooks are looked up in a hash table anyway, might just as well
> use the strings directly as key then if we don't have an array or such
> 
> I get the sense that this is all done to allow code outside of GStreamer
> core (say, libgstgl) to add additional hooks, but this all doesn't seem
> quite right to me yet.

This is api for the tracers. I could probably make hook_id() a static helper.
Since the quark stable is static, it cannot be expanded right now. If we need
hooks in plugin we need a more dynamic approach, but maybe having all this
static (which is much better for perf reasons) is actually enough.

> 
> - is hook callback registration bindings friendly as is? (no user_data +
> user_data_weak_notify; GSignal uses GClosure, no?)

Tracers can only be written in C right now. If someone wants to make it
possible to write them in python/js/... I don't mind, but its outside of the
scope I am looking at. If you look at the history of my branch you can see that
we used Signals, but thats way too slow (involves locks and expensive lookups).
We really need direct callbacks here.

> 
> - do we have a canonical list somewhere of implemented hooks + arguments
> they will get? If not, that should probably be added to the docs of
> gst_tracing_register_hook() ? Is gsttraceutils.h that list?

Yes. I'll think of a nice way to add those to the docs.

> 
> - core tracers seem to put the thread ID into a guint by casting the result
> of g_thread_self(), don't think that's right

It is not perfect, but unfortunately gst_value_serialize() turns G_POINTER into
NULL regardless of the value, bug or feature? I guess it is because
g_value_transform() will not transform a pointer into a string rep of the
address.

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