I don't like this because it's semantically orthogonal to the current log levels.

Also, GST_LEVEL_LOG is exactly meant for refcounting - it's even saying so in the docs. So it makes more sense to me to tell people to create debug logs with *:4 in the future and make sure the other messages are DEBUG messages.

> I don't like this because it's semantically orthogonal to the current log
> levels.

I don't think it is. I'm sure we can bike-shed about this extensively though.
 

> Also, GST_LEVEL_LOG is exactly meant for refcounting - it's even saying so in
> the docs. So it makes more sense to me to tell people to create debug logs with
> *:4 in the future and make sure the other messages are DEBUG messages.

No, the INFO/DEBUG vs. LOG distinction is very useful in practice. It is also reasonably well-defined and used fairly consistently in practice (LOG is for recurring things in streaming threads etc.)

Of course there's nothing *wrong* with putting refcounting info into the LOG level, but it's not very *useful* in practice. In fact, it's counter-productive and annoying, since 99% of the times it just adds a lot of noise to the debug logs, which makes it harder to read and handle.

Additionally, a new log level means that you can output refcounts more fine-grained in future by using the right debug category for them (EVENT, CAPS, MINIOBJECT etc.), which makes it slightly easier to debug refcounting issues when you're unfortunate enough to be in such a position.

We could call the new category SPAM instead of REFCOUNT, but I don't know when else one would ever want to use that and not LOG (or MEMDUMP), so it seems better to just call it what it is.

we could also do something to explicitely disable a catgory, like:

GST_DEBUG=-REFCOUNT:5

> we could also do something to explicitely disable a catgory, like:
> 
> GST_DEBUG=-REFCOUNT:5

True, but why add something to disable something you don't want in 99% of the cases anyway? That seems doing it the wrong way round.

(In reply to comment #3)
> we could also do something to explicitely disable a catgory, like:
> 
> GST_DEBUG=-REFCOUNT:5

You can do that today with *:5,GST_REFCOUNT:0 but I agree with Tim that we're looking for a solution that does the right thing by default.

We could have categories that don't trigger with wildcards and require an explicit match to be enabled, but I'm not convinced that buys us anything. In particular because - as Tim noted on IRC - refcounting is done in a lot of categories and you might want to put ie. X context refcounting into the ximagesink category or use own categories for messages, elements, queries and buffers.

I'm still not convinced "REFCOUNTING" is the best name for it, as it basically excludes everything else, but it's by far the best solution for the given problem.

I've never liked all the refcount log messages, and would just prefer to replace them by adding the object refcount to GST_DEBUG_OBJECT() messages.

What I usually do to track refcounting problems is dump the stack when gst_object_ref/gst_object_unref() matches a particular object, either by patching the source or scripting gdb.

Created attachment 162687 [details] [review]
hack patch to do backtraces when reffing/unreffing a named object

Example output:

UNREF: object identity0 unref 2 -> 1
  [1]: /usr/local/lib/libgstreamer-0.10.so.0(gst_object_unparent+0x10b) [0xd7f6a
  [2]: /usr/local/lib/libgstreamer-0.10.so.0(+0x28118) [0xd86118]
  [3]: /usr/local/lib/libgstreamer-0.10.so.0(gst_bin_remove+0x120) [0xd82ea0]
  [4]: /usr/local/lib/libgstreamer-0.10.so.0(+0x2b736) [0xd89736]
  [5]: /usr/local/lib/libgstreamer-0.10.so.0(+0x5fabf) [0xdbdabf]
UNREF: object identity0 unref 1 -> 0
  [1]: /usr/local/lib/libgstreamer-0.10.so.0(+0x2816c) [0xd8616c]
  [2]: /usr/local/lib/libgstreamer-0.10.so.0(gst_bin_remove+0x120) [0xd82ea0]
  [3]: /usr/local/lib/libgstreamer-0.10.so.0(+0x2b736) [0xd89736]
  [4]: /usr/local/lib/libgstreamer-0.10.so.0(+0x5fabf) [0xdbdabf]
  [5]: /usr/lib/libgobject-2.0.so.0(g_object_unref+0x10b) [0xf4832b]

You can get better debugging information by asking addr2line, which is kind of clumsy:

  $ addr2line -e /usr/local/lib/libgstreamer-0.10.so.0 0x5fabf
  /home/ds/gst/gstreamer/gst/gstpipeline.c:251

I think we all have our little hacks like these (I added something like that to GstAllocTrace at some point, with _last_seen() tracepoints for buffers and events). It's all really drifting a bit off-topic though.

I would like the refcount logging noise removed from *:5 by default. Removing it entirely seems unnecessary, as sometimes that output is actually useful. Modifying libgstreamer is not always an option (or too cumbersome), and gdb/debuggers are often not fully functional on some platforms. 

Displaying the object refcount in GST_DEBUG_OBJECT would be possible, but is far less useful, since then you'd lack proper coverage, and it doesn't work well for events and buffers and caps.

I still think my approach is a good compromise. It costs us nothing and solves a real problem (noise).

Adding further ways to investigate memory leaks and/or refcount issues is always cool, but I don't see the need to link these to this bug or what I propose.

Maybe the category should be called something like TRACE? Then we could add more things to it, like normal memory allocations, etc...

I like TRACE, it's more generic than REFCOUNT.

+1 from me for TRACE

commit 66fc4c8ba26d7907cf895b3d1f85ba5f00dfd9f8
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Thu Jun 3 01:49:51 2010 +0100

    info: add new TRACE log level and move refcounting there from LOG level
    
    This makes it possible to easily get a *:5 debug log without all
    the refcounting noise, and drastically reduces the number of lines
    output for a normal log (46m to 28m for a 20min video). The full log
    including refcounting information can still be gotten using *:7.
    
    Fixes #620460.