Fixing Cmake error for OPUS_FIXED_POINT=ON,OPUS_ENABLE_FLOAT_API=OFF

[peaksound]

Only include floating point inner product when the OPUS_ENABLE_FLOAT_API has been enabled.

See issue 348

[xnorpx]

lgtm fyi @jmvalin

[peaksound]

@xnorpx
Do you support building with fixed and floating point at the same time? If so, then this pull request isn't enough. I just tested with OPUS_FIXED_POINT=ON and OPUS_ENABLE_FLOAT_API=ON. It's showing the same error as issue 348.

[peaksound]

It looks like I need to do a similar thing to add opus\silk\float to the include directories. Give me a few minutes to find where that needs to go, and then I'll have an update.

[peaksound]

It looks like float and fixed point are currently mutually exclusive. I'll wait for someone else to weigh in on what the design intent is before pushing an update.

if(OPUS_FIXED_POINT)
  add_sources_group(opus silk ${silk_sources_fixed})
  target_include_directories(opus PRIVATE silk/fixed)
  target_compile_definitions(opus PRIVATE FIXED_POINT=1)
else()
  add_sources_group(opus silk ${silk_sources_float})
  target_include_directories(opus PRIVATE silk/float)
endif()

[xnorpx]

If I remember correctly for CMake if one choose fixed point it means fixed point only build with no floating point. (might be that the flags don't catch all combinations here)

While if one run floating point api there will be fixed point code running in some components.

The fixed point is there mainly for hardware that doesn't have FPU. Only other reason to run fixed point only would be binary size maybe?

[peaksound]

Okay, that's great news. This pull-request should be good to go, then.

For my clarification, when building with OPUS_ENABLE_FLOAT_API=ON, I can encode 16-bit signed PCM data to encoded 16-bit opus and decode back to 16-bit PCM? If that's the case, that is the simplest solution for me. That 2x memory footprint saving of the encoded data is what I was hoping for by using OPUS_FIXED_POINT=ON. Or was it a flawed assumption from the beginning that have the encoded data in fixed point would shrink the memory footprint.

[jmvalin]

Why are you basing this on FLOAT_API, shouldn't it just be about float vs fixed-point? In other words, IIRC silk_inner_product_FLP never gets used for fixed-point, even if FLOAT_API is enabled

[peaksound]

I'm happy to change this to if(NOT OPUS_FIXED_POINT) , but I'm a bit confused about the details here. Maybe I can add some clarifying notes to CMake while I'm at it.

Currently this is the help prompt in CMakeLists.txt
OPUS_FIXED_POINT - compile as fixed-point (for machines without a fast enough FPU)
OPUS_ENABLE_FLOAT_API - compile with the floating point API (for machines with float library)

Does OPUS_ENABLE_FLOAT_API equal NOT OPUS_FIXED_POINT? OPUS_FIXED_POINT seems like a way to enable fixed point if you want the performance savings. OPUS_ENABLE_FLOAT_API seems to give an additional API that supports floats. What combination of these flags are intended to work together? Or do both flags mean opposite of the other?

• OPUS_ENABLE_FLOAT_API=ON, OPUS_FIXED_POINT=OFF :: This is the default configuration where floating point is used.
• OPUS_ENABLE_FLOAT_API=OFF, OPUS_FIXED_POINT=ON :: This is used for fixed point only
• OPUS_ENABLE_FLOAT_API=ON, OPUS_FIXED_POINT=ON :: Can this be used if you have a floating point processing unit, but want better performance? It allows the float API, but processes in float?
• OPUS_ENABLE_FLOAT_API=OFF, OPUS_FIXED_POINT=OFF :: I don't know what this would be.

[jmvalin]

Basically, the Opus API includes both integer and floating-point calls for encoding and decoding. No matter how you build the library, the API should remain the same otherwise you might break things. When you compile as float, the integer encoder call just converts the data to float and then calls the float internal functions. When you compile as fixed-point, then the float calls just convert everything to/from integer. That's the recommended way of building Opus -- it'll just work all the time and use whatever is the most efficient on the arch being used.
Now, in some applications where space is really constrained (or whatever other reason), you might not want to have these float functions you're never going to use. So disabling the float API just saves some space and/or avoid linking with a float emulation library on chips without an FPU. If you're already building as fixed-point and using the integer API, then disabling the float API changes nothing except the size of the library. But then you need to be careful because the library isn't a drop-in replacement for the "standard" Opus library.
Honestly, I'm not sure whether disabling the float API works for a floating-point build (probably not), but it doesn't make much sense either way.

[theyyg]

Thanks for the clarification. That clears up my confusion.

I've updated the pull request to use the flag, NOT OPUS_FIXED_POINT.

[jmvalin]

Merged. Next make make sure you make it a single patch rather than a buggy one followed by a fix.