[GTK][jhbuild] Switch to GStreamer 1.0 build

[Philippe Normand]

We should at least switch build-webkit to GStreamer 0.11 so that later on we're more confident to to switch the ./configure option by default for GNOME 3.6 too.

[Philippe Normand]

Created attachment 153222 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 153222 [details]
Patch

Attachment 153222 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13281897

[Philippe Normand]

Created attachment 153252 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 153252 [details]
Patch

Attachment 153252 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13280873

[Philippe Normand]

Comment on attachment 153252 [details]
Patch

The gstreamer build makes the git checkout dirty because of .po files. I'd like to see if this issue can be fixed upstream, tomorrow. Pulling out of review queue for now.

[Philippe Normand]

Gustavo can you please rm -fr WebKitBuild/Dependencies/Source/gstreamer in your EWS?

I'm preparing a new patch and I'd like your EWS to process it instead of mine :)

[Philippe Normand]

I updated the gstreamer .po files in http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=80f94703f985d8f125196dec1baa08d46657923f

Not sure though why our jhbuild stuff updated them in the first place...

[Philippe Normand]

Created attachment 153499 [details]
Patch

[Philippe Normand]

Comment on attachment 153499 [details]
Patch

Clearing flags on attachment: 153499

Committed r123220: <http://trac.webkit.org/changeset/123220>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstElement]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:31:146: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstPad]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:51:146: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstPadTemplate]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:71:146: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstTask]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:105:138: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstBus]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:125:138: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp: In function ‘WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstElementFactory]’:
../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:145:138: error: ‘GST_OBJECT_IS_FLOATING’ was not declared in this scope

[WebKit Review Bot]

Re-opened since this is blocked by 91880

[Philippe Normand]

I'll rework this on Monday.

[Philippe Normand]

Created attachment 153786 [details]
Patch

[Philippe Normand]

Created attachment 153787 [details]
Patch

[Philippe Normand]

Comment on attachment 153787 [details]
Patch

Clearing flags on attachment: 153787

Committed r123339: <http://trac.webkit.org/changeset/123339>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92006

[Philippe Normand]

I think the media test failures on the 64-bit Release bot were related with Pulseaudio. Now that I fixed this issue I'll attempt to land this patch again and keep an eye on the bots.

[Philippe Normand]

Committed r124614: <http://trac.webkit.org/changeset/124614>

[Philippe Normand]

Reverted r124614 for reason:

gstreamer core .po files mess up the build again

Committed r124617: <http://trac.webkit.org/changeset/124617>

[Philippe Normand]

Created attachment 157212 [details]
Patch

I think it would be good to manually clean the previous checkouts of
gst* on the bots before landing this patch.

[Philippe Normand]

GTK EWS failure is due to a previous gst* checkout, I checked it build locally with clean checkouts

[Philippe Normand]

Gustavo, I'd like to land this patch (again) tomorrow. Can you please clean the gst* checkouts on your bots beforehand?

[Gustavo Noronha (kov)]

Done! Sorry for the delay, I was on vacations and am still catching up with mail.

[Xabier Rodríguez Calvar]

When we finish bug 98804, we can switch.

[Xabier Rodríguez Calvar]

Created attachment 175665 [details]
Patch

This is the rebased patch including compiling against tag 1.0.3, removing the flag for unstable API and enabling only 1.0 if not specified otherwise.

[Early Warning System Bot]

Comment on attachment 175665 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 175665 [details]
Patch

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

[Build Bot]

Comment on attachment 175665 [details]
Patch

Attachment 175665 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14961399

[kov's GTK+ EWS bot]

Comment on attachment 175665 [details]
Patch

Attachment 175665 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14961411

[Xabier Rodríguez Calvar]

Created attachment 175677 [details]
Patch

Fixed the perl issues.

[Philippe Normand]

Comment on attachment 175677 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2048
> +    # Enable upcoming GStreamer 1.0 build support through build-webkit.

"upcoming" can be removed now. I wrote this patch before 1.0 was officially released.

[Xabier Rodríguez Calvar]

Created attachment 175682 [details]
Patch

Fixed the perl issues.

[Xabier Rodríguez Calvar]

(In reply to comment #34)
> Fixed the perl issues.

Too much command reuse. Fixed the comment of the perl code.

[kov's GTK+ EWS bot]

Comment on attachment 175682 [details]
Patch

Attachment 175682 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14959452

[Philippe Normand]

I tried this patch locally, clean build, no issue. I wonder what's up with Gustavo's EWS here.

[Philippe Normand]

Gustavo, since 1.0.3 is packaged in Debian Sid maybe we can avoid to build GStreamer in jhbuild? I originally wrote this patch before 1.0 and back then it made sense to build gst in jhbuild but not now anymore, IMHO.

So would you agree to install gstreamer-1.0 in your bots?

[Xabier Rodríguez Calvar]

Created attachment 176006 [details]
Patch

Removed GStreamer from jhbuild. We still keep backwards compatibility with --with-gstreamer=0.10.

[Martin Robinson]

(In reply to comment #38)
> Gustavo, since 1.0.3 is packaged in Debian Sid maybe we can avoid to build GStreamer in jhbuild? I originally wrote this patch before 1.0 and back then it made sense to build gst in jhbuild but not now anymore, IMHO.
> 
> So would you agree to install gstreamer-1.0 in your bots?

Does having the wrong version of Gstreamer 1.0 cause the tests to fail? If so, gstreamer should remain installed by jhbuild. Jhbuild isn't used to fulfill dependencies, but make it simpler to get all tests passing reliably.

[Philippe Normand]

(In reply to comment #40)
> (In reply to comment #38)
> > Gustavo, since 1.0.3 is packaged in Debian Sid maybe we can avoid to build GStreamer in jhbuild? I originally wrote this patch before 1.0 and back then it made sense to build gst in jhbuild but not now anymore, IMHO.
> > 
> > So would you agree to install gstreamer-1.0 in your bots?
> 
> Does having the wrong version of Gstreamer 1.0 cause the tests to fail? 

Few tests will fail with gst < 1.0.3.

> If so, gstreamer should remain installed by jhbuild. Jhbuild isn't used to fulfill dependencies, but make it simpler to get all tests passing reliably.

1.0.3 is in Debian sid already, we can have configure check for that version.
And for now people can keep building for 0.10.x anyway.

[Martin Robinson]

(In reply to comment #41)
> Few tests will fail with gst < 1.0.3.

Since it's possible that in some circumstances having the wrong version of GStreamer can cause test failures, I'm wondering if we should leave it in the modules file. Then we are protected from anything that breaks in future versions. I often had issues in the past with media tests failing because of GStreamer version/codec issues.

[Martin Robinson]

Comment on attachment 175682 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2054
> +    # Enable GStreamer 1.0 build support through build-webkit.
> +    if ($project eq 'WebKit') {
> +        my @gstBuildArgs = grep { /--with-gstreamer=.+/ } @buildArgs;
> +        if (!@gstBuildArgs) {
> +            push @buildArgs, "--with-gstreamer=1.0";
> +        }
> +    }

Not everyone who runs tests uses build-webkit to build. Is it possible to simply detect if 1.0 is present in configure.ac and use it by default if so?

> Tools/gtk/jhbuild.modules:277
> +  <autotools id="libogg" autogen-sh="configure" >
> +     <branch repo="xiph" module="ogg/libogg-1.3.0.tar.gz"
> +             hash="sha256:a8de807631014615549d2356fd36641833b8288221cea214f8a72750efe93780"
> +             md5sum="0a7eb40b86ac050db3a789ab65fe21c2"
> +             version="1.3.0"/>
> +  </autotools>
> +  <autotools id="libvorbis" autogen-sh="configure" autogenargs="--disable-examples" >
> +     <branch repo="xiph" module="vorbis/libvorbis-1.3.3.tar.gz"
> +             hash="sha256:6d747efe7ac4ad249bf711527882cef79fb61d9194c45b5ca5498aa60f290762"
> +             md5sum="6b1a36f0d72332fae5130688e65efe1f"
> +             version="1.3.3"/>
> +  </autotools>
> +  <autotools id="libtheora" autogen-sh="configure" autogenargs="--disable-examples">
> +     <branch repo="xiph" module="theora/libtheora-1.1.1.tar.gz"
> +             hash="sha256:40952956c47811928d1e7922cda3bc1f427eb75680c3c37249c91e949054916b"
> +             md5sum="bb4dc37f0dc97db98333e7160bfbb52b"
> +             version="1.1.1"/>
> +    <dependencies>
> +      <dep package="libogg"/>
> +      <dep package="libvorbis"/>
> +    </dependencies>
> +  </autotools>

It seems that we could rely on system dependencies for these. I'm guessing that these are very stable packages and tests will pass with any version of them.

[Xabier Rodríguez Calvar]

(In reply to comment #43)
> (From update of attachment 175682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175682&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2054
> > +    # Enable GStreamer 1.0 build support through build-webkit.
> > +    if ($project eq 'WebKit') {
> > +        my @gstBuildArgs = grep { /--with-gstreamer=.+/ } @buildArgs;
> > +        if (!@gstBuildArgs) {
> > +            push @buildArgs, "--with-gstreamer=1.0";
> > +        }
> > +    }
> 
> Not everyone who runs tests uses build-webkit to build. Is it possible to simply detect if 1.0 is present in configure.ac and use it by default if so?

Roger that. Actually I can also add the check for GStreamer >= 1.0.3, that I forgot.

> > Tools/gtk/jhbuild.modules:277
> > +  <autotools id="libogg" autogen-sh="configure" >
> > +     <branch repo="xiph" module="ogg/libogg-1.3.0.tar.gz"
> > +             hash="sha256:a8de807631014615549d2356fd36641833b8288221cea214f8a72750efe93780"
> > +             md5sum="0a7eb40b86ac050db3a789ab65fe21c2"
> > +             version="1.3.0"/>
> > +  </autotools>
> > +  <autotools id="libvorbis" autogen-sh="configure" autogenargs="--disable-examples" >
> > +     <branch repo="xiph" module="vorbis/libvorbis-1.3.3.tar.gz"
> > +             hash="sha256:6d747efe7ac4ad249bf711527882cef79fb61d9194c45b5ca5498aa60f290762"
> > +             md5sum="6b1a36f0d72332fae5130688e65efe1f"
> > +             version="1.3.3"/>
> > +  </autotools>
> > +  <autotools id="libtheora" autogen-sh="configure" autogenargs="--disable-examples">
> > +     <branch repo="xiph" module="theora/libtheora-1.1.1.tar.gz"
> > +             hash="sha256:40952956c47811928d1e7922cda3bc1f427eb75680c3c37249c91e949054916b"
> > +             md5sum="bb4dc37f0dc97db98333e7160bfbb52b"
> > +             version="1.1.1"/>
> > +    <dependencies>
> > +      <dep package="libogg"/>
> > +      <dep package="libvorbis"/>
> > +    </dependencies>
> > +  </autotools>
> 
> It seems that we could rely on system dependencies for these. I'm guessing that these are very stable packages and tests will pass with any version of them.

Phil can correct me if I am wrong, but I think we can. Actually I have 1.3.2 in my system when compiling my own GStreamer master and this caused no trouble.

[Philippe Normand]

Yes we can exclude ogg/theora/vorbis from the moduleset. Those container/codecs are not used for the tests anyway (AFAIK).

[Xabier Rodríguez Calvar]

Created attachment 176228 [details]
Patch

Readded GStreamer to jhbuild but removed the dependencies of those. Made GStreamer 1.0 default in the configure.ac

[Philippe Normand]

Comment on attachment 176228 [details]
Patch

We want to fallback on 0.10 if 1.0 is not found.

[Xabier Rodríguez Calvar]

Created attachment 176294 [details]
Patch

configure.ac checking for GStreamer 1.0 in case no version is specified. If 1.0 is not found, it falls back to 0.10. If a version is specified, it tries that and fails if it can't find it

[Martin Robinson]

Comment on attachment 176294 [details]
Patch

Okay. This seems reasonable.

[Philippe Normand]

Comment on attachment 176294 [details]
Patch

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

> configure.ac:344
> +if test -z "$with_gstreamer"; then

Isn't this test always true given that AC_ARG_WITH() is called later on?
Also I think we want to perform this only if video or web-audio is enabled.

[Xabier Rodríguez Calvar]

(In reply to comment #50)
> (From update of attachment 176294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176294&action=review
> 
> > configure.ac:344
> > +if test -z "$with_gstreamer"; then
> 
> Isn't this test always true given that AC_ARG_WITH() is called later on?
> Also I think we want to perform this only if video or web-audio is enabled.

Martin asked my the very same question :)

At least when I tested it in my environment, it worked because the variable is set before that macro code is invoked. What the macro probably does is checking the "with_variable" variable value.

[Philippe Normand]

My second point is still valid though :) Care to update the patch?

[Xabier Rodríguez Calvar]

Created attachment 176737 [details]
Patch

gst-ffmpeg becomes gst-libav and not checking for GStreamer in case we don't want web audio or video.

[Xabier Rodríguez Calvar]

Created attachment 177518 [details]
Patch

Added a default --with-gstreamer=auto option to make the configure.ac clearer. I moved those checks to another place to be sure that enable video and web audio arguments are already processed.

[Xabier Rodríguez Calvar]

(In reply to comment #54)
> Created an attachment (id=177518) [details]
> Patch
> 
> Added a default --with-gstreamer=auto option to make the configure.ac clearer. I moved those checks to another place to be sure that enable video and web audio arguments are already processed.

If it is ok, we can land it tomorrow to properly sync with the bots.

[Philippe Normand]

Comment on attachment 177518 [details]
Patch

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

> configure.ac:1062
> +if test "$have_gstreamer" != "yes" || test "$GST_API_VERSION" = "1.0"; then

In which case have_gstreamer is set to "yes" ? Isn't it enough to test GST_API_VERSION?

[Xabier Rodríguez Calvar]

(In reply to comment #56)
> (From update of attachment 177518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177518&action=review
> 
> > configure.ac:1062
> > +if test "$have_gstreamer" != "yes" || test "$GST_API_VERSION" = "1.0"; then
> 
> In which case have_gstreamer is set to "yes" ? Isn't it enough to test GST_API_VERSION?

No, because GST_API_VERSION is set before we actually check that GStreamer is present. It is set when the version is selected, but no when it is checked, which happens later, and that's why I also check for $have_gstreamer.

I realized it could when testing --with-gstreamer=1.0  and --disable-video --disable-web-audio or GStreamer 1.0 not present. In that case, the version is already set to 1.0, but there is no GStreamer in the end because either it is not installed or it is not going to be used because no video or web audio are requested.

[Philippe Normand]

Comment on attachment 177518 [details]
Patch

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

Looks good, just two small nits to fix before landing.

> configure.ac:819
> +GSTREAMER_1_0_REQUIRED_VERSION=0.11.90
> +GSTREAMER_1_0_PLUGINS_BASE_REQUIRED_VERSION=0.11.90

Should be 1.0.3

> configure.ac:853
> +   AC_DEFINE([GST_USE_UNSTABLE_API], [1], [Using unstable GStreamer API])

That should be removed.

[Xabier Rodríguez Calvar]

Created attachment 178572 [details]
Patch

Fixing version and unstable API

[kov's GTK+ EWS bot]

Comment on attachment 178572 [details]
Patch

Attachment 178572 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15230712

[Philippe Normand]

Comment on attachment 178572 [details]
Patch

Clearing flags on attachment: 178572

Committed r137271: <http://trac.webkit.org/changeset/137271>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104667

[Philippe Normand]

The timeouts were due to missing libs (theora, vorbis, faad) on the bots.