-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement optional sample-accurate event processing #78
Implement optional sample-accurate event processing #78
Conversation
23f932f
to
9bb6a09
Compare
Cool, I think this should be ready now! At the moment I've tested with BYOD and Monique, since they're not implementing |
: (numSamples - n); | ||
} | ||
}; | ||
|
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.
So this is going to make some very odd sized blocks. I wonder if we want to quantize somehow. While most juce plugins will handle blocks of size 3 or whatever, its a bit antisocial it seems. For surge effects it will result in us having to always run with latency on for instance, even with systems which give us %32 sized and fixed blocks.
While this absolutely accurate mode makes sense I do wonder if you want to allow a preferred or best efforts subblock floor.
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.
That's a good point... how about this instead:
auto getSamplesToProcess = [&]() {
if (CLAP_EVENT_RESOLUTION_SAMPLES <= 0)
{
// Sample-accurate events are turned off, so just process the
// whole block.
return numSamples;
}
else
{
// the number of samples left is less than CLAP_EVENT_RESOLUTION_SAMPLES
// so let's just process the rest of the block
if (numSamples - n < CLAP_EVENT_RESOLUTION_SAMPLES)
return numSamples - n;
// process up until the next event, rounding up to the nearest multiple
// of CLAP_EVENT_RESOLUTION_SAMPLES
return ((nextEventTime - n) + CLAP_EVENT_RESOLUTION_SAMPLES - 1) / CLAP_EVENT_RESOLUTION_SAMPLES;
}
};
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.
So we want to be careful. A mid block midi event for instance is sent with a time stamp into process so this will break note streams pointlessly - we definitely have to have some event types which don’t split us. Midi and note on and the like.
I also kinda was thinking - we just always do 32 no matter what (or multiples thereof) might be useful mode.
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.
Gotcha... so basically, we don't want to split a block for an event, if the event is of type CLAP_EVENT_NOTE_ON
, CLAP_EVENT_NOTE_OFF
, or CLAP_EVENT_MIDI
? Are there any event types I'm missing there (maybe note expression events?).
I don't love the idea of always splitting the block, since it seems wasteful for situations where there is no parameter automation, or where the host event resolution is coarser than the plugin event resolution, but I guess it couldn't hurt to have as an option.
…esolution, as long as the block size provided by the host cooperates
eb0949c
to
acd23c3
Compare
So there's now an option to always split the block if the user wants. if we're not in that mode, then the block will only get split for parameter events. Are there any other types of events that we might want to split the block for? The block-splitting code seems to be getting a little ugly, so I'll take a pass at cleaning it up a little later (suggestions are always appreciated in the meantime :) ). |
I don't know if it's useful, but you may want to take a look at my block splitting implementation: https://github.com/robbert-vdh/nih-plug/blob/master/src/wrapper/clap/wrapper.rs#L1659 (starting from there) The only thing missing there that's on my list of things to add once it becomes necessary is a plugin-configurable lower limit for the minimum block size to prevent the performance from tanking when this would otherwise cause a bunch of single sample splits. |
I’ll take a look tonight on my iPad - sorry for delay! |
Yeah, this is very helpful. At the moment the logic looks pretty similar, except that:
|
I don't think any host will be sending parameter gesture events to you. That's a hint for the plugin to the host so it can group automation for undo events. And you can't make any assumptions about what events in other namespaces are doing. If an event in another namespace uses the same ID as |
Ah yeah, I was forgetting that Thanks again, having your implementation as a reference is definitely helpful! |
… the core namespace
WIP pull request for #76.
So far I've only tested with some toy examples, so I'm leaving this marked as WIP until it's been tested with a few more plugins, but I figured it would be good to get some eyes on it, and ask a couple questions.