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

sf::SoundStream::setPlayingOffset() restarts playing even when paused #203

Closed
Ceylo opened this Issue Apr 11, 2012 · 13 comments

Comments

Projects
None yet
7 participants
@Ceylo
Contributor

Ceylo commented Apr 11, 2012

When the audio stream is stopped, calling setPlayingOffset() makes it (re)start whereas control should be left to the user. The stream should be restarted only if the stream was indeed already playing at the time its offset was changed.

See void SoundStream::setPlayingOffset(Time timeOffset) in SoundStream.cpp :

////////////////////////////////////////////////////////////
void SoundStream::setPlayingOffset(Time timeOffset)
{
    // Stop the stream
    stop();

    // Let the derived class update the current position
    onSeek(timeOffset);

    // Restart streaming
    m_samplesProcessed = static_cast<Uint64>(timeOffset.asSeconds() * m_sampleRate * m_channelCount);
    m_isStreaming = true;
    m_thread.launch();
}

@ghost ghost assigned LaurentGomila Apr 11, 2012

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 3, 2013

Contributor

I tried to find a solution for this issue, but it turns out, that the fix is not as trivial as it looks a first. It turns out some methods probably have to be rewritten. For more information see this forum thread: http://en.sfml-dev.org/forums/index.php?topic=10168

Contributor

Foaly commented Jan 3, 2013

I tried to find a solution for this issue, but it turns out, that the fix is not as trivial as it looks a first. It turns out some methods probably have to be rewritten. For more information see this forum thread: http://en.sfml-dev.org/forums/index.php?topic=10168

binary1248 added a commit that referenced this issue May 1, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203).

Signed-off-by: binary1248 <binary1248@hotmail.com>

binary1248 added a commit that referenced this issue May 1, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203).

Signed-off-by: binary1248 <binary1248@hotmail.com>
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

Hmm... it seems like GitHub doesn't want to link the author field with Foaly's account. I wanted to attribute the patch to Foaly, since it was adapted from the implementation provided on the forum. Foaly, if you could provide me with the required information, I can amend the commit with the proper information to get it linked :).

Member

binary1248 commented May 1, 2014

Hmm... it seems like GitHub doesn't want to link the author field with Foaly's account. I wanted to attribute the patch to Foaly, since it was adapted from the implementation provided on the forum. Foaly, if you could provide me with the required information, I can amend the commit with the proper information to get it linked :).

binary1248 added a commit that referenced this issue May 1, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203). Signed-off-by: binary1248 <binary1248@hotmail.com>

binary1248 added a commit that referenced this issue May 1, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203).

Signed-off-by: binary1248 <binary1248@hotmail.com>
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

Nevermind... managed to get it linked after all. :)

Member

binary1248 commented May 1, 2014

Nevermind... managed to get it linked after all. :)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 1, 2014

Member

The fix looks complicated. What about this? 94daf9e

Member

TankOs commented May 1, 2014

The fix looks complicated. What about this? 94daf9e

@Ceylo

This comment has been minimized.

Show comment
Hide comment
@Ceylo

Ceylo May 1, 2014

Contributor

TankOs, as Foaly stated in his thread, doing that kind of fix doesn't allow resuming playback.
As for the committed fix, why is the mutex necessary? I mean… a boolean is either true or false, it can't be in any intermediate state. So even in case of race condition you either get the old value (no problem, will loop once more) or the new one. Am I missing something here?

Contributor

Ceylo commented May 1, 2014

TankOs, as Foaly stated in his thread, doing that kind of fix doesn't allow resuming playback.
As for the committed fix, why is the mutex necessary? I mean… a boolean is either true or false, it can't be in any intermediate state. So even in case of race condition you either get the old value (no problem, will loop once more) or the new one. Am I missing something here?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

If there is a race condition, the main thread will go ahead and try to stop/pause before the secondary thread starts, then the sound will continue playing, although that was not intended.

Also, there is cache coherency ;). Without mutexes you have no idea whether the variable has been completely committed to RAM or not. It might switch back and forth a few times before stabilizing. And since the loop waits for the first time the bool becomes true, it might break before the secondary is actually ready.

There is a reason why std::atomic_bool exists ;).

Member

binary1248 commented May 1, 2014

If there is a race condition, the main thread will go ahead and try to stop/pause before the secondary thread starts, then the sound will continue playing, although that was not intended.

Also, there is cache coherency ;). Without mutexes you have no idea whether the variable has been completely committed to RAM or not. It might switch back and forth a few times before stabilizing. And since the loop waits for the first time the bool becomes true, it might break before the secondary is actually ready.

There is a reason why std::atomic_bool exists ;).

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly May 1, 2014

Contributor

Awesome. I am glad to see that this ticket is getting some attention and that my investigation is of some use :)
@binary1248: What information do you need? Name, URL...?

Contributor

Foaly commented May 1, 2014

Awesome. I am glad to see that this ticket is getting some attention and that my investigation is of some use :)
@binary1248: What information do you need? Name, URL...?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

@Foaly: Nevermind, I tried to get your commit info from your previous commits, there were a few where your E-Mail address had a typo in it (wee.de instead of web.de) and GitHub didn't like it, then I tried the @beuth-hochschule.de one and GitHub didn't like it either. But now I am using @web.de and it seems like GitHub is linking it properly. :)

Member

binary1248 commented May 1, 2014

@Foaly: Nevermind, I tried to get your commit info from your previous commits, there were a few where your E-Mail address had a typo in it (wee.de instead of web.de) and GitHub didn't like it, then I tried the @beuth-hochschule.de one and GitHub didn't like it either. But now I am using @web.de and it seems like GitHub is linking it properly. :)

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly May 1, 2014

Contributor

ok! i just verifyed the @beuth-hochschule.de one for github. But take whatever you want :)

Contributor

Foaly commented May 1, 2014

ok! i just verifyed the @beuth-hochschule.de one for github. But take whatever you want :)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

But from what I understand, GitHub requires the name associated with that E-Mail address to be associated with your account as well, i.e. Maximilian Wagenbach. Maybe it just takes time for their cache to update or something.

You can see what it looks like in this commit: 4c6a8b9

Member

binary1248 commented May 1, 2014

But from what I understand, GitHub requires the name associated with that E-Mail address to be associated with your account as well, i.e. Maximilian Wagenbach. Maybe it just takes time for their cache to update or something.

You can see what it looks like in this commit: 4c6a8b9

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly May 1, 2014

Contributor

Like i said I don't really care what name is displayed. Thanks for the attribution anyway!

Contributor

Foaly commented May 1, 2014

Like i said I don't really care what name is displayed. Thanks for the attribution anyway!

TankOs added a commit that referenced this issue May 6, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203).

Signed-off-by: binary1248 <binary1248@hotmail.com>

binary1248 added a commit that referenced this issue May 25, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203), guard m_isStreaming by a mutex, minor documentation fix in SoundStream.

Signed-off-by: binary1248 <binary1248@hotmail.com>

binary1248 added a commit that referenced this issue May 29, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203), guard m_isStreaming by a mutex, fixed calling SoundStream::pause() before the stream thread starts not properly pausing the stream (http://en.sfml-dev.org/forums/index.php?topic=15197.0), minor documentation fix in SoundStream. Signed-off-by: binary1248 <binary1248@hotmail.com>

@Bromeon Bromeon added the s:accepted label Jun 6, 2014

binary1248 added a commit that referenced this issue Jul 4, 2014

Fixed calling SoundStream::setPlayingOffset() unpausing a paused Soun…
…dStream (#203), guard m_isStreaming by a mutex, fixed calling SoundStream::pause() before the stream thread starts not properly pausing the stream (http://en.sfml-dev.org/forums/index.php?topic=15197.0), minor documentation fix in SoundStream. Signed-off-by: binary1248 <binary1248@hotmail.com>
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 7, 2014

Member

Fixed in e625d79

Member

eXpl0it3r commented Jul 7, 2014

Fixed in e625d79

@eXpl0it3r eXpl0it3r closed this Jul 7, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.2, 2.x Jul 7, 2014

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 7, 2014

Contributor

Awesome! Good to see this being fixed! Thanks everybody :)

Contributor

Foaly commented Jul 7, 2014

Awesome! Good to see this being fixed! Thanks everybody :)

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