[MPlayer-dev-eng] [PATCH 5/9] libavcodec: add AVR32 specific optimizations

[Diego Biurrun]

On Mon, Feb 16, 2009 at 05:16:54PM +0100, Hans-Christian Egtvedt wrote:
> Implemented by Ronny Pedersen.
> 
> --- a/cfg-common.h
> +++ b/cfg-common.h
> @@ -6,6 +6,10 @@
>  
> +#ifdef ARCH_AVR32
> +extern int avr32_use_pico;
> +#endif

useless #ifdef around extern declaration

> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -462,6 +462,10 @@ OBJS-$(HAVE_ARMV6)                     += arm/simple_idct_armv6.o       \
>  
> +OBJS-$(ARCH_AVR32)                     += avr32/dsputil_avr32.o         \
> +                                          avr32/idct.o avr32/fdct.o     \
> +                                          avr32/mc.o avr32/h264idct.o   \
> +

one per line, in alphabetical order

> --- /dev/null
> +++ b/libavcodec/avr32/dsputil_avr32.c
> @@ -0,0 +1,2492 @@
> +
> +union doubleword {
> +  int64_t doubleword;
> +  struct {
> +    int32_t top;
> +    int32_t bottom;
> +  } words;
> +};

FFmpeg uses 4 spaces indentation.

> +static void put_h264_chroma_mc2_pico(uint8_t *dst/*align 8*/, uint8_t *src/*align 1*/, int stride, int h, int x, int y){
> +
> +  for(i=0; i<h; i++)
> +    {

Please use K&R style, i.e. place the opening { after function declarations
on the next line and on  the same line after for/if/while.  And use some
more spaces, i.e. similar to

static void put_h264_chroma_mc2_pico(uint8_t *dst/*align 8*/,
                                     uint8_t *src/*align 1*/,
                                     int stride, int h, int x, int y)
{

    for (i = 0; i < h; i++) {

> +  alpha = PACKW_SH(alpha, alpha);
> +  alpha = PACKSH_UB(alpha, alpha);
> +  beta = PACKW_SH(beta, beta);
> +  beta = PACKSH_UB(beta, beta);

This could be nicely aligned, the same likely applies in other places.

> --- /dev/null
> +++ b/libavcodec/avr32/pico.h
> @@ -0,0 +1,255 @@
> +#ifndef __PICO_H__
> +#define __PICO_H__

This is illegal, identifiers starting with __ or _ and a capital are
reserved for the system.  Please follow the naming scheme for FFmpeg
multiple inclusion guards.

> +/* Config Register */
> +#define PICO_COEFF_FRAC_BITS_OFFSET  0
> +#define PICO_COEFF_FRAC_BITS_SIZE  4
> +#define PICO_OFFSET_FRAC_BITS_OFFSET  4
> +#define PICO_OFFSET_FRAC_BITS_SIZE  4
> +#define PICO_INPUT_MODE_OFFSET  8
> +#define PICO_INPUT_MODE_SIZE  2
> +#define PICO_OUTPUT_MODE_OFFSET 10
> +#define PICO_OUTPUT_MODE_SIZE 1

These could be aligned, same in other places.

> +#define __str(x...) #x
> +#define __xstr(x...) __str(x)

also illegal

> +#endif

missing #endif comment

> --- a/libavcodec/bitstream.h
> +++ b/libavcodec/bitstream.h
> @@ -181,6 +181,7 @@ typedef struct RL_VLC_ELEM {
>      uint8_t run;
>  } RL_VLC_ELEM;
>  
> +
>  #ifndef ALT_BITSTREAM_WRITER
>  static inline void put_bits(PutBitContext *s, int n, unsigned int value)
>  {

cosmetics

> @@ -799,6 +800,44 @@ void free_vlc(VLC *vlc);
>   * if the vlc code is invalid and max_depth>1 than the number of bits removed
>   * is undefined
>   */
> +
> +#if defined(ARCH_AVR32)

#if ARCH_AVR32

more below

> @@ -807,7 +846,7 @@ void free_vlc(VLC *vlc);
>      code = table[index][0];\
>      n    = table[index][1];\
>  \
> -    if(max_depth > 1 && n < 0){\
> +    if(max_depth > 1 && n < 0 ){\

cosmetics and an uglification to boot

Note that such things will get your patches immediately rejected on
ffmpeg-devel.

> @@ -829,7 +868,38 @@ void free_vlc(VLC *vlc);
>      }\
>      SKIP_BITS(name, gb, n)\
>  }
> +#endif

Please add an #endif comment.

> @@ -853,7 +923,7 @@ void free_vlc(VLC *vlc);
>      run= table[index].run;\
>      SKIP_BITS(name, gb, n)\
>  }
> -
> +#endif

again, add comment

> --- a/libavcodec/h264.c
> +++ b/libavcodec/h264.c
> @@ -2010,7 +2010,12 @@ static void free_tables(H264Context *h){
>  
>  static void init_dequant8_coeff_table(H264Context *h){
>      int i,q,x;
> +#ifdef ARCH_AVR32
> +    const int transpose = 0;
> +#else
>      const int transpose = (h->s.dsp.h264_idct8_add != ff_h264_idct8_add_c); //FIXME ugly
> +#endif

Code with such comments will never pass review on ffmpeg-devel.

> @@ -2033,7 +2038,13 @@ static void init_dequant8_coeff_table(H264Context *h){
>  
>  static void init_dequant4_coeff_table(H264Context *h){
>      int i,j,q,x;
> +    // Yes this is ugly as hell....
> +#ifdef ARCH_AVR32
> +    const int transpose = 0;
> +#else
>      const int transpose = (h->s.dsp.h264_idct_add != ff_h264_idct_add_c); //FIXME ugly
> +#endif

ditto

Diego