by using gst_is_initialized() in initializeGStreamer() This may be useful for WK1, so the the application can initialize GStreamer before.
Created attachment 175824 [details] Patch
Comment on attachment 175824 [details] Patch Attachment 175824 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14970528
Comment on attachment 175824 [details] Patch Attachment 175824 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14962777
What version of GStreamer does Qt port use? The gst_is_initialized() appeared in 0.10.31 http://gstreamer.freedesktop.org/data/doc/gstreamer/0.10.36/gstreamer/html/gstreamer-Gst.html#gst-is-initialized
(In reply to comment #4) > What version of GStreamer does Qt port use? > > The gst_is_initialized() appeared in 0.10.31 > > http://gstreamer.freedesktop.org/data/doc/gstreamer/0.10.36/gstreamer/html/gstreamer-Gst.html#gst-is-initialized If the EWS is the same machine as http://build.webkit.org/buildslaves/szeged-linux-1 then it's Debian Squeeze (6.0) so 0.10.30
Comment on attachment 175824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175824&action=review Looks good overall, have a look at the other ChangeLog entries to see how they're structured :) > Source/WebCore/ChangeLog:5 > + Verify if GStreamer was previously initialized > + > + The patch also contains the proof of concept for GtkLaunch Please move this down the Reviewed by line. Also it's GtkLauncher. And maybe you can explain why the same is not done for the WK2 MiniBrowser. > Source/WebCore/ChangeLog:12 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). This one should be replaced by the 2 lines above. > Source/WebCore/ChangeLog:14 > + No new tests (OOPS!). Usually for a small patch like this without any impact on the tests we state something like "No new tests, existing media tests cover this change." > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:32 > + if (gst_is_initialized ()) We don't allow space between the function name and the parenthesis, usually. Maybe the style bot can check that. Also as this is an API introduced in 0.10.31 and we require 0.10.30 in configure.ac it'd be nice to wrap this code in a #if GST_CHECK_VERSION(0, 10, 31) block. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:33 > + return true; A blank line wouldn't hurt after this one :) > Tools/ChangeLog:5 > + Verify if GStreamer was previously initialized > + > + The patch also contains the proof of concept for GtkLaunch Well the same apply to this ChangeLog and please don't copy-paste from the other one :) It'd be fine if this entry mentions only the change related to the GtkLauncher.
Created attachment 176038 [details] Patch
Comment on attachment 176038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176038&action=review Ok almost perfect! > Source/WebCore/ChangeLog:17 > + No new tests (OOPS!). I think the commit queue will not like the OOPS. I think I mentioned previously that you can state existing media tests cover this change. > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:33 > + if (gst_is_initialized ()) Can you please remove the space after the function name? :) > Tools/ChangeLog:14 > + This approach is not valid for MiniBrowser because it uses WebKit2, > + where the GStreamer backend lives in the web process, which is > + different from the UI process. Nice explanation!
Created attachment 176260 [details] Patch
(In reply to comment #8) > (From update of attachment 176038 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176038&action=review > > Ok almost perfect! Hi Phil! Thanks again for the review. I hope now everything correct.
Comment on attachment 176260 [details] Patch Clearing flags on attachment: 176260 Committed r135863: <http://trac.webkit.org/changeset/135863>
All reviewed patches have been landed. Closing bug.