[FFmpeg-devel] [PATCH] MPEG-4 Parametric Stereo decoder

[Vitor Sessak]

On 05/29/2010 07:30 PM, Alex Converse wrote:
> On Sun, May 23, 2010 at 10:29 AM, Vitor Sessak<vitor1001 at gmail.com>  wrote:
>> On 05/20/2010 07:51 PM, Alex Converse wrote:
>>>
>>> Yes there are some places where it can be further optimized but I'm
>>> losing motivation on this quickly so perhaps some review will respark
>>> my interest.
>>
>> I gave a look at it, a few comments...
>>
>>> --- a/libavcodec/sbr.h
>>> +++ b/libavcodec/sbr.h
>>> @@ -31,6 +31,7 @@
>>>
>>>   #include<stdint.h>
>>>   #include "fft.h"
>>> +#include "ps.h"
>>>
>>>   /**
>>>   * Spectral Band Replication header - spectrum parameters that invoke a
>>> reset if they differ from the previous header.
>>> @@ -133,6 +134,7 @@ typedef struct {
>>>      ///The number of frequency bands in f_master
>>>      unsigned           n_master;
>>>      SBRData            data[2];
>>> +    PSContext          ps;
>>
>> Missing doxy (what does ps == -1 means?).
>>
>
> This is never compared to -1, perhaps you are confusing it with
> MPEG4AudioConfig.ps which does have doxy?

Probably.

[...]

>> You seems to assume that these constants are at<  10, so I'd put
>>
>> enum {
>>     huff_iid_df1 = 0,
>>     huff_iid_dt1,
>>     huff_iid_df0,
>>     huff_iid_dt0,
>>     huff_icc_df,
>>     huff_icc_dt,
>>     huff_ipd_df,
>>     huff_ipd_dt,
>>     huff_opd_df,
>>     huff_opd_dt,
>> };
>>
>
> Doesn't the C spec imply that?

Yes it does. If anyone else is wondering, in section 6.7.2.2:

> The identifiers in an enumerator list are declared as constants that have type int and
> may appear wherever such are permitted. An enumerator with = defines its
> enumeration constant as the value of the constant expression. If the first enumerator has
> no =, the value of its enumeration constant is 0. Each subsequent enumerator with no =
> defines its enumeration constant as the value of the constant expression obtained by
> adding 1 to the value of the previous enumeration constant. (The use of enumerators with
> = may produce enumeration constants with values that duplicate other values in the same
> enumeration.) The enumerators of an enumeration are also known as its members.


>>> +static int ps_extension(GetBitContext *gb, PSContext *ps, int
>>> ps_extension_id)
>>> +{
>>> +    int e;
>>> +    int count = get_bits_count(gb);
>>> +    if (!ps_extension_id) {
>>> +        ps->enable_ipdopd = get_bits1(gb);
>>> +        if (ps->enable_ipdopd) {
>>> +            for (e = 0; e<  ps->num_env; e++) {
>>> +                int dt = get_bits1(gb);
>>> +                ipd_data(gb, ps, e, dt);
>>> +                dt = get_bits1(gb);
>>> +                opd_data(gb, ps, e, dt);
>>> +            }
>>> +        }
>>> +        skip_bits1(gb);      //reserved_ps
>>> +    }
>>> +    return get_bits_count(gb) - count;
>>> +}
>>
>> if (ps_extension_id)
>>     return 0;

If you prefer without this change, please say so. Or have you simply 
forgot this hunk?

>>> +
>>> +int ff_ps_data(AVCodecContext *avctx, GetBitContext *gb, PSContext *ps)
>>> +{
>>
>> [...]
>>
>>> +    //Fix up envelopes
>>> +    if (!ps->num_env) {
>>> +        ps->num_env = 1;
>>> +        ps->border_position[1] = 31;
>>> +        if (ps->enable_iid&&  ps->num_env_old>  1) {
>>> +            memcpy(ps->iid_par, ps->iid_par+ps->num_env_old-1,
>>> sizeof(ps->iid_par[0]));
>>> +        }
>>> +        if (ps->enable_icc&&  ps->num_env_old>  1) {
>>> +            memcpy(ps->icc_par, ps->icc_par+ps->num_env_old-1,
>>> sizeof(ps->icc_par[0]));
>>> +        }
>>> +        if (ps->enable_ipdopd&&  ps->num_env_old>  1) {
>>> +            memcpy(ps->ipd_par, ps->ipd_par+ps->num_env_old-1,
>>> sizeof(ps->ipd_par[0]));
>>> +            memcpy(ps->opd_par, ps->opd_par+ps->num_env_old-1,
>>> sizeof(ps->opd_par[0]));
>>> +        }
>>> +    } else if (ps->border_position[ps->num_env]<  numQMFSlots - 1) {
>>> +        //Create a fake envelope
>>> +        if (ps->enable_iid&&  ps->num_env_old>  1) {
>>> +            memcpy(ps->iid_par+ps->num_env, ps->iid_par+ps->num_env-1,
>>> sizeof(ps->iid_par[0]));
>>> +        }
>>> +        if (ps->enable_icc&&  ps->num_env_old>  1) {
>>> +            memcpy(ps->icc_par+ps->num_env, ps->icc_par+ps->num_env-1,
>>> sizeof(ps->icc_par[0]));
>>> +        }
>>> +        if (ps->enable_ipdopd) {
>>> +            memcpy(ps->ipd_par+ps->num_env, ps->ipd_par+ps->num_env-1,
>>> sizeof(ps->ipd_par[0]));
>>> +            memcpy(ps->opd_par+ps->num_env, ps->opd_par+ps->num_env-1,
>>> sizeof(ps->opd_par[0]));
>>> +        }
>>> +        ps->num_env++;
>>> +        ps->border_position[ps->num_env] = numQMFSlots - 1;
>>> +    }
>>> +
>>
>> The memcpy's shouldn't be much hard to factorize.

?

[...]

A few more suggestions:

> int ff_ps_read_data(AVCodecContext *avctx, GetBitContext *gb_host, PSContext *ps, int bits_left)
> {

[...]

>     if (ps->enable_iid)
>         for (e = 0; e < ps->num_env; e++) {
>             int dt = get_bits1(gb);
>             if (iid_data(avctx, gb, ps, e, dt))
>                 goto err;
>         }
>     else
>         memset(ps->iid_par, 0, sizeof(ps->iid_par));

Is the ps->iid_par ever used if ps->enable_iid is 0? (If yes, is it 
doing useless time consuming calculations with 0?)

> static void decorrelation(PSContext *ps, float (*out)[32][2], const float (*s)[32][2], int is34)
> {
>     float power[34][PS_QMF_TIME_SLOTS];

[...]

>     memset(power, 0, sizeof(power));

float power[34][PS_QMF_TIME_SLOTS] = {0};
?

-Vitor