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 613081 - info: make GST_PTR_FORMAT etc. work on win32, OSX and non-glibc systems
info: make GST_PTR_FORMAT etc. work on win32, OSX and non-glibc systems
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 696885 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-03-16 20:54 UTC by Tim-Philipp Müller
Modified: 2013-04-13 10:19 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Tim-Philipp Müller 2010-03-16 20:54:21 UTC
On Windows, OSX and other systems (embedded builds etc.) neither register_printf_specifier() nor register_printf_function() are available.

This makes GStreamer debug logs much less useful, since you don't get the caps serialised in the debug log. This applies to other GST_PTR_FORMAT arguments as well, but I think 95% of those are caps.

Therefore I propose to add some new API that works like

 GST_LOG_CAPS ("get_caps returned: ", caps);
 GST_LOG_OBJECT_CAPS (pad, "get_caps returned: ", caps);

etc.

so people on other platforms are not left out in the cold.
Comment 1 David Schleef 2010-03-16 22:37:38 UTC
Alternatively, we could handle %P right before calling g_strdup_vprintf() in gst_debug_message_get() by replacing all instances of %P in the pattern with the caps string.  This would only work if %P is the first % instance, though.

Or... implement %P in glib, since glib's printf implementation is used if glibc is not used.  (Er, maybe.  Don't know if that's true always.)
Comment 2 Tim-Philipp Müller 2010-03-16 23:57:19 UTC
> Or... implement %P in glib, since glib's printf implementation is used if glibc
> is not used.  (Er, maybe.  Don't know if that's true always.)

Not sure that's always true (surely then we wouldn't problems with NULL string pointers crashing on win32?). There's a define now in glibconfig.h that tells us which implementation is used, but I don't think that's particularly useful here.

Besides, it's not really nice to hog these specifiers as a library, just like setting up signal handlers isn't very nice, so if we can find a way to avoid that (in future versions), all the better, no?
Comment 3 Benjamin Otte (Company) 2010-03-17 23:09:35 UTC
Also, it would be really awesome if we could get rid of the %P specifier somehow. It's unportable, unstable glibc API and we can't use G_GNUC_PRINTF() on the debug function.
Comment 4 Sebastian Dröge (slomo) 2011-05-24 09:19:34 UTC
Any progress on this?
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2011-05-24 15:48:13 UTC
Can't we just have a *big* static array of *big* string buffers, a counter and a couple of functions like GST_DEBUG_FORMAT_CAPS(caps). The function behind the macro obtains the next array slot, inc the counter, format the parameter into the string buffer and return the reference. The counter will wrapp around on the number of slots. That is if we have 20 slots, we support log prints with up to 20 formatted args. There might be more slots in use due to threads. Still we should be able to get away with a large number of slots.
Comment 6 Tim-Philipp Müller 2011-05-24 16:35:22 UTC
> Can't we just have a *big* static array of *big* string buffers, a counter and
> a couple of functions like GST_DEBUG_FORMAT_CAPS(caps).

That's possible, and could probably even be done without the artificial limitation using GPrivate or GStaticPrivate or so), but I think I'd still prefer dedicated API for this, for at least two reasons:

 - dedicated API (macros) makes this much more
   discoverable than GST_PTR_FORMAT or
   GST_DEBUG_FORMAT_CAPS(). A counter-argument
   to this would be the fact that many newbies just
   do GST_LOG("%s", gst_caps_to_string (caps)), so
   offering something along those lines, maybe a
   gst_caps_to_static_string() could be seen as
   more intuitive.

 - we log a lot of caps. This is really frequently
   used. It would be nice to have nice API for that,
   so the code looks nice. IMHO GST_FORMAT_TIME /
   GST_TIME_ARGS() often make things quite ugly,
   would be nice to avoid adding more of those.


> Any progress?

Not really, still not sure about the right API yet I think.
Comment 7 Antoine Tremblay 2012-01-06 17:27:34 UTC
A gst_caps_to_static_string() seems simple enough no ? 

This is a major pain on osx, I'm forced to patch the code each time I want some caps with a gst_caps_to_string/g_free ...

I'm willing to do a lot of the boring patching job if we can agree on an API... quickly ?
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2012-01-06 18:23:46 UTC
(In reply to comment #7)
> A gst_caps_to_static_string() seems simple enough no ? 

cannot be used easily as t allocated a buffer and one has to free it.
Comment 9 Antoine Tremblay 2012-01-06 18:42:54 UTC
That's why it's to_"static"_string rather then to string... 

I'm not sure how tim planned to do the static part of it... ?

My first guess is to keep the string in the caps like GstStaticCaps does... 
And use it as a cache, each time we call to_static_string it could free the previous caps
and generate a new one... and the caps would be responsible to destroy the string in the end...

But that would break the ABI I guess , but might be an idea for 0.11 ?
Comment 10 Tim-Philipp Müller 2013-03-31 16:16:06 UTC
Current plan is to simply add our own printf implementation for the debugging functions.

http://cgit.freedesktop.org/~tpm/gstreamer/log/?h=gnulib-printf

has some work-in-progress (though the cgit web interface seems to be a bit slow to pick up forced updates in the branch view)
Comment 11 Sebastian Dröge (slomo) 2013-04-04 09:30:38 UTC
What's missing for that gnulib-printf branch to get merged?
Comment 12 Tim-Philipp Müller 2013-04-04 09:42:28 UTC
It's not complete yet :)

The extension isn't hooked up yet (I've mostly done that locally), and we also need to add support for the old extension markers %P and %Q for binary compatibility.
Comment 13 Tim-Philipp Müller 2013-04-05 09:27:25 UTC
*** Bug 696885 has been marked as a duplicate of this bug. ***
Comment 14 Tim-Philipp Müller 2013-04-07 19:45:34 UTC
Updated http://cgit.freedesktop.org/~tpm/gstreamer/log/?h=gnulib-printf some more.

Works now, at least for me. Appears to be 20-25% faster even (for decoding a flac file with uridecodebin to fakesink with log level 9 to /dev/null).

Left to do:

1) someone needs to go through the list of HAVE_XYZ in the code, extract the configure checks from glib's configure.ac and add them to ours to make sure we check for everything we need to check for (alternatively, maybe we can just use GLib's g_snprintf and assume most issues covered).

2) build and test on 32-bit x86

3) build and test on Windows

4) build and test on OS/X
Comment 15 Tim-Philipp Müller 2013-04-13 10:19:33 UTC
Well, let's try this and see what happens:


commit 0e1dd050a56dcbfbd9583fc4f43ed3c3adc1a5f2
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Fri Apr 12 23:58:52 2013 +0100

    printf: don't build if debugging subsystem was disabled

commit 5299a0cd65e3233dd61a5c8862519be4961f1b27
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Wed Apr 10 11:51:37 2013 +0100

    printf: deal with some of the HAVE_FOO used in the printf code
    
    Probably needs some more work for MSVC.

commit 3c1d9c6d41e4f209af5be1baa933234d6ef51560
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Mon Apr 8 19:42:27 2013 +0100

    printf: fix alloca use for windows with mingw32
    
    Don't use just GLIB_HAVE_ALLOCA_H to check if alloca is available,
    that's just for the header. GLib may define alloca for us otherwise
    too irrespective of GLIB_HAVE_ALLOCA_H.
    
    Fixes compiler warning with mingw32:
    gst/printf/vasnprintf.c:73:0: warning: "alloca" redefined

commit ff292d530c126191bf220f70a8d114aa8f54e4b9
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 20:11:21 2013 +0100

    printf: enable and fix compiler warnings
    
    But suppress -Wformat-nonliteral warnings since sprintf
    is used with a runtime-generated format string in our
    vasnprintf implementation.

commit 97b3948a9fd86da7f80b0703060a0a3324f899e4
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 18:21:00 2013 +0100

    printf: fix up dodgy use of #if HAVE_FOO and #if !HAVE_FOO
    
    Should use #ifdef and #ifndef.

commit 79d6b91e27fd187cdaad78728c91a59104a3c85d
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 17:36:29 2013 +0100

    printf: mark internal functions as internal

commit 3778c1878cf59b486228fcf3c500a091e7607e71
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 17:29:02 2013 +0100

    printf: skip pointer extension signifier chars after %p
    
    So they don't get printed after the serialised pointer string.

commit 6d8a6470d59142ff1de3841894bb33e71d133566
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 17:21:10 2013 +0100

    printf: don't leak serialised pointer extension strings

commit 9a9b449c5f1e872b85b2888754fa41b5b4b271a2
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 17:02:55 2013 +0100

    printf: handle old GST_PTR_FORMAT %P and GST_SEGMENT_FORMAT %Q defines too
    
    For binary backwards compatibility.

commit 5803da553ca354362f3673503988745b7eb60607
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sun Apr 7 16:41:40 2013 +0100

    printf: make printf parser recognise our pointer extension format
    
    and call the hook to get a string for the pointer instead.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=613081

commit fe7f7135e0399bd5a970164a5ac962c6800a69bd
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Mar 30 18:28:38 2013 +0000

    printf: add infrastructure for pointer extensions hook
    
    Does not do anything yet. On a sidenote, we can't just use
    %p\001 or so to signal the extension because g-i complains
    about an invalid ascii character then, so have to resort to
    something more elaborate, such as %p\aA etc.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=613081

commit 7b1994428074a05795d7fe4418e29f921c2c0a09
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Mar 30 17:20:13 2013 +0000

    info: use new internal printf for debug message printing
    
    and remove all the printf extension/specifier stuff for
    the system printf. Next we need to add back the custom
    specifiers to our own printf implementation.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=613081

commit 8fc876f09fb4f76902cedcb2edb650fad9f76021
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Mar 30 15:13:32 2013 +0000

    printf: add our own printf implementation for debug logging
    
    We will add support for our own printf modifiers, so we can
    get nice debug log output on all operating systems irrespective
    of the specific libc version used.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=613081