Bug 168015 – [GStreamer][MSE] Some GStreamer log messages are generated with the 'default' category

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

Messages generated by the GStreamer MSE classes should use the "webkitmse" (in general) and "webkitmediasrc" (for the WebKitMediaSourceGStreamer element) debug categories. Currently some messages use the right category and some others use the "default" one:

0:00:09.913288495  6379       0xf80920 TRACE              webkitmse MediaPlayerPrivateGStreamerMSE.cpp:105:MediaPlayerPrivateGStreamerMSE: creating the player (0x7f17e9133900)
0:00:09.956051567  6379       0xf80920 DEBUG                default PlaybackPipeline.cpp:174:setWebKitMediaSrc: webKitMediaSrc=0xf6a5a0
0:00:09.956252086  6379       0xf80920 DEBUG              webkitmse MediaPlayerPrivateGStreamerMSE.cpp:600:updateStates: Async: State: READY, pending: PAUSED
...
0:00:09.967249757  6379       0xf80920 TRACE                default MediaSourceClientGStreamerMSE.cpp:65:addSourceBuffer: Adding SourceBuffer to AppendPipeline: this=0x7f17e9c68ca8 sourceBuffer=0x7f17e90c3100 appendPipeline=0x7f17e91180c8
...
0:00:09.982858490  6379       0xf80920 DEBUG         webkitmediasrc WebKitMediaSourceGStreamer.cpp:508:webKitMediaSrcLinkParser:<source> pad not linked yet
0:00:09.982865817  6379       0xf80920 DEBUG         webkitmediasrc WebKitMediaSourceGStreamer.cpp:477:webKitMediaSrcLinkStreamToSrcPad:<source> linking stream to src pad (id:
...
0:00:10.028238975  6379      0x15195e0 TRACE                default AppendPipeline.cpp:843:reportAppsrcNeedDataReceived: received need-data signal at appsrc, reposting to bus
...
0:00:12.109490639  6379      0x1519680 TRACE                default WebKitMediaSourceGStreamer.cpp:140:enabledAppsrcNeedData: ready-for-more-samples message posted to the bus

This makes debugging more difficult and should be fixed.

[Vanessa Chipirrás Navalón]

Created attachment 300946 [details]
Patch

[Enrique Ocaña]

Comment on attachment 300946 [details]
Patch

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

> Source/WebCore/ChangeLog:363
> [phone]  Vanessa Chipirrás Navalón  <[e-mail]>

Please, review your ChangeLog file. This block shouldn't be here.

[Vanessa Chipirrás Navalón]

Created attachment 300953 [details]
Patch

[Vanessa Chipirrás Navalón]

Created attachment 300961 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 300961 [details]
Patch

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

Though some stuff needs some polishing, good patch!

> Source/WebCore/ChangeLog:14
> +        (WebCore::AppendPipeline::AppendPipeline): Initialize the Gstreamer logging category

It should be written as GStreamer

> Source/WebCore/ChangeLog:19
> +        (WebCore::PlaybackPipeline::PlaybackPipeline): Initialize the Gstreamer logging category

Ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:45
> +GST_DEBUG_CATEGORY(webkit_appendpipeline_debug);
> +#define GST_CAT_DEFAULT webkit_appendpipeline_debug

Please use webkit_mse_debug, that is defined in MediaPlayerPrivateGStreamerMSE.cpp. For this you will need to make the declaration in MediaPlayerPrivateGStreamerMSE.cpp as _EXTERN as it is done for GStreamer and GStreamerBase.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:122
> +    GST_DEBUG_CATEGORY_INIT(webkit_appendpipeline_debug, "webkitmse", 0, "WebKit MSE media player");

As you will be using an already initialized category you won't need this and it can be removed.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:30
> +#include <gst/gstinfo.h>
> +
> +GST_DEBUG_CATEGORY(webkit_mse_client_debug);
> +#define GST_CAT_DEFAULT webkit_mse_client_debug

Same here as for AppendPipeline. Check if the include is needed if you remove the initialization below.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:51
> +    GST_DEBUG_CATEGORY_INIT(webkit_mse_client_debug, "webkitmse", 0, "WebKit MSE media player");

Remove this.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:44
> +GST_DEBUG_CATEGORY(webkit_playbackpipeline_debug);
> +#define GST_CAT_DEFAULT webkit_playbackpipeline_debug

Same here.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:98
> +PlaybackPipeline::PlaybackPipeline()
> +{
> +    GST_DEBUG_CATEGORY_INIT(webkit_playbackpipeline_debug, "webkitmse", 0, "WebKit MSE media player");
> +}
> +

You don't need this.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.h:73
> -    PlaybackPipeline() = default;
> +    PlaybackPipeline();

You don't need this.

[Enrique Ocaña]

Comment on attachment 300961 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:45
>> +#define GST_CAT_DEFAULT webkit_appendpipeline_debug
> 
> Please use webkit_mse_debug, that is defined in MediaPlayerPrivateGStreamerMSE.cpp. For this you will need to make the declaration in MediaPlayerPrivateGStreamerMSE.cpp as _EXTERN as it is done for GStreamer and GStreamerBase.

In any case, shouldn't the define GST_CAT_DEFAULT be still needed? I've explored the GST_TRACE() macro expansion and it expands to this code (in my particular case):

do{		\
  if ((GST_LEVEL_TRACE <= GST_LEVEL_COUNT && GST_LEVEL_TRACE <= _gst_debug_min)) {						\
    gst_debug_log ((GST_CAT_DEFAULT), (GST_LEVEL_TRACE), "/home/enrique/WebKit/Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp", ((const char*) (__FUNCTION__)), 65,	\
        (GObject *) (0), "Adding SourceBuffer to AppendPipeline: this=%p sourceBuffer=%p appendPipeline=%p", this, sourceBufferPrivate.get(), appendPipeline.get());				\
  }									\
}while (0)

This seems to imply that some value is needed for GST_CAT_DEFAULT. Since that define is inside MediaPlayerPrivateGStreamerMSE.cpp and not in the header (because it shouldn't be in the header in order to avoid clashing with local definitions when the header in included from other cpp files), it would remain undefined in the case of AppendPipeline, PlaybackPipeline and MediaSourceClientGStreamer. Therefore, I think we need to define it here by ourselves with the same value used in MediaPlayerPrivateGStreamerMSE.cpp:

#define GST_CAT_DEFAULT webkit_mse_debug

[Xabier Rodríguez Calvar]

(In reply to comment #6)
> In any case, shouldn't the define GST_CAT_DEFAULT be still needed? I've
> 
> This seems to imply that some value is needed for GST_CAT_DEFAULT. Since
> that define is inside MediaPlayerPrivateGStreamerMSE.cpp and not in the
> header (because it shouldn't be in the header in order to avoid clashing
> with local definitions when the header in included from other cpp files), it
> would remain undefined in the case of AppendPipeline, PlaybackPipeline and
> MediaSourceClientGStreamer. Therefore, I think we need to define it here by
> ourselves with the same value used in MediaPlayerPrivateGStreamerMSE.cpp:
> 
> #define GST_CAT_DEFAULT webkit_mse_debug

Yes, I didn't mean that we didn't need that. We definitely do, but for that to work with the already initialized category con *GStreamerMSE.cpp, you need to declare the one at *GStreamerMSE.cpp as _EXTERN. It's more clear and easy to abstract if you grep for GST_DEBUG_CATEGORY on WebKit code.

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> Yes, I didn't mean that we didn't need that. We definitely do, but for that
> to work with the already initialized category con *GStreamerMSE.cpp, you
> need to declare the one at *GStreamerMSE.cpp as _EXTERN. It's more clear and
> easy to abstract if you grep for GST_DEBUG_CATEGORY on WebKit code.

Actually I think I am wrong on this one. I think the _EXTERN would go in the AppendPipeline and frieds and the private player would remain as it is now.

[Vanessa Chipirrás Navalón]

Created attachment 301165 [details]
Patch

[Vanessa Chipirrás Navalón]

Created attachment 301178 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 301178 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:27
> +#include <gst/gst.h>

This include does not seem to be needed, please confirm it and remove it.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:59
> +#define WEBKIT_MEDIA_SRC_CATEGORY_INIT GST_DEBUG_CATEGORY_INIT(webkit_media_src_debug, "webkitmediasrc", 0, "websrc element");

Taking advantage of having to remove the include above and having to upload the patch again, let's change:

"websrc element" -> "web media source element"

[Vanessa Chipirrás Navalón]

(In reply to comment #11)
> Comment on attachment 301178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301178&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:27
> > +#include <gst/gst.h>
> 
> This include does not seem to be needed, please confirm it and remove it.
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:59
> > +#define WEBKIT_MEDIA_SRC_CATEGORY_INIT GST_DEBUG_CATEGORY_INIT(webkit_media_src_debug, "webkitmediasrc", 0, "websrc element");
> 
> Taking advantage of having to remove the include above and having to upload
> the patch again, let's change:
> 
> "websrc element" -> "web media source element"

This is necessary because I got a error from EFL. Also, in the documentation about GST_DEBUG_CATEGORY indicates this header in the Include section.

[Xabier Rodríguez Calvar]

Comment on attachment 301178 [details]
Patch

(In reply to comment #12)
> This is necessary because I got a error from EFL. Also, in the documentation
> about GST_DEBUG_CATEGORY indicates this header in the Include section.

Ok. I am removing the cq- you may ask for landing this patch when you're ready.

[WebKit Commit Bot]

Comment on attachment 301178 [details]
Patch

Clearing flags on attachment: 301178

Committed r212285: <http://trac.webkit.org/changeset/212285>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.