[FFmpeg-devel] [PATCH] MPEG-4 Parametric Stereo decoder
Vitor Sessak
vitor1001
Sat May 29 20:16:53 CEST 2010
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
More information about the ffmpeg-devel
mailing list