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

PaAlsa_EnableRealtimeScheduling fix #3160

Merged
merged 5 commits into from
Oct 23, 2020
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 7, 2020

This fixes a crash I have uncounted during my testes with different Portaudio version.
Without this patch, the PaAlsa_EnableRealtimeScheduling writes a random byte in the JACK case and if the PaAlsaStream structure changes.

@uklotzde
Copy link
Contributor

uklotzde commented Oct 8, 2020

What's the precise reason for the crash? Are we passing a structure layout that has been determined at compile time to a dynamically loaded library that might use a different layout internally?

@daschuer
Copy link
Member Author

daschuer commented Oct 9, 2020

Yes, that is one reason, that only happens during development.
The more serious reason is that in case of Jack, Pa casts a PaAlsaStream structure to a PaJackStream structure with different data and length.

@Holzhaus
Copy link
Member

There's a merge conflict. Also, I can try to do a code review but I have no idea how to test this properly.

@daschuer
Copy link
Member Author

@Holzhaus There is not much to test. If it does not crash it is fine.
The PR basically replaces error prone code with missing definition in the portaudio API.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

There is not much to test. If it does not crash it is fine.

It doesn't crash. I can't really judge the code though.

@daschuer
Copy link
Member Author

Do you have questions?

@uklotzde
Copy link
Contributor

I still don't completely understand how this works and why it is safe. Do you have any references or links that we could add to the code?

@daschuer
Copy link
Member Author

I have just realized the the pa_linux_alsa.h is now part of the distribution.
So we have now no hack at all.

Here is the reference link. I don't think there is any need to put it into the code, now we have the header.
http://portaudio.com/docs/v19-doxydocs-dev/pa__linux__alsa_8h.html

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

LGTM, awaiting CI results.

@uklotzde
Copy link
Contributor

Check the CI failures. All seem to be unrelated. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants