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 733187 - integrating the tracer branch
integrating the tracer branch
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.x
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 736466
 
 
Reported: 2014-07-15 07:21 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2016-01-22 09:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch contains missing hooks implementation (10.60 KB, patch)
2015-06-07 22:41 UTC, Marcin Kolny (IRC: loganek)
none Details | Review
patch contains missing hooks implementation (10.55 KB, patch)
2015-08-26 14:16 UTC, Marcin Kolny (IRC: loganek)
committed Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2014-07-15 07:21:27 UTC
I just rebased by tracer branch at:
http://cgit.freedesktop.org/~ensonic/gstreamer/?h=tracer
and want to start some discussion about integrating this into GStreamer.
I have already squashed a few things in the branch, but it still
contains a bit of history to show what I have been trying so far. I'll continue to massage the branch over the coming days; so beware the SHA1s will change.

A design doc is here (containing command-line examples):
http://cgit.freedesktop.org/~ensonic/gstreamer/tree/docs/design/draft-tracing.txt?h=tracer

The whole topic is quite complex and it is not easy to just 'add' the
feature. What I am proposing is to merge the changes (after review and
cleanups), but mark the API as unstable (with a define a la
GST_TRACER_ENABLE_UNSTABLE_API). That gives us time to evolve things.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2014-07-16 07:47:07 UTC
Rebased again and consolidated more patches. It is now down to 30 patches.
Comment 2 Thibault Saunier 2014-09-12 07:50:27 UTC
Overal that API makes a lot of sense to me but there are a few point I would like to talk about:

1- Why did you choose to go with var_args to invoke the hooks? That is a bit weird to use and that means that we won't be able to use the system from the Introspection. I have the impression that for that case using GClosures or even signals could make sense. This way the Tracer implementers would know what the arguments are and potentially let us use the system from the introspection.

  The solution I can think of is (which is basically the same way of doing things as with the bus messgage system):

  You do not actually need the notion of GstTracerHookId as it is anyway repeated in GstTracerMessageId (and then it could be renamed to GstTracerHookId). You would need a structure containing the information about the hook, a GstMiniObject that has a GstTracerHookId and a GstStructure with the various arguments.

  Then I would imagine something using signals and in the gst_tracer_dispatch method. So it would look like:

  The GstTracer::hook:hook-id messsage definition:

    gst_tracer_signals[HOOK_SIGNAL] =
      g_signal_new ("hook", G_TYPE_FROM_CLASS (klass),
      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
      G_STRUCT_OFFSET (GstTracerClass, hook), NULL, NULL,
      g_cclosure_marshal_generic, G_TYPE_NONE, 1, GST_TYPE_TRACER_HOOK_DATA);


Signal emition in gst_tracer_dispatch:

    GQuark details = gst_tracer_type_to_quark;
    g_signal_emit_valist (tracer, gst_tracer_signals[HOOK_SIGNAL], details,
                          GstTracerMessageId, var_args);

Then in the tracer implementation you would connect to the signals you need, for example, a hook could need to operate on GST_TRACER_MESSAGE_ID_PAD_PUSH_PRE only, in the init method of that tracer you would do:

    g_signal_connect (some_tracer, "hook:pad-push-pre", _pad_push_pre_cb);

The _pad_push_post_cb method:

      static void
      _pad_push_pre_cb (tracer, GstTracerHookData *hook_data)
      {
        GstBuffer * buffer;

        gst_tracer_hook_data_parse_pad_push_pre (hook_data, &buffer);

        /* Do you business here */
      }


2- I did not understand what the GstTracer:params property was about.

3- The GstTracer:masks property is rather a class property to me, or am I missing something?


What do you think?
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2014-09-12 08:28:38 UTC
Thanks for taking a look!

1) I started with GstStructures to pass all the arguments and that turned out to cause quite a bit of overhead and the valist looked much simpler. On the other hand, using a GstStructure would make it much easier to provide extra fields in an extensible way. But thanks for pointing out that this would be an issue for introspection.

I like the signal idea and will think about it. So the baseclass will implement the hook signal and trigger it instead of calling dispatch. As the signal supports GQuark detail - that could be a way to extend the hooks - right now I am not happy about the GstTracerMessageId/GstTracerHookId as they are static enums.

2) The GstTracer:params is for passing parameters to the tracer. You can set flags when specifying the tracers: GST_TRACE="log(events,buffers);stats(all)" and in this case 'stats' would receive 'all' on the params proeprty.

3) The GstTracer:masks property is set by each tracer instance to tell what it wants to be invoked for. This is probably going away if we use the signal method you propose. Allthough then the hooks would be all dispatched to all tracers and they must quickly return for those that they don't care about.
Comment 4 Thibault Saunier 2014-09-12 08:32:25 UTC
(In reply to comment #3)
> Thanks for taking a look!
> 
> 1) I started with GstStructures to pass all the arguments and that turned out
> to cause quite a bit of overhead and the valist looked much simpler. On the
> other hand, using a GstStructure would make it much easier to provide extra
> fields in an extensible way. But thanks for pointing out that this would be an
> issue for introspection.
> 
> I like the signal idea and will think about it. So the baseclass will implement
> the hook signal and trigger it instead of calling dispatch. As the signal
> supports GQuark detail - that could be a way to extend the hooks - right now I
> am not happy about the GstTracerMessageId/GstTracerHookId as they are static
> enums.
> 
> 2) The GstTracer:params is for passing parameters to the tracer. You can set
> flags when specifying the tracers: GST_TRACE="log(events,buffers);stats(all)"
> and in this case 'stats' would receive 'all' on the params proeprty.

Ah, I see, nice.

> 3) The GstTracer:masks property is set by each tracer instance to tell what it
> wants to be invoked for. This is probably going away if we use the signal
> method you propose. Allthough then the hooks would be all dispatched to all
> tracers and they must quickly return for those that they don't care about.

The hooks would be dispach only for tracers that are connected to the specific signals only.
Comment 5 Sebastian Dröge (slomo) 2014-09-12 10:47:33 UTC
Note that signals also impose a lot of overhead, they're everything but cheap
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2014-10-02 19:40:21 UTC
The current code got rid of the var_args. The API also looks like it is introspectable, but unfortunately would not work. I am using GCallback to store callback, but then cast the callback to the right pointer in the trace probe. GI marshal everything to GCallback though.

What we can do is to add a gst_tracing_register_hook_structure () and make it so that the dispatch code only creates a GstStructure if there are callbacks that need one. In python picking the args out of the structure is easy, e.g. args["pad"].
Comment 7 Tim-Philipp Müller 2014-10-02 20:13:32 UTC
I'm not sure we should worry about this being introspectable at all. We should worry about overhead and making it as useful as possible for plugins written in C at this point.
Comment 8 Thibault Saunier 2014-10-02 22:57:58 UTC
(In reply to comment #6)
> The current code got rid of the var_args. The API also looks like it is
> introspectable, but unfortunately would not work. I am using GCallback to store
> callback, but then cast the callback to the right pointer in the trace probe.
> GI marshal everything to GCallback though.
> 
> What we can do is to add a gst_tracing_register_hook_structure () and make it
> so that the dispatch code only creates a GstStructure if there are callbacks
> that need one. In python picking the args out of the structure is easy, e.g.
> args["pad"].

That new API looks good now, and it is most probably as lightweight as the old one.

I can imagine people wanting to implement tracer in other language than C, but right it is not high priority for now I would say.
Comment 9 Thibault Saunier 2014-10-04 08:47:22 UTC
I just had a new look at that branch, and I am not sure we should use GstStructure for the reporting. We will probably have more reports than actual hooks, so that part should clearly be optimized. at least as much as the hook calls.

I have the impression that we need an actual reporting system in the tracing system, maybe similare to what we have in GstValidate?

Let me explain briefly how it works:

The object that are abe to do reporting have to implement the GstValidateReporter interface.

Reports are classified by categories, each category being registered in the reporting system.

All reports are concatenated into what we call a Runner. 

We have a GstValidateReport simple structure which contains: the category, the source of the report, and the message.

Do you think such a reporting system would make sense in the tracing system?
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2014-10-04 16:52:59 UTC
Huh? I said several times, that using GstStructures for logging is not perfect - as well as logging the tracing data to the debug log stream. I filed
https://bugzilla.gnome.org/show_bug.cgi?id=733188 to start reorganizing the logging env vars, but that hasn't seen a lot of discussion.
Also right now the logging via structures is the main source of overhead.

Thibault, did you read this draft?
http://cgit.freedesktop.org/~ensonic/gstreamer/tree/docs/design/draft-tracing.txt?h=tracer

One thing I am wondering is where we should just have a structured log and make the debug logging as special case.

Re your suggestion - the extra interface sounds cumbersome. The categories sounds similar to the logging system.

Right now the logging is marked with TODO comments and I think we can figure this in a 2nd run as this is not part of the API.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2014-10-04 17:55:42 UTC
Thibault, I hope this did not came over as unfriendly - I am definitely thankful for the discussions. Regarding the GstStructure use, I mostly use this now as it is convenient (easy to emit the data and also easy to parse it). From profiling one part of the performance issues can be fixed using quarks instead of strings. The other part can probably fixed by using a gst_structure_to_string() variant that takes a char buffer and skips the GString object. As we log lots of record that follow the same template we can probably add some api/helper to compute the size such a buffer would need.

Otherwise we can figure out some binary logging.
Comment 12 Thibault Saunier 2014-10-04 20:09:34 UTC
I did read that draft, and my main focus is to see how useful that could be for GstValidate, and how I would like it to look like so it properly fit into my use case.

I said I think the main API looks good to me and it should be useful for validate, now the other point I wanted to discuss is the logging system, I can not really think about porting GstValidate to gsttrace without having a proper logging system in gsttrace, it would not make much sense to me and it is why I started that conversation.
Comment 13 Thibault Saunier 2014-12-09 20:57:57 UTC
OK, as I already stated the main tracing API looks good. The logging system can for sure be added later.

Should we just go and push that branch? Has anyone something against doing that?
Comment 14 Tim-Philipp Müller 2014-12-10 17:53:07 UTC
I would like to get this in for 1.6, but I would also like more people to review the latest version/branch before it lands.

Stefan, could you rebase your fdo branch on top of master?
Comment 15 Mathieu Duponchelle 2015-01-13 21:14:51 UTC
Hey, I've started having a look at the code, looks promising, I pushed a branch ( remote is git@github.com:MathieuDuponchelle/gstreamer.git ), fixing a crasher with the first example in the design doc, I'll hopefully have more consistent stuff to share in the future :)
Comment 16 Mathieu Duponchelle 2015-01-13 21:16:29 UTC
(In reply to comment #14)
> I would like to get this in for 1.6, but I would also like more people to
> review the latest version/branch before it lands.
> 
> Stefan, could you rebase your fdo branch on top of master?

The branch I pushed on my repo is rebased on master FWIW, had no conflicts and it builds just fine.
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-16 16:50:47 UTC
I have rebased and pushed my branch again and I will pick from Mathieu's branch.
Comment 18 Sebastian Dröge (slomo) 2015-03-20 14:09:11 UTC
Two short questions about the API:
1) Is it possible to get the pre/post push buffer/event/query instances to inspect them?
2) Should we add API to queue, audio/video encoder/decoder, etc to tell us to which input buffer an output buffer belongs? Guessing that without help from elements is not necessarily trivial
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2015-03-21 12:34:12 UTC
(In reply to Sebastian Dröge (slomo) from comment #18)
> Two short questions about the API:
> 1) Is it possible to get the pre/post push buffer/event/query instances to
> inspect them?

who is 'them'? Sorry I don#t understand this?

> 2) Should we add API to queue, audio/video encoder/decoder, etc to tell us
> to which input buffer an output buffer belongs? Guessing that without help
> from elements is not necessarily trivial.

While that sound like a good idea in the first place, I don't think it is generic enough. How would you do that for demuxers? I think in priciple it would be nice but it needs a design so that it actually works. Also I think we can add this later on, right?
Comment 20 Sebastian Dröge (slomo) 2015-03-21 13:33:53 UTC
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #19)
> (In reply to Sebastian Dröge (slomo) from comment #18)
> > Two short questions about the API:
> > 1) Is it possible to get the pre/post push buffer/event/query instances to
> > inspect them?
> 
> who is 'them'? Sorry I don't understand this?

The buffers/events/queries :)

> > 2) Should we add API to queue, audio/video encoder/decoder, etc to tell us
> > to which input buffer an output buffer belongs? Guessing that without help
> > from elements is not necessarily trivial.
> 
> While that sound like a good idea in the first place, I don't think it is
> generic enough. How would you do that for demuxers? I think in priciple it
> would be nice but it needs a design so that it actually works. Also I think
> we can add this later on, right?

Sure, that shouldn't block merging this, just a thought about a feature that seems to be useful to have :) I think the latency tracer module currently uses serialized events for doing this?
Comment 21 Thiago Sousa Santos 2015-03-21 15:35:10 UTC
FWIW I have 2 patches here: http://cgit.freedesktop.org/~thiagoss/gstreamer/log/?h=tracer

One adds hooks for pads queries and the other adds thread-ids to the callbacks (there might be a more generic way of doing it and that would be done from the core of tracer and not from the handlers)
Comment 22 Stefan Sauer (gstreamer, gtkdoc dev) 2015-03-21 16:59:17 UTC
(In reply to Sebastian Dröge (slomo) from comment #20)
> (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #19)
> > (In reply to Sebastian Dröge (slomo) from comment #18)
> > > Two short questions about the API:
> > > 1) Is it possible to get the pre/post push buffer/event/query instances to
> > > inspect them?
> > 
> > who is 'them'? Sorry I don't understand this?
> 
> The buffers/events/queries :)

Yes the hooks can access all the parameters that are passed. They should not modify them though.
> 
> > > 2) Should we add API to queue, audio/video encoder/decoder, etc to tell us
> > > to which input buffer an output buffer belongs? Guessing that without help
> > > from elements is not necessarily trivial.
> > 
> > While that sound like a good idea in the first place, I don't think it is
> > generic enough. How would you do that for demuxers? I think in priciple it
> > would be nice but it needs a design so that it actually works. Also I think
> > we can add this later on, right?
> 
> Sure, that shouldn't block merging this, just a thought about a feature that
> seems to be useful to have :) I think the latency tracer module currently
> uses serialized events for doing this?

Yep, and t has a bunch of comments to explain under which conditions the results make sense.
Comment 23 Stefan Sauer (gstreamer, gtkdoc dev) 2015-03-21 17:19:11 UTC
Thiago, I merged your changes (with a small fix on the thread-id commit). I also rebased the branch.

Regarding the thread id, yes, there are a few things that we could add to each trace from the core. The problem is that modules need to know about it so that its serialized in the trace message. An alternative idea would be to use a thread-tracer that runs on all hooks and logs the current thread. The app that analyzes the logs would need to combine logs based on timestamps. What I like about it is that we can add all kind of other ressources this way. Right now the rusage tracer already logs the thread-id. So if you agree tha aggregation from the app is the way to go, we could drop the "tracer: gststats: add thread-id to log line" commit again. WDYT?
Comment 24 Julien Isorce 2015-03-31 16:45:38 UTC
Hi, I am trying to understand this new feature, some questions came up:

1- There is hard coded relation between PRE/POST, it is up to the GstTracer plugin to make the relation right ? (So the tracer can only hook all the PRE without the POST if it wants)

2- In the draft you mentioned  "We'll wrap interesting api calls with two macros" but it does not necessarly work by pairs right ?

3- In the draft you mentioned "I'd like to know about ref-counts of parts in the pipeline to find ref-count issues" but there is no such hook for the moment right ?

4- With such hook GST_TRACER_OBJECT_REF / GST_TRACER_OBJECT_UNREF there is no PRE/POST wording and these hooks are not surrounding a specific function, would this be still valid ?

5- Then I am a bit confused, if such ref/unref hook exist, it will replace "GST_CAT_TRACE_OBJECT (GST_CAT_REFCOUNTING," in gst_object_ref / gst_object_unref ? Actually it will allow to a GstTracer to match refs with unrefs ?

6- Could _gst_alloc_trace_new/_gst_alloc_trace_free/(_gst_alloc_trace_register) be replaced by GST_TRACER_OBJECT_NEW / GST_TRACER_OBJECT_FREE ? (live/mem-live, and replace all the _gst_alloc_trace api in general ?)

7- Would it be possible to add support to print the backtrace of the hooks from a GstTracer ? parenting hierachy ? And who own a ref on a "traced" object ?
It would give some hints to fin ref-count issues.

I guess most of the answers will be yes but not sure :)

In the draft you mentioned "opengl", indeed that would be awesome to have some hooks in GstGL. I can give a try in the future.

Thx for this feature, can't wait it to be merged!
Comment 25 Stefan Sauer (gstreamer, gtkdoc dev) 2015-04-05 14:17:57 UTC
(In reply to Julien Isorce from comment #24)
> Hi, I am trying to understand this new feature, some questions came up:
> 
> 1- There is hard coded relation between PRE/POST, it is up to the GstTracer
> plugin to make the relation right ? (So the tracer can only hook all the PRE
> without the POST if it wants)

Yes it is hardcoded and it is mostly meant to log return values as well.

> 
> 2- In the draft you mentioned  "We'll wrap interesting api calls with two
> macros" but it does not necessarly work by pairs right ?

Yes void methods would not need a 2nd entry and there could be api entries where we want to have 3 hook if we'd need to track some inbetween state. I did not come across such one so far though.

> 
> 3- In the draft you mentioned "I'd like to know about ref-counts of parts in
> the pipeline to find ref-count issues" but there is no such hook for the
> moment right ?

That part is an attempt what people would like to do with tracing data. I did not received a lot of contributions here though.

> 
> 4- With such hook GST_TRACER_OBJECT_REF / GST_TRACER_OBJECT_UNREF there is
> no PRE/POST wording and these hooks are not surrounding a specific function,
> would this be still valid ?

The main issues with refcounts is that we'd need to do this in glib and not in gstreamer.

> 
> 5- Then I am a bit confused, if such ref/unref hook exist, it will replace
> "GST_CAT_TRACE_OBJECT (GST_CAT_REFCOUNTING," in gst_object_ref /
> gst_object_unref ? Actually it will allow to a GstTracer to match refs with
> unrefs ?

Yes, tracing can often replace logging. Matching unref to refs is tricky. One can figure easily whether the refs and unrefs are balanced, but if not, there is not easy way to point to where the imbalance comes from.

> 
> 6- Could
> _gst_alloc_trace_new/_gst_alloc_trace_free/(_gst_alloc_trace_register) be
> replaced by GST_TRACER_OBJECT_NEW / GST_TRACER_OBJECT_FREE ? (live/mem-live,
> and replace all the _gst_alloc_trace api in general ?)

These are deprecated. I believe we consider dropping them in 2.0.

> 
> 7- Would it be possible to add support to print the backtrace of the hooks
> from a GstTracer ? parenting hierachy ? And who own a ref on a "traced"
> object ?
> It would give some hints to fin ref-count issues.

glib has support for backtraces, but it is weak. There are solutions to get gcc style backtraces, but they are all 1) complicatied and 2) not very portable :/

> 
> I guess most of the answers will be yes but not sure :)
> 
> In the draft you mentioned "opengl", indeed that would be awesome to have
> some hooks in GstGL. I can give a try in the future.

I think this is one thing holding the merge back, getting some testing to use this outside of the core.

> 
> Thx for this feature, can't wait it to be merged!
Comment 26 Marcin Kolny (IRC: loganek) 2015-06-07 22:41:07 UTC
Created attachment 304739 [details] [review]
patch contains missing hooks implementation

Hi,
I added missing hooks (some of them are necessary to continuing my GSoC project http://www.google-melange.com/gsoc/project/details/google/gsoc2015/loganek/5651442522128384).
Please review it and apply, or let me know if something's wrong and my patch needs improvements.
Comment 27 Stefan Sauer (gstreamer, gtkdoc dev) 2015-08-26 13:06:22 UTC
Review of attachment 304739 [details] [review]:

Thanks & looks good. I'll rebase the branch and merge the patch tonight.
Comment 28 Marcin Kolny (IRC: loganek) 2015-08-26 14:16:55 UTC
Created attachment 310034 [details] [review]
patch contains missing hooks implementation

I just noted, that element passed to GST_TRACER_BIN_REMOVE_POST macro might be (and usually, is) already removed. I think, for safety reasons, it would be better if we don't pass element to bin-remove-post hook.
Comment 29 Stefan Sauer (gstreamer, gtkdoc dev) 2015-08-26 21:43:49 UTC
Branch has been rebased and pushed.
Comment 30 Sebastian Dröge (slomo) 2015-09-27 11:19:18 UTC
Let's get this merged now? :)
Comment 31 Marcin Kolny (IRC: loganek) 2015-10-05 07:10:57 UTC
I thought it will be done before 1.6 release :) Anyway, I've yesterday merged the tracer branch with the master branch (without conflicts), and everything seems to work fine.
Comment 32 Sebastian Dröge (slomo) 2015-10-05 10:51:38 UTC
So, let's merge this next Monday (12th) until someone has any objections.
Comment 33 Stefan Sauer (gstreamer, gtkdoc dev) 2015-10-05 19:32:06 UTC
And it is merged \o/.
Comment 34 Tim-Philipp Müller 2016-01-05 13:15:08 UTC
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..

- 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.

- 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?

- 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.

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

- 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?

- 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
Comment 35 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-06 12:15:30 UTC
(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@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@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.
Comment 36 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-06 19:50:08 UTC
(In reply to Tim-Philipp Müller from comment #34)
> - 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?
> 
> - 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

commit 5e40639be7a3b8b809d216d49a79012dbf1dc37d
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Wed Jan 6 20:41:26 2016 +0100

    tracer: make gst_tracing_register_hook_id static
    
    We don't need to expose this as public API. Change the only plugin that was
    using it.
Comment 37 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-06 21:24:35 UTC
> - 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?

I could document all the func pointer typedefs in gsttracerutils. This way I'd document the params and how the quark string is called. I'd add the doc-blobs to the GstTracer section. WDYT?

> - 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

I've been thinking about the options:
a) add G_TYPE_POINTER serialisation to GstValue
   pro: the string would be e.g. thread=(Ptr)0xdeadf00d
   cons: serializing and deserializign a pointer is not meaningful
         (except if the only use is to compare the ids)

b) convert the thread_id to a string in the tracer
   pro: no gstreamer api additons
   cons: we'd serialize it as thread=(String)"0xdeadf00d"

c) generate some 'id' by storing all thread-ids into a array and serializing the index, the stats-tracer already does something like that for element/pad-ids. That would become some helper api to be used by all tracers.
Comment 38 Tim-Philipp Müller 2016-01-07 13:52:03 UTC
Discussion regarding gst_tracer_log_trace() and GstStructure etc. moved to bug #760267.

docs: sounds good to me

pointer serialization: In general I wouldn't be opposed to gst_value_serialize() serialising the actual pointer values instead of NULL, it's confusing to see misc pointer/boxed fields that couldn't be serialized in the debug log that say =NULL but are actually there and valid. What I probably wouldn't be too keen on is pointers getting deserialized again into the actual values.

Regarding thread IDs: stuffing them into a uint64 would probably be sufficient too, but there's a more general question about how to serialise/track certain objects, like threads, pads, elements, possibly also events/buffers by ID. I wonder if a general system makes sense for that, ie. make such an ID an integral part of the system somehow. At least threads are part of the tracing system so special-casing something for them would be fine too. How would one get the name/label of a thread to a possibly external tool later, for example?
Comment 39 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-07 22:13:44 UTC
Docs are now added:
commit 8968af3589465ad22a2e31464fef0d66e5d070d1
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Thu Jan 7 19:13:03 2016 +0100

    tracerutils: document the tracer hook functions
    
    Document all tracer hook function pointer together with the detail string that
    one needs to use with gst_tracing_register_hook().

Marcin, I also cleaned the query hooks up - you'll need to update your tracer.

commit 2f41e7bc6a842044cb73bc0470601200575c378a
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Thu Jan 7 23:03:48 2016 +0100

    tracer: harmonize the query hooks
    
    In post hooks always pass the return value as the last param. Pass the query
    also to post hooks since it is still alive.

Marcin, and another question for the ELEMENT_NEW we discussed why we add now pre/post, but for element-{add,remove}-pad we did not. Any thoughts.

Finally for all. The hook-details are called e.g. "pad-push-pre" or "pad-push-post". It is essentially the method name without the "gst_" prefix, with '_' changed to "-" and '-pre'/'-post' added. I wonder if "pad-push:pre" or "pad-push:post" would look better?
Comment 40 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-22 09:13:00 UTC
Just a heads up; More API changes in git master.
https://bugzilla.gnome.org/show_bug.cgi?id=760267
https://bugzilla.gnome.org/show_bug.cgi?id=760821