openAL error when calling sf::SoundBuffer::loadFrom*() more than once #354

Closed
Ancurio opened this Issue Feb 16, 2013 · 18 comments

Comments

Projects
None yet
7 participants
@Ancurio

Ancurio commented Feb 16, 2013

Minimal example:

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

int main(int, char**)
{
    sf::SoundBuffer buffer;
    buffer.loadFromFile("sound1.ogg");

    sf::Sound sound(buffer);
    sound.play();

    sf::sleep(sf::seconds(5));

    buffer.loadFromFile("sound2.ogg");
    sound.setBuffer(buffer);
    sound.play();

    sf::sleep(sf::seconds(5));

    return 0;
}

"sound1.ogg" is played the second time, instead of "sound2.ogg". Apparently the openAL error is

An internal OpenAL call failed in SoundBuffer.cpp (253) : AL_INVALID_VALUE, a numeric argument is out of range

@ghost ghost assigned LaurentGomila Feb 16, 2013

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 16, 2013

Member

The discussion on the forum can be found here.

Member

eXpl0it3r commented Feb 16, 2013

The discussion on the forum can be found here.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 12, 2013

Contributor

I did a quick fix, works but really sloppy.

kd7tck@3efea48
Cherry pick it if you want to try it out.

Contributor

kd7tck commented Mar 12, 2013

I did a quick fix, works but really sloppy.

kd7tck@3efea48
Cherry pick it if you want to try it out.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 13, 2013

Contributor

Here is a newer diff file, hurry on it. Mega tends to delete things after a couple weeks.

https://mega.co.nz/#!uYBU2Q7C!eUmZuwAjmgZqMkRqcXSH6gk0FEA7PhetCEepWy8I0dc

I also updated my commits with a correction.

Contributor

kd7tck commented Mar 13, 2013

Here is a newer diff file, hurry on it. Mega tends to delete things after a couple weeks.

https://mega.co.nz/#!uYBU2Q7C!eUmZuwAjmgZqMkRqcXSH6gk0FEA7PhetCEepWy8I0dc

I also updated my commits with a correction.

@Ancurio

This comment has been minimized.

Show comment
Hide comment
@Ancurio

Ancurio Mar 13, 2013

Mega tends to delete things after a couple weeks.

Why not just use pastebin?

Ancurio commented Mar 13, 2013

Mega tends to delete things after a couple weeks.

Why not just use pastebin?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 13, 2013

Member

Why not just use pastebin?

Or github's gists...

Member

LaurentGomila commented Mar 13, 2013

Why not just use pastebin?

Or github's gists...

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 13, 2013

Contributor

This will be easier to access.

edit.
kd7tck@6e8f6ba

kd7tck@71c62cf

Contributor

kd7tck commented Mar 13, 2013

This will be easier to access.

edit.
kd7tck@6e8f6ba

kd7tck@71c62cf

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 13, 2013

Contributor

Any more Ideas on how I should clean the patch up further?

Contributor

kd7tck commented Mar 13, 2013

Any more Ideas on how I should clean the patch up further?

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 14, 2013

Contributor

Hmm... Does this issue only happen with loadFromFile() ? Because if it also happens with the other load methods you should put your code in the initialize() method. I think you have to do some more investigation/testing.

Edit: I just saw that the loadFromSamples() method doesn't call initialize() , so if loadFromSamples() has the same error you might want to put your code in a seperate method (maybe something like unload()). Either way you should test a little more and see what you find out.

Contributor

Foaly commented Mar 14, 2013

Hmm... Does this issue only happen with loadFromFile() ? Because if it also happens with the other load methods you should put your code in the initialize() method. I think you have to do some more investigation/testing.

Edit: I just saw that the loadFromSamples() method doesn't call initialize() , so if loadFromSamples() has the same error you might want to put your code in a seperate method (maybe something like unload()). Either way you should test a little more and see what you find out.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 14, 2013

Contributor

I did a quick modification and test, seems to work. Look over and tell me if changes will fit.

Contributor

kd7tck commented Mar 14, 2013

I did a quick modification and test, seems to work. Look over and tell me if changes will fit.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 14, 2013

Contributor

This looks better. But I find the return type confusing, because it says if it returns false, the function failed to unload the buffer, but it actually means there is nothing to unload. Also I find unload more clear, because you are not reloading anything, you are reseting and clearing the buffer.
What about loadFromSamples() have you tested if the problem also persists there? Also have you actually done some testing with the other load functions before and after your fix?
We'll see what Laurent says.

Contributor

Foaly commented Mar 14, 2013

This looks better. But I find the return type confusing, because it says if it returns false, the function failed to unload the buffer, but it actually means there is nothing to unload. Also I find unload more clear, because you are not reloading anything, you are reseting and clearing the buffer.
What about loadFromSamples() have you tested if the problem also persists there? Also have you actually done some testing with the other load functions before and after your fix?
We'll see what Laurent says.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 15, 2013

Contributor

There are two audio patch branches each with a different approach. The second audio_patch is the one I prefer, but does not work with loadFromSamples(). In that case you would need the first patch, which can easily be applied to loadFromSamples() if you wanted. I did tests with all load functions, turns out they all have this bug including the loadFromSamples().

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

int main(int, char**)
{
    sf::SoundBuffer buffer, buffer2;
    buffer.loadFromFile("cars001.wav");
    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());

    sf::Sound sound;
    sound.setBuffer(buffer2);
    sound.play();

    sf::sleep(sf::seconds(5));
    sound.stop();
    buffer.loadFromFile("cars002.wav");
    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());

    sound.setBuffer(buffer2);
    sound.play();

    sf::sleep(sf::seconds(5));

    return 0;
}

I will reword the comment in the next commit.

Contributor

kd7tck commented Mar 15, 2013

There are two audio patch branches each with a different approach. The second audio_patch is the one I prefer, but does not work with loadFromSamples(). In that case you would need the first patch, which can easily be applied to loadFromSamples() if you wanted. I did tests with all load functions, turns out they all have this bug including the loadFromSamples().

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

int main(int, char**)
{
    sf::SoundBuffer buffer, buffer2;
    buffer.loadFromFile("cars001.wav");
    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());

    sf::Sound sound;
    sound.setBuffer(buffer2);
    sound.play();

    sf::sleep(sf::seconds(5));
    sound.stop();
    buffer.loadFromFile("cars002.wav");
    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());

    sound.setBuffer(buffer2);
    sound.play();

    sf::sleep(sf::seconds(5));

    return 0;
}

I will reword the comment in the next commit.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 15, 2013

Contributor

I like the second commit better too. But there is one thing I don't get. If you put a reload(); before this line, does the problem still persist? Because the fix will obviously not be applied if it only works for 3 out of 4 load methods.
Also I still think that unload() would be a more discribtive name, but Laurent will have to decide that.
Once you have done all the changes, I would make one clean pull request, so Laurent can merge it easily.

Contributor

Foaly commented Mar 15, 2013

I like the second commit better too. But there is one thing I don't get. If you put a reload(); before this line, does the problem still persist? Because the fix will obviously not be applied if it only works for 3 out of 4 load methods.
Also I still think that unload() would be a more discribtive name, but Laurent will have to decide that.
Once you have done all the changes, I would make one clean pull request, so Laurent can merge it easily.

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Mar 15, 2013

Contributor

Foaly commented:

But I find the return type confusing, because the function failed to unload the buffer, but it actually means there is nothing to unload. Also I find unload more clear, because you are not reloading anything, you are reseting and clearing the buffer.

That's a good point. But why does the reload method returns a boolean in the first place? The returned value is never used, so void may be more suitable (and less confusing).

Contributor

abodelot commented Mar 15, 2013

Foaly commented:

But I find the return type confusing, because the function failed to unload the buffer, but it actually means there is nothing to unload. Also I find unload more clear, because you are not reloading anything, you are reseting and clearing the buffer.

That's a good point. But why does the reload method returns a boolean in the first place? The returned value is never used, so void may be more suitable (and less confusing).

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 15, 2013

Contributor

Noted.

Contributor

kd7tck commented Mar 15, 2013

Noted.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 15, 2013

Contributor

@Foaly, If reload is placed behind loadFromSamples() it works.

Contributor

kd7tck commented Mar 15, 2013

@Foaly, If reload is placed behind loadFromSamples() it works.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck Mar 15, 2013

Contributor

Here is the corrected first patch

kd7tck@82c621f

Contributor

kd7tck commented Mar 15, 2013

Here is the corrected first patch

kd7tck@82c621f

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

Fixed soundbuffer contents not being able to be updated when still at…
…tached to sounds (#354).

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

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 1, 2014

Member

I rewrote the submitted patch to not require any changes to the public API. kd7tck is attributed as the author since it was his work and initial implementation.

Member

binary1248 commented May 1, 2014

I rewrote the submitted patch to not require any changes to the public API. kd7tck is attributed as the author since it was his work and initial implementation.

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

Fixed soundbuffer contents not being able to be updated when still at…
…tached to sounds (#354), sounds now detach from their buffer when it is reset. Signed-off-by: binary1248 <binary1248@hotmail.com>

eXpl0it3r added a commit that referenced this issue May 26, 2014

Fixed soundbuffer contents not being able to be updated when still at…
…tached to sounds (#354), sounds now detach from their buffer when it is reset. Signed-off-by: binary1248 <binary1248@hotmail.com>
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

Issue has been resolved through pull request #589 and got merged in 0375d75.

Member

eXpl0it3r commented May 26, 2014

Issue has been resolved through pull request #589 and got merged in 0375d75.

@eXpl0it3r eXpl0it3r closed this May 26, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.2, 2.x May 26, 2014

@eXpl0it3r eXpl0it3r added resolved and removed confirmed labels May 26, 2014

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this issue Jun 13, 2014

Fixed soundbuffer contents not being able to be updated when still at…
…tached to sounds (#354), sounds now detach from their buffer when it is reset. Signed-off-by: binary1248 <binary1248@hotmail.com>

jcowgill added a commit to jcowgill/SFML that referenced this issue Sep 22, 2014

Fixed soundbuffer contents not being able to be updated when still at…
…tached to sounds (#354), sounds now detach from their buffer when it is reset. Signed-off-by: binary1248 <binary1248@hotmail.com>

(cherry picked from commit 0375d75)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment