mackron / miniaudio

Audio playback and capture library written in C, in a single source file.
https://miniaud.io
Other
4.07k stars 361 forks source link

Fix-ups for macOS #840

Closed barracuda156 closed 6 months ago

barracuda156 commented 7 months ago

This PR fixes a several issues for macOS:

  1. Macro for __ppc64__ is missing.
  2. The header uses macros for macOS versions without including a header which defines them.
  3. MAC_OS_X_VERSION_MIN_REQUIRED should not have a leading __.
  4. Add fallback aliases to unbreak this on < 10.9.
mackron commented 7 months ago

Thanks. Just a few things. OS-specific headers are not allowed in the top section, so that AvailabilityMacros.h include needs to be moved down into the implementation section, probably just above that #include <mach/mach_time.h> part in the "DEVICE I/O" section.

I'm a bit unsure about the MAC_OS_X_VERSION_MIN_REQUIRED double underscore thing, but looking up references online I can't really find many suggestions one way or the other, nor can I remember how I determined the use of that macro in the first place. Are you getting a compilation error with the double underscore version? I don't expect it'll hurt anything using your proposal, but I'd like to understand the issue a bit more.

Happy with the __ppc64__ change.

barracuda156 commented 7 months ago

Thanks. Just a few things. OS-specific headers are not allowed in the top section, so that AvailabilityMacros.h include needs to be moved down into the implementation section, probably just above that #include <mach/mach_time.h> part in the "DEVICE I/O" section.

Sure. It was a bit confusing for me where to place the include, sorry.

I'm a bit unsure about the MAC_OS_X_VERSION_MIN_REQUIRED double underscore thing, but looking up references online I can't really find many suggestions one way or the other, nor can I remember how I determined the use of that macro in the first place. Are you getting a compilation error with the double underscore version? I don't expect it'll hurt anything using your proposal, but I'd like to understand the issue a bit more.

Versions with underscores will not be recognized on older macOS from <AvailabilityMacros.h> (though <Availability.h> uses those, I think). Later macOS will not care, both gonna work. As a real-life example: https://github.com/AcademySoftwareFoundation/openexr/pull/1416 We got failures with old clang-based systems on buildbots, because I tested on powerpc, and powerpc was fine due to another macro taking care of it. The header now uses both, that is not good in any case. If you have some reasons to use underscored ones (I know of none, to be honest), the <Availability.h> might work for those (and make them consistent). Otherwise I would go with <AvailabilityMacros.h>, simply because it is far better tested: we use them across MacPorts.

Happy with the __ppc64__ change.

Great!

mackron commented 7 months ago

Yeah there's a lot going on in miniaudio.h so it can be hard to track where things should be placed. I'm happy with your explanation on the MAC_OS_X_VERSION_MIN_REQUIRED thing and happy to make that change. Indeed, I take pride in my stuff working on older setups, so that suits me nicely (miniaudio compiles with VC6 and works on Windows 95).

Just another quick question. Regarding this part:

/* kAudioDevicePropertyScope* were renamed to kAudioObjectPropertyScope* in 10.8. */
#ifndef MAC_OS_X_VERSION_10_8
#define kAudioObjectPropertyScopeInput kAudioDevicePropertyScopeInput
#define kAudioObjectPropertyScopeOutput kAudioDevicePropertyScopeOutput
#endif

Should the #ifndef MAC_OS_X_VERSION_10_8 part instead be a range check for <= 10.8 instead of checking for just 10.8?

barracuda156 commented 7 months ago

Should the #ifndef MAC_OS_X_VERSION_10_8 part instead be a range check for <= 10.8 instead of checking for just 10.8?

@mackron As of now, it is perhaps equivalent, but you are right, let’s do it as a comparison, that is arguably safer.

barracuda156 commented 7 months ago

@mackron I have moved the include now (is it right?) and changed the condition for < 10.8.

In fact instead of #if !defined(MAC_OS_X_VERSION_10_8) || (MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_8) we could just write #if MAC_OS_X_VERSION_MIN_REQUIRED < 1080, but then we have different style used just above.

mackron commented 6 months ago

Thanks for those updates. All looks good to me. Merged.

barracuda156 commented 6 months ago

Thank you!