[FFmpeg-devel] [PATCH] WMA Voice decoder

Ronald S. Bultje rsbultje
Thu Feb 11 04:30:57 CET 2010


Hi,

On Sat, Feb 6, 2010 at 11:19 AM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Ronald S. Bultje wrote:
>> + ? ?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.

Left as-is as discussed earlier.

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

Removed, since I use nmax now.

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

Yes, if aw_n_pulses[] == 0.

>> + ? ?} else /* FCB_TYPE_EXC_PULSES */ {
>> + ? ? ? ?int offset_nbits = 5 - frame_desc->log_n_blocks;
>> +
>> + ? ? ? ?fcb.no_repeat_mask = -1;
>> + ? ? ? ?for (n = 0; n < 5; n++) {
>> + ? ? ? ? ? ?float pulse = get_bits1(gb) ? 1.0 : -1.0;
>> + ? ? ? ? ? ?int idx1, idx2;
>> +
>> + ? ? ? ? ? ?idx1 ? ? ? ? ? = get_bits(gb, offset_nbits);
>> + ? ? ? ? ? ?fcb.x[fcb.n] ? = n + 5 * idx1;
>> + ? ? ? ? ? ?fcb.y[fcb.n++] = pulse;
>> + ? ? ? ? ? ?if (n < frame_desc->dbl_pulses) {
>> + ? ? ? ? ? ? ? ?idx2 ? ? ? ? ? = get_bits(gb, offset_nbits);
>> + ? ? ? ? ? ? ? ?fcb.x[fcb.n] ? = n + 5 * idx2;
>> + ? ? ? ? ? ? ? ?fcb.y[fcb.n++] = (idx1 >= idx2) ? pulse : -pulse;
>> + ? ? ? ? ? ?}
>> + ? ? ? ?}
>> + ? ?}
>>
>
> The else{} case is very close to ff_decode_10_pulses_35bits().

Documented as suggested, and I renamed all variables to match. As
noted, I believe that variables in ff_decode_10pulses_35bits() have a
confusing name (pos1 is called pos2, and conversely), so that's still
the wrong way around. I'd go for renaming them in
ff_decode_10pulses_35bits() rather than here, though.

>> + ? ?/*
>> + ? ? * Pitch (per ACB type):
>> + ? ? * - type 0: unused
>> + ? ? * - type 1: provided (globally) for the whole frame. In
>> #synth_block(),
>> + ? ? * ? ? ? ? ? ?we derive the "pitch-per-sample" for adaptive codebook
>> + ? ? * ? ? ? ? ? ?reading.
>> + ? ? * - type 2: provided per block (see just before the call to
>> + ? ? * ? ? ? ? ? ?#synth_block()), so not read here.
>> + ? ? */
>
> I think it would be more readable if you move the comments inside the "case
> ACB_XX:" statements

Done as suggested.

>> + ? ?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?

Removed.

New patch coming in my next email, hopefully.

Ronald



More information about the ffmpeg-devel mailing list