Closed Bug 1100632 Opened 5 years ago Closed 5 years ago

Remove support for "--disable-opus"

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: dholbert, Assigned: awake, Mentored)

References

Details

(Whiteboard: [mentor=rillian] [lang=C++])

Attachments

(1 file, 1 obsolete file)

As noted in bug 929427 comment 6, rillian would like to remove "--disable-opus", because it's unmaintained & doesn't build successfully (& presumably is not worth maintaining as a build option).

Filing this bug on actually removing support for this build flag, and the MOZ_OPUS #define that it controls.

(rillian, can you confirm that this is still what we want to do? & add any other info that you think is appropriate here)
Flags: needinfo?(giles)
Sounds good to me! Are you doing the patch?
Flags: needinfo?(giles)
(In reply to Daniel Holbert [:dholbert] from comment #0)
> As noted in bug 929427 comment 6, rillian would like to remove

Sorry, I meant "bug 964528 comment 6". (wrong bug number)

(In reply to Ralph Giles (:rillian) from comment #1)
> Sounds good to me! Are you doing the patch?

Wasn't planning on it; just wanted to make sure that this action-item was tracked somewhere, since I only just rediscovered that this build config is broken.  Maybe you'd be up for making this a mentored bug?  (I imagine it's just a search-and-remove type of operation.)
Whiteboard: [mentor=rillian] [lang=C++]
Mentor: giles
I removed all the MOZ_OPUS macros that I found.
Attachment #8529728 - Flags: review?(giles)
Assignee: nobody → bugzillafx
Status: NEW → ASSIGNED
Comment on attachment 8529728 [details] [diff] [review]
Remove --disable-opus option and MOZ_OPUS #define macros

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

Thanks for doing this. r=me with these comments addressed.

You missed one in modules/libpref/init/all.js Please remove that as well, or your patch will disable opus support at runtime!

::: dom/media/ogg/OggReader.cpp
@@ +671,5 @@
>  
>  bool OggReader::DecodeAudioData()
>  {
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> +  DebugOnly<bool> haveCodecState = mVorbisState != nullptr || mOpusState != nullptr;

Please wrap this after the '=' or the '||' to stay in 80 columns.
Attachment #8529728 - Flags: review?(giles) → review+
Attached patch bug1100632.diffSplinter Review
Fixed both issues. Thanks for spotting that!
Attachment #8529728 - Attachment is obsolete: true
Attachment #8530196 - Flags: review?(giles)
Comment on attachment 8530196 [details] [diff] [review]
bug1100632.diff

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

Looks good, thanks!

Pushed to the try server for testing: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=144bb0335922
Attachment #8530196 - Flags: review?(giles) → review+
Try push looks ok. Ready to land, I think.
Keywords: checkin-needed
Comment on attachment 8530196 [details] [diff] [review]
bug1100632.diff

Adding Ted for build peer review, since we're removing a configure option.
Attachment #8530196 - Flags: review?(ted)
Comment on attachment 8530196 [details] [diff] [review]
bug1100632.diff

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

Always happy to r+ the removal of configure options. :)
Attachment #8530196 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/c2fc9658f6e9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.