Dne 30.6.2011 14:48, Takashi Iwai napsal(a):
At Tue, 28 Jun 2011 09:11:02 +0200,
Thanks for the patches. The basic strategy looks good to me, but a few things to be considered:
- The extension of API should be done in binary-compatible way; i.e. append new stuff to the existing structure, not inserting in the middle. In that way, you can use still old plugins.
Hi Takashi, thanks a lot for your reply. Would moving the new method to the end of snd_pcm_rate_ops struct suffice? Or do I have to create a new API number with corresponding checks? I am afraid the latter is the case :)
- From the performance POV, we may need to have the built-in float conversion in rate plugin indeed. Anyway, if rate plugin supports float, it should add FLOAT as the supported format. This needs changes in the parameter setup in rate plugin. Then it should accept the float format as is without conversions.
If I understand correctly, the rate plugin accepts only linear formats now. What was the original reason to exclude float? The "convert" API method is generic enough, perhaps I should add float support to all the rate converters using the API "convert" method. Again there is the question of new API version - the new one would offer convert_float and allow float samples in the generic convert method.
The specific convert_s16/float methods in pcm_rate.c can be modified to accept float too.
- The float-support has to be disabled when the float is suppressed via configure option. Also this needs to be exposed somehow to the plugin.
Is softfloat/HAVE_SOFT_FLOAT the corresponding configure option? BTW, is the ifndef check in pcm_softvol.c correct?
#ifndef HAVE_SOFT_FLOAT svol->dB_value = calloc(resolution, sizeof(unsigned int)); if (! svol->dB_value) { SNDERR("cannot allocate dB table"); return -ENOMEM; } svol->min_dB = min_dB; svol->max_dB = max_dB; for (i = 0; i <= svol->max_val; i++) { double db = svol->min_dB + (i * (svol->max_dB - svol->min_dB)) / svol->max_val; double v = (pow(10.0, db / 20.0) * (double)(1 << VOL_SCALE_SHIFT)); svol->dB_value[i] = (unsigned int)v; } if (svol->zero_dB_val) svol->dB_value[svol->zero_dB_val] = 65535; #else SNDERR("Cannot handle the given dB range and resolution"); return -EINVAL; #endif
It occurs to me the check should be ifdef, but perhaps I just get it wrong :)
- At best, the rate converters (plugin of plugin) should support both cases with and without float.
Well, the libsamplerate is using float internally, I am afraid the float-less case would require disabling this rate converter alltogether.
Thanks a lot for your help and guidance.
Pavel.