Help with the design for selecting audio capture device

[Foaly]

Hello everybody!
In an effort to make the requested community driven development happen (on a small scale ) and also extend SFMLs multimedia functionality (but also for me personally to learn what I could have done better) I would like to invite everybody to start a discussion here with the goal to find a good design for the selection of the audio capture device.
Yesterday I proposed a "design" on GitHub. It was more or less the code I was using and now I would like to make sure that the design holds up to the standarts of SFML. So I would like to welcome everybody to rip it apart and put it back together in a better way!
Instead of endless commits changing little things and littering the pull request, I would like to find a solid design here and then hand it over to Laurent to review it.
Laurent already mentioned in the pull request that maybe the selection of the audio capture device should be something that is only possible on initialization. I only agree partially. This is something that you should also be able to set in the initilization step. But I don't see a reason why you shouldn't be able to change it while recording. I tested it with two mics and there is no noticable gap in the samples.
But like I said I would like to hear everybody's input on this.
So please everybody come and help.
And now fire away!

[Laurent]

First, thanks for your efforts. SFML 2.2 is not progressing as fast as I wish, so I really appreciate active help from users

If you keep the ability to change the audio device at any time, you should call start() and stop() inside of setAudioCaptureDevice. There are indeed two problems with your current code: it is less maintainable because code is duplicated, and more importantly, onStop() is called but not onStart(). In this case both should be called for consistency, or none (this is to be decided).

Anther minor comment: you should remove "Audio" from the function names. I'd say you can even remove "Capture", since we are in the scope of sf::SoundRecorder.

And don't forget to add a space between an "if" and its opening "("

[Mario]

Think this should also include some kind of error handling, e.g. right now it seems like you'll get an odd or broken state if switching the device fails (i.e. revert back to default device?).

Also think this should at the same time cause the same functionality to be added to audio playback in some way as well. Just not sure how that should be grouped/associated (global? passed to sf::Sound's constructor with reference counting?).

[Laurent]

Think this should also include some kind of error handling, e.g. right now it seems like you'll get an odd or broken state if switching the device fails (i.e. revert back to default device?).

Are you sure? The code should output an error message after the failure, and then reject any attempt to use the recording device. So this is perfectly clean.

Also think this should at the same time cause the same functionality to be added to audio playback in some way as well. Just not sure how that should be grouped/associated (global? passed to sf::Sound's constructor with reference counting?).

The playback device could be handled the same way at the OpenAL level, but the design has to be totally different since it impacts almost all the SFML audio classes. So I agree that the same must be done for playback, but for now let's focus on the recording device.

[Foaly]

First of all thanks for the feedback! I am glad to see that the code is getting some attention!

Think this should also include some kind of error handling[...]

Like Laurent said the code outputs an error message and does not start recording if you pass a wrong name to the setAudioCaptureDevice method. But while testing wrong names I noticed a "bug" that confused me and made me think it's left in a broken state. I used sf::SoundBufferRecorder for testing and if you try to save a completely empty buffer you get an error message saying "Format not recognised" which is confusing and not very discribtiv in this case. But this has nothing to do with sf::SoundRecorder it's an error in sf::SoundBuffer. This should be discussed in a seperate thread.
As of for this class, it should handle wrong input in a clean way.

[...]you should call start() and stop() inside of setAudioCaptureDevice[...]

I decided not to call start() and stop(), because it makes it impossible to change of the device while recording. In start() m_samples.clear(); is called and the call to onStart() does the same thing to derived classes (for example in sf::SoundBufferRecorder onStart() clears the buffer too). The way I see it setAudioCaptureDevice() doesn't stop and (re)start the recording. It simply changes the device. The user shouldn't worry about losing samples, because he changed the device.

Before you pointed it out I didn't even noticed that onStop() is called! Of course this is a bug. But now that I look at it I am asking myself why is onStop() being called in the record() method anyway? It should be called in stop() after m_thread.wait();. That would also fix this problem.

I know that this means a little code duplication, but as I explained the code works a little different and I think the functionality overweights.

You are definitly right about the method names. They are way to long! I'm terrible at finding names... I'll change that later.

[Laurent]

onStop() is called in the recording thread so that it's the same thread as onProcessSamples(). But I guess it won't hurt if it's called in the caller thread instead, being consistent with onStart().

[Foaly]

OK I made a few changes. I added the possiblity to pass the name of the capture device to the start() method. I also renamed the methods and changed the caller of the onStop() thread.

Edit: i just noticed a little error in the documentation of start(), but I'll fix that tomorrow.

[Foaly]

Alright I updated the documetation.
Are there any other suggestions?

[Laurent]

My suggestions:

1. You must choose: it's either "device" or "device name"; don't mix both in function and variable names (I'd suggest "device" -- if one day we change what identifies a device, the function names won't have to be adjusted).

2. You can set the device with a dedicated function, or with the start() function. I don't like providing two ways of doing the same thing, it's confusing. Again, you should choose one or the other. The behaviour of getCurrentDevice() must be clarified too, according to what you'll keep for changing the device. In the end, the choice is between a persistent device that you set once, or a temporary one that you set everytime a capture starts.

3. If setDevice is called in the middle of a capture, and it fails, onStop() will never be called.

[Foaly]

Thanks for your input!
I almost agree with everything you said. Here is what I'm thinking:

1. I partially agree. The function names are too long. I think "device" should be used. From the outside (for the user) there is no difference between "device" and "deviceName". But I think for variable names deviceName should be used instead of device, so it won't be confused with captureDevice, which is the actual handle.

2. I can definitely see the point of not providing two different way for the same task. I will chose to make use of a dedicated function, because it gives you more flexiblity like changing the device while recording. If start() is called alone, the default device will be used. If setDevice() will be called before or after, the device specified will be used. I don't understand what needs to be clarified in getCurrentDeviceName()? Could you explain that again?

3. Indeed you're absolutly right. I'll add a call to onStop() in setDevice().

I'll try to implement the changes tomorrow.

[Laurent]

But I think for variable names deviceName should be used instead of device, so it won't be confused with captureDevice, which is the actual handle.

Ah, you're right

I don't understand what needs to be clarified in getCurrentDeviceName()? Could you explain that again?

What I meant is: does it return an empty string when not recording, or the name of the last device used?

[Foaly]

I see what you mean now! Well I'd say if it's not recording it should return the name of the device that was last specified. If none was set before it will return the name of the default device.

[Laurent]

Sounds good.

[Foaly]

Alright! I implemented the changes/bug fixes.
Can anybody think of something else I might be missing?

[Laurent]

getAvailableDevices() should be static.