[FFmpeg-devel] [PATCH 2/3] mips: optimization for float aac decoder (sbr module)

[Babic, Nedeljko]

>On Wed, Feb 06, 2013 at 03:10:47PM +0000, Babic, Nedeljko wrote:
>> Hi,
>> 
>> Is this patch ok, or something more should be done with it?
>
>Iam a bit concerned about maintainability of the way the
>optimizations are integrated into the codec.
>
>normally optimization are done by having a struct of function pointers
>each function pointer has a clear specification on what it does. And
>that what it does is a small and relatively atomic operation, like
>calculating the dot product of the specified vector length of 2
>given float arrays.
>
>what this and the other aac patch do in part at least though is they
>simply replace whole high level functions taking a pointer to the
>aac context.

I am not quite sure what do you suggest here.

Some optimizations were done on higher level indeed, but it was done in 
order to better optimize code. For example in function sbr_qmf_synthesis
10 calls of function vector_fmul_add are merged into one loop and loop 
is unrolled 4 times which enabled more efficient access to memory and 
minimized function call overhead. I can see the problem with 
maintainability of such code, but the question is where to draw a line 
between maximizing optimization and maintainability of code.

Of course we can rewrite this and similar optimizations to be done on 
lover level (for example by optimizing only vector_fmul_add in above case) 
if this is what is requested but then we will slightly loose on performance.

>Also they mix use of a struct of pointers with:
>
>> --- a/libavcodec/aacsbr.c
>> +++ b/libavcodec/aacsbr.c
>> @@ -43,6 +43,10 @@
>>  #define ENVELOPE_ADJUSTMENT_OFFSET 2
>>  #define NOISE_FLOOR_OFFSET 6.0f
>>
>> +#if ARCH_MIPS
>> +#include "mips/aacsbr_mips.h"
>> +#endif /* ARCH_MIPS */
>
>it at the least is confusing to mix these 2 aprouches
>

We are using both of these approaches from the first codec that we 
optimized (AMR NB and WB decoders).  In one of the first mails you 
suggested that we should use function pointers or #ifndef's. 
I guess that we can use function pointers everywhere, but we will need 
to change API of some functions in order to achieve this (because they 
do not have pointers to structures that we will need in that case or 
because they are defined as static, etc.).

>a few more comments below
>
>> 
>> -Nedeljko
>> ________________________________________
>> From: ffmpeg-devel-bounces at ffmpeg.org [ffmpeg-devel-bounces at ffmpeg.org] on behalf of Bojan Zivkovic [bojan at mips.com]
>> Sent: Monday, January 28, 2013 17:47
>> To: ffmpeg-devel at ffmpeg.org
>> Cc: Vulin, Mirjana (c); Lukac, Zeljko
>> Subject: [FFmpeg-devel] [PATCH 2/3] mips: optimization for float aac decoder    (sbr module)
>> 
>> From: Mirjana Vulin <mvulin at mips.com>
>> 
>> Signed-off-by: Mirjana Vulin <mvulin at mips.com>
>> ---
>>  libavcodec/aac.h                |    2 -
>>  libavcodec/aacsbr.c             |   30 ++-
>>  libavcodec/aacsbr.h             |    2 +
>>  libavcodec/mips/Makefile        |    4 +-
>>  libavcodec/mips/aacsbr_mips.c   |  618 +++++++++++++++++++++++++
>>  libavcodec/mips/aacsbr_mips.h   |  493 ++++++++++++++++++++
>>  libavcodec/mips/sbrdsp_mips.c   |  940 +++++++++++++++++++++++++++++++++++++++
>>  libavcodec/sbr.h                |   28 ++-
>>  libavcodec/sbrdsp.c             |    2 +
>>  libavcodec/sbrdsp.h             |    1 +
>>  libavutil/mips/float_dsp_mips.c |   40 ++
>>  11 files changed, 2151 insertions(+), 9 deletions(-)
>>  create mode 100644 libavcodec/mips/aacsbr_mips.c
>>  create mode 100644 libavcodec/mips/aacsbr_mips.h
>>  create mode 100644 libavcodec/mips/sbrdsp_mips.c
>> 
>> diff --git a/libavcodec/aac.h b/libavcodec/aac.h
>> index d17366c..008b18f 100644
>> --- a/libavcodec/aac.h
>> +++ b/libavcodec/aac.h
>> @@ -257,8 +257,6 @@ typedef struct ChannelElement {
>>      SpectralBandReplication sbr;
>>  } ChannelElement;
>> 
>> -typedef struct AACContext AACContext;
>> -
>
>why is this removed ?
>

It is moved to file libavcodec/sbr.h
There is a function in structure AACSBRContext that has pointer to 
AACContext as its first argument.

>
>[...]
>> +static void ff_aacsbr_init(AACSBRContext *c);
>> +
>>  av_cold void ff_aac_sbr_init(void)
>
>these 2 function names differ only by a _ this is certainly not a good
>idea, noone will know which is which

This function will be renamed.

-Nedeljko