--disable-ogg build fails with "dist/include/vorbis/codec.h:26:21: fatal error: ogg/ogg.h: No such file or directory" from WebMReader.h

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: rillian)

Tracking

Trunk
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I just tried a build with these mozconfig options:
ac_add_options --enable-debug --disable-optimize
ac_add_options --disable-ogg

and it failed with:
{
 0:24.78 In file included from ../../dist/include/WebMReader.h:22:0,
 0:24.78                  from /scratch/work/builds/mozilla-inbound/mozilla/content/media/DecoderTraits.cpp:26:
 0:24.79 ../../dist/include/vorbis/codec.h:26:21: fatal error: ogg/ogg.h: No such file or directory
 0:24.79  #include <ogg/ogg.h>
 0:24.79                      ^
 0:24.79 compilation terminated.
 0:25.42 
}

Looks like WebMReader.h incorrectly includes vorbis/codec.h without checking whether ogg support is enabled:
http://mxr.mozilla.org/mozilla-central/source/content/media/webm/WebMReader.h?rev=473c72edc9ca#22

(Not sure what our options are there if ogg is disabled; if we really need it, maybe we should error out during configure if webm is enabled but ogg is disabled?)
WebM support implies vorbis support, which depends on the ogg library, which is what --disable-ogg removes. 

Do you want --disable-ogg to disable only .ogg playback?
I think we should just remove --disable-ogg; we have the media.ogg.enabled runtime pref to control format support.
(In reply to Ralph Giles (:rillian) from comment #1)
> Do you want --disable-ogg to disable only .ogg playback?

I don't particularly care; I was just playing around with different configure.in options, and noticed that this one is a footgun.

> I think we should just remove --disable-ogg

That'd be fine with me. My main concern here is removing footguns that don't go off until you've waited for half of a full-build-time.

Alternately, we could error out during configure, with a helpful message (along the lines of "ogg support is required for webm support; please either enable ogg, or disable webm").  I lean slightly towards that, but I also don't really know our media code.
If a build configuration doesn't make sense, configure should bail.

If a build will fail due to something detectable by configure, configure should detect that and fail fast.

We don't want people sitting through 30 minutes of build on an ancient machine only to find out their time was wasted.
I think we're in desperate need for input from the third parties using gecko for something else as to whether they (still) need those options. Short of this, i'd rather keep them, and make them fail at configure time where it makes sense. That is, if webm requires ogg, then --disable-ogg should either disable webm or barf if webm is enabled. I'd lean towards the latter.
See also bug 929398 about "--disable-webm".

(So currently, if you have either of "--disable-webm" or "--disable-ogg", your build will fail partway through. You have to have neither option or both options in order to get a successful build.)

(I think the dependency makes less sense in the other bug, though. That is to say -- it seems reasonable sense that turning off ogg support will breaks webm support (this bug), and that's something we could catch early at configure-time.  But I don't think it makes sense that turning off webm support would break ogg support.)
See Also: → 929398
Blocks: 929398
Add checks at the end of configure. Any combinations I missed?
Assignee: nobody → giles
Attachment #820420 - Flags: review?(ted)
Comment on attachment 820420 [details] [diff] [review]
Add guards for invalid combinations

Review of attachment 820420 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know how to answer your question, but this seems fine. Ugh, I hate configure arguments. :-(
Attachment #820420 - Flags: review?(ted) → review+
Thanks. The question was more for Daniel, since he's been poking at the failure modes. Better than what we had before, regardless.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 929427
Comment on attachment 820420 [details] [diff] [review]
Add guards for invalid combinations

>+if test -n "$MOZ_VORBIS" -a -z "$MOZ_OGG"; then
>+    AC_MSG_ERROR([MOZ_VORBIS requires MOZ_OGG which is disabled.
>+Note that you need vorbis support for WebM playback.])

That second line isn't 100% correct, right? It looks like you need *either* vorbis *or* tremor for WebM playback, IIUC, from looking at this chunk elsewhere in configure.in:

5372 if test "$MOZ_WEBM"; then
5373     MOZ_CUBEB=1
5374     if test "$MOZ_SAMPLE_TYPE_FLOAT32"; then
5375         MOZ_VORBIS=1
5376     else
5377         MOZ_TREMOR=1
5378     fi
5379 fi
http://mxr.mozilla.org/mozilla-central/source/configure.in#5372

Maybe s/need/may need/?  (or s/you need/some platforms need/?)
(by "second line" I mean "second line of the error message")
I wasn't entirely happy with the second line either, but couldn't come up with anything better.

Both the Ogg and the WebM media formats require the vorbis audio codec, which is provided by *either* MOZ_VORBIS or MOZ_TREMOR. We don't have a configure switch for vorbis either way. It's enabled if webm is enabled, choosing the libvorbis or libtremor (fixed-point decoder only) as appropriate for the platform.

So the only way for MOZ_VORBIS to be set without MOZ_OGG is if webm if --disable-ogg is passed to configure for a target which uses the floating point libvorbis without --disable-webm. I indended the second line as a hint to add --disable-webm if you really want --disable-ogg, without being specific about which implementation we're using.
(In reply to Ralph Giles (:rillian) from comment #7)
> Add checks at the end of configure. Any combinations I missed?

I think you caught all the ones I've run up against. (with the patches for bug 929398, bug 929430, and bug 929404 applied)  Well done! :)

(In reply to Ralph Giles (:rillian) from comment #12)
> I indended the second line as a hint
> to add --disable-webm if you really want --disable-ogg, without being
> specific about which implementation we're using.

Understood - I just thought it might be worth softening the language, for correctness. Doesn't really matter though.
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (In reply to Ralph Giles (:rillian) from comment #7)
> > Add checks at the end of configure. Any combinations I missed?

(Actually -- if you throw "--disable-wave" into the mix, then bug 928547 might be another thing to check in this same chunk of code.  But that might also just be a place where we need some #ifdefs. In any case, probably best to sort that out over there.)
(In reply to Daniel Holbert [:dholbert] from comment #14)

> if you throw "--disable-wave" into the mix, then bug 928547
> might be another thing to check in this same chunk of code.

I think that is an #ifdef case.
https://hg.mozilla.org/mozilla-central/rev/d8d851a1ed2a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.