[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Oct 30, 2008, at 3:39 PM, Michael Niedermayer wrote:

> On Thu, Oct 30, 2008 at 03:18:45PM -0700, Kenan Gillet wrote:
>>
>> On Oct 30, 2008, at 6:16 AM, Michael Niedermayer wrote:
>>
>>> On Tue, Oct 28, 2008 at 04:47:02PM -0700, Kenan Gillet wrote:
> [...]
>>
>> [...]
>>>> +static void interpolate_lspf(float *interpolated_lspf,
>>>> +                             const float curr_weight,
>>>> +                             const float *curr_lspf,
>>>> +                             const float *prev_lspf) {
>>>> +    int   i;
>>>> +
>>>> +    for (i = 0; i < 10; i++)
>>>> +        interpolated_lspf[i] = (1.0 - curr_weight) * prev_lspf[i]
>>>> +                             +        curr_weight  * curr_lspf[i];
>>>> +}
>>>
>>> iam ok with the code but the names have to be changed
>>> both fixed point (ff_acelp_weighted_vector_sum) and this should have
>>> similar names
>>> now imho *celp and lsp dont belong in the names as the code is not
>>> specific
>>> for that.
>>
>> what about makin and using  a more generic
>> ff_weighted_vector_sumf(
>>         float* out,
>>         const float *in_a,
>>         const float *in_b,
>>         float weight_coeff_a,
>>         float weight_coeff_b,
>>         int length);
>
> fine

done

> [...]
>
>> and I am not sure if bigger is better or lower is faster ;)
>
> lower is faster ...

so no change then


> [...]
>>
>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * Determine the framerate from the frame size and/or the first
>>>> byte of the frame.
>>>> + *
>>>> + * @return 0 on success, negative error number otherwise.
>>>> + */
>>>> +static int determine_framerate(AVCodecContext *avctx,
>>>> +                               QCELPContext *q,
>>>> +                               const int buf_size,
>>>> +                               uint8_t **buf) {
>>>> +    int claimed_rate = -1;
>>>> +
>>>> +    switch (buf_size) {
>>>> +    case 35: claimed_rate = RATE_FULL;
>>>> +    case 34: q->framerate = RATE_FULL;
>>>> +             break;
>>>> +
>>>> +    case 17: claimed_rate = RATE_HALF;
>>>> +    case 16: q->framerate = RATE_HALF;
>>>> +             break;
>>>> +
>>>> +    case  8: claimed_rate = RATE_QUARTER;
>>>> +    case  7: q->framerate = RATE_QUARTER;
>>>> +             break;
>>>> +
>>>> +    case  4: claimed_rate = RATE_OCTAVE;
>>>> +    case  3: q->framerate = RATE_OCTAVE;
>>>> +             break;
>>>> +
>>>> +    case  1: claimed_rate = SILENCE;
>>>> +    case  0: q->framerate = SILENCE;
>>>> +             break;
>>>> +
>>>> +    default:
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    if (claimed_rate >= 0) {
>>>> +        q->framerate = *(*buf)++;
>>>> +
>>>> +        if (claimed_rate != q->framerate)
>>>> +            av_log(avctx, AV_LOG_WARNING, "Claimed framerate and
>>>> buffer size mismatch.\n");
>>>> +    } else
>>>> +        av_log(avctx, AV_LOG_WARNING,
>>>> +               "Framerate byte is missing, guessing the framerate
>>>> from packet size.\n");
>>>> +
>>>
>>> putting a switch for 34,16,7,3,0 in a seperate function the
>>> following could
>>> be used:
>>>
>>> q->framerate= foo(buf_size);
>>> if(q->framerate < 0){
>>>   q->framerate = *(*buf)++;
>>>
>>>   if (foo(buf_size+1) != q->framerate)
>>>       av_log(avctx, AV_LOG_WARNING, "Claimed framerate and buffer
>>> size mismatch.\n");
>>> }else{
>>>
>>> ...
>>>
>>
>> done, use 35, 17,8, 4 and 1 because it is much more probable that the
>> buffer contains a framerate byte.
>> Actually, I have never seen any files where the framerate byte is
>> missing; I just kept this check because
>> it was in the SOC code.
>
>> Should I remove it ? or keep it ?
>
> hmmmmm
> do what you prefer ...

I prefer keeping it until knowing what problem it was solving if any.

new updated patch coming

Kenan