964559 - Remove MOZ_OGG

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

We have a forest of configure-time defines for disabling bits of media code, but we don't test all combinations, and they tend to bit-rot. Most recently with the MediaEncoder implementation.

Ogg is one of the lower-level libraries. It's small and required for Opus recording and playback in HTML <audio> so it's a good candidate for removing the conditional. Actually support will still be gated by the 'media.ogg.enabled' user pref.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Remove MOZ_OGG combined patch

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8366337 [details] [diff] [review]
Remove MOZ_OGG combined patch

Adding Ted for build peer review.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8366337 [details] [diff] [review]
Remove MOZ_OGG combined patch

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

::: configure.in
@@ +5397,4 @@
>  AC_SUBST(MOZ_LIBVPX_CFLAGS)
>  AC_SUBST(MOZ_LIBVPX_LIBS)
>  
> +if test "$MOZ_WEBM"; then

This seems like you're subtly changing the meaning here, right? Will this fail to set MOZ_VORBIS=1 in the --disable-webm case?

::: python/mozbuild/mozbuild/mozinfo.py
@@ -83,4 @@
>      d['tests_enabled'] = substs.get('ENABLE_TESTS') == "1"
>      d['bin_suffix'] = substs.get('BIN_SUFFIX', '')
>  
> -    d['ogg'] = bool(substs.get('MOZ_OGG'))

You probably want to clean the skip-if/run-if ogg conditionals out of mochitest.ini before you remove this. You can just set this to True if you don't want to roll that into this patch.

Comment 4

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=b8e530886c99

Comment 5

[Daniel Holbert [:dholbert]]

Looks like the Try run was good! needinfo=rillian to make sure this doesn't fall off the radar. :)

Comment 6

[Ralph Giles (:rillian)]

Yeah, sorry. Ted pointed out an issue in his review and I haven't had time to look at it since.

Comment 7

[Ralph Giles (:rillian)]

Attachment: Remove MOZ_OGG

Update patch to set d['ogg'] = True; in mozbuild per Ted's review comment.

We do fail to enable MOZ_VORBIS with --disable-webm, but --disable-webm doesn't build for other reasons, and is tracked in a bug we're blocking. I'd like to fix that separately.

Carrying forward r=doublec,ted.

Comment 8

[Ralph Giles (:rillian)]

Attachment: Always run ogg mochitests.

Part 2: also per Ted's comment, remove the 'run-if = ogg' and 'skip-if = ogg' conditionals on the media mochitests. The tests now assume ogg is available.

Comment 9

[Ralph Giles (:rillian)]

Both patches rebased and pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=4cebc938da3f

Comment 10

[Ralph Giles (:rillian)]

Attachment: Always run ogg mochitests.

Fix a typo in the no_ogg test. Carrying forward r=doublec.

Comment 11

[Ralph Giles (:rillian)]

Attachment: Part 3: Unbreak canPlayType tests for disabled formats

canPlayType() returns "" not "no" for disabled formats. The mpeg and dash tests were already aware of this.

Comment 12

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=685c383ed517

Comment 13

[Ralph Giles (:rillian)]

Green on try with the patch from bug 976211.

https://tbpl.mozilla.org/?tree=Try&rev=05eb7003cf9c

Comment 14

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7840db3e63c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/662f7982dc8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/98316f2aba5d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7840db3e63c2
https://hg.mozilla.org/mozilla-central/rev/662f7982dc8b
https://hg.mozilla.org/mozilla-central/rev/98316f2aba5d

Comment 16

[maillist-mozilla]

imo rather than removing compile time options that make it possible to disable additional (and unneeded dependencies), a buildbot should try to build all possible combinations of optional features as a pre-release sanity test, and issues be properly fixed in a way that preserves the ability to disable unneeded features.

Comment 17

[(not currently active) Ted Mielczarek]

That is your opinion but we disagree. Sorry.

Comment 18

[maillist-mozilla]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> That is your opinion but we disagree. Sorry.

how can you know what everyone else thinks? do you have telepathic powers?

Comment 19

[(not currently active) Ted Mielczarek]

I know what the consensus is among Mozilla developers who have a stake in this discussion, which is all that matters. We don't have to make everyone in the world happy. You are always welcome to patch the source as you see fit, but don't expect us to cater to every nonstandard desire.

Comment 20

[phil.krylov]

This configure patch portion is wrong:

    2.96  
    2.97 -if test "$MOZ_WEBM" -o "$MOZ_OGG"; then
    2.98 +if test "$MOZ_WEBM"; then
    2.99      if test "$MOZ_SAMPLE_TYPE_FLOAT32"; then

If MOZ_OGG is now always true, the condition should translate to:

    if test "$MOZ_WEBM" || true

It means it's always true.

Comment 21

[Daniel Holbert [:dholbert]]

Note that we're intending to drop the MOZ_WEBM switch as well, in bug 964484. (If I'm understanding correctly, that means comment 20 doesn't really matter; correct me if I'm misunderstanding though.)