[FFmpeg-devel] Indeo3 replacement, part 2

[Maxim]

Hi, first of all thank you for your really good suggestions IMHO!

> On Tue, Sep 22, 2009 at 12:09:23AM +0200, Maxim wrote:
>   
> [...]
>> /**
>>  *  Average 4/8 pixels at once without rounding using softSIMD
>>  */
>> #define AVG_32(dst, src, ref)   AV_WN32((dst), ((AV_RN32(src) + AV_RN32(ref)) >> 1) & 0x7F7F7F7F)
>> #define AVG_64(dst, src, ref)   AV_WN64((dst), ((AV_RN64(src) + AV_RN64(ref)) >> 1) & 0x7F7F7F7F7F7F7F7F)
>>     
>
> Are all of src, dst, ref unaligned in general? If not, you should be
> using casts instead of AV_RN*

The alignment depends on picture width (sometimes pictures can have odd
width like 183) but it can be changed in "allocate_frame_buffers" (see
my FIXME comments) by aligning each "Y" line. Which alignment would be
preferable: 4, 8 or 16 bytes?
Which is the difference between a cast and AV_RN*?

> [...]
>
>   
>>     if (mode == 1 || mode == 4) {
>>         code        = ctx->alt_quant[vq_index];
>>         prim_indx   = (code >> 4)  + ctx->cb_offset;
>>         second_indx = (code & 0xF) + ctx->cb_offset;
>>
>>         assert(prim_indx <= 23 && second_indx <= 23);
>>
>>         prim_delta   = &delta_tabs   [prim_indx]  [0];
>>         prim_sel     = &selector_tabs[prim_indx]  [0];
>>         second_delta = &delta_tabs   [second_indx][0];
>>         second_sel   = &selector_tabs[second_indx][0];
>>     } else {
>>         vq_index += ctx->cb_offset;
>>         assert(vq_index <= 23);
>>
>>         prim_delta   = &delta_tabs   [vq_index][0];
>>         prim_sel     = &selector_tabs[vq_index][0];
>>         second_delta = prim_delta;
>>         second_sel   = prim_sel;
>>     }
>>     
>
> Is it really impossible to trigger these assert with a specially crafted
> input file?
>   

Sure! I'll change it to appropriate return statements instead...

>   
>>     /* FIXME: if (vq_index >= 8 && (mode == 0 || mode == 3 || mode == 10) [win32] */
>>     
>
> It's unclear to me what that is supposed to mean.
>   

There are two general indeo3 implementations: the one is C-based (Mac
and Xanim) and the other is ASM-based (Win16/32). In the Win version the
code above is triggered using a slightly different condition (given in
the comment). As already been tested the output of my decoder is
bitexact compared against the windows one so it seems that both
conditions are right...

>   
> [...]
>>     buf_ptr += 3; // skip reserved byte and checksum
>>
>>     /* check frame dimensions */
>>     height = bytestream_get_le16(&buf_ptr);
>>     width  = bytestream_get_le16(&buf_ptr);
>>     y_offset = bytestream_get_le32(&buf_ptr);
>>     v_offset = bytestream_get_le32(&buf_ptr);
>>     u_offset = bytestream_get_le32(&buf_ptr);
>>     
>
> Which I suspect it doesn't these alone are already more than the 8 bytes
> padding (I may just have missed the check though).
>   

Not sure I understand what you mean?

>   
>>         /* convert four pixels at once using softSIMD */
>>         for (x = 0; x < plane->width >> 2; x++)
>>             *dst32++ = (*src32++ & 0x7F7F7F7F) << 1;
>>
>>         for (x <<= 2; x < plane->width; x++)
>>             dst[x] = (src[x] & 0x7F) << 1;
>>     
>
> The & 0x7F is useless.
>   

Will be removed...

>   
>>     res = decode_frame_headers(ctx, avctx, buf, buf_size);
>>     if (res < 0)
>>         return -1;
>>     if (res)
>>         return 0; /* skip sync(null) frames */
>>     
>
> Uh, if you return 0 that means "none of the passed data was processed",
> which seems like exactly the wrong thing for a null frame.
> Also if you return >= 0 I think you are supposed to set data_size.
>   

Always return 16 in this case because the null frames are always 16
bytes long, ok?
FFmpeg will detect frame duplication in this case because some bytes
were processed but there is no output (empty frame)...

Regards
Maxim