Bug 91727

Created attachment 153222 [details] Patch

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

Created attachment 153252 [details] Patch

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

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.

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 :)

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

Created attachment 153499 [details] Patch

Comment on attachment 153499 [details] Patch Clearing flags on attachment: 153499 Committed r123220: <http://trac.webkit.org/changeset/123220>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by 91880

I'll rework this on Monday.

Created attachment 153786 [details] Patch

Created attachment 153787 [details] Patch

Comment on attachment 153787 [details] Patch Clearing flags on attachment: 153787 Committed r123339: <http://trac.webkit.org/changeset/123339>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 92006

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.

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

Reverted r124614 for reason: gstreamer core .po files mess up the build again Committed r124617: <http://trac.webkit.org/changeset/124617>

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.

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

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

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

When we finish bug 98804, we can switch.

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.

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

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

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

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

Created attachment 175677 [details] Patch Fixed the perl issues.

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.

Created attachment 175682 [details] Patch Fixed the perl issues.

(In reply to comment #34) > Fixed the perl issues. Too much command reuse. Fixed the comment of the perl code.

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

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

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?

Created attachment 176006 [details] Patch Removed GStreamer from jhbuild. We still keep backwards compatibility with --with-gstreamer=0.10.

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

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

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

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.

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

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

Created attachment 176228 [details] Patch Readded GStreamer to jhbuild but removed the dependencies of those. Made GStreamer 1.0 default in the configure.ac

Comment on attachment 176228 [details] Patch We want to fallback on 0.10 if 1.0 is not found.

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

Comment on attachment 176294 [details] Patch Okay. This seems reasonable.

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.

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

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

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.

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.

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

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?

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

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.

Created attachment 178572 [details] Patch Fixing version and unstable API

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

Comment on attachment 178572 [details] Patch Clearing flags on attachment: 178572 Committed r137271: <http://trac.webkit.org/changeset/137271>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 104667

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