Closed Bug 964559 Opened 6 years ago Closed 6 years ago

Remove MOZ_OGG

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: rillian, Assigned: rillian)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch Remove MOZ_OGG combined patch (obsolete) — Splinter Review
Attachment #8366337 - Flags: review?(chris.double)
Comment on attachment 8366337 [details] [diff] [review]
Remove MOZ_OGG combined patch

Adding Ted for build peer review.
Attachment #8366337 - Flags: review?(ted)
Attachment #8366337 - Flags: review?(chris.double) → review+
Blocks: 964488
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.
Attachment #8366337 - Flags: review?(ted) → review+
Looks like the Try run was good! needinfo=rillian to make sure this doesn't fall off the radar. :)
Flags: needinfo?(rillian)
Yeah, sorry. Ted pointed out an issue in his review and I haven't had time to look at it since.
Flags: needinfo?(rillian)
Attached patch Remove MOZ_OGGSplinter Review
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.
Attachment #8366337 - Attachment is obsolete: true
Attachment #8379403 - Flags: review+
Attached patch Always run ogg mochitests. (obsolete) — Splinter Review
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.
Attachment #8379405 - Flags: review?(chris.double)
Both patches rebased and pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=4cebc938da3f
Attachment #8379405 - Flags: review?(chris.double) → review+
Fix a typo in the no_ogg test. Carrying forward r=doublec.
Attachment #8379405 - Attachment is obsolete: true
Attachment #8379978 - Flags: review+
canPlayType() returns "" not "no" for disabled formats. The mpeg and dash tests were already aware of this.
Attachment #8379980 - Flags: review?(chris.double)
Depends on: 975640
Attachment #8379980 - Flags: review?(chris.double) → review+
Depends on: 976211
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.
That is your opinion but we disagree. Sorry.
(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?
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.
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.
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.)
You need to log in before you can comment on or make changes to this bug.