compiling with system libvpx

[Paweł Hajdan, Jr.]

The latest dev channel release requires system libvpx when using system ffmpeg. Gentoo Linux does both, and I'd like to upstream the resulting patch (http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/www-client/chromium/files/chromium-system-vpx-r0.patch?revision=1.1&view=markup).

What do you think about switching all libpvx #includes from "third_party/libvpx/include/vpx" to just "vpx"? This will work both when we use system vpx and when we don't (that would just require adding third_party/libvpx/include to the header search path). With #include "third_party/libvpx/include" the bundled header would always be used, which is bad (we don't want to mix bundled and system headers, or compile with system library against bundled headers).

Note that this has already been accepted for ICU in our codebase. An alternative solution would be to have a shim header, that would include the bundled headers or system ones based on some preprocessor symbol, like USE_SYSTEM_VPX.

What do you think?

[Evan Martin]

On Wed, Nov 10, 2010 at 1:16 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> Note that this has already been accepted for ICU in our codebase. An
> alternative solution would be to have a shim header, that would include the
> bundled headers or system ones based on some preprocessor symbol, like
> USE_SYSTEM_VPX.

I think like the shim header approach is less fragile -- there's less
potential of accidentally including a mixture of the system libvpx and
the Chrome copy when you have both installed. It appears that within
libvpx, it includes its own files via #include
"filename_without_path.h" so I think a trivial shim header will do the
right thing, but I haven't thought about it too hard.

[Scott Hess]

+1. Adding to the header search path pushes the problem onto all of
our code. Using a shim concentrates the problem in one place.
Short-term the shim is a little more painful, but it keeps the
abstraction from leaking all over the place.

-scott

[Andrew Scherkus]

Revisiting this in the context of http://codereview.chromium.org/5193003/, it might make more sense to switch VPX includes to the system include path.

If I'm understanding the situation correctly, we already might be mixing system header and chromium header versions :(

Out of curiosity what would a header shim look like?  Something like a simple header file that includes all necessary VPX headers via system or third_party based on a define like USE_SYSTEM_VPX?

Andrew

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Paweł Hajdan, Jr.]

On Mon, Nov 22, 2010 at 19:40, Andrew Scherkus <sche...@chromium.org> wrote:

Revisiting this in the context of http://codereview.chromium.org/5193003/, it might make more sense to switch VPX includes to the system include path.

If I'm understanding the situation correctly, we already might be mixing system header and chromium header versions :(

Fortunately, I think we're safe. The #includes always use full path, like "third_party/libvpx/...", and my Gentoo patches ensure that we also can't mix headers there (the bundled ones are completely removed).

 

Out of curiosity what would a header shim look like?  Something like a simple header file that includes all necessary VPX headers via system or third_party based on a define like USE_SYSTEM_VPX?

Yes. For an example, take a look at src/third_party/sqlite/sqlite3.h.

In the case of vpx, we have multiple headers, but not a large number of them (it seems like 3 or 4 that are used). We could either create a separate shim header for each of them, or we can create one shim header that would #include all of them. What to you think? 

[Andrew Scherkus]

The single shim header sounds reasonable to me.

Andrew