[FFmpeg-cvslog] r23646 - in trunk/libavcodec: mpegaudio.h mpegaudiodec.c

[Måns Rullgård]

Vitor Sessak <vitor1001 at gmail.com> writes:

> On 06/19/2010 01:13 PM, M?ns Rullg?rd wrote:
>> vitor<subversion at mplayerhq.hu>  writes:
>>
>>> Author: vitor
>>> Date: Sat Jun 19 11:56:05 2010
>>> New Revision: 23646
>>>
>>> Log:
>>> Factorize the mpegaudio windowing code in a function and call it by a
>>> function pointer. Should allow for ASM optimizations.
>>>
>>> Modified:
>>>     trunk/libavcodec/mpegaudio.h
>>>     trunk/libavcodec/mpegaudiodec.c
>>>
>>> +/* 32 sub band synthesis filter. Input: 32 sub band samples, Output:
>>> +   32 samples. */
>>> +/* XXX: optimize by avoiding ring buffer usage */

BTW, any plans for doing that?

>>> +#if CONFIG_FLOAT
>>> [...]
>>> +#endif
>>> +
>>> +    offset = *synth_buf_offset;
>>> +    synth_buf = synth_buf_ptr + offset;
>>> +
>>> +#if FRAC_BITS<= 15&&  !CONFIG_FLOAT
>>
>> !CONFIG_FLOAT is always true here.
>
> Of course. I noticed that but I thought it would be better not to
> touch the original function in the first patch. If we settle for the
> code as is, I will remove it.

OK, just don't forget it.

>>> +    dct32(tmp, sb_samples);
>>> +    for(j=0;j<32;j++) {
>>> +        /* NOTE: can cause a loss in precision if very high amplitude
>>> +           sound */
>>> +        synth_buf[j] = av_clip_int16(tmp[j]);
>>> +    }
>>> +#else
>>> +    dct32(synth_buf, sb_samples);
>>> +#endif
>>> +
>>> +    apply_window_mp3_c(synth_buf, window, dither_state, samples, incr);
>>
>> Why doesn't this use a function pointer too?
>
> Because this is a public function used by other codecs (mpc and
> qdm2).

It's not public API, so we can change it whenever we please.

> It would need either to make them store and initialize a
> MPADecodeContext just for this function or to create some
> MPADSPContext just for this function, which is also silly.

There are probably other functions that could benefit from asm
optimisations too.  Starting with one might look silly, but it's still
the right way to go.  I did that with DTS and nobody objected.

> Of course, everything changes if you are planning to write a NEON
> optimized version of this function...

Maybe not NEON, but optimised fixed-point functions would make sense
on many CPUs (and DSPs).

>>>       offset = (offset - 32)&  511;
>>>       *synth_buf_offset = offset;
>>>   }
>>> +#endif
>>>
>>>   #define C3 FIXHR(0.86602540378443864676/2)
>>>
>>> @@ -2227,7 +2262,11 @@ static int mp_decode_frame(MPADecodeCont
>>>       for(ch=0;ch<s->nb_channels;ch++) {
>>>           samples_ptr = samples + ch;
>>>           for(i=0;i<nb_frames;i++) {
>>> -            RENAME(ff_mpa_synth_filter)(s->synth_buf[ch],&(s->synth_buf_offset[ch]),
>>> +            RENAME(ff_mpa_synth_filter)(
>>> +#if CONFIG_FLOAT
>>> +                         s,
>>> +#endif
>>
>> Do I really need to tell you that this is hideously ugly?
>
> No, that's why I was expecting some suggestion ;)

If we pointerise it as discussed above, this will go away, right?

-- 
M?ns Rullg?rd
mans at mansr.com