Bug 103151 – [GStreamer] verify if GStreamer was previously initialized

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Víctor M. Jáquez L.]

by using gst_is_initialized() in initializeGStreamer()

This may be useful for WK1, so the the application can initialize GStreamer before.

[Víctor M. Jáquez L.]

Created attachment 175824 [details]
Patch

[Early Warning System Bot]

Comment on attachment 175824 [details]
Patch

Attachment 175824 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14970528

[Early Warning System Bot]

Comment on attachment 175824 [details]
Patch

Attachment 175824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14962777

[Víctor M. Jáquez L.]

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

[Alexis Menard (darktears)]

(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

[Philippe Normand]

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.

[Víctor M. Jáquez L.]

Created attachment 176038 [details]
Patch

[Philippe Normand]

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!

[Víctor M. Jáquez L.]

Created attachment 176260 [details]
Patch

[Víctor M. Jáquez L.]

(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.

[WebKit Review Bot]

Comment on attachment 176260 [details]
Patch

Clearing flags on attachment: 176260

Committed r135863: <http://trac.webkit.org/changeset/135863>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.