[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [1/7] - filters

[Diego Biurrun]

On Sat, May 10, 2008 at 02:59:49PM +0700, Vladimir Voroshilov wrote:
> 2008/5/10 Diego Biurrun <diego at biurrun.de>:
> > On Sat, May 10, 2008 at 12:44:25PM +0700, Vladimir Voroshilov wrote:
> 
> [...]
> 
> >> +    /* Since there are few pulses over all subframe (i.e. almost all
> >
> > subframeS
> 
> I meant "few pulses over ONE subframe". Changed to "over entire subframe",
> but not sure is this better, though.

It could still be misunderstood as I did before.  If you add an article
this ambiguity goes away, i.e. "over the/an entire subframe".  In any
case, that sentence still needs some work.

> --- /dev/null
> +++ b/libavcodec/acelp_filters.c
> @@ -0,0 +1,125 @@
> +    memset(fc_out, 0, subframe_size * sizeof(int16_t));
> +
> +    /* Since there are few pulses over entire subframe (i.e. almost all

an entire subframe

> +       fc_in[i] are zero, in case of G.729D the buffer contains two non-zero

the case

> +       samples before the call to ff_acelp_enhance_harmonics, and (due to
> +       pitch_delay bounded to [20; 143]) a maximum four non-zero samples

bounded by

> +       for a total of 40 after the call to it), it is faster to swap two loops
> +       and process non-zero samples only.

This sentence is still awfully long, which makes it cumbersome and hard
to understand.  Try splitting it like this:

  Since there are few pulses over an entire subframe it is faster to
  swap two loops and process non-zero samples only, i.e. almost all
  fc_in[i] are zero. In the case of G.729D the buffer contains two
  non-zero samples before the call to ff_acelp_enhance_harmonics and,
  due to pitch_delay being bounded by [20; 143], a maximum of four
  non-zero samples for a total of 40 after the call.

If I managed to keep the intended meaning intact I think this is easier
to understand.

> --- /dev/null
> +++ b/libavcodec/acelp_filters.h
> @@ -0,0 +1,113 @@
> +
> + * \remark It is safe to pass the same array in in and out parameters

.

> + * \remark AMR uses mostly the same filter (cut-off frequency 60Hz, same formula,

nit: long line

Diego