[FFmpeg-devel] [PATCH] WMA Voice decoder

[Ronald S. Bultje]

Hi,

some more:

On Sat, Feb 6, 2010 at 11:19 AM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Ronald S. Bultje wrote:
>> +static void dequant_lsp10i(GetBitContext *gb, double *lsps)
>> +{
>> + ? ?static const uint16_t vec_sizes[4] = { 256, 64, 32, 32 };
>> + ? ?static const double mul_lsf[4] = {
>> + ? ? ? ?5.2187144800e-3, ? ?1.4626986422e-3,
>> + ? ? ? ?9.6179549166e-4, ? ?1.1325736225e-3
>> + ? ?};
>> + ? ?static const double base_lsf[4] = {
>> + ? ? ? ?M_PI * -2.15522e-1, M_PI * -6.1646e-2,
>> + ? ? ? ?M_PI * -3.3486e-2, ?M_PI * -5.7408e-2
>> + ? ?};
>>
>
> I think just putting the values multiplied by pi would be more readable.

Reason I did it this way (here, at least) is that the values are
actually exact (as far as I can see), so this is the most exact
representation I can give of the number. Multiplied by pi, it'd always
be rounded to something depending on how I type it.

If that's a big thing, I'll change it, I just figured I'd explain the
reason why I did it this way.

>> +#define NO_OFFSET -255
[..]
>> + ? ?if (n < 11)
>> + ? ? ? ?memset(&off_table[n], -1, (11 - n) * sizeof(int));
>
> -1 or NO_OFFSET?

I changed the code so in this particular function, any value < 0 is
invalid (later, I check if (table_off[x] >= 0), so it's the same. Only
for WMAVoiceContext->aw_first_pulse_off[0-1], NO_OFFSET is a specific
value (since negative offsets can be OK here).

>> + ? ?s->aw_first_pulse_off[0] = s->aw_first_pulse_off[1] = NO_OFFSET;
>
> Is this initialization really needed?

Yes, since each of the two blocks can have no values at all (if
aw_n_pulses[0-1] == 0). That happens occasionally. I could move it to
the end under if (aw_n_pulses[] == 0), but thought that was ugly.

>> + ? ?switch (frame_descs[bd_idx].acb_type) {
>> + ? ?case ACB_TYPE_NONE:
>> + ? ? ? ?memset(pitch, 0, sizeof(pitch[0]) *
>> frame_descs[bd_idx].n_blocks);
>>
>
> If it is unused, is it really needed to waste time zero'ing it out?

It prevents a valgrind warning because I specify it as an (in this
case unused) function argument to synth_block(). One way around that,
which you use in sipro as well, is to create a WMAFrameParams struct,
but I didn't do that yet. That could also house stuff that doesn't
need to be cached between frames but is currently in WMAVoiceContext,
such as silence_gain. Shall I do that instead?

Ronald