Add opus in third_party.

Add opus in third_party.

Opus will be used by chromoting for audio encoding.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162922

[Niklas Enbom, 2012-10-17 21:12:26]

Why do we land all the source and not only the gyp file? In WebRTC we just add:
"third_party/opus/source":"http://git.xiph.org/opus.git@v1.0.1",
to DEPS.

This will require a local mirror to be set up for Chromium.

On 2012/09/05 21:45:00, sergeyu wrote:

[Sergey Ulanov, 2012-10-17 21:36:53]

On 2012/10/17 21:12:26, Niklas Enbom wrote:
> Why do we land all the source and not only the gyp file? In WebRTC we just
add:
> "third_party/opus/source":"http://git.xiph.org/opus.git@v1.0.1",
> to DEPS.

I'm planning to land it in svn://svn.chromium.org/chrome/deps/third_party and
then pull it via DEPS - similar to how it works for other thrid-party
dependencies. I combined it all in one CL here just to simplify review process.

I don't think it's a good idea to pull directly from a third-party repo. Most
third-party deps in chromium are pulled from google-owned servers. WebKit is the
only exception. One problem is that it would make developers dependent on the
third-party server being available, but there are also security issues (which
may be mitigated if you pull from git and specify revision hash instead of a
tag).

> 
> This will require a local mirror to be set up for Chromium.
> 
> On 2012/09/05 21:45:00, sergeyu wrote:

[Daniel Berlin, 2012-10-17 21:42:41]

lgtm

[Sergey Ulanov, 2012-10-17 22:43:53]

Uploaded new patch-set that pulls opus from
svn://svn.chromium.org/chrome/trunk/deps/third_part

[Sergey Ulanov, 2012-10-17 22:59:50]

+wez

[Wez, 2012-10-18 20:54:09]

lgtm

http://codereview.chromium.org/10905108/diff/23001/third_party/opus/README.ch...
File third_party/opus/README.chromium (right):

http://codereview.chromium.org/10905108/diff/23001/third_party/opus/README.ch...
third_party/opus/README.chromium:11: This library is user by Chrome Remote
Desktop for audio stream
typo: user -> used

[Sergey Ulanov, 2012-10-18 21:17:47]

http://codereview.chromium.org/10905108/diff/23001/third_party/opus/README.ch...
File third_party/opus/README.chromium (right):

http://codereview.chromium.org/10905108/diff/23001/third_party/opus/README.ch...
third_party/opus/README.chromium:11: This library is user by Chrome Remote
Desktop for audio stream
On 2012/10/18 20:54:09, Wez wrote:
> typo: user -> used

Done.

[commit-bot: I haz the power, 2012-10-18 21:18:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10905108/24006

[commit-bot: I haz the power, 2012-10-18 21:19:01]

Presubmit check for 10905108-24006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    tools/checklicenses

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Sergey Ulanov, 2012-10-18 21:31:42]

+Pawel for tools/checklicenses OWNERS approval

[Paweł Hajdan Jr., 2012-10-18 21:41:25]

Please add an option to use the system version, as described in
http://www.chromium.org/developers/adding-3rd-party-libraries . Feel free to
ping me with any questions you may have about that.

https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
File tools/checklicenses/checklicenses.py (right):

https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
tools/checklicenses/checklicenses.py:324: 'third_party/opus/src': [
Please notify upstream about missing license headers and add the link to that
report here.

[Sergey Ulanov, 2012-10-18 23:42:02]

> Please add an option to use the system version, as described in
> http://www.chromium.org/developers/adding-3rd-party-libraries . Feel free to
> ping me with any questions you may have about that.

There is no easy way to test this configuration, e.g. ubuntu doesn't have opus
precompiled. In any case, system libraries build is not a supported
configuration, and trying to add support for it here would be waste of my time.
It can always be implemented later when distros start packaging libopus.

https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
File tools/checklicenses/checklicenses.py (right):

https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
tools/checklicenses/checklicenses.py:324: 'third_party/opus/src': [
On 2012/10/18 21:41:25, Paweł Hajdan Jr. wrote:
> Please notify upstream about missing license headers and add the link to that
> report here.

Done. There is no public bugtracker for libopus that I could find, so I opened
http://crbug.com/156738 to report and fix this issue.

How important is it to get this issue resolved? 
There are many other third-party dependencies that have the same issue, and I
don't think this problem will ever go away.

[Paweł Hajdan Jr., 2012-10-19 00:00:24]

On 2012/10/18 23:42:02, sergeyu wrote:
> > Please add an option to use the system version, as described in
> > http://www.chromium.org/developers/adding-3rd-party-libraries . Feel free to
> > ping me with any questions you may have about that.
> 
> There is no easy way to test this configuration, e.g. ubuntu doesn't have opus
> precompiled. In any case, system libraries build is not a supported
> configuration, and trying to add support for it here would be waste of my
time.
> It can always be implemented later when distros start packaging libopus.

Gentoo does package libopus. Please add speculative support using template from
http://www.chromium.org/developers/adding-3rd-party-libraries . Again, I'm happy
to help, just would like to change the rend from "avoiding waste of time" to
being a good Open Source citizen. This shouldn't take you more than few minutes,
really (I mean, the speculative part).

>
https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
> File tools/checklicenses/checklicenses.py (right):
> 
>
https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
> tools/checklicenses/checklicenses.py:324: 'third_party/opus/src': [
> On 2012/10/18 21:41:25, Paweł Hajdan Jr. wrote:
> > Please notify upstream about missing license headers and add the link to
that
> > report here.
> 
> Done. There is no public bugtracker for libopus that I could find, so I opened
> http://crbug.com/156738 to report and fix this issue.
> 
> How important is it to get this issue resolved? 
> There are many other third-party dependencies that have the same issue, and I
> don't think this problem will ever go away.

Have you reported this to upstream in some other way? Please do. You will be
able to land with the exception added (but I'd like the system lib thing to get
fixed first), but it is important to do our best to have license headers in all
our files, so that Linux distros can easily verify we're OK about licensing.

Feel free to contact me off-Rietveld if you have questions about why we're doing
this, and how you can do it efficiently. I don't want those parts to take too
much of your time, but in my experience they don't, and they make Chromium
better.

[Sergey Ulanov, 2012-10-19 00:24:09]

On 2012/10/19 00:00:24, Paweł Hajdan Jr. wrote:
> On 2012/10/18 23:42:02, sergeyu wrote:
> > > Please add an option to use the system version, as described in
> > > http://www.chromium.org/developers/adding-3rd-party-libraries . Feel free
to
> > > ping me with any questions you may have about that.
> > 
> > There is no easy way to test this configuration, e.g. ubuntu doesn't have
opus
> > precompiled. In any case, system libraries build is not a supported
> > configuration, and trying to add support for it here would be waste of my
> time.
> > It can always be implemented later when distros start packaging libopus.
> 
> Gentoo does package libopus. 

You are not suggesting I install gentoo to test it, do you? :-)

> Please add speculative support using template from
> http://www.chromium.org/developers/adding-3rd-party-libraries . Again, I'm
happy
> to help, just would like to change the rend from "avoiding waste of time" to
> being a good Open Source citizen. This shouldn't take you more than few
minutes,
> really (I mean, the speculative part).

Ok, I've added use_system_opus and shim opus.h, but didn't test them.

> 
> >
>
https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
> > File tools/checklicenses/checklicenses.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10905108/diff/24006/tools/checklicense...
> > tools/checklicenses/checklicenses.py:324: 'third_party/opus/src': [
> > On 2012/10/18 21:41:25, Paweł Hajdan Jr. wrote:
> > > Please notify upstream about missing license headers and add the link to
> that
> > > report here.
> > 
> > Done. There is no public bugtracker for libopus that I could find, so I
opened
> > http://crbug.com/156738 to report and fix this issue.
> > 
> > How important is it to get this issue resolved? 
> > There are many other third-party dependencies that have the same issue, and
I
> > don't think this problem will ever go away.
> 
> Have you reported this to upstream in some other way? Please do. You will be
> able to land with the exception added (but I'd like the system lib thing to
get
> fixed first), but it is important to do our best to have license headers in
all
> our files, so that Linux distros can easily verify we're OK about licensing.

Not yet, but I will do it later today.

> 
> Feel free to contact me off-Rietveld if you have questions about why we're
doing
> this, and how you can do it efficiently. I don't want those parts to take too
> much of your time, but in my experience they don't, and they make Chromium
> better.

[Paweł Hajdan Jr., 2012-10-19 01:09:12]

LGTM, I'll handle the testing, no worries. :)

I appreciate your work on this use_system_opus switch and shim header. It's
easier when things are done this way from the start, rather than e.g. converting
all "call sites" to the shim header afterwards etc.

https://chromiumcodereview.appspot.com/10905108/diff/26004/third_party/opus/o...
File third_party/opus/opus.gyp (right):

https://chromiumcodereview.appspot.com/10905108/diff/26004/third_party/opus/o...
third_party/opus/opus.gyp:7: ['use_system_opus==0', {
You should add the variable at the top of the file, i.e.

'variables': {
  'use_system_opus%': 0,
},

[Sergey Ulanov, 2012-10-19 01:26:22]

https://chromiumcodereview.appspot.com/10905108/diff/26004/third_party/opus/o...
File third_party/opus/opus.gyp (right):

https://chromiumcodereview.appspot.com/10905108/diff/26004/third_party/opus/o...
third_party/opus/opus.gyp:7: ['use_system_opus==0', {
On 2012/10/19 01:09:13, Paweł Hajdan Jr. wrote:
> You should add the variable at the top of the file, i.e.
> 
> 'variables': {
>   'use_system_opus%': 0,
> },

Done.

[commit-bot: I haz the power, 2012-10-19 01:27:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10905108/32003

[commit-bot: I haz the power, 2012-10-19 03:43:16]

Change committed as 162922

[nsylvain, 2012-10-19 17:04:11]

Don't forget to add the svn:ignore for it. Right now it shows up as an
unversionned directory and it is being deleted and refetched for every single
build on the bots.