-
-
Notifications
You must be signed in to change notification settings - Fork 52
fix: stop() takes too much time #319
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
Conversation
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.
Hey Marco, thanks for the quick turnaround! I have a few questions below.
if (player.get() == nullptr || !player.get()->isInited() || | ||
!player.get()->isValidHandle(handle)) |
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.
Wait, why are we no longer checking whether the handle is valid? Does Player::stop()
not care?
Oh wait, we checked whether the handle was not valid? And then we stopped it? Sorry, I'm getting confused.
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.
Now the handle validity is managed by the voiceEndedCallbak
. Whether or not it is valid, the callback is called and the SoLoud engine does nothing if the handle doesn't exist.
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.
Thanks for the explanation!
|
||
void Player::stop(unsigned int handle) | ||
{ | ||
removeHandle(handle); |
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.
[nit] I guess I'd like to see it documented (in comments in this file, ideally) who is responsible for removing the handle (e.g. calling Player::removeHandle()
).
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.
Removing the handle here is now an error because it is now the voiceEndedCallck
duty. I have written comments in that callback.
Hey Filip, sorry for the late. I agree, I'm stingy with comments. I have added some in the callback. If some more are needed, please let me know. |
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.
LGTM, thanks for adding the additional comments!
if (player.get() == nullptr || !player.get()->isInited() || | ||
!player.get()->isValidHandle(handle)) |
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.
Thanks for the explanation!
Description
As discussed in #312, calling
stop
immediately after callingplay
, produces the warning.2 problems were causing the issue:
play()
doesn't have the time to produce a handle before thestop()
is called.stop()
call after another event likescheduleStop
or a sound ended that already stopped the handle, ie:This fixes both problems.
Type of Change