Summary: | [GStreamer] RTSP playback broken | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | mrobinson, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Philippe Normand
2011-03-23 09:30:42 PDT
Created attachment 88462 [details]
proposed patch
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.
Created attachment 88592 [details]
proposed patch
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. (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 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. Created attachment 88985 [details]
proposed patch
Created attachment 88987 [details]
proposed patch
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. :) Committed r83566: <http://trac.webkit.org/changeset/83566> |