Fix build with system libvpx....

Fix build with system libvpx.

This is  upstreaming a Gentoo Linux patch.

TEST=none
BUG=none

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

[Paweł Hajdan Jr., 2010-11-19 20:19:49]

This is hopefully a simple review. Feel free to remove yourself from reviewers.
I included more of you to see who will respond faster. ;-)

[fbarchard1, 2010-11-19 20:28:51]

On 2010/11/19 20:19:49, Paweł Hajdan Jr. wrote:
> This is hopefully a simple review. Feel free to remove yourself from
reviewers.
> I included more of you to see who will respond faster. ;-)

I'm against use-system-vpx.
Our version is built with bug fixes and at times, needs to be in sync, API wise
with chrome ffmpeg-mt.  This has been problematic.
Currently, chromoting does not build with use-system-vpx.
Our version is built with ICC, which improves performance, and is what we've
tested with.

What is the rationale for adding support.

[Paweł Hajdan Jr., 2010-11-19 20:36:52]

On 2010/11/19 20:28:51, fbarchard1 wrote:
> I'm against use-system-vpx.
> Our version is built with bug fixes and at times, needs to be in sync, API
wise
> with chrome ffmpeg-mt.  This has been problematic.

It's a bit weird. Do those fixes go "upstream"?

Note that Linux distributions can and do handle dependency requirements like
that.

> Currently, chromoting does not build with use-system-vpx.

I've built chromium-9.0.587.0 with remoting and system vpx. This patch is a part
of an effort to upstream the fixes made locally for Gentoo Linux.

> Our version is built with ICC, which improves performance, and is what we've
> tested with.

Not sure how's that relevant. Is there a checked in binary that is used when
use_system_vpx==0?

> What is the rationale for adding support.

Well, as always, Linux distributions do not want bundled copies of software
libraries.

A random link about that:
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

By the way, having use_system_vpx option in the build system that is broken is
strange. I'd say it should either be fixed or removed completely. Of course I'd
prefer fixing. Linux distributions can send you patches when it becomes broken.

[Alpha Left Google, 2010-11-19 21:08:49]

I think not linking against libvpx when use_system_vpx is specified was a
mistake, so LGTM from me. Should check with scherkus@ to see if this is right
though.

[scherkus (not reviewing), 2010-11-22 07:17:34]

http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp
File source/ffmpeg.gyp (right):

http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp#newcode905
source/ffmpeg.gyp:905: '-lvpx',
hmm so we already include -lpvx but that's when use_system_ffmpeg!=0 (see line
415 or so) and we have a section for checking use_system_vpx up there as well

I'm a bit confused as to what this section is accomplishing... when using both
the system ffmpeg *and* the system libvpx?

if that's the case, isn't the system ffmpeg sufficient assuming that it was
built with libvpx?

[scherkus (not reviewing), 2010-11-22 07:24:28]

Also reading through
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries I have to say
Chromium's usage of ffmpeg-mt (a forked version of ffmpeg that to my knowledge
won't be upstreamed any time soon) that Chromium takes time to update +
stabilize ourselves seems to constitute as a reasonable ground for an exception
(I know that's for Fedora and this patch is for Gentoo, but whatever)

I'm still not completely convinced that distributions should be attempting to
use the system version of ffmpeg.  It's caused pain for everybody involved and
the number of patches we've had to accept fixing use_system_ffmpeg and
use_system_libvpx seems to back it up.

[Paweł Hajdan Jr., 2010-11-22 18:13:36]

On Mon, Nov 22, 2010 at 08:17, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp
> File source/ffmpeg.gyp (right):
>
> http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp#newcode905
> source/ffmpeg.gyp:905: '-lvpx',
> hmm so we already include -lpvx but that's when use_system_ffmpeg!=0
> (see line 415 or so) and we have a section for checking use_system_vpx
> up there as well
>

I'd probably need to remove those -lvpx sections then, right?


> I'm a bit confused as to what this section is accomplishing... when
> using both the system ffmpeg *and* the system libvpx?
>

I think it fixes build with bundled ffmpeg but system libvpx.


> if that's the case, isn't the system ffmpeg sufficient assuming that it
> was built with libvpx?


I'm not sure.

On Mon, Nov 22, 2010 at 08:24, <scherkus@chromium.org> wrote:

> Also reading through
> http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries I have to say
> Chromium's usage of ffmpeg-mt (a forked version of ffmpeg that to my
> knowledge
> won't be upstreamed any time soon) that Chromium takes time to update +
> stabilize ourselves seems to constitute as a reasonable ground for an
> exception
> (I know that's for Fedora and this patch is for Gentoo, but whatever)
>

Note that the patch is not distro-specific.

I'm not sure how the above affects the usage of system ffmpeg. If the distro
can provide a recent enough system ffmpeg, that's fine. And Gentoo can, and
actually does (of course with a slight delay).


> I'm still not completely convinced that distributions should be attempting
> to
> use the system version of ffmpeg.  It's caused pain for everybody involved
> and
> the number of patches we've had to accept fixing use_system_ffmpeg and
> use_system_libvpx seems to back it up.


I think reviewing patches isn't a big maintenance burden for you, right?
When rolling ffmpeg, you don't test whether use_system_* things work anyway
(that's why they break), so there is no difference for you. Please be
distro-friendly. :)

[fbarchard1, 2010-11-22 20:57:04]

http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp
File source/ffmpeg.gyp (right):

http://codereview.chromium.org/5193003/diff/1/source/ffmpeg.gyp#newcode905
source/ffmpeg.gyp:905: '-lvpx',
We need -lvpx if building ffmpeg.

[fbarchard, 2010-11-24 02:09:40]

Can you test all permutations of use_system ffmpeg and vpx
Then you'll need to roll ffmpeg in src/DEPS
Chrome still wont build until chromoting.gyp is fixed

[Paweł Hajdan Jr., 2010-11-24 20:38:12]

On Wed, Nov 24, 2010 at 03:09, <fbarchard@chromium.org> wrote:

> Can you test all permutations of use_system ffmpeg and vpx
>

Tested those configurations:

use_system_ffmpeg==1 and use_system_vpx==1
use_system_ffmpeg==0 and use_system_vpx==1

For use_system_vpx==0 this change is a no-op.


> Then you'll need to roll ffmpeg in src/DEPS
>

Right.


> Chrome still wont build until chromoting.gyp is fixed


Not sure about the above. None of my local Gentoo patches modifies
chromoting.gyp iirc, and I built 9.0.587.0 successfully.

[scherkus (not reviewing), 2010-11-24 20:44:26]

Would doing the header shim thing remove the need for this patch?

[fbarchard1, 2010-11-24 22:57:43]

On 2010/11/24 20:38:12, Paweł Hajdan Jr. wrote:
> On Wed, Nov 24, 2010 at 03:09, <mailto:fbarchard@chromium.org> wrote:
> 
> > Can you test all permutations of use_system ffmpeg and vpx
> >
> 
> Tested those configurations:
> 
> use_system_ffmpeg==1 and use_system_vpx==1
> use_system_ffmpeg==0 and use_system_vpx==1

ok

> Not sure about the above. None of my local Gentoo patches modifies
> chromoting.gyp iirc, and I built 9.0.587.0 successfully.

if you build chrome and dont have chromoting disabled (do you?), use_system_vpx
will have include problems.

header shim?

[Paweł Hajdan Jr., 2010-11-25 07:09:29]

On Wed, Nov 24, 2010 at 23:57, <fbarchard@google.com> wrote:

> if you build chrome and dont have chromoting disabled (do you?),
> use_system_vpx
> will have include problems.
>

Gentoo builds with chromoting enabled (default), see <
http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/www-client/chromium/c...
>.

However, there are some additional changes, see <
http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/www-client/chromium/f...
>.

For upstreaming, I'm going to use a shim header.

[Paweł Hajdan Jr., 2010-11-29 13:52:22]

ping

[scherkus (not reviewing), 2010-11-29 22:49:08]

I looked into this a bit more and I think the proper fix consists of:
  - moving use_system_vpx code to libvpx.gyp
  - have ffmpeg depend on libvpx target
  - use header shim for vpx includes

How does that sound?

[Paweł Hajdan Jr., 2010-11-30 09:35:49]

Makes sense. I'll have a better patch later.

On Mon, Nov 29, 2010 at 23:49, <scherkus@chromium.org> wrote:

> I looked into this a bit more and I think the proper fix consists of:
>  - moving use_system_vpx code to libvpx.gyp
>  - have ffmpeg depend on libvpx target
>  - use header shim for vpx includes
>
> How does that sound?
>
>
> http://codereview.chromium.org/5193003/
>

[scherkus (not reviewing), 2010-11-30 20:06:56]

On 2010/11/30 09:35:49, Paweł Hajdan Jr. wrote:
> Makes sense. I'll have a better patch later.

Just to confirm do you still want this patch or should we close this issue?

[Paweł Hajdan Jr., 2010-11-30 20:39:45]

On 2010/11/30 20:06:56, scherkus wrote:
> Just to confirm do you still want this patch or should we close this issue?

I have closed the issue. I'm going to submit a better patch later.

[Paweł Hajdan Jr., 2010-12-02 08:42:33]

After looking again at libvpx.gyp and ffmpeg.gyp, I'm re-opening this CL. Can I
get it committed?

The problem is that libvpx.gyp is not really used, and ffmpeg.gyp is full of
vpx-related hacks, and I don't really feel like fixing that (seems quite risky).

Alternatively, feel free to just remove use_system_vpx and use_system_ffmpeg.

[Paweł Hajdan Jr., 2010-12-03 18:29:55]

ping

[scherkus (not reviewing), 2010-12-14 16:27:15]

I have no time to do the proper fix (patches welcome!) but let's go w/ this and
file a bug

maybe someone from chromoting can do the clean up since I think they also depend
on libvpx

[scherkus (not reviewing), 2010-12-14 16:27:26]

in other words LGTM

[fbarchard1, 2010-12-14 18:53:30]

We switched chromoting to use libvpx that is contained in ffmpeg, so the
dependency is now less direct.
Chromoting doesn't need to link vpx, but it does need its' headers.

On Tue, Dec 14, 2010 at 8:27 AM, <scherkus@chromium.org> wrote:

> I have no time to do the proper fix (patches welcome!) but let's go w/ this
> and
> file a bug
>
> maybe someone from chromoting can do the clean up since I think they also
> depend
> on libvpx
>
> http://codereview.chromium.org/5193003/
>

[Paweł Hajdan Jr., 2010-12-16 08:46:18]

I'll see what happens with the next dev channel release. However, I don't
understand the change you've described below.

On Tue, Dec 14, 2010 at 19:53, Frank Barchard <fbarchard@google.com> wrote:

> We switched chromoting to use libvpx that is contained in ffmpeg, so the
> dependency is now less direct.
> Chromoting doesn't need to link vpx, but it does need its' headers.
>
>
> On Tue, Dec 14, 2010 at 8:27 AM, <scherkus@chromium.org> wrote:
>
>> I have no time to do the proper fix (patches welcome!) but let's go w/
>> this and
>> file a bug
>>
>> maybe someone from chromoting can do the clean up since I think they also
>> depend
>> on libvpx
>>
>> http://codereview.chromium.org/5193003/
>>
>
>

[Paweł Hajdan Jr., 2011-01-06 11:40:40]

I've committed this change based on scherkus' "LGTM". If there are any concerns
about it, feel free to revert or request a follow-up.