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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Holbert [:dholbert]]

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
}

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: wrap function calls in #ifdef MOZ_OPUS

This patch wraps calls to only-available-in-opus-buils functions in #ifdef MOZ_OPUS. With this, I can compile this file again.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: wrap member-vars in #ifdef MOZ_OPUS

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.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1: wrap function calls in #ifdef MOZ_OPUS [necessary to build with --disable-opus]

[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.]

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: wrap variables & their usages in #ifdef MOZ_OPUS [for hygiene/consistency (not strictly necessary)]

Comment 5

[Ralph Giles (:rillian)]

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.

Comment 6

[Ralph Giles (:rillian)]

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.

Comment 7

[Daniel Holbert [:dholbert]]

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. :)

Comment 8

[Daniel Holbert [:dholbert]]

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 9

[Daniel Holbert [:dholbert]]

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");

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9a536ae43b0
https://hg.mozilla.org/mozilla-central/rev/9b5e4b85bf1e