[FFmpeg-devel] [PATCH] Indeo5 decoder

[Maxim]

Reimar D?ffinger schrieb:
> On Thu, Apr 02, 2009 at 11:40:49AM +0200, Maxim wrote:
>   
>>> useless & [0]
>>> indeo5patch.txt:676:+                band->dequant_intra = &deq4x4_intra[0][0];
>>> indeo5patch.txt:677:+                band->dequant_inter = &deq4x4_inter[0][0];
>>>   
>>>       
>> changing it to
>>
>> band->dequant_intra = deq4x4_intra;
>>
>> causes the following compiler's message:
>> warning: assignment from incompatible pointer type
>>     
>
> One & cancels with one [0], so that would be
> band->dequant_intra = deq4x4_intra[0];
>   

thanks, fixed...

>>> indeo5patch.txt:1035:+    av_freep(&planes[0].buf1);
>>> indeo5patch.txt:1036:+    av_freep(&planes[0].buf2);
>>>   
>>>       
>> Hmm, shouldn't av_freep receive a POINTER to a POINTER as it's declared
>> in its prototype? Will then the removing of the "&" lead to a wrong
>> program behavior?
>>     
>
> I think those a false positives, if you still haven't noticed that was
> an automated "review" by tools/patcheck
>   

Aha, thanks! I didn't noticed it because I'm not familiar with this
tool/script...

> You could probably use &planes->buf1 here though, but that's probably
> worse (i.e. uglier)
>   

I would leave it as is because there are several other statements like
&planes[x]->buf1
so the line
&planes[0]->buf1
looks more readable IMHO...
It would be possible to use constants instead of those numbers, i.e.

av_freep(&planes[LUMA]->buf1);

would it be better?

>>> missing const?
>>> indeo5patch.txt:1248:+    int32_t *src, *dst, tmp[64];
>>> indeo5patch.txt:1295:+    int32_t *src, *dst, tmp[16];
>>>       
>> Yes, this is abit complicated! The src pointer should be declared as a
>> pointer to a const data in fact. But due to the given design of the
>> transform it cannot be made so because both IVI_IREFLECT and
>> IVI_SLANT_BFLY make changes to their operands. I need to redesign the
>> whole transform in order to make the src const. Will it worth it?
>>     
>
> This might just be a false positive by the script.
> I think Michael just wanted to make you look over the obvious things a
> simple tool can detect before "wasting" a human's time with a review
> (there are just too few people doing patch reviews).
>   

Ok, I'll leave the code as is then...

Regards
Maxim