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

[Michael Niedermayer]

Hi

On Fri, Jun 30, 2006 at 09:27:02PM -0400, Justin Ruggles wrote:
[...]
> > 
> > [...]
> > 
> > 
> >>+    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.

ok, leave the code as is, if the copy is needed iam fine with it, its
just a few bytes anyway


> 
> > [...]
> > 
> >>+    /**
> >>+     * 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?

hmm, iam more in favor of check & error

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is