[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters

[Vladimir Voroshilov]

On Thu, Apr 24, 2008 at 10:13 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Apr 24, 2008 at 08:24:25AM +0700, Vladimir Voroshilov wrote:
>  > Michael Niedermayer wrote:
>  > > On Thu, Apr 24, 2008 at 02:15:51AM +0700, Vladimir Voroshilov wrote:
>  > > > Michael Niedermayer wrote:
>  > > [...]
>  > > > > > +        v=0;
>  > > > > > +        for(i=0; i<10; i++)
>  > > > > > +        {
>  > > > >
>  > > > > > +            /*  R(x):=ac_v[-k+x] */
>  > > > > > +            v += ac_v[n - pitch_delay_int - i    ] * ff_g729_interp_filter[i][    pitch_delay_frac];
>  > > > > > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n-i)*ff_g729_interp_filter(t+3i)
>  > > > > > +            v += ac_v[n - pitch_delay_int + i + 1] * ff_g729_interp_filter[i][3 - pitch_delay_frac];
>  > > > > > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n+i+1)*ff_g729_interp_filter(3-t+3i)
>  > > > >
>  > > > > The cliping is incorrect for generic code. Also i doubt g729 really needs
>  > > > > it. What happens without that cliping or at least with it at the end, just
>  > > > > before storing in ac_v?
>  > > >
>  > > > Removing those line breaks OVERFLOW test (regardless of clipping outside loop),
>  > > > significantly reduces PSNR from bitexact's 99,99 to 18,54 without outside
>  > > > clipping and to 26,01 with it.
>  > >
>  > > What is the OVERFLOW test anyway? Is this a normal valid bitstream generated
>  > > from a pcm wave? Or some sythetic overflow excercise which cannot be generated
>  > > by any input wave?
>  > > (If you dont know you can test it by encoding the wave from overflow to see if
>  > > there are still any overflows happening from the resulting bitstream)
>  >
>  > Completely synthetic test, imho. ITU does not provide any PCM stream related to
>  > it. I don't know is it possible to generate such stream from regular PCM or not.
>
>  So a reencoded version of that synthetic vector is not affected by the
>  existence of these checks?

Yes. Reencoded file passes all tests even with removed clipping.

>  > > > +        filter_data[10+n] = out[n] = sum;
>  > >
>  > > This duplicated storeage is unacceptable.
>  >
>  > First for all assigned to filter data values will be used in loop later.
>  > Thus filter_data can not be eliminated.
>  > I can't use "out" instead of it due to necessary 10 items
>  > with data from previous subframe at top).
>  > Extending out with 10 items at top will require another temporary buffer
>  > one memcpy somewhere later (because i will not be able to use output buffer
>  > directly).
>
>  The double write is definitly useless after the first 10 iterations as
>  after that you can just work in the out buffer.
>
>  foobar_filter(filter_data+10, 10);
>  memcpy(out, filter_data+10, 10);
>  foobar_filter(out+10, N-10);
>
>  should work fine and will for large N (dunno how large it is, so maybe
>  this isnt worth it ...) be faster. Also it allows filter_data to be smaller.

... and code will look like :(

if(foobar_filter(filter_data+10, 10)!=OVERFLOW)
{
  memcpy(out, filter_data+10, 10);
  if(foobar_filter(out+10, N-10)==OVERFLOW)
  {
     for(i=0;i<len;i++) out>>=2;
     foobar_filter(filter_data+10, 10);
     memcpy(out, filter_data+10, 10);
     foobar_filter(out+10, N-10);
  }
}
else
{
     for(i=0;i<len;i++) out>>=2;
     foobar_filter(filter_data+10, 10);
     memcpy(out, filter_data+10, 10);
     foobar_filter(out+10, N-10);
}

>
>
>
>  >
>  > > > +    }
>  > > > +
>  > > > +    // Saving data for using in next subframe
>  > > > +    if(update_filter_data)
>  > > > +        memcpy(filter_data, filter_data + subframe_size, 10 * sizeof(int16_t));
>  > >
>  > > This design requires the memcpy to be duplicated.
>  >
>  > Don't see. Please explain.
>  > I can put this memcpy outside, but this will look like cosmetic
>  > change since the overall count of memcpy will be the same.
>
>  +        if(ff_acelp_lp_synthesis_filter(
>  +                &lp[i][0],
>  +                ctx->exc  + i * ctx->subframe_size,
>  +                out_frame + i * ctx->subframe_size,
>  +                ctx->syn_filter_data,
>  +                ctx->subframe_size,
>  +                0))
>  +            //Overflow occured, downscale excitation signal...
>  +            for(j=0; j<2*MAX_SUBFRAME_SIZE+PITCH_LAG_MAX+INTERPOL_LEN; j++)
>  +                ctx->exc_base[j] >>= 2;
>  +
>
> +            ff_acelp_lp_synthesis_filter(
>  +                    &lp[i][0],
>  +                    ctx->exc  + i * ctx->subframe_size,
>  +                    out_frame + i * ctx->subframe_size,
>  +                    ctx->syn_filter_data,
>  +                    ctx->subframe_size,
>  +                    1);
>
>  Above unconditionally executes the code twice. If you skip the second
>  one (like the indention suggests) it will fail due to the missing
>  update_filter_data based code IMHO

Yes it does. Indentation was wrong.
First pass checks for overflow, second - does real filtering.
Note also that G.729D looks like:

if(filter(exc,out,0))
   ecx[*]>>=2

if(format==G729D)
{
  make_new_exc(exc,new_exc)
  filter(new_exc, out,1)
}else
  filter(exc,out,1)


>
>  [...]
>
>  --
>  Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
>  I do not agree with what you have to say, but I'll defend to the death your
>  right to say it. -- Voltaire
>
> -----BEGIN PGP SIGNATURE-----
>  Version: GnuPG v1.4.6 (GNU/Linux)
>
>  iD8DBQFID/q6YR7HhwQLD6sRApGdAJ9KrE71M5pm7FSGF9gewrjh5qENdACdG82d
>  lDTAS5B5ipgiFOJgIeI78ss=
>  =/yBq
>  -----END PGP SIGNATURE-----
>
> _______________________________________________
>  ffmpeg-devel mailing list
>  ffmpeg-devel at mplayerhq.hu
>  https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>



-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719