Re: Conversion Layer.

[Daniel K. <ko...@ta...>]

On Mon, Mar 26, 2001 at 01:34:14PM +0200, Richard Guenther wrote:
> Few comments to the API:
> - as glame does not have a plugin A_B_I, but only a A_S_I (application
>   source interface) (i.e. plugins compiled for glame-0.4.0 are not
>   guaranteed to work with glame-0.4.1) the versioning stuff is pointless.

Sure, they are not guarranteed to work, but they not guarranteed to break
in obvious ways either. Interfaces may get messed up in more subtle ways
than what makes the dynamic loader scream. In other words, the versioning
stuff is about notifying external plugins of semantical changes in the
API, not only about syntactical ones. Admittedly, I'm not too fond of this
part either, but I thinks it can make life easier. If however it's commonly
agreed to be a bad thing, I'm not too keen on forcefully keeping it in.

> - if the gcv_layout_t ever becomes something more than just a collection
>   of attributes the gcv_set_* routines will impose difficulties on
>   the implementation. So I suggest to just have gcv_{get|set|drop}_layout.

Ummm..., could you elaborate on this one? The layout stuff as I see it is 
designed to hold the complete set of attributes of a data stream and nothing 
more. Hence the twofold process--set up static attributes in *layout*,
commit layouts in *conversion*. I don't quite see how gcv_set_* might
hinder further development. On the contrary, I'd be much more inclined to
drop the all-in-one gcv_set_layout() as it means I'll have to change the
source of each and every i/o plugin when a new flag gets added to *layout.
Think about the #ifdef mess this will cause on a plugin that's meant to
run on GLAME 0.3.99 to 0.5.28. Ugh! This may be a non-issue today with a
unified source tree, but one day we want external plugins and hopefully
we'll get them. I'm not advocating a stable ABI, but we should at least
try to stay source compatible across version, especially when it's as
easy as here. If you want to see a good example of how to piss off 
developers by adding new parameters to API functions in every fscking
minor revision, have a look at ImageMagick.

> - same goes for gcv_update_conversion - drop it.

The way I see it, gcv_update_conversion() is not strictly part of the
API. It's merely a wrapper around *drop_conversion() and *get_conversion().
Why? Because this sequence is likely to be used in each and every
connect*() and set_param() method. But I can leave it out of the API
proper easily.

>   Note that you want an
>   additional parameter to gcv_get_conversion denoting if you allow an
>   in-place conversion. Have two gcv_do_conversion routines, one for
>   not-in-place (as it is now), one for in-place conversion (that does
>   an additional copy from bounce dest to source, if required) - of course
>   check, if the right one is chosen by the user (obviously only the one
>   for in-place can be used, if the conversion was set up for in-place).
>   Perhaps as for do_conversion have two different functions for setup,
>   too (rather than an additional parameter).

Yup, that's what my FIXME is about, and I agree. I'll decide between flag
and new API call when I actually implement gcv_do_conversion. It has to
be clever about bounce buffers anyway, so maybe this additional step
comes in at almost zero cost, and a simple flag is sufficient. We'll see.

Thanks for your comments!

*nold.