Hi Jim, Actually, I don't see anything wrong with the internal structure having a different type than the interface, as long both types are big enough to hold the possible values (in this case 0 and 1). Though, as you pointed out, testenc needs to be fixed to use spx_int32_t instead of int. I'll change that. Jean-Marc Jim Crichton a ?crit :> st->highpass_enabled is typed inconsistently. It is "int" in the > structure, but "spx_int32_t" in the SET, GET cases in > nb(sb)_encoder(decoder)_ctl. The parameter is "int" in testenc.c, and > in the TI version ti\testenc-TI-C5x.c. This breaks for 16 bit platforms, > where "int" is 16 bits. > > The types in the state structure and the _ctl routines really should > match. For now, I have changed testenc-TI-C5x.c to pass a 32-bit > parameter, which works with no change elsewhere. I can send a patch for > this. > > Another inconsistency is that GET_COMPLEXITY returns a spx_int32_t, but > the field st->complexity is an int. > > - Jim > > _______________________________________________ > Speex-dev mailing list > Speex-dev@xiph.org > http://lists.xiph.org/mailman/listinfo/speex-dev >
Jean-Marc, The trouble is that one cannot identify the type difference without reading the nb_celp.c source file, and this parameter is treated differently than other, similar parameters. It would seem like the most intuitive approach would be to match the interface to the structure. This is a trivial point, of course. I have attached a patch to the two TI interface files, which adds the configuration for the filter, sized as spx_int32_t. I also included the modified files (I know that you have had some trouble with patches that I have sent before). - Jim ----- Original Message ----- From: "Jean-Marc Valin" <jean-marc.valin@usherbrooke.ca> To: "Jim Crichton" <jim.crichton@comcast.net> Cc: <speex-dev@xiph.org> Sent: Tuesday, August 22, 2006 8:12 PM Subject: Re: [Speex-dev] Please test upcoming release> Hi Jim, > > Actually, I don't see anything wrong with the internal structure having > a different type than the interface, as long both types are big enough > to hold the possible values (in this case 0 and 1). Though, as you > pointed out, testenc needs to be fixed to use spx_int32_t instead of > int. I'll change that. > > Jean-Marc > > Jim Crichton a ?crit : >> st->highpass_enabled is typed inconsistently. It is "int" in the >> structure, but "spx_int32_t" in the SET, GET cases in >> nb(sb)_encoder(decoder)_ctl. The parameter is "int" in testenc.c, and >> in the TI version ti\testenc-TI-C5x.c. This breaks for 16 bit platforms, >> where "int" is 16 bits. >> >> The types in the state structure and the _ctl routines really should >> match. For now, I have changed testenc-TI-C5x.c to pass a 32-bit >> parameter, which works with no change elsewhere. I can send a patch for >> this. >> >> Another inconsistency is that GET_COMPLEXITY returns a spx_int32_t, but >> the field st->complexity is an int. >> >> - Jim >> >> _______________________________________________ >> Speex-dev mailing list >> Speex-dev@xiph.org >> http://lists.xiph.org/mailman/listinfo/speex-dev >> >-------------- next part -------------- A non-text attachment was scrubbed... Name: ti_11791_patch.zip Type: application/x-zip-compressed Size: 7074 bytes Desc: not available Url : http://lists.xiph.org/pipermail/speex-dev/attachments/20060823/0f01afe5/ti_11791_patch.bin
> The trouble is that one cannot identify the type difference without > reading the nb_celp.c source file, and this parameter is treated > differently than other, similar parameters. It would seem like the most > intuitive approach would be to match the interface to the structure.There are two issues here: 1) what type should be used in the API and 2) whether it should match the internal representation. Regarding 1), the question is whether we should have a mix of int and spx_int32_t. I tend to think it's less confusing to change all the int into spx_int32_t. What do you think? Regarding 2), I see no reason to change the implementation as long as it works and the API is OK.> This is a trivial point, of course. I have attached a patch to the two > TI interface files, which adds the configuration for the filter, sized > as spx_int32_t. I also included the modified files (I know that you > have had some trouble with patches that I have sent before).Thanks, I'll apply these. Jean-Marc> - Jim > > ----- Original Message ----- From: "Jean-Marc Valin" > <jean-marc.valin@usherbrooke.ca> > To: "Jim Crichton" <jim.crichton@comcast.net> > Cc: <speex-dev@xiph.org> > Sent: Tuesday, August 22, 2006 8:12 PM > Subject: Re: [Speex-dev] Please test upcoming release > > >> Hi Jim, >> >> Actually, I don't see anything wrong with the internal structure having >> a different type than the interface, as long both types are big enough >> to hold the possible values (in this case 0 and 1). Though, as you >> pointed out, testenc needs to be fixed to use spx_int32_t instead of >> int. I'll change that. >> >> Jean-Marc >> >> Jim Crichton a ?crit : >>> st->highpass_enabled is typed inconsistently. It is "int" in the >>> structure, but "spx_int32_t" in the SET, GET cases in >>> nb(sb)_encoder(decoder)_ctl. The parameter is "int" in testenc.c, and >>> in the TI version ti\testenc-TI-C5x.c. This breaks for 16 bit platforms, >>> where "int" is 16 bits. >>> >>> The types in the state structure and the _ctl routines really should >>> match. For now, I have changed testenc-TI-C5x.c to pass a 32-bit >>> parameter, which works with no change elsewhere. I can send a patch for >>> this. >>> >>> Another inconsistency is that GET_COMPLEXITY returns a spx_int32_t, but >>> the field st->complexity is an int. >>> >>> - Jim >>> >>> _______________________________________________ >>> Speex-dev mailing list >>> Speex-dev@xiph.org >>> http://lists.xiph.org/mailman/listinfo/speex-dev >>> >>