At Fri, 01 Jul 2011 11:27:03 +0200, Pavel Hofman wrote:
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?
This is needed together with the API version bump. Increase SND_PCM_RATE_PLUGIN_VERSION, change rate_open_func() to call all older versions, and don't use the new methods for older versions.
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?
Because the existing rate-converter are only for linear formats :)
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.
I don't mind how it'll be implemented as long as the backward compatibility is kept somehow.
- 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?
HAVE_SOFT_FLOAT is a bit confusing name. It means actually NO_FLOAT, or better to say, NO_FLOAT_CALCULATION. Thus ifndef is correct below.
#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.
Currently yes, but who knows in future? The library internal can change.
Takashi