RE: [MP3 ENCODER] TagItem issues...
Mathew Hendry schrieb am Don, 28 Sep 2000: > > From: Mathew Hendry > > > > is pre-ANSI and non-standard - use for > > the standard stuff. Should be > > > > va_start(ap) > > Oops! I mean > > va_start(ap, lastarg) > > where ap is your va_list and lastarg is the last non-variadic parameter. > > > va_arg(ap, type) > > And not forgetting > > va_end(ap) > > although on most systems it doesn't do much. Best to be compliant if > possible, though... > > -- Mat. Seeing all those stuff and having printf in mind with all its affinity to trash the output, I'm getting more and more to the conclusion, the best way would be to have one function for every parameter to set. This way it will be easier to check for valid ranges etc. But Frank already pointed that out. Ciao Robert -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
RE: [MP3 ENCODER] TagItem issues...
> From: Mathew Hendry > > is pre-ANSI and non-standard - use for > the standard stuff. Should be > > va_start(ap) Oops! I mean va_start(ap, lastarg) where ap is your va_list and lastarg is the last non-variadic parameter. > va_arg(ap, type) And not forgetting va_end(ap) although on most systems it doesn't do much. Best to be compliant if possible, though... -- Mat. -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
RE: [MP3 ENCODER] TagItem issues...
> -Original Message- > From: David Balazic [mailto:[EMAIL PROTECTED]] > > Sigbjřrn Skjćret wrote: > > >/* in my man page va_start has only one argument (ap) ... */ > > > > Your man-page is wrong then I think, the second argument is > so that va_arg() > > knows where to start (ie, it makes ap point to the next argument). > > HP-UX 10.20 man varargs say that it has only one arg. > /usr/include/varargs.h on the same system has definitions > with both two or one argument. Which one is used is dependent > on some preprocessor variables ( platform/CPU type , I guess ). > > A linux libc5 system has a definition in include files with > one arg too. is pre-ANSI and non-standard - use for the standard stuff. Should be va_start(ap) va_arg(ap, type) -- Mat. -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Frank Klemm wrote:
>
> :: /* Usage :
> :: lame_set_parameters(gfp, LAME_SAMP_FREQ__INT, 44100 ,
> :: LAME_NR_CHANNELS__INT , 2 ,
> :: LAME_LOW_PASS_VALUE__FLOAT , 16.050 ,
> :: LAME_LOW_PASS_WIDTH__FLOAT , 0.75,
> :: LAME_COMMENT__STRING , "This is cool" ,
> :: LAME_END_MARKER);
> ::
> :: LAME_END_MARKER informs the lame_set_parameters to stop reading values,
> :: as AFAIK
> :: there is not other way to determine whether all the args has been read
> ::
> :: the __FLOAT and __INT attachments are there only to help illustrate my
> :: idea,
> :: but could be helpful in the actual implementation too
> ::
> :: */
> ::
> One function, but is this simple?
>
> No error checking,
Well , your functions below don't even have a return code.
But the compiler can not check if the arguments are of the correct type,
as already noticed.
> a lot of really strange constants, mixing Hz and kHz.
What strange constants ? The tag names ? You need somehow to
differentiate
between them, just the same as you used different names for each
function.
The names I used and the parameters were just as an example, I'm not
saying
that samp.freq. should be Hz in integer and filter band in kHz float !
> Do
> you like bugs? Do you love bugs? Do you buy a car with only one joystick to
> do all operations (including wheel- and oil-change)?
No. No. There are no such cars :-)
> Only one joystick, that means very simple operations! Welcome on the graveyard.
>
> typedef struct {
> void* ptr;
> const size_t size;
> size_t len;
> } bytebuffer_t;
>
> bytebuffer_t bb;
>
> LAME* gfp;
>
> gfp = lame_open ();
> lame_set_samplefreq( gfp, 44100.0 );
> lame_set_channels ( gfp, 2 );
> lame_set_lowpass ( gfp, 16050.0 );
> lame_set_lowpass_width ( gfp, 750.0 );
> lame_set_lowpass_type ( gfp, lame_filter_chebychev, 8, 0.5 );
> lame_set_comment ( gfp, "This is not error prone" );
> lame_add_comment ( gfp, "Developers will not hate lame developers" );
> lame_add_comment ( gfp, "Lame developers do not need a body guard" );
What more error checking is involved in the above than in my example ?
Off course this is a valid alternative to tagitems.
I not saying "use tagitems", but "if you use tagitems, then this idea of
mine
would make it better".
I don't think the following code example is related to the tagitems
topic.
> alloc_bytebuffer ( &bb, 16384 );
>
> error = lame_encode_buffer ( gfp, &bb, 88200, channel_left,
>channel_right );
> if (error)
> lame_perror ( "The following error occured", error );
> fwrite ( fp, 1, bb.len, bb.ptr );
>
> error = lame_encode_interleaved_buffer ( gfp, &bb, 88200, interleaved_buffer );
> if (error)
> lame_perror ( "The following error occured", error );
> fwrite ( fp, 1, bb.len, bb.ptr );
>
> error = lame_encode_finish ( gfp, &bb );
> if (error)
> lame_perror ( "The following error occured", error );
> fwrite ( fp, 1, bb.len, bb.ptr );
>
> if ( bb.size > 16384 )
> printf ( "BitBuffer was increased by a subroutine, instead of crashing!\n" );
>
> free_bytebuffer ( &bb );
>
> error = lame_close ( gfp );
> if (error)
> lame_perror ( "Shit, the last operation and a error", error );
>
> --
> Mit freundlichen Grüßen
> Frank Klemm
>
> eMail | [EMAIL PROTECTED] home: [EMAIL PROTECTED]
> phone | +49 (3641) 64-2721home: +49 (3641) 390545
> sMail | R.-Breitscheid-Str. 43, 07747 Jena, Germany
>
> --
> MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
--
David Balazic
--
"Be excellent to each other." - Bill & Ted
- - - - - - - - - - - - - - - - - - - - - -
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Sigbjřrn Skjćret wrote: > >/* in my man page va_start has only one argument (ap) ... */ > > Your man-page is wrong then I think, the second argument is so that va_arg() > knows where to start (ie, it makes ap point to the next argument). HP-UX 10.20 man varargs say that it has only one arg. /usr/include/varargs.h on the same system has definitions with both two or one argument. Which one is used is dependent on some preprocessor variables ( platform/CPU type , I guess ). A linux libc5 system has a definition in include files with one arg too. > >> I can still implement a way to have the tag also contain the type of the > >> parameter, but since the general consensus seemed to be that this would be > >> too complicated I went for this approach... ;) > >But your existing tag "contain" the type , at least as much as they > >would in my method. > > What I meant was that I could device another way of passing the tags that > would allow me to supply information of the parameters type (ptr/float/int), > but this isn't too convenient at the calling end. > > >#ifdef YOU > >#define SOME_TAG some_int_value > >#else > >#define SOME_TAG some_int_value > >#end > > What was the purpose of this? To demonstrate that your tags have the same as much type information as mine :-) > >Unless I use > >SOME_TAG__WARNING_THIS_IS_FLOAT_AND_NOT_INT_OR_ANYTHING_ELSE, > >but that is not the point , since it would work both with your and my > >code. > > The advantage of having 3 different functions is also that you at a later > stage can change the type of the internal parameter, and still accept the > old without having to add a duplicate tag (f.ex. if you change from int to > float, then you can just add the tag to lame_set_float(), and add a cast in > lame_set_int)... Oh , code complexity for the sake of backward compatibility :-) -- David Balazic -- "Be excellent to each other." - Bill & Ted - - - - - - - - - - - - - - - - - - - - - - -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
:: /* Usage :
:: lame_set_parameters(gfp, LAME_SAMP_FREQ__INT, 44100 ,
:: LAME_NR_CHANNELS__INT , 2 ,
:: LAME_LOW_PASS_VALUE__FLOAT , 16.050 ,
:: LAME_LOW_PASS_WIDTH__FLOAT , 0.75,
:: LAME_COMMENT__STRING , "This is cool" ,
:: LAME_END_MARKER);
::
:: LAME_END_MARKER informs the lame_set_parameters to stop reading values,
:: as AFAIK
:: there is not other way to determine whether all the args has been read
::
:: the __FLOAT and __INT attachments are there only to help illustrate my
:: idea,
:: but could be helpful in the actual implementation too
::
:: */
::
One function, but is this simple?
No error checking, a lot of really strange constants, mixing Hz and kHz. Do
you like bugs? Do you love bugs? Do you buy a car with only one joystick to
do all operations (including wheel- and oil-change)?
Only one joystick, that means very simple operations! Welcome on the graveyard.
typedef struct {
void* ptr;
const size_t size;
size_t len;
} bytebuffer_t;
bytebuffer_t bb;
LAME* gfp;
gfp = lame_open ();
lame_set_samplefreq( gfp, 44100.0 );
lame_set_channels ( gfp, 2 );
lame_set_lowpass ( gfp, 16050.0 );
lame_set_lowpass_width ( gfp, 750.0 );
lame_set_lowpass_type ( gfp, lame_filter_chebychev, 8, 0.5 );
lame_set_comment ( gfp, "This is not error prone" );
lame_add_comment ( gfp, "Developers will not hate lame developers" );
lame_add_comment ( gfp, "Lame developers do not need a body guard" );
alloc_bytebuffer ( &bb, 16384 );
error = lame_encode_buffer ( gfp, &bb, 88200, channel_left, channel_right
);
if (error)
lame_perror ( "The following error occured", error );
fwrite ( fp, 1, bb.len, bb.ptr );
error = lame_encode_interleaved_buffer ( gfp, &bb, 88200, interleaved_buffer );
if (error)
lame_perror ( "The following error occured", error );
fwrite ( fp, 1, bb.len, bb.ptr );
error = lame_encode_finish ( gfp, &bb );
if (error)
lame_perror ( "The following error occured", error );
fwrite ( fp, 1, bb.len, bb.ptr );
if ( bb.size > 16384 )
printf ( "BitBuffer was increased by a subroutine, instead of crashing!\n" );
free_bytebuffer ( &bb );
error = lame_close ( gfp );
if (error)
lame_perror ( "Shit, the last operation and a error", error );
--
Mit freundlichen Grüßen
Frank Klemm
eMail | [EMAIL PROTECTED] home: [EMAIL PROTECTED]
phone | +49 (3641) 64-2721home: +49 (3641) 390545
sMail | R.-Breitscheid-Str. 43, 07747 Jena, Germany
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> This is how it looks now, better than using va_arg() at each case imho... >Why is it better ? >You wasted an additional variable and gained ( maybe ) some code space >by not doing a function call ( or maybe it is a macro , probably >platform dependant ) every time. It's usually a (rather messy) macro. ;) >By writing almost the same code 3 times you increase the likehood of >bugs 3 times :-) >Alltough most of the code would be repeated anyway. True, but there's not much space for error. ;) [...] >#define CASE(TAG,elem,type) case LAME_ ## TAG : gfp -> elem = >va_arg(args, type); break; > CASE( SET_SAMPLING_FREQ , samp_freq, int ) [...] That's actually not a bad idea... >/* in my man page va_start has only one argument (ap) ... */ Your man-page is wrong then I think, the second argument is so that va_arg() knows where to start (ie, it makes ap point to the next argument). >> I can still implement a way to have the tag also contain the type of the >> parameter, but since the general consensus seemed to be that this would be >> too complicated I went for this approach... ;) >But your existing tag "contain" the type , at least as much as they >would in my method. What I meant was that I could device another way of passing the tags that would allow me to supply information of the parameters type (ptr/float/int), but this isn't too convenient at the calling end. >#ifdef YOU >#define SOME_TAG some_int_value >#else >#define SOME_TAG some_int_value >#end What was the purpose of this? >Unless I use >SOME_TAG__WARNING_THIS_IS_FLOAT_AND_NOT_INT_OR_ANYTHING_ELSE, >but that is not the point , since it would work both with your and my >code. The advantage of having 3 different functions is also that you at a later stage can change the type of the internal parameter, and still accept the old without having to add a duplicate tag (f.ex. if you change from int to float, then you can just add the tag to lame_set_float(), and add a cast in lame_set_int)... - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Sigbjřrn Skjćret wrote:
>
> >> and parsing gets abit more messy...
> >How ?
> [...]
>
> This is how it looks now, better than using va_arg() at each case imho...
Why is it better ?
You wasted an additional variable and gained ( maybe ) some code space
by not doing a function call ( or maybe it is a macro , probably
platform dependant ) every time.
By writing almost the same code 3 times you increase the likehood of
bugs 3 times :-)
Alltough most of the code would be repeated anyway.
My "more clear" version of the day :
( after the code are some more replyes to the original message )
int lame_set_parameters(lama_global_flags *gfp,...)
{
va_list args;
LAME_TAG_TYPE current_tag;
va_start(args);
current_tag = va_arg(args,LAME_TAG_TYPE);
while(current_tag != LAME_TAG_END )
{
#define CASE(TAG,elem,type) case LAME_ ## TAG : gfp -> elem =
va_arg(args, type); break;
switch(current_tag) {
CASE( SET_SAMPLING_FREQ , samp_freq, int )
CASE( SET_NR_CHAN , nr_channels , int )
CASE( FILTER_WIDTH , filter_width , float )
CASE( SOME_COMMENT , some_comment , char* )
/* some other integer, string or float or whatever parameters
. */
#undef CASE
default : return RET_TAG_ERROR_CODE; /* also print a warning ?
probably not if this is a library.
default : return -current_tag; so the app knows which tag is
problematic
or maybe return the position of the
bad tag ? */
}
current_tag = va_arg(args,LAME_TAG_TYPE);
}
va_end(args);
}
Here is the old version for reference :
( there is a bug already !
it should be lame_struct -> xxx and not
lame_struct.xxx
)
lame_set_parameters(lama_global_flags *lame_struct,...)
{
va_list args;
LAME_TAG_TYPE current_tag;
va_start(args);
current_tag = va_arg(args,LAME_TAG_TYPE);
while(current_tag != LAME_TAG_END )
{
switch(current_tag) {
case LAME_SET_SAMPLING_FREQ : lame_struct.samp_freq =
va_arg(args,int); break;
case LAME_SET_NR_CHAN : lame_struct.nr_channels =
va_arg(args,int); break;
case LAME_FILTER_WIDTH : lame_struct.filter_width =
va_arg(args,float); break;
/* some other integer, string or float or whatever parameters */
}
current_tag = va_arg(args,LAME_TAG_TYPE);
}
va_end(args);
}
> void lame_set_int(lame_global_flags *gfp, lame_param_tag tag1, ...)
> {
>va_list ap;
>int param; /* (varargs promotes integrals to int) */
>
>va_start(ap, tag1);
/* in my man page va_start has only one argument (ap) ... */
>while (tag1 != TAG_END)
>{
> param = va_arg(ap, int);
>
> switch (tag1)
> {
>case LAMEtag1:
> gfp->test1 = param;
> break;
>
>case LAMEtag2:
> gfp->test2 = param;
> break;
>
>default:
> break;
> }
>
> tag1 = va_arg(ap, lame_param_tag);
>}
>va_end(ap);
> }
>
> I can still implement a way to have the tag also contain the type of the
> parameter, but since the general consensus seemed to be that this would be
> too complicated I went for this approach... ;)
But your existing tag "contain" the type , at least as much as they
would
in my method.
#ifdef YOU
#define SOME_TAG some_int_value
#else
#define SOME_TAG some_int_value
#end
Unless I use
SOME_TAG__WARNING_THIS_IS_FLOAT_AND_NOT_INT_OR_ANYTHING_ELSE,
but that is not the point , since it would work both with your and my
code.
I'm in hurry, forgive any mistakes :-)
> - CISC
>
> --
> MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
--
David Balazic
--
"Be excellent to each other." - Bill & Ted
- - - - - - - - - - - - - - - - - - - - - -
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> and parsing gets abit more messy...
>How ?
[...]
This is how it looks now, better than using va_arg() at each case imho...
void lame_set_int(lame_global_flags *gfp, lame_param_tag tag1, ...)
{
va_list ap;
int param; /* (varargs promotes integrals to int) */
va_start(ap, tag1);
while (tag1 != TAG_END)
{
param = va_arg(ap, int);
switch (tag1)
{
case LAMEtag1:
gfp->test1 = param;
break;
case LAMEtag2:
gfp->test2 = param;
break;
default:
break;
}
tag1 = va_arg(ap, lame_param_tag);
}
va_end(ap);
}
I can still implement a way to have the tag also contain the type of the
parameter, but since the general consensus seemed to be that this would be
too complicated I went for this approach... ;)
- CISC
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Sigbjřrn Skjćret wrote:
> >there is not other way to determine whether all the args has been read
> >the __FLOAT and __INT attachments are there only to help illustrate my
> >idea,
> >but could be helpful in the actual implementation too
>
> Yes, but this still requires the parameters to be correct,
I don't think there is a way to make it work with incorrect
parameters :-)
> and parsing gets abit more messy...
How ?
The 3 functions method , the one for int parameters :
lame_set_parameter_int(lama_global_flags *lame_struct,...)
{
va_list args;
LAME_TAG_TYPE current_tag;
va_start(args);
current_tag = va_arg(args,LAME_TAG_TYPE);
while(current_tag != LAME_TAG_END )
{
switch(current_tag) {
case LAME_SET_SAMPLING_FREQ : lame_struct.samp_freq =
va_arg(args,int); break;
case LAME_SET_NR_CHAN :lame_struct.nr_channels =
va_arg(args,int); break;
/* some other integer parameters */
}
current_tag = va_arg(args,LAME_TAG_TYPE);
}
va_end(args);
}
the "unified function version :
lame_set_parameters(lama_global_flags *lame_struct,...)
{
va_list args;
LAME_TAG_TYPE current_tag;
va_start(args);
current_tag = va_arg(args,LAME_TAG_TYPE);
while(current_tag != LAME_TAG_END )
{
switch(current_tag) {
case LAME_SET_SAMPLING_FREQ : lame_struct.samp_freq =
va_arg(args,int); break;
case LAME_SET_NR_CHAN : lame_struct.nr_channels =
va_arg(args,int); break;
case LAME_FILTER_WIDTH : lame_struct.filter_width =
va_arg(args,float); break;
/* some other integer, string or float or whatever parameters */
}
current_tag = va_arg(args,LAME_TAG_TYPE);
}
va_end(args);
}
--
David Balazic
--
"Be excellent to each other." - Bill & Ted
- - - - - - - - - - - - - - - - - - - - - -
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> o main.c can't hold lame_global_flags, change lame_init() to allocate and >> return a pointer to lame_global_flags. >> o Move lame_global_flags out of lame.h, it should never be accessed >> externally. >I think we have to keep the old interface, since several applications >use it. So lame_global_flags will still have to be instantiated by the >calling program and exposed to the calling program. I dont think this >is incompatiable with a shared library? Yes, it is, because the structure itself can expand and change at any given time internally, and if it's allocated and/or changed externally it will most surely get corrupted by any program using a different version of the structure than the library itself... - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>IIRC, if you use varargs then the compiler can't detect if the supplied >parameters >are of wrong type. Example : [...] >The compiler won't notice that the parameter is of wrong type >and when the function will try to extract an int from the stack, >it will get garbage. This is indeed true, and that's why I chose to divide the function in 3 to clarify this to the user. >But if this is of no concern , then a simpler interface can be used : [...] >LAME_END_MARKER informs the lame_set_parameters to stop reading values, >as AFAIK This is exactly how my current interface works (based on TagItems when passed on stack). >there is not other way to determine whether all the args has been read >the __FLOAT and __INT attachments are there only to help illustrate my >idea, >but could be helpful in the actual implementation too Yes, but this still requires the parameters to be correct, and parsing gets abit more messy... - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
> "R" == Robert Hegemann <[EMAIL PROTECTED]> writes: R> Mark Taylor schrieb am Die, 26 Sep 2000: >> I think we have to keep the old interface, since several >> applications use it. So lame_global_flags will still have to >> be instantiated by the calling program and exposed to the >> calling program. I dont think this is incompatiable with a >> shared library? >> >> Mark R> Doing so would require to recompile every client if we add an R> entry in lame_global_flags, even if the client will not care R> about the new entry. Yes, That is the most heavy problem. even worse, changing compiler or compiler option will make it require to recompile every client. R> The client shouldn't know about internal data structures. So, R> doing as Frank previously suggested would be OK. I agree, too. --- Takehiro TOMINAGA // may the source be with you! -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Sigbjřrn Skjćret wrote: > > >> >This way there is no need to parse any strings, we don't pass > >> >any pointers, the setup routine would just be a big switch/case. > >> This is basically the way TagItems work when passed on stack, in fact, that > >> combined with another identical function that does float we're pretty much > >> set. > > Why do we need float at this point ?? > > Because several of the parsed arguments are floats? > > Well, anyway, I just finished up the following: > > /* Set a parameter with string-pointer */ > void lame_set_string(lame_global_flags *gfp, lame_param_tag tag1, ...); > > /* Set a parameter with float or double value (varargs promotes floats to double) */ > void lame_set_float(lame_global_flags *gfp, lame_param_tag tag1, ...); > > /* Set a parameter with char, short or int value (varargs promotes integrals to int) >*/ > void lame_set_int(lame_global_flags *gfp, lame_param_tag tag1, ...); IIRC, if you use varargs then the compiler can't detect if the supplied parameters are of wrong type. Example : /* sample freq is an int , but be supply a float */ lame_set_int(gfp, LAME_SAMP_FREQ, 44101.213 ); The compiler won't notice that the parameter is of wrong type and when the function will try to extract an int from the stack, it will get garbage. But if this is of no concern , then a simpler interface can be used : lame_set_parameters(lame_global_flags *gfp, ...); /* or this one , as the type of the first argument is always the same */ lame_set_parameters(lame_global_flags *gfp, lame_param_tag tag1, ...); /* Usage : lame_set_parameters(gfp, LAME_SAMP_FREQ__INT, 44100 , LAME_NR_CHANNELS__INT , 2 , LAME_LOW_PASS_VALUE__FLOAT , 16.050 , LAME_LOW_PASS_WIDTH__FLOAT , 0.75, LAME_COMMENT__STRING , "This is cool" , LAME_END_MARKER); LAME_END_MARKER informs the lame_set_parameters to stop reading values, as AFAIK there is not other way to determine whether all the args has been read the __FLOAT and __INT attachments are there only to help illustrate my idea, but could be helpful in the actual implementation too */ > This can cater for any kind of parameter, and the functions themselves are > very simple, yet can take several parameters in a row... > > Should I start implementing this now, or wait after 3.87 release? > > Mark? > > What needs to be changed to make a real shared library: > > o Exchange all direct access to lame_global_flags in parse.c with calls to > lame_set_xxx(). > > o main.c can't hold lame_global_flags, change lame_init() to allocate and > return a pointer to lame_global_flags. > > o Move lame_global_flags out of lame.h, it should never be accessed externally. > > o ..and any other changes still needed for re-entrance ofcos. > > - CISC > > -- > MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ ) -- David Balazic -- "Be excellent to each other." - Bill & Ted - - - - - - - - - - - - - - - - - - - - - - -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
:: Sigbjørn Skjæret schrieb am Die, 26 Sep 2000: :: > > Why do we need float at this point ?? :: > :: > Because several of the parsed arguments are floats? :: :: ie. frequencies could be passed in Hertz as ints, :: was just something to think about now while we :: change the API anyway :: What about not integer value sampling frequencies? For stand alone audio rounding is no problem. But for audio/video and unbuffered streaming this may produce really unnecessary problems. For instance AIFF stores sampling frequency as 80 bit-IEEE-754 long double. Avoid problems instead of patching the results of this problems. -- Mit freundlichen Grüßen Frank Klemm eMail | [EMAIL PROTECTED] home: [EMAIL PROTECTED] phone | +49 (3641) 64-2721home: +49 (3641) 390545 sMail | R.-Breitscheid-Str. 43, 07747 Jena, Germany -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Mark Taylor schrieb am Die, 26 Sep 2000: > I think we have to keep the old interface, since several applications > use it. So lame_global_flags will still have to be instantiated by the > calling program and exposed to the calling program. I dont think this > is incompatiable with a shared library? > > Mark Doing so would require to recompile every client if we add an entry in lame_global_flags, even if the client will not care about the new entry. The client shouldn't know about internal data structures. So, doing as Frank previously suggested would be OK. Ciao Robert -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
> > Should I start implementing this now, or wait after 3.87 release? > Hi CISC, 3.87 was released last night. It's mostly just to have a last release before the code re-orginization that Takehiro is going to do this week. That will involve splitting the C code into a frontend and library directory. So it would be good if we could hold off on all commits until this is done. > > What needs to be changed to make a real shared library: > > o Exchange all direct access to lame_global_flags in parse.c with calls to > lame_set_xxx(). > > o main.c can't hold lame_global_flags, change lame_init() to allocate and > return a pointer to lame_global_flags. > > o Move lame_global_flags out of lame.h, it should never be accessed externally. > > o ..and any other changes still needed for re-entrance ofcos. > > I think we have to keep the old interface, since several applications use it. So lame_global_flags will still have to be instantiated by the calling program and exposed to the calling program. I dont think this is incompatiable with a shared library? Mark -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> >Why do we need float at this point ?? >> Because several of the parsed arguments are floats? > ie. frequencies could be passed in Hertz as ints, > was just something to think about now while we > change the API anyway Yes, but no reason not to have the possibility there. ;) >> Should I start implementing this now, or wait after 3.87 release? > I think 3.87 is already out, CVS is tagged 3.88 alpha 1 Ah, yes, so I see... > You may get in contact with Takehiro to see what will be done > next. Takehiro, wanna drop me a line when you are finished moving stuff about? ;) - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
Sigbjørn Skjæret schrieb am Die, 26 Sep 2000: > > Why do we need float at this point ?? > > Because several of the parsed arguments are floats? ie. frequencies could be passed in Hertz as ints, was just something to think about now while we change the API anyway > Should I start implementing this now, or wait after 3.87 release? I think 3.87 is already out, CVS is tagged 3.88 alpha 1 You may get in contact with Takehiro to see what will be done next. Ciao Robert -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>This sounds more complex than my proposed "void*" change. Keep in mind: More complex, but not that hard. >>>"KISS". >> I'm not even gonna ask. ;) > "Keep It Simple, Stupid". [...] Right, well, I believe that's exactly what it is now. ;) - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> >This way there is no need to parse any strings, we don't pass >> >any pointers, the setup routine would just be a big switch/case. >> This is basically the way TagItems work when passed on stack, in fact, that >> combined with another identical function that does float we're pretty much >> set. > Why do we need float at this point ?? Because several of the parsed arguments are floats? Well, anyway, I just finished up the following: /* Set a parameter with string-pointer */ void lame_set_string(lame_global_flags *gfp, lame_param_tag tag1, ...); /* Set a parameter with float or double value (varargs promotes floats to double) */ void lame_set_float(lame_global_flags *gfp, lame_param_tag tag1, ...); /* Set a parameter with char, short or int value (varargs promotes integrals to int) */ void lame_set_int(lame_global_flags *gfp, lame_param_tag tag1, ...); This can cater for any kind of parameter, and the functions themselves are very simple, yet can take several parameters in a row... Should I start implementing this now, or wait after 3.87 release? Mark? What needs to be changed to make a real shared library: o Exchange all direct access to lame_global_flags in parse.c with calls to lame_set_xxx(). o main.c can't hold lame_global_flags, change lame_init() to allocate and return a pointer to lame_global_flags. o Move lame_global_flags out of lame.h, it should never be accessed externally. o ..and any other changes still needed for re-entrance ofcos. - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
On 25 Sep, Sigbjørn Skjæret wrote: > simply making a new TagItems interface which allows you to specify the > type of data the TagItem contains. This will be much better in the long > run, and will allow the TagItems to contain any type of data. This sounds more complex than my proposed "void*" change. Keep in mind: >>> More complex, but not that hard. >>"KISS". > > I'm not even gonna ask. ;) ---snip--- KISS Principle /kis' prin'si-pl/ n. "Keep It Simple, Stupid". A maxim often invoked when discussing design to fend off creeping featurism and control development complexity. Possibly related to the marketroid maxim on sales presentations, "Keep It Short and Simple". ---snip--- Bye, Alexander. -- I believe the technical term is "Oops!" http://www.Leidinger.net Alexander @ Leidinger.net GPG fingerprint = 7423 F3E6 3A7E B334 A9CC B10A 1F5F 130A A638 6E7E -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
::
:: proposal:
::
:: int lame_open();
:: // allocates internal structures
::
:: int lame_set_param( int handle, LameParameter Parameter, int Value );
:: // assigns Lame's parameter a value
::
a) Not every variable is an integer. C programmer try to store everything
(food, women, cars ;-) in an int, but from the state of good programming
style this is brain dead and not far from FORTRAN 4.
b) Brain dead interfaces creates things like:
double d = 3.14159265358979;
float f = d;
func ( *(int*)&f ); // transmit float value through int
func ( (int)stdout );// fails on sizeof(void*) > sizeof(int)
c) It is wise to return the actual parameter
d) it is wise to return an status word (error code)
e) it is wise not to use in-band-signaling for error codes
f) Sometimes it is useful to change some parameters atomically
(For RT systems this is often an indispensable need).
So we got something like:
int lame_set_param (
inthandle,
LameParameter ParameterType,
... );
// assigns Lame's parameter a value
Examples:
long double ld;
double d;
int i1;
int i2;
FILE* fp;
ld = 44055.938461538; // NTSC PCM, lips synchr.
printf ( "Input fs is %lf, ", ld );
err = lame_set_param ( lh, Lame_LD_InputSampleFreq, &ld )
printf ( "set to %lf.\n", ld );
i1 = 128000;
i2 = 256000;
printf ( "VBR bitrate should be between %7.3f...%7.3f, ", i1*1.e-3, i2*1.e-3 );
err = lame_set_param ( lh, Lame_I2_VBRBitrateRange, &i1, &i2 )
printf ( "set to %7.3f...%7.3f\n", i1*1.e-3, i2*1.e-3 );
fp = fopen ( "report", "w" );
err = lame_set_param ( lh, Lame_fp_Reportfp, &fp );
Another question: Why not using a set of functions to setup parameters.
It is not more difficult, but less, it does not disable type checking and
link checking.
This is also something I call: "cosmetic fake simplification".
It:
* moves complexity instead of reducing (virtual hiding)
* disables type checking
* disables functionality checking by the linker
* it clouds errors (compile/link time => run time or later)
Okay. It's C ;-)
:: int lame_init_params( int handle );
:: // fills up rest of needed parameters by default values
::
Not necessary. Can be done by lame_encode stuff.
lame__flags has an internal flag:
0: invalid, before open() or after close()
1: valid, open() was done, ready for setup
2: the first lame_encode_...() stuff was called (finalization of params)
::
:: usual lame_encode() stuff
::
:: void lame_close( int handle );
:: // frees all allocated internal structures
::
::
:: just enumerate all parameters LAME will use, we can allways append
:: new ones if needed. A 32 bit int should provide us enough internal
:: parameters ;)
::
:: typedef enum{
:: lp_bitrate,
:: lp_vbrmode,
:: lp_vbrquality,
:: lp_vbrmin,
:: lp_vbrmax,
:: lp_crc,
:: :
:: } LameParameter;
::
Another point is: It is not wise to mix
-vbr -b and -cbr -b
I would use three parameters for -vbr -b, -vbr -B and -cbr -b.
::
:: // different channel modes
:: #define CM_MONO 1
:: #define CM_LR_STEREO 2
:: #define CM_J_STEREO 3
:: #define CM_MS_STEREO 4
:: ...
::
:: Example setup sequence:
::
:: handle = lame_open();
::
:: lame_set_param( handle, lp_bitrate, 128 );
:: lame_set_param( handle, lp_crc, FALSE );
:: lame_set_param( handle, lp_original, FALSE );
:: lame_set_param( handle, lp_channelmode, CM_J_STEREO );
:: lame_set_param( handle, lp_quality, 2 );
::
:: lame_set_param( handle, lp_lowpass, 5500 );
:: lame_set_param( handle, lp_lowpasswidth, 500 );
:: lame_set_param( handle, lp_highpass, 100 );
::
:: lame_set_param( handle, lp_resample, 11025 );
::
:: lame_set_param( handle, lp_voice, TRUE );
::
:: lame_init_params( handle );
::
::
::
:: lame_close( handle );
::
:: This way there is no need to parse any strings, we don't pass
:: any pointers, the setup routine would just be a big switch/case.
::
An additional demultiplexer/multiplexer.
::
handle = lame_open ();
lame_set_cbr_bitrate ( handle, 128000 );
lame_set_crc ( handle, disable );
lame_set_genuine ( handle, false );
lame_set_channel_mode ( handle, channelmode_jstereo );
lame_set_quality ( handle, 100-2*100/9 ); // arbitrary values are scaled
(bad=0,best=100)
lame_set_lowpass ( handle, 5500.0 );
lame_set_lowpass_width( handle, 500.0 );
lame_set_lowpass_width_ratio ( handle, 0.0909 );
lame_set_highpass ( handle, 100.0 );
lame_set_output_samplefreq ( handle, 11025.0 );
lame_set_presets ( handle, preset_voice );
lame_close ( handle );
Handles have advantages and disadvantage
Re: [MP3 ENCODER] TagItem issues...
Sigbjørn Skjæret schrieb am Mon, 25 Sep 2000: > [...] > >This way there is no need to parse any strings, we don't pass > >any pointers, the setup routine would just be a big switch/case. > > This is basically the way TagItems work when passed on stack, in fact, that > combined with another identical function that does float we're pretty much > set. Why do we need float at this point ?? > > > - CISC Ciao Robert -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>Why not something like: >lame_set_float(char *variable_name, float val); >lame_set_int(char *variable_name, int val); Hey, gr8, idea, and maybe another with void*? >But this brings up another question: The code is going to do a lot of >parsing, and it looks like each variable will require several lines of >code? If that is the case, then we dont actually save anything and we >might as well just creating a short function for every variable? This >way there are no type casting problems. Ah, no, I just got a gr8 idea, what we need is 3 functions: lame_set_string(lame_global_flags *gfp, StringItem tag1, ...); lame_set_float(lame_global_flags *gfp, FloatItem tag1, ...); lame_set_int(lame_global_flags *gfp, IntItem tag1, ...); Dead simple parsing using 3 different TagItem clones, and you can pass several items at once (within the same type anyway). - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
[...] >This way there is no need to parse any strings, we don't pass >any pointers, the setup routine would just be a big switch/case. This is basically the way TagItems work when passed on stack, in fact, that combined with another identical function that does float we're pretty much set. - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
> > >>>The first proposal is IMHO enough for everyday use, the second is for > >>>architectures which didn't allow lossless transportation of information > >>>between a void* and an integer type (in case you use ti_data to contain > >>>the information instead of a pointer to the information). > >> Only "problem" I can see immediately with void* is that all values needs to > >> be casted, which wasn't strictly necessary with unsigned long, and float/ > >> double still needs to be temporarily stored. :P > >I'm waiting for your solution which didn't need casts (and remember the > >mail which talked about casts in lame). > > Eheh, well, we'll see what I can cook up. ;) > > > - CISC > Why not something like: lame_set_float(char *variable_name, float val); lame_set_int(char *variable_name, int val); But this brings up another question: The code is going to do a lot of parsing, and it looks like each variable will require several lines of code? If that is the case, then we dont actually save anything and we might as well just creating a short function for every variable? This way there are no type casting problems. Mark -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
I think we can make a very simple library setup stuff:
proposal:
int lame_open();
// allocates internal structures
int lame_set_param( int handle, LameParameter Parameter, int Value );
// assigns Lame's parameter a value
int lame_init_params( int handle );
// fills up rest of needed parameters by default values
usual lame_encode() stuff
void lame_close( int handle );
// frees all allocated internal structures
just enumerate all parameters LAME will use, we can allways append
new ones if needed. A 32 bit int should provide us enough internal
parameters ;)
typedef enum{
lp_bitrate,
lp_vbrmode,
lp_vbrquality,
lp_vbrmin,
lp_vbrmax,
lp_crc,
:
} LameParameter;
// different channel modes
#define CM_MONO 1
#define CM_LR_STEREO 2
#define CM_J_STEREO 3
#define CM_MS_STEREO 4
...
Example setup sequence:
handle = lame_open();
lame_set_param( handle, lp_bitrate, 128 );
lame_set_param( handle, lp_crc, FALSE );
lame_set_param( handle, lp_original, FALSE );
lame_set_param( handle, lp_channelmode, CM_J_STEREO );
lame_set_param( handle, lp_quality, 2 );
lame_set_param( handle, lp_lowpass, 5500 );
lame_set_param( handle, lp_lowpasswidth, 500 );
lame_set_param( handle, lp_highpass, 100 );
lame_set_param( handle, lp_resample, 11025 );
lame_set_param( handle, lp_voice, TRUE );
lame_init_params( handle );
lame_close( handle );
This way there is no need to parse any strings, we don't pass
any pointers, the setup routine would just be a big switch/case.
Ciao Robert
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
1.) Floats and doubles will lose all it's precision (this can be simply avoided by passing the string-pointer from arg-parsing and converting when parsing TagItems instead) >>>What about passing the pointer to the float/double instead? >> Sure, but that means you will have to temporarily store the value somewhere >> else. >Yes. Are there many fixed values in lame? If you read parse.c you'll see that there's a few values that's atof()ed straight in. simply making a new TagItems interface which allows you to specify the type of data the TagItem contains. This will be much better in the long run, and will allow the TagItems to contain any type of data. >>>This sounds more complex than my proposed "void*" change. Keep in mind: >> More complex, but not that hard. >"KISS". I'm not even gonna ask. ;) >>>The first proposal is IMHO enough for everyday use, the second is for >>>architectures which didn't allow lossless transportation of information >>>between a void* and an integer type (in case you use ti_data to contain >>>the information instead of a pointer to the information). >> Only "problem" I can see immediately with void* is that all values needs to >> be casted, which wasn't strictly necessary with unsigned long, and float/ >> double still needs to be temporarily stored. :P >I'm waiting for your solution which didn't need casts (and remember the >mail which talked about casts in lame). Eheh, well, we'll see what I can cook up. ;) - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
On 24 Sep, Sigbjørn Skjæret wrote: >>> 1.) Floats and doubles will lose all it's precision (this can be simply >>> avoided by passing the string-pointer from arg-parsing and converting >>> when parsing TagItems instead) >>What about passing the pointer to the float/double instead? > > Sure, but that means you will have to temporarily store the value somewhere > else. Yes. Are there many fixed values in lame? >>> simply making a new TagItems interface which allows you to specify the type >>> of data the TagItem contains. This will be much better in the long run, and >>> will allow the TagItems to contain any type of data. >>This sounds more complex than my proposed "void*" change. Keep in mind: > > More complex, but not that hard. "KISS". >>the Amiga TagItem implementation was done with 32bit pointers in mind >>and not with portability. An (IMHO) more elegant (and still simple) >>solution for the data part would look like > [...] >>The first proposal is IMHO enough for everyday use, the second is for >>architectures which didn't allow lossless transportation of information >>between a void* and an integer type (in case you use ti_data to contain >>the information instead of a pointer to the information). > > Only "problem" I can see immediately with void* is that all values needs to > be casted, which wasn't strictly necessary with unsigned long, and float/ > double still needs to be temporarily stored. :P I'm waiting for your solution which didn't need casts (and remember the mail which talked about casts in lame). Bye, Alexander. -- Intel: where Quality is job number 0.9998782345! http://www.Leidinger.net Alexander @ Leidinger.net GPG fingerprint = 7423 F3E6 3A7E B334 A9CC B10A 1F5F 130A A638 6E7E -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
>> 1.) Floats and doubles will lose all it's precision (this can be simply >> avoided by passing the string-pointer from arg-parsing and converting >> when parsing TagItems instead) >What about passing the pointer to the float/double instead? Sure, but that means you will have to temporarily store the value somewhere else. >> 3.) 64bit pointers will be truncated (not too much trouble unless you have >> and use more than 4gig of memory in your machine). >Don't keep the AmigaOS compatibility of my functions (see bellow why) >and change the data part (ti_Data) of the struct to "void*". Hmmm, yes, maybe, if the compiler allows it. ;) >> simply making a new TagItems interface which allows you to specify the type >> of data the TagItem contains. This will be much better in the long run, and >> will allow the TagItems to contain any type of data. >This sounds more complex than my proposed "void*" change. Keep in mind: More complex, but not that hard. >the Amiga TagItem implementation was done with 32bit pointers in mind >and not with portability. An (IMHO) more elegant (and still simple) >solution for the data part would look like [...] >The first proposal is IMHO enough for everyday use, the second is for >architectures which didn't allow lossless transportation of information >between a void* and an integer type (in case you use ti_data to contain >the information instead of a pointer to the information). Only "problem" I can see immediately with void* is that all values needs to be casted, which wasn't strictly necessary with unsigned long, and float/ double still needs to be temporarily stored. :P - CISC -- MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
Re: [MP3 ENCODER] TagItem issues...
On 23 Sep, Sigbjørn Skjæret wrote:
> Ok, after thinking about this for a while, and experimenting a little, I
> thought of a couple of issues with using TagItems to pass configuration
> settings in LAME...
>
> 1.) Floats and doubles will lose all it's precision (this can be simply
> avoided by passing the string-pointer from arg-parsing and converting
> when parsing TagItems instead)
What about passing the pointer to the float/double instead?
> 3.) 64bit pointers will be truncated (not too much trouble unless you have
> and use more than 4gig of memory in your machine).
Don't keep the AmigaOS compatibility of my functions (see bellow why)
and change the data part (ti_Data) of the struct to "void*".
> I've been thinking about simply ditching AmigaOS TagItem compatability, and
I think there's no need to use any Amiga library in the LAME library, so
it didn't buys you anything to keep the compatibility (or use the real
Amiga functions). If you improve the usability of the LAME TagItem
(like) implementation then go for it.
> simply making a new TagItems interface which allows you to specify the type
> of data the TagItem contains. This will be much better in the long run, and
> will allow the TagItems to contain any type of data.
This sounds more complex than my proposed "void*" change. Keep in mind:
the Amiga TagItem implementation was done with 32bit pointers in mind
and not with portability. An (IMHO) more elegant (and still simple)
solution for the data part would look like
void *ti_data;
or
union { void *pointer; unsigned long number; } ti_data;
instead of just
unsigned long ti_data;
The first proposal is IMHO enough for everyday use, the second is for
architectures which didn't allow lossless transportation of information
between a void* and an integer type (in case you use ti_data to contain
the information instead of a pointer to the information).
Bye,
Alexander.
--
The dark ages were caused by the Y1K problem.
http://www.Leidinger.net Alexander @ Leidinger.net
GPG fingerprint = 7423 F3E6 3A7E B334 A9CC B10A 1F5F 130A A638 6E7E
--
MP3 ENCODER mailing list ( http://geek.rcc.se/mp3encoder/ )
