Skip to content
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

Introduce a callback queue on a tap #2815

Merged
merged 1 commit into from Jan 30, 2023
Merged

Introduce a callback queue on a tap #2815

merged 1 commit into from Jan 30, 2023

Conversation

jcavar
Copy link
Contributor

@jcavar jcavar commented Jan 8, 2023

Fixes #2814, but it feels clumsy as we have two syncronisation mechanisms - locks and queue.

We should probably rewrite a tap to work on one dispatch queue and remove all the locks.

Fixes AudioKit#2814, but it feels clumsy as we have two syncronisation
mechanisms - locks and queue.

We should probably rewrite a tap to work on one dispatch queue and
remove all the locks.
@@ -18,10 +18,10 @@ open class RawDataTap: BaseTap {
/// - input: Node to analyze
/// - bufferSize: Size of buffer to analyze
/// - handler: Callback to call when results are available
public init(_ input: Node, bufferSize: UInt32 = 1_024, handler: @escaping Handler = { _ in }) {
public init(_ input: Node, bufferSize: UInt32 = 1_024, callbackQueue: DispatchQueue, handler: @escaping Handler = { _ in }) {
Copy link

Choose a reason for hiding this comment

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

Line Length Violation: Line should be 120 characters or less: currently 129 characters (line_length)

@jcavar jcavar mentioned this pull request Jan 8, 2023
@wtholliday
Copy link
Member

Why don’t we call all the tap callbacks on the main queue?

@jcavar
Copy link
Contributor Author

jcavar commented Jan 8, 2023

Hey @wtholliday, I've added a comment on the issue, but copying it here too:

I think we should try to avoid dispatching on the main queue from the framework.

In case of e.g. AmplitudeTap, calculation will be happening on the main queue, which might not be desirable.

If framework does things on the main queue, you have no choice as a consumer. If it doesn’t, and you need it, it is easy to switch to main queue at a call-site.

But, putting that aside, I don’t think this solves it either. You might be doing operations on the tap (stop, start) from another queue (as we do).

@aure aure merged commit 9f6d1ff into AudioKit:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSAN data race
3 participants