On Tue, 05 Dec 2017 17:14:18 +0100, Maciej S. Szmigiero wrote:
On 05.12.2017 15:57, Takashi Iwai wrote:
On Tue, 05 Dec 2017 15:37:09 +0100, Takashi Sakamoto wrote:
Hi,
(..)
In my opinion, addition of constraint of PCM hardware parameters to these PCM plugins is preferable to insertion of the assertions, because abort inner library is bad idea itself. Proper errors should be returned to applications in protocol via alsa-lib PCM APIs. It's more friendly to usual developers and users.
Well, it's basically moot to discuss about it, as snd_pcm_lib_linear_get_index() is an internal function, i.e. it's not supposed to be called from applications.
In that sense, assert() wouldn't be too bad. However, the primary question is -- would it really catch anything in practice? What drove you to put that assert() at all?
If a check of the valid formats, then calling snd_pcm_format_linear() is the safest.
The reason I added these asserts is that snd_pcm_linear_{get,put}_index() use result from snd_pcm_format_width() directly to calculate an array index. If, for any reason, this function returns an unexpected width a subtle bug will emerge.
Three reasons that I could think of why an unexpected width would be returned is a bug in a plugin that uses these functions (currently 9 plugins use them) that fail to properly restrict source / destination format space to linear formats only, a bug in general format refining code or somebody adding a format to linear formats mask without adapting conversion arrays, too.
I know that these may sound like rare occurrences but since this check is close to zero-cost (it only happens on plugin stack initialization time and not, for example, at every sample) I think it is worth the extra time (and one can always compile the library with NDEBUG to skip it).
So it has nothing to do with the 20bit PCM support itself. Please put in another patch with more contexts.
thanks,
Takashi