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

AudioStream: Add destructor. Enables dynamic creation and destruction. #755

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chipaudette
Copy link

@chipaudette chipaudette commented Sep 9, 2024

There's been great work on dynamic creation and destruction of AudioConnection instances. But, one has not been able to dynamically destroy AudioStream instances. This pull request seeks to remove this limitation.

In my view, the primary issue preventing dynamic destruction of AudioStream instancs is that the user had no way of removing an AudioStream instance from the AudioStream update list. You could go ahead and delete your instance, but the update list would still try to use that stale pointer. That would cause the system to hang.

This pull request adds a destructor to AudioStream. Now, when an AudioStream instance is destroyed, it removes the instance from the AudioStream update list.

The user must still remember to close out all of the AudioConnections that use the destroyed AudioStream instance, but that's at least possible for the user to do. Without AudioStream having a destructor, there is no way for the user to remove the instance to the update list.

This pull request includes both Teensy3 and Teensy4. It has been tested on a Teensy 4.1 and a Teensy 3.6.


//release any audio blocks held in the input queue for this instance of AudioStream
for (int i=0; i<num_inputs; i++) {
audio_block_t *block = inputQueue[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work - the inputQueue[] array is part of the derived class, and has already been destroyed by the time this code executes


//release any audio blocks held in the input queue for this instance of AudioStream
for (int i=0; i<num_inputs; i++) {
audio_block_t *block = inputQueue[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work - the inputQueue[] array is part of the derived class, and has already been destroyed by the time this code executes

Copy link
Author

@chipaudette chipaudette Sep 9, 2024

Choose a reason for hiding this comment

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

Thanks for pointing this out. It's been removed from this minimal pull request and replaced with a comment about the need for derived classes to release their inputQueues.

Since no existing AudioStream classes in the Teensy Audio library do this, the system will likely see an orphaning of AudioMemory block(s) with every destruction of an AudioStream instance from the Teensy Audio library. This is unfortunate and should be addressed through subsequent pull requests.

As a user of the Teensy Audio library, however, it is at least possible (within the rules of C++ and within my skillset) to modify my AudioStream derived classes to release their inputQueues. It is currently NOT possible to get my instances out of AudioStream's update list. I'm completely frozen out. So, I still request that we include this minimal destructor as it lets me move forward.

@A-Dunstan
Copy link
Contributor

I think the list removal has a couple of errors:

  • it only ever checks if next_update == this, so the stream pointed to by first_update can never be unlinked
  • since any stream instance will only be in the list once, there should be a break statement to terminate the loop once it's found/unlinked

@chipaudette
Copy link
Author

I believe that I have now addressed the last two bullets ("first_update" and "break"). All of the latest edits have been tested on Teensy 4.1 and Teensy 3.6.

@chipaudette
Copy link
Author

chipaudette commented Oct 25, 2024

I'm plugging for this Pull Request again. Presuming that one trusts Bjarne Stroustrup and Herb Sutter, their ISO C++ guidelines suggest that a base class destructor should be public and virtual (or protected and non-virtual).

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual

AudioStream has no destructor (well, it has the default one), which means that it's not virtual. This pull request is giving AudioStream an explicit destructor and this pull request makes it virtual. This is in line with the ISO C++ guidance.

Any chance it could be accepted?

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.

3 participants