Closed Bug 929430 Opened 6 years ago Closed 6 years ago

Compilation fails with --disable-opus, in content/media, with "OggReader.cpp:134:54: error: no matching function for call to ‘mozilla::OpusState::Reset(bool&)’"

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(2 files, 2 obsolete files)

STR:
Build with
 ac_add_options --disable-opus
in your mozconfig.

(You may error out on something else earlier in the build (e.g. bug 929427); if so, ignore that error, and run "./mach build content/media" to skip ahead to the code that I'm filing this bug about.)

RESULTS: These build errors when we get to OggReader:
{
/mozilla-central/content/media/ogg/OggReader.cpp:134:54: error: no matching function for call to ‘mozilla::OpusState::Reset(bool&)’
   if (mOpusState && NS_FAILED(mOpusState->Reset(start))) {
                                                      ^
../../../dist/include/mozilla/Likely.h:17:48: note: in definition of macro ‘MOZ_UNLIKELY’
 #  define MOZ_UNLIKELY(x) (__builtin_expect(!!(x), 0))
                                                ^
/mozilla-central/content/media/ogg/OggReader.cpp:134:21: note: in expansion of macro ‘NS_FAILED’
   if (mOpusState && NS_FAILED(mOpusState->Reset(start))) {
                     ^
/mozilla-central/content/media/ogg/OggReader.cpp:134:54: note: candidate is:
   if (mOpusState && NS_FAILED(mOpusState->Reset(start))) {
                                                      ^
../../../dist/include/mozilla/Likely.h:17:48: note: in definition of macro ‘MOZ_UNLIKELY’
 #  define MOZ_UNLIKELY(x) (__builtin_expect(!!(x), 0))
                                                ^
/mozilla-central/content/media/ogg/OggReader.cpp:134:21: note: in expansion of macro ‘NS_FAILED’
   if (mOpusState && NS_FAILED(mOpusState->Reset(start))) {
                     ^
In file included from /mozilla-central/content/media/ogg/OggReader.h:17:0,
                 from /mozilla-central/content/media/ogg/OggReader.cpp:12:
/mozilla-central/content/media/ogg/OggCodecState.h:132:20: note: virtual nsresult mozilla::OggCodecState::Reset()
   virtual nsresult Reset();
                    ^
/mozilla-central/content/media/ogg/OggCodecState.h:132:20: note:   candidate expects 0 arguments, 1 provided
/mozilla-central/content/media/ogg/OggReader.cpp: In member function ‘bool mozilla::OggReader::ReadOggChain()’:
/mozilla-central/content/media/ogg/OggReader.cpp:664:14: warning: unused variable ‘newOpusState’ [-Wunused-variable]
   OpusState* newOpusState = nullptr;
              ^
/mozilla-central/content/media/ogg/OggReader.cpp: In member function ‘virtual nsresult mozilla::OggReader::GetBuffered(mozilla::dom::TimeRanges*, int64_t)’:
/mozilla-central/content/media/ogg/OggReader.cpp:1826:61: error: no matching function for call to ‘mozilla::OpusState::Time(int&, int64_t&)’
         startTime = OpusState::Time(mOpusPreSkip, granulepos);
                                                             ^
/mozilla-central/content/media/ogg/OggReader.cpp:1826:61: note: candidate is:
In file included from /mozilla-central/content/media/ogg/OggReader.h:17:0,
                 from /mozilla-central/content/media/ogg/OggReader.cpp:12:
/mozilla-central/content/media/ogg/OggCodecState.h:111:19: note: virtual int64_t mozilla::OggCodecState::Time(int64_t)
   virtual int64_t Time(int64_t granulepos) { return -1; }
                   ^
/mozilla-central/content/media/ogg/OggCodecState.h:111:19: note:   candidate expects 1 argument, 2 provided
make[2]: *** [OggReader.o] Error 1
}
This patch wraps calls to only-available-in-opus-buils functions in #ifdef MOZ_OPUS. With this, I can compile this file again.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #820297 - Flags: review?(kinetik)
This part adds some more (not-strictly-necessary) #ifdefs around "mOpusState" and "mOpusEnabled" and all of their usages.

(I'm not 100% sure we want this part, but it seems like a good idea, for consistency with other blocks of this file that use these variables and are ifdeffed.)
Attachment #820299 - Flags: review?(kinetik)
[actually, the previous "part 1" had some changes that really belong in "part 2".  Here's a simplified "part 1" that's *really* just scoped to Opus function-calls.]
Attachment #820297 - Attachment is obsolete: true
Attachment #820299 - Attachment is obsolete: true
Attachment #820297 - Flags: review?(kinetik)
Attachment #820299 - Flags: review?(kinetik)
Attachment #820304 - Flags: review?(kinetik)
Attachment #820307 - Attachment description: part 2: wrap variables & their usages in #ifdef MOZ_OPUS → part 2: wrap variables & their usages in #ifdef MOZ_OPUS [for hygiene/consistency (not strictly necessary)]
Attachment #820304 - Flags: review?(kinetik) → review+
Comment on attachment 820304 [details] [diff] [review]
part 1: wrap function calls in #ifdef MOZ_OPUS [necessary to build with --disable-opus]

Oops, didn't mean to cancel kinetik's review. I wrote the code though. Thanks for the fix.
Attachment #820304 - Flags: review?(kinetik)
Comment on attachment 820307 [details] [diff] [review]
part 2: wrap variables & their usages in #ifdef MOZ_OPUS [for hygiene/consistency (not strictly necessary)]

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

NB I still think we should remove the build-time switch and rely just on the runtime pref.

1) We never test these configurations adequately.
2) We've been shipping Opus for two years, we're sure we want to keep it.
3) It's required for WebRTC which is an important feature.
Attachment #820307 - Flags: review+
Comment on attachment 820304 [details] [diff] [review]
part 1: wrap function calls in #ifdef MOZ_OPUS [necessary to build with --disable-opus]

(In reply to Ralph Giles (:rillian) from comment #5)
> Oops, didn't mean to cancel kinetik's review. I wrote the code though.
> Thanks for the fix.

Thanks! I picked him somewhat at random; I'm happy to take your r+ instead. :)
Attachment #820304 - Flags: review?(kinetik)
Attachment #820307 - Flags: review?(kinetik)
Comment on attachment 820307 [details] [diff] [review]
part 2: wrap variables & their usages in #ifdef MOZ_OPUS [for hygiene/consistency (not strictly necessary)]

This part actually failed to compile on Windows (apparently #ifdef-inside-NS_ASSERTION causes problems there), so I pulled out the assertion's boolean condition into a DebugOnly<bool> (with #ifdefs) and made us assert about the bool's truthfulness.
https://tbpl.mozilla.org/php/getParsedLog.php?id=29512253&tree=Try

Try liked the updated version: https://tbpl.mozilla.org/?tree=Try&rev=7a79f720c953

So I landed with that tweak:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a536ae43b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5e4b85bf1e
Comment on attachment 820307 [details] [diff] [review]
part 2: wrap variables & their usages in #ifdef MOZ_OPUS [for hygiene/consistency (not strictly necessary)]

Sorry, I meant to quote the following (non-compiling-on-windows) code at the beginning of prev. comment:
> bool OggReader::DecodeAudioData()
> {
>   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>-  NS_ASSERTION(mVorbisState != nullptr || mOpusState != nullptr,
>-    "Need audio codec state to decode audio");
>+  NS_ASSERTION(mVorbisState != nullptr
>+#ifdef MOZ_OPUS
>+               || mOpusState != nullptr
>+#endif /* MOZ_OPUS */
>+               , "Need audio codec state to decode audio");
https://hg.mozilla.org/mozilla-central/rev/d9a536ae43b0
https://hg.mozilla.org/mozilla-central/rev/9b5e4b85bf1e
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.