For the SFML Developers: SoundStream Chunk

[Andrew]

Hi,

This may seem like a minor request, but could you make the SoundStream Chunk's "samples" field be non-const?

https://github.com/SFML/SFML/blob/master/include/SFML/Audio/SoundStream.hpp#L55
(Line 55)

It's hard to explain why this is such a big deal for me, but basically it would make my code a lot cleaner, faster, and easier if I could change its value over time. I guess this is the best way to explain why:

I'm generating sound data programmatically, and I am not using a file or stream source of any kind. I would prefer to be able to just store the sound data and add to it all within the Chunk. Instead, I have to do something more like this:

short* soundData = new short[200000];

for (blahblah)
{
    soundData[whatever] = etc.;
}

sf::SoundStream::Chunk c;

c.samples = soundData;

In addition, I might want to make modifications to the Chunk over time. I would prefer not to have to maintain two arrays/vectors/etc. when there only needs to be one. Hopefully this makes sense.

(I'm also a little curious as to why it was made const in the first place and why that would be necessary?)

Thanks,

- Andrew

[Laurent]

I would prefer to be able to just store the sound data and add to it all within the Chunk

This is just a pointer, even if it was not const you would have to make it point to your own allocated array of samples. The const makes absolutely no difference.

In addition, I might want to make modifications to the Chunk over time.

What does "over time" means? Within the call to onGetData, or even after? Once the onGetData function returns, the samples are copied in an internal buffer and you can't access them anymore, they are sent to the audio driver for processing.

So, in short, it is const because it wouldn't make sense to make it non-const. You're supposed to work on your own array and then make the "samples" pointer point to it before leaving onGetData, so that SFML knows where the new samples are stored and can send them to the audio driver.

[Andrew]

Here's what I can't do with const in place:

sf::SoundStream::Chunk c;

c.samples = new short[100000];
c.samples[0] = 5; //It says "expression must be a modifiable value".

[Laurent]

And what difference would it make, compared to your first code, if you could do that?

[Andrew]

sf::SoundStream::Chunk c;

c.samples = new short[200000];

for (blahblah)
{
    c.samples[whatever] = etc.;
}

Note: it may not all be generated at the same time. If this were to be the case, I would want to pass around the Chunk object reference, not a short* and an int. I'll give you an example:

void AddSample(Chunk& c)
{
    c.samples[c.sampleCount - 1] = 5;
    ++c.sampleCount;
}

[Laurent]

I still don't understand what your problem is, or what you think you'll gain by accessing the "samples" pointer directly.

Work on your own array, and at the end of onGetData, before returning, set chunk.samples and chunk.sampleCount to what you've just generated. What's wrong with that?

[Andrew]

So when you're driving and you're going to make a left turn, do you instead take three right turns? Or would you rather access the left direction directly?

You SFML people are so difficult. You disallow or heavily discourage certain ways of doing things because either you don't like them, you don't understand why other people would do them, or you wouldn't do them yourself. It's this mentality of "I want you to write your code like this".

Sure, it's your project and your code. Technically you have the right to do what you want with it, but are you developing it for you or also for all of the other people who use it? Because I guarantee that this (general mentality) has caused unnecessary and unwanted frustration for plenty of others besides myself; if I've had unsolvable problems with these sorts of things, so have others.

I am saying this for SFML's benefit. If I'm coming across too negative then sorry, I'm just very frustrated. I spent the last couple of hours determining that the SoundStream must be inherited a certain way, else it will give linker errors that do not help you. Apparently you can either not specify any constructor at all or you must specify a constructor like this:

class AnotherSoundStream: public sf::SoundStream
{
    public:
        AnotherSoundStream(Chunk& chunk);

Why do I get linker errors if I don't have either of those two constructors? I have no idea, but I bet it's because "I can't see anyone doing it any other way." There goes hours of development.

Actually, I don't think it's allowing any constructors...

[Laurent]

You shouldn't have any problem with constructurs. If you do, post some code together with the error messages please.

I'm trying hard but I still don't understand the point of this thread, really.

Here is what I've understood so far: you want to do this

bool onGetData(Chun& chunk)
{
    chunk.samples = new short[10000]; // memory leak by the way
    chunk.sampleCount = ...;

    ... generate the samples ...
    return true;
}

... but you're forced to do this because of the const keyword

bool onGetData(Chun& chunk)
{
    sf::Int16* samples = new short[10000];
    unsigned int sampleCount = ...;

    ... generate the samples ...

    chunk.samples = samples;
    chunk.sampleCount = sampleCount;

    return true;
}

Are you really so much frustrated because of these two additional lines of code? Because yes, it's just really two extra lines of code, the result is of course equally efficient performance and memory wise. That's why I still don't get it. Why you complain, and where this discussion is going

[binary1248]

I spent the last couple of hours determining that the SoundStream must be inherited a certain way, else it will give linker errors that do not help you. Apparently you can either not specify any constructor at all or you must specify a constructor like this:

class AnotherSoundStream: public sf::SoundStream
{
    public:
        AnotherSoundStream(Chunk& chunk);

Why do I get linker errors if I don't have either of those two constructors? I have no idea, but I bet it's because "I can't see anyone doing it any other way." There goes hours of development.

So... how does sf::Music manage to do what you assume is impossible then?
https://github.com/SFML/SFML/blob/master/include/SFML/Audio/Music.hpp#L52

Instead of assuming that something is outright impossible and asking for an API change, you could at least ask if whether your initial assumption is correct first. You were right in that you lost hours of development, but wrong in why you did.

If you are willing to start from step one and provide us with more information other than "I need the API to conform to my code because I didn't find another way to do it." I'm sure we can help you. The first step would consist of a description of the problem/use case that you have and how you imagine you can solve it. No code has to even be written yet.

The XY-problem might be of interest.

[Andrew]

I figured out my constructor problem. Apparently:

#define SFML_STATIC

is not equivalent to setting it in the preprocessor definitions, as the internet said.


Anyways, I have in fact given you a description of why I want const removed. Take a look at my AddSample method from before and see if you can get a working example of that (actually working) with const Chunk. You won't be able to; then you'll tell me that it's none of my business to write code intuitively like that and that I should just create a Chunk last minute. My response to that is that I want to use Chunk for the same reasons that you did in SFML! Instead, I had to make my own stupid class:

class ManagedSoundThing
{
        public:
                short* data;
                unsigned int numberOfSamples;

                ~ManagedSoundThing()
                {
                        delete [] data;
                }
};

Note that I have just shown you, in this code, yet another reason why it should not be const. Not only does my class store a short* and unsigned int at the same time, but if I were able to use a Chunk in it (properly), I would not be able to call "delete [] chunk.samples;" on it.

As I said from the beginning, this may seem like a minor problem to you, but it simply doesn't make sense to have inconsistent code. Why create a package for data that only you can use? The real life equivalent to this would be something like me having to mail you a keyboard and a mouse in separate packages even though I could have sent them both in one package - and you yourself are going to use your own combined package, but you won't let me use it.

Also, this has further implications for other code in SFML, which is also what I was getting at. A software library should have the goal of being as usable as possible in as many ways as possible. Unnecessary restrictions are the last thing you want to have.

[Laurent]

Storing your own array (std::vector please...) as a class member is actually the only way to avoid a memory leak. So you have no choice but to have your own, persistent (i.e. longer lifetime than onGetData) storage for chunks of samples. It's not because I want you to do that, it's because this is the only way which works correctly. And guess what? The const keyword tries to force you to use this correct solution and avoid the memory leak.

So instead of blaming SFML, please admit that you're trying to do something wrong and do it the right way -- then SFML will not try to annoy you anymore.

[Andrew]

Please explain to me how my ManagedSoundThing still has a memory leak. I am very curious. When it gets out of scope and reference and/or is explicitly deconstructed, it deletes the array pointer from memory.

[binary1248]

As Laurent said, sf::SoundStream does not take ownership of the memory pointed to by the pointer. You simply tell SFML where to look for when it needs data to play. Storing your data in the Chunk makes little to no sense because either you would expect to pass ownership of the memory to SFML or just accept the fact that the block will leak, thus the pointer points to immutable data that you are in charge of deallocating when you are done. Chunks aren't meant for storage. The name might be a bit misleading, since it actually points to a chunk of data instead of being one.

Also, if you originally planned on passing the same Chunk to onGetData every time it is called, that won't work. It is called every time new audio data is needed, and passing the same chunk to it over and over will cause only the front of the data stream to be played. You really need to generate new data every time it is called, or change its pointer as an index into your own data storage. Either way, you will be the one storing data in your own container and you will have to manage its lifetime. As Laurent already said, std::vector would be good for this purpose.

This really all comes down to assigning the const pointer to some location in your data storage. There is no point in storing the owning pointer within the Chunk. SFML promises you that through the const, it will not modify the data. This whole discussion is just about that one little assignment... And instead of passing the Chunk around the place, why not just pass a reference to your data storage instead? It has exactly the same effect while protecting against "I forgot to delete[] the Chunk pointer" situations. Judging from this discussion, I have a feeling you don't use smart pointers much, or even give much thought about proper memory management. SFML is designed to be as clean as possible and to mitigate programmer errors through a robust interface. Making the pointer const is just one example of how it does this.