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

[Michael Niedermayer]

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

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 ?


[...]
> +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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130206/0a4c06ae/attachment.asc>