[FFmpeg-devel] [PATCH 2/2] Add SIPR decoder for 5k0, 6k5 and 8k5 modes

[Vitor Sessak]

Ronald S. Bultje wrote:
> Hi Vitor,
> 
> On Wed, Jan 6, 2010 at 7:47 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
>> Ronald S. Bultje wrote:
>>> +static void lsf_decode_fp(float *lsfnew, float *lsf_history,
>>> +                          const SiprParameters *parm)
>>> [..]
>>> +    ff_set_min_dist_lsf(lsfnew, LSFQ_DIFF_MIN / 4000., LP_FILTER_ORDER -
>>> 1);
>>>
>>> That value is only used once, you could just adjust the DIFF_MIN directly.
>>>
>>> +    lsfnew[9] = FFMIN(lsfnew[LP_FILTER_ORDER - 1], 1.3);
>>>
>>> Aha, so there is a max. value for the last one. I thought so. :-). OK,
>>> so here's my suggestion: let's merge this into the ff_set_min_dist()
>>> function. WMAVoice same, and so do others.
>>>
>> I'm not really happy about this for two reasons:
>>
>> 1- There is no max value for AMR and SIPR 16k
>> 2- Such a function would work in a slightly different way of what would
>> be logical (it doesn't impose a max distance for the last value):
> [..]
> 
> I'm thinking of a float version of ff_acelp_reorder_lsf() in
> libavcodec/lsp.c, I think it is 100% analogous to this code and
> WMAVoice uses it too.

There is a subtle difference (see below). Which one does WMAVoice use?

>> float in = {1, 5, 6, 7, 12}
>>
>> ff_set_min_dist_lsf_with_max(in, 2.0, 5, 14.0 /* max */);
>>
>> result: 1, 3, 6, 8, 12
> 
> min_dist, not max_dist, so I think the result would be:
> 
> result: 1, 5, 7, 9, 12.
> 
> right?

Of course, 10l. I'll try to explain my point after coffee and re-reading 
the code:

command: ff_set_min_dist_lsf_with_max(in, 2.0, 5, 14.0 /* max */);

in:                      { 1, 1.2,  7,  10, 15}
out:                     { 2,   4,  7,  10, 14}
ff_acelp_reorder_lsf():  { 2,   4,  7,  10, 14}
sane:                    { 2,   4,  7,  10, 14}

Where with "ff_acelp_reorder_lsf()" I mean replacing int16_t for floats. 
No problem for this input.

in:                      { 2, 2.2,  7,  9.5, 10  }
out:                     { 2,   4,  7,  9.5, 10  }
ff_acelp_reorder_lsf():  { 2,   4,  7,  9.5, 11.5}
sane:                    { 2,   4,  7,  9.5, 11.5}

here we see a problem

in:                      { 2, 4.5,  7,  1000, 1001}
out:                     { 2, 4.5,  7,  1000,   14}
ff_acelp_reorder_lsf():  { 2, 4.5,  7,  1000,   14}
sane:                    { 2, 4.5,  7,  12,     14}

granted that getting this one right would make the function a little 
less trivial...

-Vitor