[FFmpeg-devel] [PATCH 1/3] Indeo 5 decoder: common functions

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> On Fri, Jan 15, 2010 at 11:45:00AM +0200, Kostya wrote:
>> /** convert unsigned values into signed ones (the sign is in the LSB) */
>> /* TODO: find a way to calculate this without the conditional using bit magic */
>> #define IVI_TOSIGNED(val) (((val) & 1) ? ((val) + 1) >> 1 : -(((val) + 1) >> 1))
>
> I'm sure this kind of code is already in other decoders.
> However the + 1 in the part after : sure is useless.
> Also I strongly suggest to make this a static inline function.

Why is inline better in this case?

> And bit magic to do it should be
> ((sign_extend(val & 1, 1) ^ val) >> 1) + 1
> I think the +1 is necessary, but I can't say for sure since my request
> to document it when it was added was just ignored.

Do you meant the &1 for sign_extend().  I remember being of the
opinion that it shouldn't be required since any sane implementation on
any sane hardware will do it implicitly.  Someone disagreed, and etc...

>> /** divide the motion vector mv by 4 */
>> #define IVI_MV_DIV4(mv) (((mv) + 1 + ((mv) > 0))>>2)
>> 
>> /** divide the motion vector mv by 2 */
>> #define IVI_MV_DIV2(mv) (((mv) + ((mv) > 0))>>1)
>
> Since the mv == 0 case does not matter, you should be able to avoid
> the comparison by
> static inline int32_t ivi_mv_div4(int32_t mv)
> {
>     return (mv + 2 - ((uint32_t)mv >> 31)) >> 2;
> }

Might make sense to create some common macros/functions for rounding
division of signed numbers.  The fastest way probably differs between
architectures.

> And yes, use static inline functions if you can.
>
>>  *  @param pOut [out] where to place the generated VLC table
>
> pOut? Do we really want that kind of naming?

Most emphatically not.

>> int  ff_ivi_init_tiles(IVIPlaneDesc *planes, const int tile_width, const int tile_height);
>
> Drop the useless const.

It could be considered useful insofar it protects against typos in the
function accidentally changing it.  Probably useless in the prototype
though.

>> /**
>>  *  Reverses "nbits" bits of the value "val" and returns the result
>>  *  right-justified.
>
> "in the least significant bits" IMO is clearer.
>
>> static uint16_t inv_bits(const uint16_t val, const int nbits)
>> {
>>     uint16_t res;
>> 
>>     if (nbits <= 8) {
>>         res = av_reverse[val] >> (8-nbits);
>>     } else
>>         res = ((av_reverse[val & 0xFF] << 8) + (av_reverse[val >> 8])) >> (16-nbits);
>> 
>>     return res;
>> }
>
> Useless const.
> And a 16-bit reverse function shared with pcm.c would be welcome in
> mathematics.h I think.

The function above can be implemented in 2 cycles on ARM and similar
numbers on any machine with bit reversal instructions.  Many DSPs have
these too.

>>         prefix        = ((1 << i) - 1) << (cb->xbits[i] + last_row);
>
> Hm. Maybe some hint at what this is would be good.
> Also maybe write as (assuming I am correct)
> (1 << i) - (1 << (cb->xbits[i] + last_row))

Looks equivalent, though the first variant is possibly more obvious in
terms of the bit pattern it produces.  Both can be done in 3
instructions on ARM, and gcc manages to do it in both cases.

>>         /* luma   band buffers will be aligned on 16x16 (max macroblock size) */
>>         /* chroma band buffers will be aligned on   8x8 (max macroblock size) */
>>         align_fac       = p ? 7 : 15;
>>         width_aligned   = (b_width  + align_fac) & ~align_fac;
>>         height_aligned  = (b_height + align_fac) & ~align_fac;
>
> Maybe
> align_fac = p ? 8 : 16; (or 8 << !!p possibly?)
> FFALIGN(b_width, align_fac)

I say use FFALIGN one way or other.

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