Skip to content

[AudioToolbox] Add missing MusicSequence.SetUserCallback method #48

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.

Sign up for GitHub

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

Merged
spouliot merged 4 commits into from
Jun 9, 2016
Merged

[AudioToolbox] Add missing MusicSequence.SetUserCallback method #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a832718
[AudioToolbox] Add missing MusicSequence.SetUserCallback method
olegoid May 12, 2016
c2da173
[AudioToolbox] MusicEventUserData internal .ctor refactoring
olegoid May 17, 2016
aa775e7
[AudioToolbox] Correct callbacks storage
olegoid May 18, 2016
8b393b3
[AudioToolbox] Remove deprecated checks and fix code style issues
olegoid May 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/AudioToolbox/MusicSequence.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
//
// Copyright 2012-2014 Xamarin Inc.
//
// MISSING:
// MusicSequenceSetUserCallback
//

#if IOS || TVOS

Expand All @@ -30,6 +27,12 @@

namespace XamCore.AudioToolbox {

#if !COREBUILD
public delegate void MusicSequenceUserCallback (MusicTrack track, double inEventTime, MusicEventUserData inEventData, double inStartSliceBeat, double inEndSliceBeat);

delegate void MusicSequenceUserCallbackProxy (/* void * */ IntPtr inClientData, /* MusicSequence* */ IntPtr inSequence, /* MusicTrack* */ IntPtr inTrack, /* MusicTimeStamp */ double inEventTime, /* MusicEventUserData* */ IntPtr inEventData, /* MusicTimeStamp */ double inStartSliceBeat, /* MusicTimeStamp */ double inEndSliceBeat);
#endif

// MusicPlayer.h
public class MusicSequence : INativeObject
#if !COREBUILD
Expand All @@ -42,6 +45,10 @@ internal MusicSequence (IntPtr handle) {
this.handle = handle;
}

static Dictionary <IntPtr, MusicSequenceUserCallback> userCallbackHandles = new Dictionary <IntPtr, MusicSequenceUserCallback> ();

static MusicSequenceUserCallbackProxy userCallbackProxy = new MusicSequenceUserCallbackProxy (UserCallbackProxy);

[DllImport (Constants.AudioToolboxLibrary)]
extern static /* OSStatus */ MusicPlayerStatus NewMusicSequence (/* MusicSequence* */ out IntPtr outSequence);

Expand Down Expand Up @@ -73,6 +80,13 @@ public IntPtr Handle {
protected virtual void Dispose (bool disposing)
{
if (handle != IntPtr.Zero){

lock (userCallbackHandles)
userCallbackHandles.Remove (handle);

// Remove native user callback
MusicSequenceSetUserCallback (handle, null, IntPtr.Zero);

DisposeMusicSequence (handle);
lock (sequenceMap){
sequenceMap.Remove (handle);
Expand Down Expand Up @@ -274,6 +288,35 @@ public double GetBeatsForSeconds (double seconds)
return beats;
return 0;
}

[DllImport (Constants.AudioToolboxLibrary)]
extern static /* OSStatus */ MusicPlayerStatus MusicSequenceSetUserCallback (/* MusicSequence */ IntPtr inSequence, MusicSequenceUserCallbackProxy inCallback, /* void * */ IntPtr inClientData);

public void SetUserCallback (MusicSequenceUserCallback callback)
{
lock (userCallbackHandles)
userCallbackHandles [handle] = callback;

MusicSequenceSetUserCallback (handle, userCallbackProxy, IntPtr.Zero);
}

#if !MONOMAC
[MonoPInvokeCallback (typeof (MusicSequenceUserCallbackProxy))]
#endif
static void UserCallbackProxy (IntPtr inClientData, IntPtr inSequence, IntPtr inTrack, double inEventTime, IntPtr inEventData, double inStartSliceBeat, double inEndSliceBeat)
{
MusicSequenceUserCallback userCallback;
lock (userCallbackHandles)
userCallbackHandles.TryGetValue (inSequence, out userCallback);

if (userCallback != null) {
var userEventData = new MusicEventUserData (inEventData);
var musicSequence = MusicSequence.Lookup (inSequence);
var musicTrack = new MusicTrack (musicSequence, inTrack, false);

userCallback (musicTrack, inEventTime, userEventData, inStartSliceBeat, inEndSliceBeat);

@spouliot spouliot May 18, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we know (sample?) how much time the callback can be called ? because this allocates memory each time it's called (and let the GC deal with it) so if it's called a lot, under a short amount of time, you could run out of memory and/or and less predictable GC pauses.

In such case using Runtime.GetINativeObject could help, as it would reuse an existing instance (e.g. MusicTrack) but maybe it's better to always dispose after the call.

@rolfbjarne rolfbjarne May 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with disposing after the call is if the callback keeps the objects around.

BTW GetINativeObject will always return a new object unless it's an NSObject subclass.

@spouliot spouliot May 18, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the contract is unclear, even on the native side :-( so even undisposed it's not clear if it's correct to hang on the values (and that API not refcounted afaik). The only clear thing is that we're allocating memory for some things that might never be used (by the callback) and that it will have some, unknown effect.

Instead of having a callback with several parameters, some costly and some likely not used, we could expose a different API with a single parameter/type that would allow getting the MusicTrack / MusicEventUserData from properties (or methods) and lazily allocate memory only when needed... that makes the creation (and if needed the dispose) part of the callback code.

@olegoid we really need a sample for this, minimally showing the rate of calls. Im fairly sure MusicSequenceSetUserCallback was not implemented originally (along with the rest of the API) because the "how it's being used" was unanswered.

@olegoid olegoid May 20, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spouliot @rolfbjarne please take a look at MIDIPlayer sample.

To invoke callback user of this API have to set events to MusicTrack and provide timestamp(when this event should be triggered)1.

Theoretically user can set multiple events using the same timestamp. But most likely user events will be used to update UI(for instance draw sound waves while music track is playing).

In sample mentioned above user events are triggered once per second.

@rolfbjarne rolfbjarne Jun 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the callback gets any big chunks of memory, so I don't think there will be any memory issues. From what I can see these native objects aren't ref counted, and they're not owned by the managed wrapper either, which means native code can free them at any time (so the conclusion is that I don't think it's much better to bundle the callback parameters in a CallbackArgs class which only creates the managed wrappers upon request).

}
}

[DllImport (Constants.AudioToolboxLibrary)]
extern static /* OSStatus */ MusicPlayerStatus MusicSequenceBeatsToBarBeatTime (/* MusicSequence */ IntPtr inSequence, /* MusicTimeStamp */ double inBeats, /* UInt32 */ int inSubbeatDivisor, out CABarBeatTime outBarBeatTime);
Expand Down
14 changes: 14 additions & 0 deletions src/AudioToolbox/MusicTrack.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,20 @@ internal override IntPtr ToUnmanaged ()

public class MusicEventUserData : MidiRawData {
public MusicEventUserData () {}

internal MusicEventUserData (IntPtr handle)

@rolfbjarne rolfbjarne May 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to check handle for IntPtr.Zero before trying to read from it (and throw an ArgumentNullException).

{
if (handle == IntPtr.Zero)
throw new ArgumentNullException (nameof (handle));

int length = Marshal.ReadInt32 (handle);

var buffer = new byte [length];
Marshal.Copy (handle + 4, buffer, 0, length);

len = length;
data = buffer;
}
}

//
Expand Down