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

Fix: prevent AudioBufferSourceNode to restart when loop is set to true #521

Merged
merged 7 commits into from
Sep 5, 2024

Conversation

b-ma
Copy link
Collaborator

@b-ma b-ma commented Sep 4, 2024

This fixes ircam-ismm/node-web-audio-api#135
and maybe ircam-ismm/node-web-audio-api#134 (TBC)

There are new unrelated clippy issues that I didn't fixed, mostly small doc things but one that concerns transmute that I think it is better that you check, let's do that in another PR?

Copy link
Owner

@orottier orottier left a comment

Choose a reason for hiding this comment

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

Cool, looks good. I will apply some fixes for CI and something around the guard

&& (computed_playback_rate > 0. && buffer_time >= buffer_duration
|| computed_playback_rate < 0. && buffer_time < 0.)
{
self.ended = true;
Copy link
Owner

Choose a reason for hiding this comment

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

You could move the send_ended_event() to here right? So the event is spawned directly and not in the next quantum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure we could I think, I just don't remember why we did it this way in the first place :) do you thing it is better?

I think the before_drop stuff would just becomes useless in this case...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the before_drop stuff would just becomes useless in this case...

Hum, no there are still some cases where is it needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok done, allowed to remove the additional onended_triggered flag too

Copy link
Owner

Choose a reason for hiding this comment

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

I just don't remember why we did it this way in the first place :)

I think it was just inconvenient in the process fn. We had multiple return points so the code would have to be duplicated. That is now different.

@orottier
Copy link
Owner

orottier commented Sep 5, 2024

Ah I realize you lag behind the main branch, could you merge it in? CI/clippy was already fixed

@b-ma
Copy link
Collaborator Author

b-ma commented Sep 5, 2024

Ah I realize you lag behind the main branch, could you merge it in? CI/clippy was already fixed

Damn, sure I'll fix that

@orottier orottier merged commit 5429ac7 into orottier:main Sep 5, 2024
3 checks passed
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.

AudioBufferSourceNode not properly ended
2 participants