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

[Maxim]

Kostya schrieb:
> On Sun, Jan 17, 2010 at 09:25:36PM +0100, Reimar D?ffinger wrote:
>   
>> On Fri, Jan 15, 2010 at 11:45:00AM +0200, Kostya wrote:
>>     
>>> #define IVI_DEBUG
>>>       
>> Probably should not be enabled in the final version.
>> Also I'd suggest to have it always defined and only
>> change the value between 0 and 1, and wherever possible
>> use
>> if (IVI_DEBUG)
>> thus ensuring the debugging code will not break completely.
>>     
>
> done
>  
>   
>>> /* see ivi_common.c for a definition */
>>>       
>> "a" definition? Also isn't this ivi_common.h, making ivi_common.c
>> the obvious place to look?
>>
>>     
>>> extern const RVMapDesc ff_ivi_rvmap_tabs[9]; /* defined in ivi_common.c */
>>>       
>> Where else should it be "defined"?
>>     
>
> dropped those
>  
>   
>>> typedef struct {
>>>     uint8_t         plane;          ///< plane number this band belongs to
>>>     uint8_t         band_num;       ///< band number
>>>     uint32_t        width;
>>>     uint32_t        height;
>>>     const uint8_t   *data_ptr;      ///< ptr to the first byte of the band data
>>>     uint32_t        data_size;      ///< size of the band data
>>>     int16_t         *buf;           ///< pointer to the output buffer for this band
>>>     int16_t         *ref_buf;       ///< pointer to the reference frame buffer (for motion compensation)
>>>     int16_t         *bufs[3];       ///< array of pointers to the band buffers
>>>     uint32_t        pitch;          ///< pitch associated with the buffers above
>>>     uint8_t         is_empty;       ///< = 1 if this band doesn't contain any data
>>>     uint8_t         mb_size;        ///< macroblock size
>>>     uint8_t         blk_size;       ///< block size
>>>     uint8_t         is_halfpel;     ///< precision of the motion compensation: 0 - fullpel, 1 - halfpel
>>>     int8_t          inherit_mv;     ///< tells if motion vector is inherited from reference macroblock
>>>     int8_t          inherit_qdelta; ///< tells if quantiser delta is inherited from reference macroblock
>>>     int8_t          qdelta_present; ///< tells if Qdelta signal is present in the bitstream (Indeo5 only)
>>>     uint8_t         quant_mat;      ///< dequant matrix index
>>>     uint8_t         glob_quant;     ///< quant base for this band
>>>       
>> Do all those exact bit sizes make sense? They might be a good bit
>> slower on some architectures, also if size is a concern you'd want
>> to reorder this a bit to not waste quite as much on padding.
>>     
>
> changed to int where possible
>  
>   
>>> static inline int ivi_pic_config_cmp(IVIPicConfig *str1, IVIPicConfig *str2)
>>>       
>> No documentation.
>>     
>
> added
>
>   
>>> /** 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.
>> 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.
>>     
>
> I've left it as is for now
>   

Let us incorporate the following simplification if there is no problem
with it:

#define IVI_TOSIGNED(val) (-(((val) >> 1) ^ -((val) & 1));

Regards
maxim