Created attachment 91733 [details] [review]
dot file generator

Created attachment 91808 [details]
sample output from rhythmbox crossfading player

This is really nice.  Much better than the indented text debug output I've been using for a while.

One problem I've noticed is that when I have two decodebins in my pipeline, the graph gets messed up a bit because their typefind elements aren't uniquely named.  The result (generated from this .dot file) looks like this: http://methlab42.itee.uq.edu.au/~jonathan/rb-xfade-player.png

Created attachment 91917 [details] [review]
dot file generator

I cleaned up the code a bit and used the memory address of the element to ensure uniqueness.

Jonathan, I have one thing that I like to add. Currently the tool draws the pipeline structure and adds the negotiated mime-types to the edged (once they are negotiated). I would like to make details optional but adding a flag parameter.
enum {
  SHOW_CAPS, /* show caps on edges */
  SHOW_NON_DEFAULT_PARAMS, /* show modified parameters on elements */
  ...
};

regarding the Element states:
VOID_PENDING, NULL, READY, PAUSED, PLAYING
One idea would be to add a acsii icon infront to the type, e.g.
+-------------------+
| [0] <GstFakeSink> |
|     fakesink      |
|                   |
+-------------------+

with these 'icons'
[~] VOID_PENDING
[0] NULL
[-] READY
[=] PAUSED
[>] PLAYING

Any other things that would be nice to know. 

The updated patch draws my pipeline correctly now, thanks.

I've previously found that showing both the current and target states is useful for tracking down state change problems.  I can't seem to think of a better way of displaying the state information.

Another useful option would be to include some properties from the caps in the edge labels (rate, channels, depth / width, height, framerate, bpp).  Adding properties separated by newlines, so the label ends up like this: "audio/x-raw-int\nrate=44100\nchannels=2\ndepth=16" seems to work OK.

Created attachment 92395 [details] [review]
feature improved version

This version has a flag in addition to select the details on the graph:
 * GST_DEBUG_GRAPH_SHOW_MEDIA_TYPE: show caps-name on edges
 * GST_DEBUG_GRAPH_SHOW_CAPS_DETAILS: show caps-details on edges
 * GST_DEBUG_GRAPH_SHOW_NON_DEFAULT_PARAMS: show modified parameters on elements
 * GST_DEBUG_GRAPH_SHOW_STATES: show element states

It's quite cool but I wonder why you use GST_DEBUG_* macros to dump it.  It doesn't look particularly GST_DEBUG* related to me other than that it's useful for debugging, among other things.  Also, having it as GST_DEBUG_* macro might lead some people to believe that this macro can be used freely just like GST_DEBUG, which probably wouldn't be a good idea, would it?

I also wonder if it should be part of libgstreamer, or if it shouldn't rather be put in some external helper library, like pbutils or so.

Furthermore, I wonder if the _WITH_TS variant is really needed, or if the normal dump function could just check with strstr() if there's a "%" GST_TIME_FORMAT template in the filename and fill in the time.

Haven't looked at the implementation, since it doesn't interfere with anything else.


In short, my suggestions:

   - turn the macros into full-fledged functions

   - either put the whole thing into gstutils.c or
      into a new pbutils sub-module

Others may disagree of course. :)

Many thanks for reviewing! Some rationale:

* I added macros so that it can be automatically disabled in production builds. I think this is mostly interesting when one plugs complex piplines and need a overview when things go wrong. I have such macro call e.g. in my bus error and warning handlers.

* I don't think a new library is a godo idea, sticking it into an existing one might be okay. But if people feel its too big - it disapprears totally when doing --disable-gst-debug

* Its not that slow. I've putted it once in state-change. I though about creating a video from the graphs. The impact on the execution was not noticable (is does not run dot, it only writes the files). Making a video failed, as first frames have to be made equal sized and then xvimagesink can render video sized 3200x2400 :). I though of misusing annodex to link that sparse video with logs.

* Regarding _WITH_TS: Yep, that would be possible.

Another thought: we should check the number of '%' without following '%' in the template string before feeding it to printf to make sure the template is correct.  Maybe even take separate directory + filename arguments (that way the caller has more control over the template and can make sure more easily that there aren't any unexpected % in the path).


(In reply to comment #8)
> * I don't think a new library is a good idea, sticking it into an existing one
> might be okay.

That's what I meant. -base/gst-libs/gst/pbutils/  already exists.


> But if people feel its too big - it disapprears totally when
> doing --disable-gst-debug

It's not about size (for me at least).  It seems to me more like a function that would never really be used in production code anyway.


> * Its not that slow. I've putted it once in state-change. I though about
> creating a video from the graphs. The impact on the execution was not 
> noticable

Everything that writes to file is potentially slow.  I was more worried about people putting it into the streaming thread and then forgetting about it and complaining that GStreamer is soo slow ;)

I don't see the advantage of separate directory and filename-template, but I can add checking for '%'.

I could rebase the patch to be in gst-plugins-base/gst/libs/gst/xxx
as dump.{c,h} and change the macros from
GST_DEBUG_BIN_TO_DOT_FILE
GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS
to
GST_DUMP_BIN_TO_DOT_FILE
GST_DUMP_BIN_TO_DOT_FILE_WITH_TS

What I don't like about it and why I still prefer it in gstinfo:
* is it still obvious that it will be disabled/enabled accourding to --enable-gst-debug?
* apps have to conditionaly link against extra libs for debug utils
* core even has stuuf like gst_parse_launch, which I belive really should be in a utitily library

@tim, the placeholeder in the template should be actually %u:%02u:%02u.%09u aka "/tmp/metadata.%" GST_TIME_FORMAT ".dot".

I have some code for checking the string, but basically I think its not covering 100% of the cases and not worth the trouble. Are you fine with some sanity check like this:

  gchar *file_name = NULL;
  const gchar *pos;
  GTimeVal now;
  GstClockTime elapsed;
  guint fmt_ct = 0;
  
  g_return_if_fail (GST_IS_BIN (bin));
  g_return_if_fail (file_tmpl != NULL);
  
  /* check file-name template */
  pos = strchr(file_tmpl, '%');
  if (pos) {
    do {
      pos++;
      if (*pos != '\0') {
        if (*pos != '%')
          fmt_ct++;
        pos++;
      }
      pos = strchr(pos, '%');
    } while(pos);
  }
  if (fmt_ct == 0) {
    GST_WARNING ("file template has no valid placeholder");
    return;
  }
  else if (fmt_ct !=4) {
    GST_WARNING ("file template must have 4 placeholders, but has %d", fmt_ct);
    return;
  }

Since noone else seems to have comments, here's my suggestion to get this moving forward:

 - forget about the formatting stuff I mentioned for now, it's not
   that important

 - put this into gstdebugutils.[ch] or gstdebug.[ch] in core

 - the macros should evaluate to nothing if GST_DISABLE_GST_DEBUG
   is defined

 - _gst_debug_bin_to_dot_file() should only actually _do_ something
   if a certain environment variable is set (similar to GST_DEBUG,
   maybe GST_DEBUG_DUMP_DOT_FILES=1?); if that environment variable
   is not set, it should just return.

How about that?

2007-10-17  Stefan Kost  <ensonic@users.sf.net>
 Allow dumping pipelines as dot graphs. Fixes #456573.

 docs/gst/gstreamer-sections.txt |    5 
 gst/Makefile.am                 |    2 
 gst/gst.c                       |    4 
 gst/gst.h                       |    1 
 gst/gstdebugutils.c             |  494 ++++++++++++++++++++++++++++++++++++++++
 gst/gstdebugutils.h             |  102 ++++++++
 gst/gstinfo.c                   |   12 
 gst/gstinfo.h                   |    4 
 tools/gst-launch.c              |   15 +
 9 files changed, 635 insertions(+), 4 deletions(-)

Dude this is AWESOME! thanks!

Right now one uses macros like:

GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(pipeline),GST_DEBUG_GRAPH_SHOW_ALL,"/tmp/gst-launch.%" GST_TIME_FORMAT ".dot");

in the code and an environment variable like GST_DEBUG_DUMP_DOT_FILES=1 at runtime to activate it. What about making the macros simpler:

GST_DEBUG_BIN_TO_DOT_FILE(GST_BIN(pipeline),GST_DEBUG_GRAPH_SHOW_ALL);

and passing the base-filename via environment var:

GST_DEBUG_DUMP_DOT_FILES="/tmp/gst-launch."

If the envvar is empty - no dumps will be made. If the envvar contains something it will create names by appending the timestamp + ".dot".


One thing I wold like to have (but I am too lazy to write) is a UI app. I get stwo arguments: a gstremaer log-file and the same basename as above.
The ui shows the picture in the upper half and the log in the lower half.
When clicking a log-line it would load the picture with the closest (?) timestamp in the name.
The tool could also run dot to make the pictures for the dot files initialy. Any volunters?

(In reply to comment #15)
...
> One thing I wold like to have (but I am too lazy to write) is a UI app. I get
> stwo arguments: a gstremaer log-file and the same basename as above.
> The ui shows the picture in the upper half and the log in the lower half.
> When clicking a log-line it would load the picture with the closest (?)
> timestamp in the name.
> The tool could also run dot to make the pictures for the dot files initialy.
> Any volunters?
> 

I planned a GUI debug viewer as a sister program to gst-inspector when it was in the planning stages.  Maybe I will resurrect this idea.

Anyways, it would be great if the .dot dumping code would emit a log message with the filename and the dumped object name.  This way, a viewer program could just take a debug log as argument, then watch out for these messages and automagically load or render the graphics if they are there.

It now does
GST_INFO("wrote bin graph to : '%s'",file_name);
after writing the file.

> What about making the macros simpler:
> 
> GST_DEBUG_BIN_TO_DOT_FILE(GST_BIN(pipeline),GST_DEBUG_GRAPH_SHOW_ALL);
> 
> and passing the base-filename via environment var:
> 
> GST_DEBUG_DUMP_DOT_FILES="/tmp/gst-launch."
> 
> If the envvar is empty - no dumps will be made. If the envvar contains
> something it will create names by appending the timestamp + ".dot".

I think something like that would be a good idea.  Either what you suggested, or maybe something like:

 - environment variable contains absolute path to directory
   where to dump all the dot files

 - caller passes in some kind of descriptive filename thing/suffix
   like 'totem-error' or 'state-change-ready-to-paused' and
   the function then writes out the pipeline state to a file called:

     $PATH_FOR_DOT_FILES/TIMESTAMP-description-foo.dot

That leaves things nicely sorted by timestamp in a central user-defined location, and applications like totem don't need to worry about what directory to drop their stuff in, and you can also get rid of the _with_timestamp() variant.

2007-10-29  Stefan Kost  <ensonic@users.sf.net>

	* docs/gst/running.xml:
	* gst/gst.c:
	* gst/gstdebugutils.c:
	* gst/gstdebugutils.h:
	* tools/gst-launch.c:
	  Improve bin graph dumping, by using the envvar to specify a path.
	  Rename the envvar to GST_DEBUG_DUMP_DOT_DIR.