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

[Alex Converse]

On Sat, May 29, 2010 at 2:16 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> 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:
[...]
>>>> +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?
>

I forgot it.  I'm not really excited about it but it removes a level
of indentation so I will go with it.

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

Wow yes. Fixed.

> [...]
>
> 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?)
>

Yes, they get dequantized via a LUT (in combination with icc) into pre
ipd/opd rotation values.
Furthermore, if the next frame has iid_enabled, and iid is deltacoded
in time (dt = 0), the last envelope value must be zero or the delta
coding suffers from a DC offset leaked from the previous frame.

>> 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};
> ?
>

Fixed