At Tue, 28 Jun 2011 09:11:02 +0200, Pavel Hofman wrote:

Dne 20.6.2011 09:35, Pavel Hofman napsal(a):

...

Hi,

This is probably a question for Jaroslav Kysela. The better-quality rate plugins offer float resolution, but currently we are deliberately loosing information by down-converting to s16 in plugin_rate API (convert_s16). In rate_samplerate.c, the samples are up-converted to float before calling the libsamplerate API. Speex offers float API too, while we are using the int16 one.

There are two ways to handle this:

A. Implementing the generic "convert" API function for these plugins, handling the sample - float conversion in each plugin individually.

B. Extending the pcm_rate API (pcm_rate.h) with a new optional function convert_float.

Since the conversion from general sample format to float is not trivial (using the plugin_ops.h operations), I would be inclined to suggest the extension (well I have it hacked already :-) ). On the other hand, the conversion routine can be located in some common code shared by all the plugins involved and we could keep the existing API, using the generic convert function.

I am attaching preliminary patches. May I ask for help with adding a new API version number if you find it necessary? I am afraid this is beyond my skills.

The patches are not checkpatched yet.

After these patches are finished, I would like to work on implementing the SoX resampler algorithm which features excellent CPU consumption/resampling quality ratio.

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.

- 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.

- 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.

- At best, the rate converters (plugin of plugin) should support both cases with and without float.

thanks,

Takashi

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