[alsa-devel] ALSA C++ API updated for 1.0.16
Here's an updated version of the C++ wrapper that I posted earlier in September 2007. I would like this to be included in the ALSA distribution after some peer review. This wrapper has been used in UltraStar NG (a game shipped by Gentoo and others) since September, with no known issues other than that it is incomplete (very PCM-centric). As a reminder, the API closely follows the C API, and also allows mixing direct C API calls with itself transparently.
Lasse Kärkkäinen wrote:
Here's an updated version of the C++ wrapper that I posted earlier in September 2007. I would like this to be included in the ALSA distribution after some peer review.
Yes, an ALSA C++ wrapper is a good idea.
error(std::string const& function, int err): std::runtime_error("ALSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) {}
I'm not sure that including the function name and all parameters as they appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header.
pcm(char const* device = "default", snd_pcm_stream_t stream = SND_PCM_STREAM_PLAYBACK, int mode = 0) {
This looks too much like a default constructor. I think neither device nor stream should have a default value.
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, returning an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
class hw_config: internal::noncopyable {
Why should this object not be copyable? And why do you have two different objects (*w_params and *w_config) for wrapping the hardware/software parameters?
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); }
I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like device unplugged). I think it would be better if this object doesn't have a public constructor, and you would be able to create one only through a pcm object, like "hw_config c = pcm.any_hw_params();".
~mmap() { // We just assume that this works (can't do anything sensible if it fails). snd_pcm_mmap_commit(pcm, offset, frames);
This is the place where all the usual write errors must be checked. This cannot be done in a destructor. I think using RAII for the non- error commit just isn't possible. Regards, Clemens
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, returning an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
I'm not an OO design guru, but at least IOC (IBM OpenClass; it used to be a rather powerful multiplatform GUI framework - before they made it AIX only) used this very paradigma all over the place. You were supposed to write statements like: obj.method1().method2().method3().___.method_n(); At least that's what all their example code looked like. Best, Michael -- Michael Gerdau email: mgerdau@tiscali.de GPG-keys available on request or at public keyserver
Thank you for your input, which is quite valuable.
error(std::string const& function, int err): std::runtime_error("ALSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) {}
I'm not sure that including the function name and all parameters as they appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header.
It is just an exception thrown, the program may catch it and display another error message. The original ALSA error code is also included within it. E.g. try { // Some code that causes error } catch (alsa::error& e) { std::cerr << "ALSA error: " << snd_strerror(e.code()) << std::endl; } Prints only the error, not the function that caused it, while try { // ... } catch (std::exception& e) { std::cerr << e.what() << std::endl; } would print the formatted message, containing the function name, too. In practice the function name seems to be quite essential for debugging, as EPIPE or some other of the standard error codes will tell little or nothing. For completeness, I think that it might be desirable to also store the function name separately (like the code is stored), but can anyone see any actual use for this?
pcm(char const* device = "default", snd_pcm_stream_t stream = SND_PCM_STREAM_PLAYBACK, int mode = 0) {
This looks too much like a default constructor. I think neither device nor stream should have a default value.
Well, it is a default constructor. One design goal was to make this wrapper as easy to use as possible, and choosing sensible defaults for that is helpful. If you are expecting alsa::pcm pcmobject; to produce an object that is not constructed, we are talking of two-stage construction, which is considered harmful by many C++ coders nowadays. A constructor should always construct the object fully and throw an exception (and thus prevent the object being created) if that is not possible. Design & Evolution of C++, by Bjarne Stroustrup, and other books discuss this matter thoroughly. (surprisingly enough, fstreams of the standard library still allow two-stage construction) Thus, I see the expected behavior of creating pcmobject that way as being to actually construct a new PCM, with the default parameters (where "default" and playback are the most common choices; and also far better than people hardcoding their applications to use "plughw:0" or whatever happens to work for them).
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, returning an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
This is for function call chaining. It is a common idiom used by the standard library and others. In this particular case, it is the very thing that allows syntax like alsa::hw_config(alsa_pcm) // Temporary object .set(SND_PCM_ACCESS_RW_INTERLEAVED) .set(SND_PCM_FORMAT_S16_LE) .rate_near(rate) .channels(1) .period_size_first(period) .commit(); // The object is disposed here Instead of { alsa::hw_config config(alsa_pcm); config.set(SND_PCM_ACCESS_RW_INTERLEAVED); config.set(SND_PCM_FORMAT_S16_LE); config.rate_near(rate); config.channels(1); config.period_size_first(period); config.commit(); } (braces are needed for limiting the config variable lifetime)
class hw_config: internal::noncopyable {
Why should this object not be copyable?
Because it contains the PCM pointer, whose copy semantics are somewhat hairy. E.g. the following code would break silently: alsa::hw_config f() { alsa::pcm pcm; return alsa::hw_config(pcm); } Please note that you can still make copies of alsa::hw_params.
And why do you have two different objects (*w_params and *w_config) for wrapping the hardware/software parameters?
The alsa::hw_config wrapper is meant to be used only temporarily, for easily setting the parameters. It differs from alsa::hw_params by also tying a PCM to it.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); }
I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like device unplugged).
It seems like a sensible fallback, anyway. The other option would be to load any() settings always and have the user call current(), if he wants those instead. Would you find this preferable? Some settings need to be loaded by the constructor in order to ensure that the structure is in a consistent state at all times.
I think it would be better if this object doesn't have a public constructor, and you would be able to create one only through a pcm object, like "hw_config c = pcm.any_hw_params();".
That seems to require unwanted hackery (friend classes, helper objects, etc). Rather, if such selection is needed, I would add a second argument to the constructor (with a suitable default value), allowing one to choose between the two (e.g. bool useCurrent = false). However, just loading the any settings in the constructor and loading the current settings on top of them as required seems easier to use (theoretically slightly slower due to the extra initialization if the current parameters are preferred, but performance cannot be an issue in configuring a PCM).
~mmap() { // We just assume that this works (can't do anything sensible if it fails). snd_pcm_mmap_commit(pcm, offset, frames);
This is the place where all the usual write errors must be checked. This cannot be done in a destructor. I think using RAII for the non- error commit just isn't possible.
Ouch. This is a big problem. I see three options 1. Drop the MMAP wrapper entirely * Easily leads to leaks when exceptions are thrown 2. Add a separate commit function * Use exceptions to signal errors from this function * Set pcm and areas to NULL after commit * Possibly throw exceptions if the object is accessed after this (either by operator overloading or by getter functions instead of public members) * Still commit in destructor if commit was not called 3. Throw in destructor only if std::uncaught_exception() returns false * Ignore all errors if stack unwinding is in progress * Weird semantics, not commonly used Option 2 is hairy, as it leaves the object in unusable state after commit is called, but it still seems highly preferable to option 1, which would require great care from the user for avoiding resource leaks. Option 3 is pretty much out of the question because programmers don't expect destructors to ever throw anything. What do you think?
Lasse Kärkkäinen wrote:
Thank you for your input, which is quite valuable.
error(std::string const& function, int err): std::runtime_error("ALSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) {}
I'm not sure that including the function name and all parameters as they appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header.
It is just an exception thrown, the program may catch it and display another error message. The original ALSA error code is also included within it.
I'm just objecting to the fact that the function name and ALSA's error string are combined into a string that cannot easily be parsed later.
In practice the function name seems to be quite essential for debugging, as EPIPE or some other of the standard error codes will tell little or nothing.
Agreed.
For completeness, I think that it might be desirable to also store the function name separately (like the code is stored), but can anyone see any actual use for this?
I'd like to have only the snd_strerror() result as the message. In this case, the function name has to be stored separately if it is needed.
pcm(char const* device = "default", snd_pcm_stream_t stream = SND_PCM_STREAM_PLAYBACK, int mode = 0) {
This looks too much like a default constructor. I think neither device nor stream should have a default value.
Well, it is a default constructor. One design goal was to make this wrapper as easy to use as possible, and choosing sensible defaults for that is helpful.
While playback will be used more often, I think the difference between playback and capture is so big that we cannot make one of them the default.
(where "default" and playback are the most common choices; and also far better than people hardcoding their applications to use "plughw:0" or whatever happens to work for them).
In practice, most applications allow the user to specify the device name, so the default value won't be used anyway.
If you are expecting alsa::pcm pcmobject; to produce an object that is not constructed, ...
No, I'm not.
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, returning an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
This is for function call chaining. It is a common idiom used by the standard library and others.
The STL streams use this to allow using the convert-to-bool operator to check the error status of the stream (because most errors aren't reported by exceptions). That this also allows chaining is just a side effect.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); }
I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like device unplugged).
It seems like a sensible fallback, anyway. The other option would be to load any() settings always and have the user call current(), if he wants those instead. Would you find this preferable?
My preference would be that the user has to indicate whether he wants to read the current setting or to begin choosing new settings. It is not possible for this constructor to guess what the user wants.
Some settings need to be loaded by the constructor in order to ensure that the structure is in a consistent state at all times.
ALSA's snd_pcm_hw_params_t is in a consistent state even before initial or current settings are loaded from a PCM device. But then the hw_config object isn't the direct wrapper for snd_pcm_hw_params_t ...
~mmap() { // We just assume that this works (can't do anything sensible if it fails). snd_pcm_mmap_commit(pcm, offset, frames);
This is the place where all the usual write errors must be checked. This cannot be done in a destructor. I think using RAII for the non- error commit just isn't possible.
Ouch. This is a big problem.
This is very similar to database transaction that must be either committed or rolled back. The usual idiom seems to be a RAII class whose destructor rolls back if a commit or rollback hasn't already happended. In our case, a rollback is equivalent to calling snd_pcm_mmap_commit with frames==0. Hmm, it should be possible to commit any number of frames as long as it isn't more than originally requested.
2. Add a separate commit function ... Option 2 is hairy, as it leaves the object in unusable state after commit is called,
After committing, it is indeed not allowed to access the mmap buffer, so this isn't too unexpected. In practice, the commit will be the last action before the destructor. BTW: The object should be called mmap_access or something like that. As far as I can see, your header tries to do two things: wrapping the ALSA API, and offering an easier-to-use API that is not as low-level as the ALSA API itself. I think these two should be put in separate header files. Anyway, we need more wrappers for all those data structures. I'll see what I can come up with. Regards, Clemens
Lasse K=E4rkk=E4inen wrote:
error(std::string const& function, int err): std::runtime_error("A= LSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) = {} I'm not sure that including the function name and all parameters as =
On Wed, Feb 27, 2008 at 8:50 AM, Clemens Ladisch <clemens@ladisch.de> wrote= : they
appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header. It is just an exception thrown, the program may catch it and display another error message. The original ALSA error code is also included within it. I'm just objecting to the fact that the function name and ALSA's error string are combined into a string that cannot easily be parsed later.
Many of those strings are not meant to be parsed, they are for human review (developer/user).
In practice the function name seems to be quite essential for debuggin= g, as EPIPE or some other of the standard error codes will tell little or nothing. Agreed.
I would prefer that the method from the C++ API should appear instead of the internal C API function. The C++ developer is calling the constructor of alsa::pcm, not snd_pcm_open().
For completeness, I think that it might be desirable to also store the function name separately (like the code is stored), but can anyone see any actual use for this? I'd like to have only the snd_strerror() result as the message. In this case, the function name has to be stored separately if it is needed.
I've found the snd_strerror() messages are quite unusable, at least for the alsaseq api... they return something like 'file not found' which seems more related to the internal implementation rather that an actual alsa-related error...
pcm(char const* device =3D "default", snd_pcm_stream_t stream =3D = SND_PCM_STREAM_PLAYBACK, int mode =3D 0) { This looks too much like a default constructor. I think neither dev= ice nor stream should have a default value. Well, it is a default constructor. One design goal was to make this wrapper as easy to use as possible, and choosing sensible defaults for that is helpful. While playback will be used more often, I think the difference between playback and capture is so big that we cannot make one of them the default.
Me too. [...]
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\ Is there a reason why you return the object itself? Usually, return= ing an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here. This is for function call chaining. It is a common idiom used by the standard library and others. The STL streams use this to allow using the convert-to-bool operator to check the error status of the stream (because most errors aren't reported by exceptions). That this also allows chaining is just a side effect.
I found this feature quite useful.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); } I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like de= vice unplugged). It seems like a sensible fallback, anyway. The other option would be t= o load any() settings always and have the user call current(), if he wan= ts those instead. Would you find this preferable? My preference would be that the user has to indicate whether he wants to read the current setting or to begin choosing new settings. It is not possible for this constructor to guess what the user wants.
_From a OO perspective, I would expect that a *_config object be created from an existing pcm object. Something like: alsa::pcm pcm; pcm.get_config().set(FOO).set(BAR); If there is something that doesn't apply to a pcm instance, the C++ API could provide an instance of a pcm_any object and call that instead: alsa::pcm_any.get_config().set(FOO).set(BAR); [...] HTH, --=20 Aldrin Martoq
Clemens wrote:
I'm just objecting to the fact that the function name and ALSA's error string are combined into a string that cannot easily be parsed later.
As Aldrin pointed out, it is not meant to be parsed. I have now provided separate functions for accessing the function names directly.
I'd like to have only the snd_strerror() result as the message. In this case, the function name has to be stored separately if it is needed.
"alsa::pcm::pcm: snd_pcm_open failed: No such file or directory" is useful for debugging, "No such file or directory" is not. Therefore, I don't see why you want only snd_strerror in that message.
While playback will be used more often, I think the difference between playback and capture is so big that we cannot make one of them the default.
I have removed the default arguments, as you requested.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); } I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like device unplugged). It seems like a sensible fallback, anyway. The other option would be to load any() settings always and have the user call current(), if he wants those instead. Would you find this preferable?
My preference would be that the user has to indicate whether he wants to read the current setting or to begin choosing new settings. It is not possible for this constructor to guess what the user wants.
I have looked at the issue again; the current system (load current if possible, any otherwise) is to make it resemble the API that sw_config provides (always loading the current settings). If the user specifically wants current or any settings (and wants an exception if that fails), he may call the any() and current() functions after first constructing the object. If not, he will get the most sensible default behavior. However, I changed to catch to only match alsa::error (not that it matters, but it's clearer that way).
Some settings need to be loaded by the constructor in order to ensure that the structure is in a consistent state at all times.
ALSA's snd_pcm_hw_params_t is in a consistent state even before initial or current settings are loaded from a PCM device. But then the hw_config object isn't the direct wrapper for snd_pcm_hw_params_t ...
The documentation says that an invalid snd_pcm_hw_params_t is allocated (by snd_pcm_hw_params_malloc). I did not look into ALSA source code to find out whether this means uninitialized memory or just a kind of "none" configuration space that could still be used by the other functions. In the latter case, removing the initialization from hw_config constructor, and having the user explicitly call any() or current(), would be a sensible choice. In this case it would be nice if this behavior was also clearly documented on the C API.
In our case, a rollback is equivalent to calling snd_pcm_mmap_commit with frames==0.
Ah, didn't think of that. Fixed to work this way, safe commit function provided.
Hmm, it should be possible to commit any number of frames as long as it isn't more than originally requested.
One could try to commit more than the original number with the old wrapper, but this is fixed now (among with the commit function).
After committing, it is indeed not allowed to access the mmap buffer, so this isn't too unexpected.
Just to stay safe, I made it throw std::logic_error if any access is attempted after commit.
BTW: The object should be called mmap_access or something like that.
This differs from the naming convention used in other parts of the wrapper (take a part of the C API function name). It seems pretty obvious what an object called mmap does on a PCM, so I don't quite see why you want another name for it.
As far as I can see, your header tries to do two things: wrapping the ALSA API, and offering an easier-to-use API that is not as low-level as the ALSA API itself. I think these two should be put in separate header files.
I don't see why they couldn't coexist in the same header. C++ programmers are used to some flexibility (things happening automatically, function overloading, default arguments, "OOP" syntax), so it seems normal to offer these in such a wrapper. However, I don't intend alsa.hpp, as it is now, as an easy solution for the user, but have instead written a separate audio library on top of it, abstracting many of the features of ALSA and also supporting other audio interfaces under the hood.
Anyway, we need more wrappers for all those data structures. I'll see what I can come up with.
True. The problem is that it is a lot of work, even with all those macros. Without helper macros one would need thousands of lines just to wrap all the functions. Any help is appreciated, of course :) Aldrin wrote:
I would prefer that the method from the C++ API should appear instead of the internal C API function. The C++ developer is calling the constructor of alsa::pcm, not snd_pcm_open().
I have fixed this. Now it prints the calling function, the C API function and the snd_strerror message. All can be requested as individual strings to order having to parse the formatted message returned by what().
From a OO perspective, I would expect that a *_config object be created from an existing pcm object. Something like:
alsa::pcm pcm;
pcm.get_config().set(FOO).set(BAR);
This does not allow storing the generated configuration, which is a requirement if we want to add some control logic to it instead of just setting things blindly. (remember, hw_config is noncopyable because it depends on a PCM, which is noncopyable)
If there is something that doesn't apply to a pcm instance, the C++ API could provide an instance of a pcm_any object and call that instead:
alsa::pcm_any.get_config().set(FOO).set(BAR);
Even the "any" configuration is always linked to a PCM: int snd_pcm_hw_params_any(snd_pcm_t *pcm, snd_pcm_hw_params_t *params); A new version is attached. Changelog for alsa.hpp 0.6: - alsa::mmap completely rewritten - alsa::error now has accessors for function names - ALSA_CHECKED and alsa::error changed to also store the name of the function using the macro - alsa::pcm device and stream default values removed - catching errors only, instead of any runtime_error - added missing *_param operator=(*_param...) - naming style changed: member variables now use m_ prefix to avoid shadowing warnings from GCC -Wshadow - other compile warnings eliminated as well (-Weffc++ etc) - documentation updates
participants (4)
-
Aldrin Martoq -
Clemens Ladisch -
Lasse Kärkkäinen -
Michael Gerdau