[Ffmpeg-devel] [PATCH] THP PCM decoder (GSoC Qualification)

[Marco Gerards]

Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:

Hi,

Thanks a lot for your comments.  I read them and used them to improve
the patch I just sent in.

> Hello,
> On Mon, Apr 02, 2007 at 05:53:54PM +0200, Marco Gerards wrote:
>> +    case CODEC_ID_ADPCM_THP:
>> +      {
>> +        GetBitContext gb;
>> +        float table[16][2];
>> +        int samplecnt;
>> +        int prev1[2], prev2[2];
>
> unaing "int prev[2][2];" might possibly simplify some code?

I don't think so.  It will remain more of less the same.

> [...]
>> +        for (ch = 0; ch < 2; ch++)
>> +          for (i = 0; i < 16; i++) {
>> +              /* Read the fixed point entry and store as floating
>> +                 point.  */
>> +              int entry = get_sbits(&gb, 16);
>> +              table[i][ch] = (float) entry / pow(2, 11);
>> +          }
>
> Trying to avoid all that float stuff might be worth it. And at least pow
> is a waste of CPU power, (1 << 11) should work just as well.

Done.

>> +            while (sample > 0) {
>> +                uint8_t index = get_bits (&gb, 4) & 7;
>> +                float exp = pow(2, get_bits (&gb, 4));
>
> float seems pointless, "int exp = 1 << get_bits(&gb, 4);" should work
> the same?

Yes, I have made this change.

>> +                float factor1 = table[index * 2][ch];
>> +                float factor2 = table[index * 2 + 1][ch];
>> +        
>> +                /* Decode 14 samples.  */
>> +                for (n = 0; n < 14; n++) {
>> +                    int sampledat = get_sbits (&gb, 4);
>> +                    *samples = prev1[ch]*factor1 
>> +                               + prev2[ch]*factor2 + sampledat * exp;
>
> Actually multiplying with a power of two is a waste of CPU.
> Any problem using
> int exp = get_bits (&gb, 4);
> above and
> ... + sampledat << exp;
> here?

You are right :-).

>> +                    prev2[ch] = prev1[ch];
>> +                    prev1[ch] = *samples++;
>> +                    sample--;
>> +                }
>
> Hm. sample isn't checked here. Can that cause any problems? If not you
> should probably just pull it out of the loop and do "sample -= 14;"

I have added another kind of test to my newest patch.  I hope this is
sufficient.  I moved sample--; out of the loop as you suggested.

--
Marco