At Fri, 23 Mar 2007 00:36:00 +1100, Jean-Marc Valin wrote:
This is different understanding between us.
I'm working at a distro vendor, and as a responsible person for distribution packages, the first thought is maintainability.
So far we agree.
Good.
For that purpase, I prefer very much a shared library approach because I know the hell of vice of copying codes. Copying code works well if it's 100% safe and mature code, which requires no longer updates and fixes. But a code being developed shouldn't be copied.
Not sure I agree here. Do you really want to depend on libspeex "blindly" not knowing what changes I make (while the code is evolving). Code copying is not necessarily bad. After all, that what the kernel does with the ALSA drivers -- sync up every once and then.
Well, maybe that's the whole point -- how to manage the included code. If it's linked to libspeex, yes, the plugin would rely on libspeex in 100%. This allows the automatic addition of new features like SIMD support. But, this is a drawback, too, if the library gets broken although in this case, the possible problem could be easily removed by removing the offending plugin. (The alsa-lib itself won't be linked to libspeex, anyway. Only the plugin will.)
That is, the only criteria for inclusion is whether the code is well tested and no more fix/update is needed. In this case, whether resample code has to be further sync'ed with speex codebase or not. If it should be kept synced, then shlib approach is clearly better. Otherwise we have to take care of two places at the same time.
I guess my main question would be: are you willing to make libspeex a *mandatory* dependency? If not, please include a copy of the resampler. From now on, there's no way alsa-lib should ever be built with linear interpolation resampling unless the user specifically configures it with --enable-awful-audio-mutilation-code or something like that.
Oh, I'm in very favor of using a better resampler as default, too.
Regarding code-rewriting, I think you misunderstood my early comment. It's not the urgent issue to rewrite the code indeed. But, it may (likely) happen to get rid of the speex-specific redundant codes afterwards or change the indentation and coding styles in near future in a long run afterwards. This is how the code is being maintained just like in the linux kernel tree.
So, as long as we'll be able to play with the code in alsa-lib tree as we like, and you agree with LGPL, the code inclusion can happen easily. Of course, for reducing the future maintenance cost, we should double-check the code before inclusion. But, in the case of resampler, the code flow is easy, so it wouldn't be a big obstacle :)
BTW, one remaining concern is architectures that don't like math library. Could sin() and floating point operations be removed from resampler code? Otherwise, I can make the built of pph resampler conditonal to softfloat configure option.
Or, in other words: the sound quality could be a matter of taste, but a security fix can't be so :)
I don't see much security issues here. The code is really simple and (unless I've done something really obviously stupid -- which I don't think I have) you can easily show that there's no buffer overflow possible.
Well, you should check the return value of speex_*alloc() at least ;)
thanks,
Takashi