[FFmpeg-devel] [PATCH] Optimization of original IFF codec

[Måns Rullgård]

Sebastian Vater <cdgs.basty at googlemail.com> writes:

> Arg, was too fast, sorry, please review this one instead.
>
> Forgot to inline the plane decoder and the declare function prototype as
> const unsigned instead of signed...
>
> Sebastian Vater a ?crit :
>> Hi again!
>>
>> Sebastian Vater a ?crit :
>>   
>>> I'll do a patch for this soon, but I want to mention though, that the
>>> code in IFF does fiddle with bit-planar modes, thus the data is expected
>>> to be calculated bit-wise. In that case I find it more readable to use
>>> << and >> instead of * and /.
>>>   
>>>     
>> So here is the patch, converting signed to unsigned where appreciated
>> for optimization.
>>
>> I also tested this patch against breaking anything (at least ASH.LBM
>> still displays correctly)...so please review it!
>>   
> -- 
>
> Best regards,
>                    :-) Basty/CDGS (-:
>
>
> Index: libavcodec/iff.c
> ===================================================================
> --- libavcodec/iff.c	(r??vision 22945)
> +++ libavcodec/iff.c	(copie de travail)
> @@ -1,6 +1,7 @@
>  /*
>   * IFF PBM/ILBM bitmap decoder
>   * Copyright (c) 2010 Peter Ross <pross at xvid.org>
> + * Copyright (c) 2010 Sebastian Vater <cdgs.basty at googlemail.com>
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -40,7 +41,7 @@
>   */
>  int ff_cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
>  {
> -    int count, i;
> +    unsigned int count, i, j;

I prefer to write just unsigned, saving a few keystrokes, but others
may disagree.  Do as you please.

>      if (avctx->bits_per_coded_sample > 8) {
>          av_log(avctx, AV_LOG_ERROR, "bit_per_coded_sample > 8 not supported\n");
> @@ -52,8 +53,8 @@
>          av_log(avctx, AV_LOG_ERROR, "palette data underflow\n");
>          return AVERROR_INVALIDDATA;
>      }
> -    for (i=0; i < count; i++) {
> -        pal[i] = 0xFF000000 | AV_RB24( avctx->extradata + i*3 );
> +    for (i=0, j=0; i < count; i++, j += 3) {
> +        pal[i] = 0xFF000000 | AV_RB24( avctx->extradata + j);
>      }
>      return 0;
>  }

Did you check if this makes any difference in the generated code?  The
original is more readable.

> @@ -71,7 +72,7 @@
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    s->planesize = avctx->width / 8;
> +    s->planesize = avctx->width >> 3;
>      s->planebuf = av_malloc(s->planesize + FF_INPUT_BUFFER_PADDING_SIZE);
>      if (!s->planebuf)
>          return AVERROR(ENOMEM);

This is most definitely a number being divided.

> @@ -95,12 +96,16 @@
>   * @param plane plane number to decode as
>   */
>  #define DECLARE_DECODEPLANE(suffix, type) \
> -static void decodeplane##suffix(void *dst, const uint8_t *const buf, int buf_size, int bps, int plane) \
> +static inline void decodeplane##suffix(void *dst, \
> +                                       const uint8_t *const buf, \
> +                                       const unsigned int buf_size, \
> +                                       const unsigned int bps, \
> +                                       const unsigned int plane) \
>  { \
>      GetBitContext gb; \
> -    int i, b; \
> +    unsigned int i, b; \
>      init_get_bits(&gb, buf, buf_size * 8); \
> -    for(i = 0; i < (buf_size * 8 + bps - 1) / bps; i++) { \
> +    for(i = 0; i < ((buf_size << 3) + bps - 1) / bps; i++) { \
>          for (b = 0; b < bps; b++) { \
>              ((type *)dst)[ i*bps + b ] |= get_bits1(&gb) << plane; \
>          } \

Once again, did you check the generated code?  If it makes no
difference, the original code is better.

And again, and again, and again...

NEVER make changes like this without looking at the output.

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