[FFmpeg-devel] [RFC] Monkey's Audio decoder
Michael Niedermayer
michaelni
Wed Sep 12 17:22:08 CEST 2007
Hi
On Wed, Sep 12, 2007 at 02:37:33PM +0300, Kostya wrote:
> On Wed, Sep 12, 2007 at 12:18:57PM +0200, Michael Niedermayer wrote:
> > Hi
> [...]
> > i dont see how either of these would require the dummy packets
> > the decoder can bswap the data into an internal buffer (like it does currently
> > IIRC) and still correctly return the size of the decoded block
> > also it knows the offset of the next block from the end of the previous block
> > only the first offset of such a huge frame is unknown but that is passed anyway
> >
>
> Fixed. No more dummy packets, decoder will report consuming of buffer by pieces (while using internal buffer).
> >
> > >
> > > The only problem with current approach - it makes error happening in decode_audio2()
> > > because input frame size is larger than output audio buffer (*frame_size_ptr < buf_size)
> > > but I am sure that condition may be dropped without any bad consequences.
> >
> > well if you check all audio decoders for buffer overflows ...
>
> still checking...
>
> > >
> > > > the problem with your approch is that if the stuff is stream copied then the
> > > > file will be full of these dummy packets
> > >
> > > I can't imagine this situation - it's another case of codec/container lock in.
> >
> > i can imaging people wanting to use it in avi, mkv, ...
> > and especially with mkv iam sure its only a matter of time until someone finds
> > a completely broken and sick way to do it (aka store the megabyte sized frames
> > as is) and without dummy packets ...
>
> But you should admit that with dummy packets it's even more broken and sick.
yes
[...]
> + /* Decoder state */
> + uint32_t CRC;
> + int frameflags;
> + int currentframeblocks;
> + int blocksdecoded;
> + APEPredictor predictor;
> +
> + /* 4608*4 = 18432 bytes per channel */
> + int32_t decoded0[BLOCKS_PER_LOOP];
> + int32_t decoded1[BLOCKS_PER_LOOP];
> +
> + /* Statically allocate the filter buffers */
> +
> + int16_t* filterbuf[APE_FILTER_LEVELS];
not doxygen compatible
[...]
> +/**
> + * Calculate culmulative frequency for next symbol. Does NO update!
> + * @param tot_f is the total frequency or (code_value)1<<shift
> + * @return the culmulative frequency
> + */
> +static inline int range_decode_culfreq(APEContext * ctx, int tot_f)
> +{
> + int tmp;
> +
> + range_dec_normalize(ctx);
> +
> + ctx->rc.help = ctx->rc.range / tot_f;
> + tmp = ctx->rc.low / ctx->rc.help;
> +
> + return tmp;
> +}
the tmp variable is unneccesary
> +
> +/**
> + * Decode value with given size in bits
> + * @param shift number of bits to decode
> + */
> +static inline int range_decode_culshift(APEContext * ctx, int shift)
> +{
> + int tmp;
> + range_dec_normalize(ctx);
> + ctx->rc.help = ctx->rc.range >> shift;
> + tmp = ctx->rc.low / ctx->rc.help;
> +
> + return tmp;
> +}
the tmp variable is unneccesary
> +
> +
> +/**
> + * Update decoding state
> + * @param sy_f the interval length (frequency of the symbol)
> + * @param lt_f the lower end (frequency sum of < symbols)
> + */
> +static inline void range_decode_update(APEContext * ctx, int sy_f, int lt_f)
> +{
> + int tmp;
> + tmp = ctx->rc.help * lt_f;
> + ctx->rc.low -= tmp;
> + ctx->rc.range = ctx->rc.help * sy_f;
> +}
and here tmp is as well not needed
ctx->rc.low -= ctx->rc.help * lt_f;
ctx->rc.range = ctx->rc.help * sy_f;
will do and is not less readable
> +
> +/** Decode 16-bit value */
> +static inline int short range_decode_short(APEContext * ctx)
> +{
> + int tmp = range_decode_culshift(ctx, 16);
> + range_decode_update(ctx, 1, tmp);
> + return tmp;
> +}
> +
> +/** Decode n bits (n <= 16) without modelling - based on range_decode_short */
> +static inline int range_decode_bits(APEContext * ctx, int n)
> +{
> + int tmp = range_decode_culshift(ctx, n);
> + range_decode_update(ctx, 1, tmp);
> + return tmp;
> +}
static inline int short range_decode_short(APEContext * ctx)
{
return range_decode_bits(ctx, 16);
}
or even better change the code to use range_decode_bits(ctx, 16)
> +
> +
> +/** Finish decoding */
> +static inline void range_done_decoding(APEContext * ctx)
> +{
> + range_dec_normalize(ctx); /* normalize to use up all bytes */
> +}
i hate wraper functions ...
[...]
> +/**
> + * Decode symbol
> + * @param counts probability range start position
> + * @param count_diffs probability range widths
> + */
> +static inline int range_get_symbol(APEContext * ctx,
> + const uint32_t counts[],
> + const uint16_t counts_diff[])
> +{
> + int symbol, cf;
> +
> + cf = range_decode_culshift(ctx, 16);
> +
> + /* figure out the symbol inefficiently; a binary search would be much better */
> + for (symbol = 0; counts[symbol + 1] <= cf; symbol++);
> +
> + range_decode_update(ctx, counts_diff[symbol], counts[symbol]);
> +
> + return symbol;
> +}
> +/** @} */ // group rangecoder
maybe the range coder should be put into its own file? though if you
prefer it in ape*.c thats fine as well ...
> +
> +static inline void update_rice(APERice *rice, int x)
> +{
> + rice->ksum += ((x + 1) / 2) - ((rice->ksum + 16) >> 5);
if x is guranteed to be >0 then the /2 can be changed to >>1 which is
faster
[...]
> + x += (overflow << tmpk);
superflous ()
> + } else {
> + int base, pivot;
> +
> + pivot = rice->ksum >> 5;
> + if (pivot == 0)
> + pivot = 1;
> +
> + overflow = range_get_symbol(ctx, counts_3980, counts_diff_3980);
> +
> + if (overflow == (MODEL_ELEMENTS - 1)) {
> + overflow = range_decode_short(ctx) << 16;
> + overflow |= range_decode_short(ctx);
> + }
> +
> + base = range_decode_culfreq(ctx, pivot);
> + range_decode_update(ctx, 1, base);
> +
> + x = base + (overflow * pivot);
superflous ()
[...]
> + A = *decoded0;
> +
> + p->buf[YDELAYA] = currentA;
> + p->buf[YDELAYA - 1] = p->buf[YDELAYA] - p->buf[YDELAYA - 1];
> +
> + predictionA = p->buf[YDELAYA ] * p->coeffsA[0][0] +
> + p->buf[YDELAYA - 1] * p->coeffsA[0][1] +
> + p->buf[YDELAYA - 2] * p->coeffsA[0][2] +
> + p->buf[YDELAYA - 3] * p->coeffsA[0][3];
> +
> + currentA = A + (predictionA >> 10);
> +
> + p->buf[YADAPTCOEFFSA] = APESIGN(p->buf[YDELAYA ]);
> + p->buf[YADAPTCOEFFSA - 1] = APESIGN(p->buf[YDELAYA - 1]);
> +
> + if (A > 0) {
> + p->coeffsA[0][0] -= p->buf[YADAPTCOEFFSA ];
> + p->coeffsA[0][1] -= p->buf[YADAPTCOEFFSA - 1];
> + p->coeffsA[0][2] -= p->buf[YADAPTCOEFFSA - 2];
> + p->coeffsA[0][3] -= p->buf[YADAPTCOEFFSA - 3];
> + } else if (A < 0) {
> + p->coeffsA[0][0] += p->buf[YADAPTCOEFFSA ];
> + p->coeffsA[0][1] += p->buf[YADAPTCOEFFSA - 1];
> + p->coeffsA[0][2] += p->buf[YADAPTCOEFFSA - 2];
> + p->coeffsA[0][3] += p->buf[YADAPTCOEFFSA - 3];
> + }
> +
> + p->buf++;
> +
> + /* Have we filled the history buffer? */
> + if (p->buf == p->historybuffer + HISTORY_SIZE) {
> + memmove(p->historybuffer, p->buf, PREDICTOR_SIZE * sizeof(int32_t));
> + p->buf = p->historybuffer;
> + }
> +
> + p->filterA[0] = currentA + ((p->filterA[0] * 31) >> 5);
> + *(decoded0++) = p->filterA[0];
this also looks quite similar to half of predictor_update_filter() maybe
something more can be factored out?
[...]
> + /* should not happen but who knows */
> + if (BLOCKS_PER_LOOP * 2 * avctx->channels > *data_size) {
> + av_log (avctx, AV_LOG_ERROR, "Packet size is too big to be handled in lavc! (max is %d where you have %d)\n", *data_size, s->samples * 2 * avctx->channels);
> + return -1;
> + }
BLOCKS_PER_LOOP * 2 * avctx->channels can overflow if channels has a insane
value
[...]
and patch ok after these, no need to resend it again
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20070912/f182ab30/attachment.pgp>
More information about the ffmpeg-devel
mailing list