Bug 89350 – [GStreamer] Use GST_DEBUG instead of LOG_VERBOSE

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

WebCore's LOG* macros work only for debug builds. Moreover I think it make sense to have the player's logging messages at the same level as gst logging messages. We can make a new gst debug category called webkit so GST_DEBUG=webkit:5 would output the current LOG_VERBOSE() messages.

[Martin Robinson]

Is that the behavior of the LOG macros on all platforms, or just a result of the GTK+ implementation of the LOG macros?

[Philippe Normand]

(In reply to comment #1)
> Is that the behavior of the LOG macros on all platforms, or just a result of the GTK+ implementation of the LOG macros?

On all platforms, see Source/WTF/wtf/Assertions.h

#if COMPILER(MSVC7_OR_LOWER)
#define LOG_VERBOSE(channel) ((void)0)
#elif LOG_DISABLED
#define LOG_VERBOSE(channel, ...) ((void)0)
#else
#define LOG_VERBOSE(channel, ...) WTFLogVerbose(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, &JOIN_LOG_CHANNEL_WITH_PREFIX(LOG_CHANNEL_PREFIX, channel), __VA_ARGS__)
#endif

[Martin Robinson]

Perhaps the best approach is to not use the LOG_VERBOSE channel rather than creating another logging mechanism?

[Philippe Normand]

(In reply to comment #3)
> Perhaps the best approach is to not use the LOG_VERBOSE channel rather than creating another logging mechanism?

What do you mean? LOG() is also disabled for Release builds.

[Philippe Normand]

Created attachment 156081 [details]
Patch

[WebKit Review Bot]

Attachment 156081 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:25:  Missing space before ( in while(  [whitespace/parens] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 156081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156081&action=review

Looks good to me, just a couple concerns that seem easy to fix before landing.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:26
> +#define logMessageToGStreamer(...) do { \
> +    GST_DEBUG(__VA_ARGS__); \
> +    LOG_VERBOSE(Media, __VA_ARGS__); } while(0)
>  }

Don't most macros in WebKit FOLLOW_THIS_NAMING_CONVENTION? I don't think that preprocessor macros exist in namespaces, so this should probably be at the top of the file.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:189
> +        GST_DEBUG_CATEGORY_INIT(webkit_media_player_debug, "webkitmediaplayer", 0, "webkit media player");

Perhaps it would be good to have the string be "WebKit media player" ?

[Philippe Normand]

Committed r124464: <http://trac.webkit.org/changeset/124464>