SoundRecorder improvments

[Foaly]

Hello everybody!
I have been using SoundRecorder quiet a bit in the past for different things and I noticed a couple things that need improvment. I have been using some of these improvments in my own code for a while, but I found the time now to make some clean commits.
So first up I noticed an error when you destroy a sf::SoundBufferRecorder while it's recording. You get a

Code: [Select]

pure virtual method called
terminate called without an active exceptionerror in the console when doing so. After diging into the code I think I found the answer. It's a threading issue. SoundBufferRecorder::onProcessSamples overwrittes the pure virtual function SoundRecorder::onProcessSamples which is called by the audio capture thread. When SoundBufferRecorder is destroyed it's method gets deleted, before the thread stops using it. Calling stop() in SoundBufferRecorders destructor fixes this problem. I think we should notice in a red box in the tutorials that a derived class has to call stop in it's destructor, since there is already a section on threading issues. Here is my fix.

Secondly I would like to suggest a new feature: Stereo recording. I have used it for quiet some time now and it comes in really handy. I saw there are also a couple threads on the forum with people requesting it. The answers suggest that it will be implemented later on. Here is my commit.

The last thing is something that I have implemented myself, but using it I noticed it would be better to do it differently. Right now if you call setDevice with an invalid device name while you are already recording the recording stops. Of course sometimes that's not what you want. So I think it would be a better behaviour to print an error message and fall back to device already in use if the one you specified is not available. Think of an application like VOIP or a sound installation where you want to make sure that there are always samples available. The commit can be found here. This commit also fixes another small issue when the method is called with an empty string.

So tell me what you think. The commits are ready to go and I'll send a PR once I heard some feedback.

[Foaly]

No comment at all?

[Nexus]

That sounds really good, but I haven't had the time yet to delve deeply into the implementation to verify those issues. And I think the same applies to others...
Anyway, I'll comment on the things I can now:

void enableStereoRecording(bool isStereo);
unsigned int getChannelCount() const;

Why this asymmetry? If there will ever be support for more than 2 channels, it would make sense to specify that right ahead instead of deprecating enableStereoRecording() later. You could also store the channel count instead of a bool.

And no function in SFML starts with "enable", you should use "set".

Regarding setDevice() during playing: you know my opinion regarding implicit fallbacks and other error-hiding mechanisms. Unfortunately, this is a problem inherent to SFML's lack of a defined error handling strategy. However, I would really like to discuss this problem at the root and not implement differently-behaved patches throughout the library, only to change every single one of them again later.

[Foaly]

Sorry for the long absence. I know it's bad to first be pushy and then don't get back at all myself, but I started a new job and had some pretty big things going on in my personal life, so I literally had no time at all.
I will split the code into seperate pull request. One for stereo recording and one for the pure virtual function call exeption. The other change is not wanted as I understand it, eventhough I still think it would make setDevice() a lot more useable and friendly to use!

As for your comment. I think it's highly unlikely that we will support more that recording 2 channels, because OpenAL doesn't, but I can change the interface to this:

void setChannelCount(unsigned int channelCount);
unsigned int getChannelCount() const;

But what should I do if the user sets a channelCount > 2 ? Fallback to 2? Return an error?

I will try to send the pull requests soon^TM

[Nexus]

You should probably wait for more opinions before opening a PR, I'm the only one who has responded so far.

[GraphicsWhale]

As for your comment. I think it's highly unlikely that we will support more that recording 2 channels, because OpenAL doesn't, but I can change the interface to this:

void setChannelCount(unsigned int channelCount);
unsigned int getChannelCount() const;

But what should I do if the user sets a channelCount > 2 ? Fallback to 2? Return an error?

I find that using an unsigned int is a bit misleading for something that only has two possible values.

Like:

void setStereoSoundEnabled(bool enabled);
bool isStereoSoundEnabled() const;
 

Or:

enum class RecordingMode
{
    MONO,
    STEREO
};

void setRecordingMode(RecordingMode recording_mode);
RecordingMode getRecordingMode();
 

[Mario]

I'd actually op for using unsigned character for both, theoretically there could be 5.1 recording in the future. It's not just limited to mono or stereo, if you think about it.

[Laurent]

If we decide to name these functions "channelCount", it is of course to be able to extend them to support more than 2 formats. It's true that OpenAL core API only supports mono and stereo, but support for more is possible with extensions, so I'd say we should go for channelCount, which is also consistent with the other audio classes.

[Foaly]

Ok I went with get/setChannelCount and made a seperate pull request. This way brings the possibility that it can be extended to more channels in the future without changing the API and its also consistent with the other sound related classes.
When set I limited the channelCount to two and print a warning if a higher value way entered. Fell free to take a look at the code and send me some feedback!

I also made a pull request for the threading issue.