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

[Vitor Sessak]

On 06/19/2010 03:46 PM, M?ns Rullg?rd wrote:
> 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?

Not for the moment...

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

Yes, mostly the 32-point DCT and the 36-point IMDCT (that IMHO belongs 
to DCTContext and MDCTContext respectivelly). But, yes, those contexts 
can be inside the MPADSPContext struct.

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

Sure, I'll just wait for Michael's feedback on that approach so I'm sure 
not to lose my time for nothing.

-Vitor

PS: this discussion do not block the patch for the asm optimization nor 
the one of moving the dct code to DCTContext...