Speex dev - Sep 2007 - innov_save, what is it? why does it hurt me so?

hi,
 
I am using speex1.2beta2 on a TI 54x on narrow band
 
I have been trying to get speex to work for a while now, and it's been
a real teeter-totter ride.  For a long time I noticed that I will get a
project to work and then without changing any code and programming it to
an eprom/flash the project will not work.  It turns out it was a value
called innov_save.  I found this bugger by zero filling data in my
program and then loading my code and running it, then filling my data
with 1s and tried running it.  when I filled it with 1's it never
worked.  I found innov_save will write over memory it shouldn't be when
I fill memory with 1's but not when I fill it with zeros.   When my DSP
gets to this code chunk:
if (innov_save){
   for (i=0;i<st->subframeSize;i++)
      innov_save[i] = EXTRACT16(PSHR32(innov[i], SIG_SHIFT));
}
it will just start filling data in, which it shouldn't.  I see that
innov_save is set at the beginning of a for loop at:
for (sub=0;sub<st->nbSubframes;sub++)
   {
      int offset;
      spx_word16_t *exc;
      spx_word16_t *sp;
      spx_word16_t *innov_save = NULL;
      spx_word16_t tmp;...
 
but for some reason that never made a difference even though I know I
stepped over it in the code.  I commented innov save out for now but I
don't know if there is some case where I might need it.  What does it do
and do I need it?  Are there any more similar data pointers like that
that you know of that may cause similar issues?
 
Thanks.
 
-Mike
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://lists.xiph.org/pipermail/speex-dev/attachments/20070913/b1a3ddec/attachment.html

> I have been trying to get speex to work for a while now, and it's been
> a real teeter-totter ride.  For a long time I noticed that I will get a
> project to work and then without changing any code and programming it to
> an eprom/flash the project will not work.  
That's an Heisenbug and the most common cause is uninitialised data.

I found innov_save will write over memory it shouldn't be
when
> I fill memory with 1's but not when I fill it with zeros. 
Which is normal, see below.
>  When my DSP
> gets to this code chunk:
> if (innov_save){
>    for (i=0;i<st->subframeSize;i++)
>       innov_save[i] = EXTRACT16(PSHR32(innov[i], SIG_SHIFT));
> }
This bit of code is for copying the innov variable to a buffer owned by
the wideband decoder -- but only if there is actually a wideband decoder.
> it will just start filling data in, which it shouldn't.  I see that
> innov_save is set at the beginning of a for loop at:
> for (sub=0;sub<st->nbSubframes;sub++)
>    {
...
>       spx_word16_t *innov_save = NULL;
>       spx_word16_t tmp;...
> but for some reason that never made a difference even though I know I
> stepped over it in the code. 
The reason is doesn't make a difference is that just a few lines later,
you have:

      if (st->innov_save)
         innov_save = st->innov_save+offset;

It means that the innov variable gets copied *unless* st->innov_save
(notice is a field of the encoder struct). So the problem is that this
field is probably not set to NULL. Normally, this is automatically set
to zero when the init function does:
st = (EncState*)speex_alloc(sizeof(EncState));

Is it possible that you did an override of speex_alloc and replaced it
by malloc() instead of calloc() as I'm using? That would explain the
garbage you're getting and it may actually explain other bugs you've
had.

	Jean-Marc
> I commented innov save out for now but I
> don't know if there is some case where I might need it.  What does it
do
> and do I need it?  Are there any more similar data pointers like that
> that you know of that may cause similar issues?
In any case, about innov_save in general, you can disable all of that if
you're not using wideband.

	Jean-Marc

This must have been an enormous pain to track down.

The manual alloc routine in the TI directory (user_misc.h) clears the 
allocated memory, but maybe you have changed this.
>> it will just start filling data in, which it shouldn't.  I see that
>> innov_save is set at the beginning of a for loop at:
>> for (sub=0;sub<st->nbSubframes;sub++)
>>    {
> ...
>>       spx_word16_t *innov_save = NULL;
>>       spx_word16_t tmp;...
>
>> but for some reason that never made a difference even though I know I
>> stepped over it in the code.
That is because this is a local variable used only within this loop, and is 
different from the field of the same name in the decoder state.
st->innov_save is initialized only in sb_celp.c, which is not in your build.

The big lesson here is that Speex depends upon all allocated memory being 
zeroed.  This is to your advantage in the long run, because it saves code 
size by eliminating all those individual sets to zero.  There probably are 
not very many variables that have this dependence, but it is best to assume 
that any variable might, and clear all allocated memory.

- Jim

----- Original Message ----- 
From: "Jean-Marc Valin" <jean-marc.valin@usherbrooke.ca>
To: "Michael Jacobson" <Michael.Jacobson@ultratec.com>
Cc: <speex-dev@xiph.org>
Sent: Thursday, September 13, 2007 7:40 PM
Subject: Re: [Speex-dev] innov_save, what is it? why does it hurt me so?

>> I have been trying to get speex to work for a while now, and it's
been
>> a real teeter-totter ride.  For a long time I noticed that I will get a
>> project to work and then without changing any code and programming it
to
>> an eprom/flash the project will not work.
>
> That's an Heisenbug and the most common cause is uninitialised data.
>
> I found innov_save will write over memory it shouldn't be when
>> I fill memory with 1's but not when I fill it with zeros.
>
> Which is normal, see below.
>
>>  When my DSP
>> gets to this code chunk:
>> if (innov_save){
>>    for (i=0;i<st->subframeSize;i++)
>>       innov_save[i] = EXTRACT16(PSHR32(innov[i], SIG_SHIFT));
>> }
>
> This bit of code is for copying the innov variable to a buffer owned by
> the wideband decoder -- but only if there is actually a wideband decoder.
>
>> it will just start filling data in, which it shouldn't.  I see that
>> innov_save is set at the beginning of a for loop at:
>> for (sub=0;sub<st->nbSubframes;sub++)
>>    {
> ...
>>       spx_word16_t *innov_save = NULL;
>>       spx_word16_t tmp;...
>
>> but for some reason that never made a difference even though I know I
>> stepped over it in the code.
>
> The reason is doesn't make a difference is that just a few lines later,
> you have:
>
>      if (st->innov_save)
>         innov_save = st->innov_save+offset;
>
> It means that the innov variable gets copied *unless* st->innov_save
> (notice is a field of the encoder struct). So the problem is that this
> field is probably not set to NULL. Normally, this is automatically set
> to zero when the init function does:
> st = (EncState*)speex_alloc(sizeof(EncState));
>
> Is it possible that you did an override of speex_alloc and replaced it
> by malloc() instead of calloc() as I'm using? That would explain the
> garbage you're getting and it may actually explain other bugs
you've had.
>
> Jean-Marc
>
>> I commented innov save out for now but I
>> don't know if there is some case where I might need it.  What does
it do
>> and do I need it?  Are there any more similar data pointers like that
>> that you know of that may cause similar issues?
>
> In any case, about innov_save in general, you can disable all of that if
> you're not using wideband.
>
> Jean-Marc
>
> _______________________________________________
> Speex-dev mailing list
> Speex-dev@xiph.org
> http://lists.xiph.org/mailman/listinfo/speex-dev
>