[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

>> +        // TODO: use this to actually do channel sorting
> 
> I think outputting channels in the correct order is quite important
> i dont mind if thats done after the decoder is in svn, but i would mind
> if that is not done within a year ...

I will be around to do some more work on the decoder after the GSoC
version is applied. So this will be done in the "near" future.



> 
> [...]
>> +/** Decodes blocks dependently.
>> + */
>> +static int decode_blocks(ALSDecContext *ctx, unsigned int ra_frame,
>> +                         unsigned int c, const unsigned int *div_blocks,
>> +                         unsigned int *js_blocks)
>> +{
>> +    ALSSpecificConfig *sconf = &ctx->sconf;
>> +    unsigned int offset = 0;
>> +    int32_t *raw_samples_R;
>> +    int32_t *raw_samples_L;
>> +    unsigned int b;
>> +
>> +    // decode all blocks
>> +    for (b = 0; b < ctx->num_blocks; b++) {
>> +        unsigned int s;
>> +        raw_samples_L = ctx->raw_samples[c    ] + offset;
>> +        raw_samples_R = ctx->raw_samples[c + 1] + offset;
>> +        if (read_block_data(ctx, ra_frame, raw_samples_L, div_blocks[b],
>> +                            &js_blocks[0], raw_samples_R) ||
>> +            read_block_data(ctx, ra_frame, raw_samples_R, div_blocks[b],
>> +                            &js_blocks[1], raw_samples_L)) {
>> +            // damaged block, write zero for the rest of the frame
>> +            zero_remaining(b, ctx->num_blocks, div_blocks, raw_samples_L);
>> +            zero_remaining(b, ctx->num_blocks, div_blocks, raw_samples_R);
>> +            return -1;
>> +        }
> 
> This code looks strange, the first call would use raw_samples_R
> that does not seem initialized at that point
> 
> have all the js cases been tested?

You're right but only negative indices are read from raw_samples_R in
that case thus no uninitialized data is accessed unintentionally.

Yes, all js_blocks values were tested.



> 
>> +    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);
> 
> has the ra_distance==0 case been tested?
> 

Yes.



> 
>> +        av_log(ctx->avctx, AV_LOG_WARNING,
>> +               "Reading frame data failed. Skipping RA unit.\n");
>> +
>> +    // increment the frame counter
>> +    ctx->frame_id++;
>> +
>> +    // 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);
> 
> data_size is not checked before writing into the buffer
> 
> 
> ...
> 
>> +    // check for size of decoded data
>> +    data_size = sconf->frame_length * avctx->channels *
>> +                (av_get_bits_per_sample_format(avctx->sample_fmt) >> 3);
>> +
>> +    if (data_size > INT_MAX) {
>> +        av_log(avctx, AV_LOG_ERROR, "Decoded data exceeds buffer size.\n");
>> +        decode_end(avctx);
>> +        return -1;
>> +    }
> 
> whatever this check should do it doesnt work, it wont ever be true as the
> multiplications are using int not int64

I did this to check data_size before writing into the buffer - you
already complained about this. Ok it is wrong as-is but is this a valid
test for the case mentioned above if done correctly (MUL64()) or not?


-Thilo