Bug 56930 – [GStreamer] RTSP playback broken

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

Source/WebCore/manual-tests/video-rtsp.html loads but no video is displayed and I hear sound-cracks, the 
rtsp:// link plays fine with gst-launch.

Needs some further investigation

[Philippe Normand]

Created attachment 88462 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:54:  The parameter name "player" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:55:  The parameter name "player" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


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

[Philippe Normand]

Created attachment 88592 [details]
proposed patch

[Martin Robinson]

Comment on attachment 88592 [details]
proposed patch

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

Looks good, though please make callback static, unless there is a reason they should be exposed in the header.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:215
> +void mediaPlayerPrivateVideoChangedCallback(GObject* element, MediaPlayerPrivateGStreamer* player)

You should omit "element" here. Why aren't all of these callbacks defined statically inside this file? Are they shared anywhere?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:220
> +void mediaPlayerPrivateAudioChangedCallback(GObject* element, MediaPlayerPrivateGStreamer* player)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:55
> +void mediaPlayerPrivateVideoChangedCallback(GObject* element, MediaPlayerPrivateGStreamer*);
> +void mediaPlayerPrivateAudioChangedCallback(GObject* element, MediaPlayerPrivateGStreamer*);
> +gboolean mediaPlayerPrivateAudioChangeTimeoutCallback(MediaPlayerPrivateGStreamer*);
> +gboolean mediaPlayerPrivateVideoChangeTimeoutCallback(MediaPlayerPrivateGStreamer*);

As above, couldn't these just be defined statically in the C++ file? Why make them public? Also, you should omit the "element" variable.

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 88592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88592&action=review
> 
> Looks good, though please make callback static, unless there is a reason they should be exposed in the header.

Making the callbacks static leads to:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff23102b7 in g_object_get (_object=<value optimized out>, first_property_name=
    0x7ffff606b9a8 "n-video") at gobject.c:1867
1867	  g_return_if_fail (G_IS_OBJECT (object));
(gdb) bt
#0  0x00007ffff23102b7 in g_object_get (_object=<value optimized out>, first_property_name=
    0x7ffff606b9a8 "n-video") at gobject.c:1867
#1  0x00007ffff505e527 in WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfVideo (this=0xcc0000)
    at ../../../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:635
#2  0x00007ffff505cd74 in WebCore::mediaPlayerPrivateVideoChangeTimeoutCallback (player=0xcc0000)
    at ../../../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:235
#3  0x00007ffff1a17edb in g_timeout_dispatch (source=0x7fffe00e01a0, callback=<value optimized out>, 
    user_data=<value optimized out>) at gmain.c:3882
#4  0x00007ffff1a165a3 in g_main_dispatch (context=0x6417e0) at gmain.c:2440
#5  g_main_context_dispatch (context=0x6417e0) at gmain.c:3013
#6  0x00007ffff1a16d80 in g_main_context_iterate (context=0x6417e0, block=1, dispatch=1, 
    self=<value optimized out>) at gmain.c:3091
#7  0x00007ffff1a173f2 in g_main_loop_run (loop=0xce5780) at gmain.c:3299
#8  0x00007ffff4199557 in IA__gtk_main () at gtkmain.c:1255
#9  0x0000000000402b09 in main (argc=2, argv=0x7fffffffd6c8) at ../../../../Tools/GtkLauncher/main.c:274
(gdb) f 1
#1  0x00007ffff505e527 in WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfVideo (this=0xcc0000)
    at ../../../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:635
635	        g_object_get(m_playBin, "n-video", &videoTracks, NULL);
(gdb) p m_playBin
$1 = (GstElement *) 0xe1

[Philippe Normand]

Sorry Martin, my initial patch was wrong in the first place...

The notify::caps signal should not reuse mediaPlayerPrivateVideoChangedCallback(). We need a new handler with a different signature.

[Philippe Normand]

Created attachment 88985 [details]
proposed patch

[Philippe Normand]

Created attachment 88987 [details]
proposed patch

[Martin Robinson]

Comment on attachment 88987 [details]
proposed patch

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

Thanks for fixing this up!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:49
>  gboolean mediaPlayerPrivateMessageCallback(GstBus* bus, GstMessage* message, gpointer data);
> -void mediaPlayerPrivateVolumeChangedCallback(GObject* element, GParamSpec* pspec, gpointer data);
> -void mediaPlayerPrivateMuteChangedCallback(GObject* element, GParamSpec* pspec, gpointer data);
>  void mediaPlayerPrivateSourceChangedCallback(GObject* element, GParamSpec* pspec, gpointer data);

It'd be great to remove to remov the remaining two callback in a followup patch. :)

[Philippe Normand]

Committed r83566: <http://trac.webkit.org/changeset/83566>