[FFmpeg-devel] [PATCH] ALS decoder

[Michael Niedermayer]

On Sat, Aug 22, 2009 at 11:36:16PM +0200, Thilo Borgmann wrote:
[...]
> >> +    // skip the header and trailer data
> >> +    if (buffer_size < ht_size)
> >> +        return -1;
> >> +
> >> +    ht_size <<= 3;
> >> +
> >> +    while (ht_size > 0) {
> >> +        int len = FFMIN(ht_size, INT32_MAX);
> >> +        skip_bits_long(&gb, len);
> >> +        ht_size -= len;
> >> +    }
> > 
> > i dont think theres sense in supporting skiping of more than 500mb
> > as valid packets should not have such sizes
> This is done once per stream. Does the loop hurts so much to limit
> ourselves to 32-bit?

is it really allowed for that to be even remotely that large? or even
possible? can the bitstream reader even handle that?


> 
> 
> >> +/** Parses the bs_info item to extract the block partitioning.
> >> + */
> > 
> > without having read the spec, that says nothing to me ...
> > can this be clarified in 1 sentance or is the reader required to have read
> > the spec?
> Reworded a bit. But I think if block partitioning is unclear, a much
> better comment would be beyond one sentence and explains block switching.

well, more explanation cant hurt, unless the spec explains it well in which
case you could add a reference to the specfic part of the spec


> 
> 
> 
> >> +/** Reads and decodes a Rice codeword.
> >> + */
> >> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
> >> +{
> >> ...
> >> +}
> > 
> > theres some obvious factorizeable code in there
> Refactorized as far as I can see.
> I rebenchmarked and it still needs around 760 dezicykles.

> The skip rate sunk to a fifth, if this indicates something?

yes it might be faster of the skips are fewer

[...]
> >> +            memcpy(ctx->prev_raw_samples, raw_samples - sconf->max_order,
> >> +                   sizeof(int64_t) * sconf->max_order);
> > 
> > instead of duplicating the type (int64_t) the type of the array should be used
> Sorry I don't understand this. Please elaborate.

sizeof(*ctx->prev_raw_samples) or something

[..]
> 
> >> +    // allocate previous raw sample buffer
> >> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> >> +        decode_end(avctx);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    // allocate raw and carried sample buffer
> >> +    if (!(ctx->raw_buffer = av_mallocz(sizeof(int64_t) *
> >> +                                       avctx->channels * channel_size))) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> >> +        decode_end(avctx);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> > 
> > factorizeable with a goto
> If you don't veto on a or'd if, I would prefer that.

the if|| is fine

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20090823/e9aeab67/attachment.pgp>