Bug 87524 – add Farstream flags/deps to WebKit, for WebRTC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Danilo de Paula]

Needed to add Farstream as a backend for the GStreamer WebRTC implementation.
It requires libnice and libcrypto as dependencies.

[Danilo de Paula]

Created attachment 144115 [details]
FLAGS for Farstream

[WebKit Review Bot]

Attachment 144115 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1
Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 144115 [details]
FLAGS for Farstream

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

> Source/WebKit/gtk/GNUmakefile.am:66
> +	$(HILDON_CFLAGS) \
> +	$(LIBCRYPTO_CFLAGS) \
> +	$(LIBFFTW_CFLAGS) \

hildong and libfftw don' t seem related, right? Merge conflicts maybe?

> Source/WebKit/gtk/GNUmakefile.am:101
> +	$(LIBFFTW_LIBS) \

Doesn' t seem related to Farstream

[Danilo de Paula]

Created attachment 144132 [details]
patch proposal

removing unused deps

[Danilo de Paula]

(In reply to comment #3)
> (From update of attachment 144115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144115&action=review
> 
> > Source/WebKit/gtk/GNUmakefile.am:66
> > +	$(HILDON_CFLAGS) \
> > +	$(LIBCRYPTO_CFLAGS) \
> > +	$(LIBFFTW_CFLAGS) \
> 
> hildong and libfftw don' t seem related, right? Merge conflicts maybe?
> 
> > Source/WebKit/gtk/GNUmakefile.am:101
> > +	$(LIBFFTW_LIBS) \
> 
> Doesn' t seem related to Farstream

I believe you're right... Fixed, thanks for the heads up.

[Philippe Normand]

Comment on attachment 144132 [details]
patch proposal

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

> configure.ac:380
> +LIBFFTW_REQUIRED_VERSION=3.2.2

That can be removed too, didn' t notice it in the first patch.

> configure.ac:1263
> +               [AS_HELP_STRING([--with-media-stream-backend=@<:@farstream/gstreamer@:>@], [the media stream backend to use (default: farstream)])],

So, hum... would it make sense to work on a single implementation? If the farstream implementation is more mature than the pure-gst one maybe the later can just be dropped?

[Martin Robinson]

(In reply to comment #6)

> So, hum... would it make sense to work on a single implementation? If the farstream implementation is more mature than the pure-gst one maybe the later can just be dropped?

I think I agree with Philippe here. More backends equal more maintenance in the end. It would be nice to hear at least a little bit about why the farstream backend might be preferred to the gstreamer one and whether or not it makes sense to drop one of them.

[Danilo de Paula]

Created attachment 153502 [details]
Patch

[Philippe Normand]

Comment on attachment 153502 [details]
Patch

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

> Tools/gtk/jhbuild.modules:21
> +      <dep package="farstream"/>

We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests?

[Danilo de Paula]

Created attachment 153532 [details]
Patch

[Danilo de Paula]

(In reply to comment #9)
> (From update of attachment 153502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153502&action=review
> 
> > Tools/gtk/jhbuild.modules:21
> > +      <dep package="farstream"/>
> 
> We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests?

Agree,

Actually I removed LayoutTests/platform/gtk/TestExpectations and most of them are Ok (I'm wondering if they're passing on google too)

[Danilo de Paula]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 153502 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=153502&action=review
> > 
> > > Tools/gtk/jhbuild.modules:21
> > > +      <dep package="farstream"/>
> > 
> > We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests?
> 
> Agree,
> 
> Actually I removed LayoutTests/platform/gtk/TestExpectations and most of them are Ok (I'm wondering if they're passing on google too)

* remove SKIP of mediastream tests.

Actually, since the implementation is quite big, I'm wondering if splitting those commits on very-small-uncompilable-commits makes sense.

[Martin Robinson]

(In reply to comment #12)
 
> Actually, since the implementation is quite big, I'm wondering if splitting those commits on very-small-uncompilable-commits makes sense.

In my opinion, an incomplete implementation is okay, but not commits that do not compile. One option here is to first stub out all classes that you need to implement and gradually add logical bits of the implementation.

[Danilo de Paula]

Comment on attachment 153532 [details]
Patch

should we add farstream-dev to the build-bot?

[Philippe Normand]

(In reply to comment #14)
> (From update of attachment 153532 [details])
> should we add farstream-dev to the build-bot?

Yes, seems like it's missing in Gustavo's EWS and maybe buildslave.
I installed libfarstream-0.1-dev on my EWS and one the Igalia buildslaves.

[Philippe Normand]

Comment on attachment 153532 [details]
Patch

Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)

[Danilo de Paula]

(In reply to comment #16)
> (From update of attachment 153532 [details])
> Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)

Agreed.

[Danilo de Paula]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 153532 [details] [details])
> > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)
> 
> Agreed.

Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream.

Although the build bot should keep media-stream enabled.

[Philippe Normand]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 153532 [details] [details] [details])
> > > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)
> > 
> > Agreed.
> 
> Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream.
> 

This is already the case in configure.ac no?

> Although the build bot should keep media-stream enabled.

Yes it's enabled by default in build-webkit already, AFAIK.

[Danilo de Paula]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (From update of attachment 153532 [details] [details] [details] [details])
> > > > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)
> > > 
> > > Agreed.
> > 
> > Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream.
> > 
> 
> This is already the case in configure.ac no?

Well, should be, but there's something wrong when I do
$ Tools/Scripts/build-webkit --gtk --debug

I'm ending with --enable-media-stream in the autogen argument list.
http://pastebin.com/wBq8TTNd

[Danilo de Paula]

> > This is already the case in configure.ac no?
> 
> Well, should be, but there's something wrong when I do
> $ Tools/Scripts/build-webkit --gtk --debug
> 
> I'm ending with --enable-media-stream in the autogen argument list.
> http://pastebin.com/wBq8TTNd

from Tools/Scripts/webkitperl/FeatureList.pm:293, I see that build-webkit enable media-stream for GTK by default. I'm not sure if we should remove that. What do you think Philippe?

[Philippe Normand]

(In reply to comment #21)
> > > This is already the case in configure.ac no?
> > 
> > Well, should be, but there's something wrong when I do
> > $ Tools/Scripts/build-webkit --gtk --debug
> > 
> > I'm ending with --enable-media-stream in the autogen argument list.
> > http://pastebin.com/wBq8TTNd
> 
> from Tools/Scripts/webkitperl/FeatureList.pm:293, I see that build-webkit enable media-stream for GTK by default. I'm not sure if we should remove that. What do you think Philippe?

I was meaning that:

AC_ARG_ENABLE(media_stream,
              AC_HELP_STRING([--enable-media-stream],
                             [enable media stream support (incomplete) [default=no]]),
              [],[enable_media_stream=$enable_unstable_features])

and you saw already that build-webkit toggles this flag on. So everything's fine, I believe.

[Danilo de Paula]

Created attachment 155304 [details]
patch

addind this patch for review again, since the build bot already has libfarstream-dev installed.

[Danilo de Paula]

Created attachment 155305 [details]
Patch

* adding the correct file.

[WebKit Review Bot]

Comment on attachment 153532 [details]
Patch

Cleared Philippe Normand's review+ from obsolete attachment 153532 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Review Bot]

Comment on attachment 155305 [details]
Patch

Rejecting attachment 155305 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 155305 [details]
Patch

Rejecting attachment 155305 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Danilo de Paula]

Comment on attachment 155305 [details]
Patch

Using the correct flags.
Waiting for review/cq

[WebKit Review Bot]

Comment on attachment 155305 [details]
Patch

Clearing flags on attachment: 155305

Committed r124182: <http://trac.webkit.org/changeset/124182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.