media/libopus/celt/rate.h:35:9: warning: 'MAX_PULSES' macro redefined [-Wmacro-redefined]

RESOLVED FIXED

Status

()

defect
P5
normal
Rank:
55
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected)

Details

Build warning:
{
In file included from $OBJ/media/libopus/Unified_c_media_libopus8.c:29:
In file included from $SRC/media/libopus/src/opus_multistream_encoder.c:42:
In file included from $SRC/media/libopus/celt/bands.h:37:
Warning: -Wmacro-redefined in $SRC/media/libopus/celt/rate.h: 'MAX_PULSES' macro redefined
$SRC/media/libopus/celt/rate.h:35:9: warning: 'MAX_PULSES' macro redefined [-Wmacro-redefined]
#define MAX_PULSES 128
        ^
$SRC/media/libopus/silk/define.h:172:9: note: previous definition is here
#define MAX_PULSES                              16
        ^
}

I'm guessing this is from building opus in unified mode (from bug 1146204).

Note that the #defines use different values, and the headers live in different subdirectories (so I'm guessing the values are supposed to be valid for files in that subdirectory).

I'll bet everything works out correctly here, because we'll just redefine MAX_PULSES at the appropriate point in the unified .c file. Still, we should consider using #undef, or using a different unified file per subdirectory, or something like that, to avoid hitting this build warning.
I'm using clang 3.7 as my compiler, BTW.
(Targeted builds confirm that we started hitting this build warning with bug 1146204's commit (mozilla-central changeset d193686b3d0b), btw.)
ehsan, maybe you could take a look?

(Not sure how we work around this sort of thing when unified-build-ifying a 3rd-party library; maybe with a carefully placed #undef at the bottom of some .c file? Or splitting these subdirs into their own unified units?)
Flags: needinfo?(ehsan)
Hmm, we typically avoid modifying third-party code for these reasons, but in this case we have some control over those projects.  If it's possible, we should consider adding an #ifdef-#undef to these headers.  Otherwise, we should probably take either silk_sources or celt_sources out of the unified build, which would be sub-optimal.
Flags: needinfo?(ehsan)
Component: Audio/Video → WebRTC
Ralph - could you look at this (if it didn't get resolved), or forward it to whomever is dealing with libopus?  Thanks
Rank: 55
Flags: needinfo?(giles)
Priority: -- → P5
Ralph is updating our snapshot of libopus (bug 1229475). It may fix or introduce some compiler warnings.
Depends on: 1229475
If it helps, I'm not opposed to changing the two MAX_PULSES to SILK_MAX_PULSES and CELT_MAX_PULSES upstream
Using separate, namespaced macro names like SILK_MAX_PULSES and CELT_MAX_PULSES sounds good to me. :)
The fix is now part of the latest Opus 1.1.2 release. According to Ralph, there's still an issue with the QA macro defined differently in two different C files.
This particular warning should be fixed now that bug 1239078 has landed.

I'll follow-up to fix the QA redefinition upstream; currently we work around that by building those two files separately.
Status: NEW → RESOLVED
Closed: 4 years ago
Depends on: 1239078
Flags: needinfo?(giles)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.