[FFmpeg-devel] [PATCH] ALAC Encoder

[Michael Niedermayer]

On Mon, Aug 18, 2008 at 02:53:35AM +0530, Jai Menon wrote:
> Hi,
> 
> On Monday 18 Aug 2008 2:38:24 am Jai Menon wrote:
> > Hi,
> >
> > On Sunday 17 Aug 2008 5:17:52 pm Michael Niedermayer wrote:
> > > On Sun, Aug 17, 2008 at 11:17:10AM +0530, Jai Menon wrote:
> > > > Hi,
> > > >
> > > > On Sunday 17 Aug 2008 8:05:14 am Michael Niedermayer wrote:
> > > > > On Sun, Aug 17, 2008 at 04:14:43AM +0530, Jai Menon wrote:
> > > > > > Hi,
> > > > > >
> > > > > > The attached ALAC encoder was written as part of GSoC and mentored
> > > > > > by Justin Ruggles. I'm posting it for inclusion into FFmpeg-svn.
> > > > >
> > > > > [...]
> > > > >
> > > > > > +static void alac_linear_predictor(AlacEncodeContext *s, int ch)
> > > > > > +{
> > > > > > +    int i;
> > > > > > +    LPCContext lpc = s->lpc[ch];
> > > > > > +
> > > > > > +    if(lpc.lpc_order == 31) {
> > > > > > +        s->predictor_buf[0] = s->sample_buf[ch][0];
> > > > > >
> > > > > > +        i = s->avctx->frame_size - 1;
> > > > > > +        while(i > 0) {
> > > > > > +            s->predictor_buf[i] = s->sample_buf[ch][i] -
> > > > > > s->sample_buf[ch][i-1];
> > > > > > +            i--;
> > > > > > +        }
> > > > >
> > > > > i suspect that 0 .. n is faster than n .. 0
> > > >
> > > > I'll need a temp variable in that case.   Is that okay?
> > >
> > > why?
> > >
> > > for(i=1; i<s->avctx->frame_size; i++)
> > >     s->predictor_buf[i] = s->sample_buf[ch][i] - s->sample_buf[ch][i-1];
> > >
> > > looks fine to me ...
> >
> > Yeah, that was my mistake.
> > Changed.
> >
> > > > > [...]
> > > > >
> > > > > > +    if(buf_size < s->max_coded_frame_size) {
> > > > > > +        av_log(avctx, AV_LOG_ERROR, "buffer size is too small\n");
> > > > > > +        return -1;
> > > > > > +    }
> > > > > > +
> > > > > > +    init_put_bits(pb, frame, buf_size);
> > > > > > +
> > > > > > +    if(s->compression_level == 0) {
> > > > > > +        // Verbatim mode
> > > > > > +        int16_t *samples = data;
> > > > > > +        write_frame_header(s, 1);
> > > > > > +        for(i=0; i<avctx->frame_size*s->channels; i++) {
> > > > > > +            put_sbits(pb, 16, *samples++);
> > > > > > +        }
> > > > > > +    } else {
> > > > > > +        init_sample_buffers(s, data);
> > > > > > +        write_frame_header(s, 0);
> > > > > > +        write_compressed_frame(s);
> > > > > > +    }
> > > > > > +
> > > > > > +    put_bits(pb, 3, 7);
> > > > > > +    flush_put_bits(pb);
> > > > > > +    out_bytes = put_bits_count(pb) >> 3;
> > > > > > +
> > > > > > +    if(out_bytes > s->max_coded_frame_size) {
> > > > >
> > > > > At this point it is possible that the encoder wrote over the end of
> > > > > the array, the check farther up should ensure that there really is
> > > > > enough space available even for larger then optimal frames
> > > >
> > > > I'm not sure If I understand this comment correctly. We already check
> > > > to see if buf_size < max_coded_frame_size. So it would not write past
> > > > the end. This check here would take care of reencoding in verbatim mode
> > > > if actual output was > max_coded_frame_size. It handles this case
> > > > similar to the flac encoder.
> > >
> > > well, if actual output was > max_coded_frame_size but actual buffer was
> > > just max_coded_frame_size, then i would assume that some of the output is
> > > after the buffers end.
> > >
> > > [...]
> >
> > Modified according to comment.
> >
> > Other review comments also addressed. Patch against FFmpeg svn is attached.
> >
> 
> Newer version of the previous patch with some redundancy removed.


@@ -38,7 +38,6 @@
  
 +#define ALAC_CHMODE_LEFT_RIGHT    1
 +#define ALAC_CHMODE_LEFT_SIDE     8
-+#define ALAC_CHMODE_RIGHT_SIDE    9
 +#define ALAC_CHMODE_MID_SIDE     10
 +
 +typedef struct RiceContext {
[...]
@@ -118,7 +117,7 @@
 +    int i, best;
 +    int32_t lt, rt;
 +    uint64_t sum[4];
-+    uint64_t score[4];
++    uint64_t score[3];
 +
 +    /* calculate sum of 2nd order residual for each channel */
 +    sum[0] = sum[1] = sum[2] = sum[3] = 0;
@@ -134,12 +133,11 @@
 +    /* calculate score for each mode */
 +    score[0] = sum[0] + sum[1];
 +    score[1] = sum[0] + sum[3];
-+    score[2] = sum[1] + sum[3];
-+    score[3] = sum[2] + sum[3];
++    score[2] = sum[2] + sum[3];
 +
 +    /* return mode with lowest score */
 +    best = 0;
-+    for(i=1; i<4; i++) {
++    for(i=1; i<3; i++) {
 +        if(score[i] < score[best]) {

doesnt look redundant to me

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080818/134926a9/attachment.pgp>