xiph / opus

Modern audio compression for the internet.
https://opus-codec.org/
Other
2.32k stars 619 forks source link

Peculiar issue calling lib from iPhone #213

Open AndrueCope opened 3 years ago

AndrueCope commented 3 years ago

We are using opus successfully from Windows and from Android (encode and decode - the Windows piece has been in use for over a year). However when called on iPhones we're getting strange results.

The calling code is C# and the code actually making the calls is shared by all platforms so we expected it to 'just work'. However what we're getting has us baffled.

Hence our bafflement. I've been programming for several decades and I have experience with calling external code. Usually if you get something wrong the code blows up. Or at least behaves very randomly. Yet our testing indicates that opus is being called correctly because it's stable. It's consistent. It's just the returned data is wrong. Very wrong.

Any thoughts?

Thank you.

xnorpx commented 3 years ago

In case you are using cmake to build the lib it's not really well tested on iOS so there might be bugs there.

In case you have your custom build I would probably take a step back and test opus by itself without any wrapper layers and ensure your build is working correctly (build flags etc).

AndrueCope commented 3 years ago

Yeah we're going to try and create a console app for iOS and see what that shows. Thanks.

AndrueCope commented 3 years ago

I think I know the problem (will confirm tomorrow after a good night's sleep). What we're seeing is that the bitrate_bps value is wrong when run on the iPhone. It should be 16,000 but is something mad like 270,000 on the iPhone. I think the reason is our use of opus_encoder_ctl() to specify the bandwidth during initialisation.

I think that the problem lies in the export declaration of this function:

OPUS_EXPORT int opus_encoder_ctl(OpusEncoder *st, int request, ...) OPUS_ARG_NONNULL(1);

It's a variadac function and last I heard exporting these from a library was asking for trouble because different compilers/platforms encode them differently. For example there's this bodgy code someone else had to implement for another library:

https://github.com/xamarin/xamarin-macios/blob/fc55e4306f79491fd269ca2495c6a859799cb1c6/src/UIKit/UIAppearance.cs#L112

I will confirm this (or refute it, lol) tomorrow by exporting a new function just for setting the bandwidth. If this does prove to be the solution I think we need an 'official' alternative to opus_encoder_ctl(). I'm currently thinking a union to replace the '...' but it's been many years since I used C/C++ so there might be a more conventional way of doing this. Any thoughts?

AndrueCope commented 3 years ago

It's confirmed. When we stopped trying to specify the output bitrate we got the results we wanted. We're mulling over what we're going to do next. I think we can just stop trying to override the bitrate. But I think someone ought to consider providing a non-variadac version of the opus_set_ctrl() function for this library.

I've suggested it to my boss but we may just go with not specifying the output bit rate.

rillian commented 3 years ago

Glad you figured out the issue. That is tricky!

I always considered vararg functions a necessary part of the C abi because of printf. Are you using a different compiler for the opus library and the rest of your app? Is this a toolchain bug you could report to Apple?

xnorpx commented 3 years ago

I think they mention it here, https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms

silverbacknet commented 3 years ago

Looking at the callouts on a few different pages, the one that stands out most to me is that args are never promoted for varargs. What if you call opus_encoder_ctl with (opus_int32)16000 ? There's an off chance it's sending int16 while Opus expects int32, since 16000 can fit in int16. Kind of a guess here since I don't have an Xcode environment set up.

If nothing else, at least Opus is pretty stable now and a bunch of constant shims shouldn't be a huge maintenance burden.

-Em