Could you add a 

 g_print ("%s: func = %p\n", __FUNCTION__, func);

to both gst_debug_add_log_function() (ca. line 1053 in gstinfo.c) and gst_debug_remove_log_function() (ca. line 1129 in gstinfo.c) and post what the output is?

These functions have already GST_DEBUG statements.

DEBUG GST_DEBUG gstinfo.c:1058:gst_debug_add_log_function: prepended log function 03877900 (user data 00000000) to log functions

DEBUG GST_DEBUG gstinfo.c:1123:gst_debug_remove_log_function: removed log function 0386E8BB 0 times from log function list

gst_debug_remove_log_function also returned 0

> These functions have already GST_DEBUG statements.

Yes, I am aware of that. Please do what I asked you to do, thanks (I want the output for the first _add that's done from within gst_init() as well).

gst_debug_add_log_function: prepended log function 10025830 (user data 00000000) to log functions
gst_debug_add_log_function: prepended log function 00411460 (user data 00000000) to log functions
gst_debug_remove_log_funcion: removed log function 0041107D 0 times from log function list

The situation is reproduced in the following simple program:

#include <stdio.h>
#include <gst/gst.h>

static void gst_log_function(GstDebugCategory *category, GstDebugLevel level,
			const gchar *file,const gchar *function,gint line,
			GObject *object,GstDebugMessage *message,
		 gpointer data) G_GNUC_NO_INSTRUMENT
{

}

int main(int argc, char *argv[])
{
	gst_init(&argc,&argv);
	gst_debug_set_active(TRUE);
	gst_debug_add_log_function((GstLogFunction)gst_log_function,NULL);
	guint removed=gst_debug_remove_log_function(gst_debug_log_default);
	return 0;
}

So it seems that the library internal code gets a different address for the function gst_debug_log_default than your application code.

I remember that there were also problems on Solaris with this. Apparently "it was something to do with the link resolution rules".

Two possible solutions come to mind

    a) add a gst_debug_remove_default_log_function()

    b) deprecate gst_debug_add_log_function() and
        gst_remove_log_function() and replace with a
        new gst_debug_set_log_function() that replaces
        the previous function. (Users can then still chain
        up to the default log handler if they want to.)
        This makes things work more in line with other
        libraries. Not sure one needs to be able to add
        multiple log handlers.

or
    c) just add a _set() without deprecating _add() and
        _remove().

Going to add a _set_log_function() and deprecate _add_log_function(), which was the consensus on IRC.

Created attachment 170893 [details] [review]
info: add gst_debug_set_log_function() and deprecate old API

commit 20c5a8dd47562061a5c823fa4b633bfa6cae2d72
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Thu Sep 23 12:14:07 2010 +0100

    info: add gst_debug_set_log_function() and deprecate old API
    
    Add gst_debug_set_log_function() to set the current log function
    whilst replacing the previous log function. This is mainly to
    work around problems with the _add()/_remove() API on some platforms
    (e.g. win32 and Solaris) where _remove_log_function(gst_debug_log_default)
    doesn't always work properly. The new API also seems more straight-forward
    though, and since no one could really come up with a use case for multiple
    log functions, it seems best to just deprecate the old API.
    
    API: add gst_debug_set_log_function()
    API: deprecate gst_debug_add_log_function()
    API: deprecate gst_debug_remove_log_function()
    API: deprecate gst_debug_remove_log_function_by_data()


Just like _add_log_function() we also don't allow for a DestroyNotify callback in _set_log_function(), because we want to avoid locking in the log handler code path, and I don't see how we can trivially implement this without (doesn't seem worth putting effort into more sophisticated approaches just for this either IMHO). We could of course just add the DestroyNotify callback to the API, but document that it will never be called at the moment. Not sure if that makes things better for bindings / gobject-introspection or if it doesn't really matter.

Anyone got any comments on this? (deprecation + useless destroy notify callback)

>     API: deprecate gst_debug_add_log_function()
>     API: deprecate gst_debug_remove_log_function()
>     API: deprecate gst_debug_remove_log_function_by_data()
> 
> Just like _add_log_function() we also don't allow for a DestroyNotify callback
> in _set_log_function(), because we want to avoid locking in the log handler
> code path, and I don't see how we can trivially implement this without (doesn't
> seem worth putting effort into more sophisticated approaches just for this
> either IMHO). We could of course just add the DestroyNotify callback to the
> API, but document that it will never be called at the moment. Not sure if that
> makes things better for bindings / gobject-introspection or if it doesn't
> really matter.

Hm I thought I commented on this already. The approach and patch is fine but I don't know either if bindings would prefer to have a destroy notify here. AFAIK g-i and binding generators assume, that functions that take a function pointer and data but no destroy notify behave like g_list_foreach(), i.e. the callback will never be called again after the function returned. OTOH we do the same elsewhere too anyway and IMHO g-i should get annotations for this case instead of making our API weird for g-i ;)

Well, having a DestroyNotify callback with the user_data would be the correct API, but arguably not be very useful ;)

Well, if you add a destroy notify for the user data we have a small race here. When setting a new log function while printing something with the old one (i.e. after the g_atomic_pointer_get()) the user data would be destroyed already although it's still used.

This should be fixed by this commit now:

 commit eb56687a6dfd207507a4ca000eae53f93b5e33ea
 Author: Stefan Kost <ensonic@users.sf.net>
 Date:   Wed Dec 15 23:19:54 2010 +0200

    info: use the publicly visible address to fix the tests
    
    The -Bsymbolic change causes us to get a different address when internaly
    looking up the function than what application would get when the use the symbol
    that they see. This made removing the default loghandler to fail, as it is set
    internally and removed externaly.


Keeping bug open for now, since I still think we should simplify the API to only allow one log handler at a time.

Which settings in MSVC in Windows give the same effect as this flag in gcc?

> Which settings in MSVC in Windows give the same effect as this flag in gcc?

Look at the MSVC docs? However, just ignore that bit. The important thing is that the gcc flag leads to the same behaviour on linux as already exists on Windows.

Just pulled gstreamer git head and compiled. 
This doesn't work in windows.

When in gstinfo.c, g_module_symbol returns 0x3ed5560 for gst_debug_log_default, and when in my application, code

gpointer ptr=&gst_debug_log_default returns 0x3e298e2

Ok, thanks for testing. Guess we need new/better API after all then.

However, the code from gstinfo.c, opening the module and requesting symbol
address, has returned correct address

Does this mean that it work with latest GIT or that it doesn't work?

(In reply to comment #17)
> Just pulled gstreamer git head and compiled. 
> This doesn't work in windows.
> 
> When in gstinfo.c, g_module_symbol returns 0x3ed5560 for gst_debug_log_default,
> and when in my application, code
> 
> gpointer ptr=&gst_debug_log_default returns 0x3e298e2

We cannot fix that behaviour in all cases. That is the reason for my change. After the change we don't compare the internal address with the external one.

With the latest git, do the tests pass? There should be no code in core left that does gpointer ptr=&gst_debug_log_default, so where do you still see a problem if any.

Generally speaking, there, probably, should be no code at all, that does such assignment. 
I used gst_debug_remove_log_function(gst_debug_log_default) in my application and it didn't work.
I've got the behavior I wanted after I've copied your latest code from gstinfo.c

So on win32 even for apps &gst_debug_log_default!=dlsym("gst_debug_log_default")?

(In reply to comment #23)
> So on win32 even for apps
> &gst_debug_log_default!=dlsym("gst_debug_log_default")?

yes.

This is a KISS solution, hope it makes things work now. Please re-open the bug if it still doesn't work:

commit 609a75eae28c770372ef1464d1e03fadf03982ea
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Thu Feb 3 10:53:27 2011 +0000

    info: make adding/removing of gst_debug_log_default() work properly
    
    Make adding/removing gst_debug_log_default() work reliably in all
    circumstances. The problem was that depending on platform and linker
    flags the function argument might resolve to different addresses,
    which made it impossible to remove the default log function added
    in gst_init() from application code (because the pointer values
    didn't match). The new approach should keep things simple by passing
    NULL for the default function, which the code in libgstreamer can
    then handle.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=625396
    https://bugzilla.gnome.org/show_bug.cgi?id=640771

commit 74ff936752fdb8715bcafb7560726fbe4d0438ae
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Thu Feb 3 10:28:01 2011 +0000

    Revert "info: use the publicly visible address to fix the tests"
    
    This reverts commit eb56687a6dfd207507a4ca000eae53f93b5e33ea.
    
    While this commit may have fixed a problem on one of the build bots,
    it didn't actually fix the original bug reported for win32.
    
    Also, it causes other problems, such as the lookup failing when
    called from C++ code (gst-phonon, amarok).
    
    This needs to be fixed differently.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=640771
    https://bugzilla.gnome.org/show_bug.cgi?id=625396