[FFmpeg-devel] [PATCH] ALS decoder

[Michael Niedermayer]

On Wed, Aug 26, 2009 at 05:34:47PM +0200, Thilo Borgmann wrote:
> 
> >> +        // reconstruct shifted signal
> >> +        if (*shift_lsbs)
> >> +            for (smp = -1; smp >= -sconf->max_order; smp--)
> >> +                raw_samples[smp] >>= *shift_lsbs;
> > 
> > has that been tested? or is shift_lsbs == 0 for all files we have?
> Yes, intensively tested.

good, i just felt the shifting right and then let later looked odd
but i had not thought about it much


> 
> 
> 
> >> +/** Computes the bytes left to decode for the current frame
> >> + */
> >> +static inline unsigned int count_remaining(unsigned int b, unsigned int b_max,
> >> +                                           unsigned int *div_blocks)
> > 
> > this doesnt need to be inline, i belive its not speed critical, but rather
> > handling just damaged bitstreams or am i misunderstanding the code?
> Yes.
> 
> > 
> > also maybe this one could do the memset as well
> I had done this, but factored it out again because it would count the
> remaining samples twice in case of joint-stereo.

if the code is irrelevant speedwise, counting twice, if that is cleaner
and simpler is no problem


> 
> 
> >> +/** Decodes an ALS frame.
> >> + */
> >> +static int decode_frame(AVCodecContext *avctx,
> >> +                        void *data, int *data_size,
> >> +                        AVPacket *avpkt)
> >> +{
> >> +    ALSDecContext *ctx       = avctx->priv_data;
> >> +    ALSSpecificConfig *sconf = &ctx->sconf;
> >> +    const uint8_t *buffer    = avpkt->data;
> >> +    int buffer_size          = avpkt->size;
> >> +    int invalid_frame        = 0;
> >> +    unsigned int c, sample, ra_frame, bytes_read, shift;
> >> +
> >> +    init_get_bits(&ctx->gb, buffer, buffer_size * 8);
> >> +    ra_frame = sconf->ra_distance && !(ctx->frame_id % sconf->ra_distance);
> >> +
> >> +    // the last frame to decode might have a different length
> >> +    if (ctx->num_frames && ctx->num_frames - 1 == ctx->frame_id) {
> >> +        ctx->cur_frame_length = ctx->last_frame_length;
> >> +    }
> >> +
> >> +    // decode the frame data
> >> +    if ((invalid_frame = read_frame_data(ctx, ra_frame)))
> >> +        av_log(ctx->avctx, AV_LOG_WARNING,
> >> +               "Reading frame data failed. Skipping RA unit.\n");
> >> +
> > 
> >> +    // increment the frame counter
> >> +    ctx->frame_id++;
> > 
> > that wont work with seeking
> > I think ive not missed some setting of frame_id to a value from the
> > bitstream
> No, it's selfmade. The decoder has to know, which frame it is in.
> How to determine it in a seeking compatible way?

the short awnser is certainly that, you cant

what is the spec saying about this? is the demuxer required to pass
some frame number to the decoder? (that makes ALS only possible in
some containers)


> 
> 
> > 
> > 
> >> +
> >> +    // transform decoded frame into output format
> >> +    #define INTERLEAVE_OUTPUT(bps)                                 \
> >> +    {                                                              \
> >> +        int##bps##_t *dest = (int##bps##_t*) data;                 \
> >> +        shift = bps - ctx->avctx->bits_per_raw_sample;             \
> >> +        for (sample = 0; sample < ctx->cur_frame_length; sample++) \
> >> +            for (c = 0; c < avctx->channels; c++)                  \
> >> +                *dest++ = ctx->raw_samples[c][sample] << shift;    \
> >> +    }
> >> +
> >> +    if (ctx->avctx->bits_per_raw_sample <= 16) {
> >> +        INTERLEAVE_OUTPUT(16)
> >> +    } else {
> >> +        INTERLEAVE_OUTPUT(32)
> >> +    }
> >> +
> >> +    *data_size = ctx->cur_frame_length * avctx->channels
> >> +                 * (av_get_bits_per_sample_format(avctx->sample_fmt) >> 3);
> > 
> > i think you dont check data_size for being large enough
> Added a check for data_size > INT_MAX in decode_init(). I hope this is
> what you meant...

*data_size should contain the size of the output buffer, you should check
if that is large enough before writing into it.
Maybe its redundant if frame_size is set but its better to check anyway

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

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/20090826/0f6afc01/attachment.pgp>