[FFmpeg-devel] [PATCH] Multi-Channel Correlation in ALS

[Michael Niedermayer]

On Mon, Dec 21, 2009 at 01:15:23AM +0100, Thilo Borgmann wrote:
> Am 16.12.09 05:07, schrieb Michael Niedermayer:
> > On Tue, Dec 15, 2009 at 04:20:33PM +0100, Thilo Borgmann wrote:
[...]
> 
> > 
> > 
> >> +
> >> +    align_get_bits(gb);
> >> +}
> >> +
> >> +
> > 
> >> +/** Recursively reverts the inter-channel correlation for a block.
> >> + */
> >> +static void revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
> >> +                                       ALSChannelData **cd, int *reverted,
> >> +                                       unsigned int offset, int c)
> >> +{
> >> +    ALSChannelData *ch = cd[c];
> >> +    unsigned int   dep = 0;
> >> +
> >> +    if (reverted[c])
> >> +        return;
> >> +
> >> +    while (!ch[dep].stop_flag) {
> >> +        if (!reverted[ch[dep].master_channel])
> >> +            revert_channel_correlation(ctx, bd, cd, reverted, offset,
> >> +                                       ch[dep].master_channel);
> >> +
> >> +        dep++;
> >> +    }
> > 
> > Make sure this cannot overflow the stack (every recursive call needs space
> > there)
> 
> Is there a way to find out if the stack memory gets close to zero and

iam not aware of a portable way to do this
but in general id say a few kb should be available on the stack a few mb is
a risky assumtation.


> would this be an appropriate check?
> In this patch, the stop_flag will always be set so the number of
> iterations of the while-loop is always finite.
> 
> > 
> > also dont forget to test the code with a fuzzer to make sure it doesnt
> > crash and please also go over it to make sure there are no other buffer
> > overflows, you know the code better then i do so i might not spot all.
> 

> I played around with the trasher a lot and got some pitfalls removed. I
> end up in demuxer segfaults for my trashed files.

please report segfaults on roundup unless they are already reported


[...]
> @@ -%ld,%ld +%ld,%ld @@
>  }
>  
>  
> +/** Reads the channel data.
> +  */
> +static int read_channel_data(ALSDecContext *ctx, ALSChannelData *cd, int c)
> +{
> +    GetBitContext *gb       = &ctx->gb;
> +    ALSChannelData *current = cd;
> +    unsigned int channels   = ctx->avctx->channels;
> +    int entries             = 0;
> +
> +    while (entries < channels && !(current->stop_flag = get_bits1(gb))) {
> +        current->master_channel = get_bits_long(gb, av_ceil_log2(channels));
> +
> +        if (current->master_channel >= channels) {
> +            av_log(ctx->avctx, AV_LOG_ERROR, "Invalid master channel!\n");
> +            return -1;
> +        }
> +
> +        if (current->master_channel != c) {
> +            current->time_diff_flag = get_bits1(gb);
> +            current->weighting[0]   = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +            current->weighting[1]   = mcc_weightings[av_clip(decode_rice(gb, 2) + 14, 0, 32)];
> +            current->weighting[2]   = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +
> +            if (current->time_diff_flag) {
> +                current->weighting[3] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +                current->weighting[4] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +                current->weighting[5] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +
> +                current->time_diff_sign  = get_bits1(gb);
> +                current->time_diff_index = get_bits(gb, ctx->ltp_lag_length - 3) + 3;
> +                if (current->time_diff_sign)
> +                    current->time_diff_index = -current->time_diff_index;
> +            }
> +        }
> +
> +        current++;
> +        entries++;
> +    }
> +
> +    if (entries == channels) {
> +        current--;
> +        current->stop_flag = 1;
> +        current->master_channel = c;
> +        return -1;
> +    }

there are 2 return -1 in this function but only one sets these things like
stop_flag, why is it needed in one but not the other ?


[...]
> @@ -%ld,%ld +%ld,%ld @@

whatever generated this patch is not too well working


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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20091221/09f9ad49/attachment.pgp>