[FFmpeg-devel] [PATCH]Fix alpha for iff deep

[Reimar Döffinger]

On Sat, Mar 10, 2012 at 02:27:31PM -0500, Don Moir wrote:
> >On 3/9/12, Carl-Eugen Hoyos <cehoyos at ag.or.at> wrote:
> >>Hi!
> >>
> >>Attached patch makes no difference for the RGBA DEEP iff sample from
> >>ticket 1045, but matches the specification (see ticket) better.
> >>
> >>Please comment, Carl Eugen
> >
> >Very strange indentation.
> 
> Seems to be a strange lack of any optimization.
> 
> if you look at this code in the patch:
> 
> +        for(x = 0; x < avctx->width; x++)
> +            row[4 * x + 3] = row[4 * x + 3] & 0xF0 | (row[4 * x + 3] >> 4);
> 
> would be better if:
> 
> +      row += 3;
> +      for(x = 0; x < avctx->width; x++, row+=4)
> +          *row = *row & 0xF0 | (*row >> 4);
> 
> But this is more of a minor point then the rest of the code being
> less than optimal in iff.c. It should be obvious but I could
> elaborate. I bring this up because sometimes optimization issues are
> sometime nic-picked to death and sometimes not.

There are several reasons for that.
In the case that you mention, it is because the code you propose
can easily be slower than the current code, particularly
on register-starved CISC configurations like 32 bit x86 with PIC.
The compiler might be able to encode the 4*x+3 into a single
instruction in the first case (thus it is "free"), whereas your
proposal might consume an extra register.
The there is also the question whether someone really cares
about performance. Fringe formats generally aren't optimized well,
and easy to read code also is rather more relevant than performance
most of the time.