[alsa-devel] ALSA C++ API updated for 1.0.16

Lasse Kärkkäinen tronic at trn.iki.fi
Tue Feb 26 17:59:05 CET 2008


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?



More information about the Alsa-devel mailing list