[FFmpeg-devel] [PATCH] s302menc: Add S302M encoder.

[Thierry Foucu]

On Tue, Apr 10, 2012 at 3:16 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de>wrote:

> On Tue, Apr 10, 2012 at 02:45:17PM -0700, Thierry Foucu wrote:
> > +    if (buf_size < s302m_frame_size + AES3_HEADER_LEN) {
> > +      av_log(avctx, AV_LOG_ERROR, "Buffer too small %d to encode %d
> Bytes.\n",
> > +             buf_size, s302m_frame_size + AES3_HEADER_LEN);
> > +      return -1;
> > +    }
> > +
> > +    PutBitContext pb;
>
> Mixing declarations and code. Please look out for compiler warnings.
>
>
Interesting.. I was using CLAN and i did not get the warning. Switching to
gcc shows it. Will fix it.


> > +    init_put_bits(&pb, o, buf_size*8);
> > +    put_bits(&pb, 16, s302m_frame_size);  // Frame size
> > +    put_bits(&pb, 2, (avctx->channels - 2) >> 1);  // Number channels
> > +    put_bits(&pb, 8, 0);  // Channel id
> > +    put_bits(&pb, 2, 0);  // bits per samples (16bit)
> > +    put_bits(&pb, 4, 0);  // alignments
> > +    flush_put_bits(&pb);
>
> Seems almost a bit like overkill to use put_bits for these, compared
> to e.g. two bytestream_put_be16.
>

Thanks about the tips. I did not know about it. WIll be using it


>
> > +    num_samples = S302M_NUMBER_SAMPLES * avctx->channels;
> > +    for (; num_samples > 0; num_samples -= avctx->channels) {
> > +        vucf = (s->framing_index == 0)? 0x10 : 0;
> > +        for (channels = 0; channels < avctx->channels; channels += 2) {
> > +            vucf = (s->framing_index == 0)? 0x10 : 0;
>
> Why are you setting vucf twice?
>

Lol.. Copy paste problem.. Will remove it.


>
> > +    .encode         = s302m_encode_frame,
>
> Please implement a encode2 function instead, "encode" is deprecated.
> Also a regression test would be very good thing to have.
>

Will use encode2

regarding regression test: Do you have some example how I can look at to
make one?
Will it be ok if the regression is part of another patch?


Will be working on a new patch this week

Thanks for the review.



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>