-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for targeting iOS #65
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes on the implementation
pm_mac/pmmacosxcm.c
Outdated
UInt64 current_host_time(void) | ||
{ | ||
#if TARGET_OS_OSX | ||
return AudioGetCurrentHostTime(); | ||
#else | ||
return mach_absolute_time(); | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mach methods are available on macOS too, so we could probably drop the macOS-specific AudioGetCurrentHostTime
, AudioGetHostClockFrequency
etc. I've kept them around under a TARGET_OS_OSX
conditional for now to avoid accidentally introducing a regression if there's a bug in the (albeit quite short) mach-based implementation for iOS. Since it introduces an additional, likely not very heavily used code path, I would also be open to removing the macOS branch again. Thoughts welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some reason to choose mach_absolute_time()?
It seems to be the officially recommended drop-in replacement: https://developer.apple.com/library/archive/qa/qa1643/_index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clock_gettime_nsec_np(CLOCK_UPTIME_RAW)
seems to be a wrapper around mach_absolute_time
:
So it's probably a question of style which one to choose? mach_absolute_time
seems pretty widely used, so I'm sure they won't deprecate that anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, because documentation for mach_absolute_time says use clock_gettime_nsec_np(CLOCK_UPTIME_RAW) (!). But OK, if Apple can't decide, I'll go with your choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link, I must have missed that note. Interesting that they mention fingerprinting. But regardless, mach_absolute_time
is the lower level API and almost all projects I've seen that use Core Audio/Core MIDI on iOS went with mach_absolute_time
too.
#include <CoreAudio/HostTime.h> | ||
#else | ||
#include <mach/mach_time.h> | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be doing what PortTime was intended to do: provide a system-independent way to access time functions.
There are actually 2 porttime implementations for Mac: ptmacosx_cf.c based on core foundation calls and ptmacosx_mach.c based on Mach calls, but it looks like ptmacosx_mach.c still uses core foundation calls for time functions.
I'd much prefer to add system-independent functions in ptmacosx_mach.c and porttime.h than here for getting and translating nanos to portmidi (milliseconds).
A critical question though, is have you established that the nanos you get from Mach calls are the same nanos you get from CoreAudio? It's important because CoreMidi uses CoreAudio timestamps (undocumented but they match in tests) and it won't work to use just any nanosecond timer. I have no idea if mach gets the same nano values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hence #65 (comment):
Ideally, we should probably share these utility methods between
porttime
andpm_mac
. The right place for that would likely beporttime
, but we'd also create a leaky abstraction by exposing these implementation details, even if only to the (also platform-specific)pm_mac
. Ideas welcome.
The question would be how pmmacosxcm.c
would consume those functions (or whether there's a way to model these conversions in a fully platform-agnostic way, so they can be part of the main porttime interface. Haven't dug deep enough into porttime to understand the architectural decisions there though).
A critical question though, is have you established that the nanos you get from Mach calls are the same nanos you get from CoreAudio? It's important because CoreMidi uses CoreAudio timestamps (undocumented but they match in tests) and it won't work to use just any nanosecond timer. I have no idea if mach gets the same nano values.
That would indeed be interesting to check, I haven't done so yet. I would be surprised though if they would differ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your additional functions to get host time and convert host time to micros and nanos is fine. At this point, only OSX and IOS would use them, and I think it's fine to make them Apple-only functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working to integrate a lot of changes into PortMidi. Returning to this, the whole reason for incorporating CoreAudio time into PortMidi was that the absolute time values are important because they are used as Midi timestamps by MacOS (I don't think this is documented). I believe many PortMidi tests would work fine with bad timestamps, so it's important to look at some MIDI packets in iOS and compare their timestamps to time sources to see what time source is being used (or find it in documentation if it is stated anywhere). Similarly, output timing should be tested to see if absolute timestamps based on mach_absolute_time() or whatever can be used to produce correctly timed output. If you can verify the time source is correct (at least usable for timing and seems correct), then this would be a nice addition to PortMidi.
pm_mac/pmmacosxcm.c
Outdated
static UInt64 current_host_time(void); | ||
static UInt64 nanos_to_host_time(UInt64 nanos); | ||
static UInt64 host_time_to_nanos(UInt64 host_time); | ||
static UInt64 micros_per_host_tick(void); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
portmidi may be overoptimized to use host_time when possible rather than always using nanos (which would give a simpler, smaller set of time functions), but OK, let's extend porttime. These additional functions can exist for apple only (since they are not called from other platforms). Names should be, e.g. Pt_Current_Host_Time -- now I kind of hate this style, but it was originally supposed to be consistent with PortAudio, and better to be consistent that not (or to break everything by changing naming conventions now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names should be, e.g. Pt_Current_Host_Time
Pt_CurrentHostTime
? That would be consistent with the PortMidi naming style:
Line 184 in 928520f
PMEXPORT int Pm_HasHostError(PortMidiStream * stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved these functions to a new header, ptmacosx.h, since they are Apple-specific and so they don't leak into the porttime interface on other platforms:
Lines 9 to 15 in 15ddc24
UInt64 Pt_CurrentHostTime(void); | |
UInt64 Pt_NanosToHostTime(UInt64 nanos); | |
UInt64 Pt_HostTimeToNanos(UInt64 host_time); | |
UInt64 Pt_MicrosPerHostTick(void); |
i.e. replace UInt32 -> uint32_t SInt32 -> int32_t UInt64 -> uint64_t SInt64 -> int64_t
The patches are based on PortMidi/portmidi#65
The patches are based on PortMidi/portmidi#65
Since iOS is largely based on the same frameworks as macOS, the changes required to support iOS are minor. It does, however, lack some time-related methods, most notably
CoreAudio/HostTime.h
and the suggested approach appears to be using mach time instead: https://developer.apple.com/library/archive/qa/qa1643/_index.htmlWith this PR
portmidi
can now be built for iOS simply by changing theCMAKE_SYSTEM_NAME
: