Help with the design for selecting audio capture device

[Nexus]

Some minor issues: You sometimes use an explicit std::string conversion although unnecessary (const char* can be implicitly converted). Some corrected examples, there are more:

m_deviceName = alcGetString(captureDevice, ALC_CAPTURE_DEVICE_SPECIFIER);
...
deviceNameList.push_back(deviceList);
...
return alcGetString(NULL, ALC_CAPTURE_DEFAULT_DEVICE_SPECIFIER);

You should also pass std::string parameters by const reference instead of value.

By the way, shouldn't m_isCapturing be protected by a mutex? Even though it's just a bool, read/write is not guaranteed to be atomic.

[Foaly]

Oh I didn't know there was an implicit conversion for std::string. I'll change that. And also so the const reference instead to value.

Since SFML is using C++03 there are no atomic operations, but I think I once heard that storing/getting of a bool can usually be assumed to be safe.

I already changed the name to getDevice(). I'll see if I find some time to do the other suff later on.

[eXpl0it3r]

Since SFML is using C++03 there are no atomic operations, but I think I once heard that storing/getting of a bool can usually be assumed to be safe.

"usually safe" implies that it's not safe in all cases, thus you're running the risk of possible race conditions etc.
It may work in 98% of all cases, but it will be that 2% who will open up a tickt in the future.

[Nexus]

but I think I once heard that storing/getting of a bool can usually be assumed to be safe.

Not really, otherwise the std::atomic_bool type would be pretty pointless.

[Hiura]

Regarding the code I spot two minor things: getDefaultDevice() should be directly after getAvailableDevices(), and getDevice() should be directly before/after setDevice(). Then the initialisation of m_deviceName could use getDefaultDevice() instead of duplicating one line of code.

Regarding potential data races in this class, if we assume the user will use a recorder's API on one thread only (or on multiple thread but with a proper use of mutex), having in mind that all attributes are private, we can say the following:

• m_thread is only used by the user's thread so it's okay.
• m_samples is written in the user thread when start() is called, but then it's not read nor written from that thread. Only the worker thread use it and it is not exposed to subclass in a writable way so it's also okay.
• m_sampleRate follow the same schema. (*) so it's also okay.
• m_processingInterval is always writable from the user thread but is also read on the worker thread. There is a potential data race here but its impact is zero on the overall system – in the worst case the value is updated on the worker thread one loop iteration (in record()) later than expected. So it's also okay.
• m_deviceName is not read / write concurrently so it's okay too.
• Finally, the last attribute is m_isCapturing. It's mainly like m_processingInterval except for one irrelevant data race: both threads can write it to false at the same time if stop() is called when onProcessSamples() return false. So it's okay too.

However, there is an undocumented (I think) possible data race on captureDevice / alcCaptureOpenDevice() when start() is called on two different recorders from two different threads. I haven't looked at the documentation of alcCaptureOpenDevice() so I can only speculate that one of the two recorders will fail if the implementation of OpenAL is thread safe. Otherwise we could simply add in the documentation of start() that this situation results in an undefined behaviour.

(*) it would be good I think to add in the documentation that getSampleRate() is valid / meaningful only after start() is called since it's not set by the user in the constructor of the class.

Let me know if you think my interpretation of the code is right, or wrong.

[Laurent]

However, there is an undocumented (I think) possible data race on captureDevice / alcCaptureOpenDevice() when start() is called on two different recorders from two different threads.

The code explicitly disallows this situation, only one active recording sessions is allowed, the second call to start() would fail (but there's probably a need for a mutex there too).

[Hiura]

Yes, that's what I'm talking about. The test is in an (unsecure) critical section. There is two solutions; either using a mutex, or adding a comment in the documentation.

[Foaly]

I implemented the proposed changes.
I am not an expert in multithreading, so if I should implement a mutex around captureDevice someone needs to show me how.
Other than that does anybody have another comment about the code?

[Laurent]

Concurrent access issues are another problem. Don't try to solve everything in your pull request, focus on the initial issue.

[Foaly]

Alright! Then if nobody else has anything to add I think the initial issue is fixed

[Laurent]

Thanks. I'll do a final review as soon as possible

[Laurent]

It looks good, so now let's fix the cosmetic issues

There are many tiny issues in your comments. It would take ages for me to describe them in detail, so I let you do a first pass yourself. Mainly: missing commas, periods, "while while",  "if(", ...

And I would modify the description of setDevice: the function doesn't open the capture device, it just changes the active device for following captures.

This is really tedious but it would be really ugly to merge a modification and then add a commit just to fix minor stuff like this. Hmm... pull requests are great but they start to consume as much of my time as if I did the modification myself

By the way, what do the device names returned by OpenAL look like?

[Nexus]

This is really tedious but it would be really ugly to merge a modification and then add a commit just to fix minor stuff like this. Hmm... pull requests are great but they start to consume as much of my time as if I did the modification myself

Interesting, back then no one believed me

Maybe you could check out Foaly's branch from your hard drive, add more commits locally, merge and push them. The web interface is just too limited for anything beyond the most basic usage. When you click on its green merge button, isn't every single commit of Foaly added to your history? If so, that might not be what you want anyway. Manually, you could squash the commits and clean everything up. If it's not possible to keep Foaly as an author, you can credit him in the commit message...

[Laurent]

Yes, I remember this discussion

I still prefer doing it this way, even if it becomes time-consuming and a little frustrating. At least the request be merged eventually, and next time Foaly contributes (I'm sure he will ), it will be perfect.

Nice new logo, by the way.

[Foaly]

Sorry about the holdup, I was busy studying for an exam.
Anyway I reread the documentation and corrected all the mistakes I could find. If you can see any other ones please let me know and I'll fix them.

By the way, what do the device names returned by OpenAL look like?

I have a USB Interface (soundboard) that looks like that: Mikrofon (2- USB Audio CODEC) and a webcam that looks like that: Mikrofon (HD Webcam C310) (those are the strings returned by getAvailableDevices().