[Ffmpeg-devel] [PATCH] flacenc - lpc and options

[Justin Ruggles]

Michael Niedermayer wrote:
> Hi
> 
> On Fri, Jun 30, 2006 at 01:40:48AM -0400, Justin Ruggles wrote:
> [...]
> 
>>2) added 8 compression options to AVCodecContext which are used by the
>>encoder.  most of them are set to -1 by default, which indicates for the
>>encoder to use its default setting.  this is so that other codecs can
>>reuse these fields if they wish, and can have different default values.
>>
>>3) modified flacenc.c to first check for the compression_level and set
>>all the options based on that value, then read the other AVCodecContext
>>values and override the defaults if they are explicitly set by the user.
>>
>>If any of this should be changed, let me know.  Michael's idea of a
>>'preset' field in AVCodecContext would work, but it was unnecessary in
>>this case.  I guess it could still be added for shortcuts like "best" or
>>"fast".
>>
>>Since I have not yet added any more extensive order search options, the
>>compression level only goes from 0 to 5 for now.  Below are the results
>>of a few tests comparing ffmpeg to the reference encoder.  The faster
>>decode times for the ffmpeg-generated files are mostly due to the fact
>>that MD5 sums are not implemented yet, so the decoder doesn't do any
>>verification.
> 
> 
> [...]
> 
>>+    if(avctx->min_prediction_order >= 0) {
>>+        if(s->options.use_lpc) {
>>+            if(avctx->min_prediction_order == 0 ||
>>+                    avctx->min_prediction_order > 32) {
> 
> 
> this could be MAX_LPC_ORDER
> 
> 
> [...]

oops. i missed that one. :)

>>+/**
>>+ * Apply Welch window function to audio block
>>+ */
>>+static void apply_welch_window(const int32_t *data, int len, double *w_data)
>>+{
>>+    int i, n2;
>>+    double w;
>>+    double c;
>>+
>>+    n2 = (len >> 1);
>>+    c = 2.0 / (len - 1.0);
>>+	for(i=0; i<n2; i++) {
>>+		w = c - i - 1.0;
>>+        w = 1.0 - (w * w);
>>+        w_data[i] = data[i] * w;
>>+        w_data[len-1-i] = data[len-1-i] * w;
>>+	}
>>+}
>>+
>>+static double *welch_window;
>>+static int window_size;
>>+
>>+static void window_init(int size)
>>+{
>>+    int i, n2;
>>+    double w;
>>+    double c;
>>+
>>+    window_size = size;
>>+    welch_window = av_malloc(size * sizeof(double));
>>+
>>+    n2 = (size >> 1);
>>+    c = 2.0 / (size - 1.0);
>>+	for(i=0; i<n2; i++) {
>>+		w = c - i - 1.0;
> 
> 
> there are tabs in the line above
> the code is duplicated from apply_welch_window
> and this is not thread safe, window_size, ... are non constant static
> variables
> 
> [...]
> 

ok. I was trying to speed things up, but it does present a problem. I'll
switch back to calculating the window each time.  It was only a small
speed-up anyway.  I think it would be a bit much to precalculate the
window for all block sizes.  An alternative would be to precalculate it
and store it in the encoder context instead of a non-constant static array.

>>+
>>+/**
>>+ * Levinson-Durbin recursion.
>>+ * Produces LPC coefficients from autocorrelation data.
>>+ */
>>+static void compute_lpc_coefs(const double *autoc, int max_order,
>>+                              double lpc[][MAX_LPC_ORDER], double *ref)
>>+{
>>+   int i, j, i2;
>>+   double r, err, tmp;
>>+   double lpc_tmp[MAX_LPC_ORDER];
>>+
>>+   for(i=0; i<max_order; i++) lpc_tmp[i] = 0;
>>+   err = autoc[0];
>>+
>>+   for(i=0; i<max_order; i++) {
>>+      r = -autoc[i+1];
>>+      for(j=0; j<i; j++) {
>>+          r -= lpc_tmp[j] * autoc[i-j];
>>+      }
>>+      r /= err;
>>+      ref[i] = fabs(r);
>>+
>>+      err *= 1.0 - (r * r);
>>+
>>+      i2 = (i >> 1);
>>+      lpc_tmp[i] = r;
>>+      for(j=0; j<i2; j++) {
>>+         tmp = lpc_tmp[j];
>>+         lpc_tmp[j] += r * lpc_tmp[i-1-j];
>>+         lpc_tmp[i-1-j] += r * tmp;
>>+      }
>>+      if(i % 2) {
> 
> 
> i&1 is faster then i%2

ok. will fix.

> 
> [...]
> 
> 
>>+    for(i=0; i<sub->order; i++) {
>>+        sub->coefs[i] = coefs[sub->order-1][i];
>>     }
>>-    return bits[sub->order];
>>+    porder = get_max_p_order(max_porder, n, sub->order);
>>+    encode_residual_lpc(res, smp, n, sub->order, sub->coefs, sub->shift);
> 
> 
> hmm, coeffs is copied into sub->coefs and then used, why isnt it used withot
> copying?
> 

The local coefs array is a 2-D array of all the coefs for all orders,
and it is allocated on the stack.  The codec context just has a 1-D
array of the coefs of the selected order and is in memory allocated
during flac_encode_init().  The local coeffs are copied into the encoder
context because they have to be accessed later in order to write them on
the output stream.  I could have used the local array to encode the
residual, but I still would have to copy them to the context.

Another solution to this could be to store the full 2-D array in the
encoder context.  I just thought it was a cleaner solution to discard
the information for the orders which are not used.

> [...]
> 
>>+    /**
>>+     * sets audio frame size using frame duration in milliseconds
>>+     * - encoding: set by user.
>>+     * - decoding: unused.
>>+     */
>>+    int block_time_ms;
> 
> 
> iam a little unsure about this, maybe you could leave this one out for now
> and set it just based on compression_level
> 
> [...]
> 

This was to give the user the option to set the block size. Do you think
the user should instead be able to set AVCodecContext.frame_size
explicitly before calling encode_init()?  Currently it is set by the
encoder only, but i see no reason not to let the user set it.  The
encoders currently don't check the value anyway and would just
over-write it with what it's supposed to be.  Would there be anything
wrong with some encoders checking first for a user-preferred value?  I
could change the comment to something like:
/**
 * samples per packet.
 * - encoding: set by user, but may be changed by 'init'.
 * - decoding: set by lavc.
 */
int frame_size;

or would it be better to let the user set it and return an error if it
is an invalid value for that codec, like is done with most of the other
parameters?


Thanks,
Justin