[FFmpeg-devel] Indeo3 replacement, take 3

[Vitor Sessak]

Vitor Sessak wrote:
> Maxim wrote:
>> Vitor Sessak schrieb:
>>> Maxim wrote:
>>> [...]
>>> It is probably missing something like
>>>
>>>>     if (ctx->frame.data[0])
>>>>         avctx->release_buffer(avctx, &ctx->frame);
>>> in free_frame_buffers()
>>
>> Thanks! Just added that release_buffer and the error message disappered!
>>
>>>> - compiling with AltiVec enabled on my PPC G5 produces wrong checksums
>>>> on delta frames. A brief check has shown that "copy_cell" function 
>>>> using
>>>> DSP util's dsp.put_no_rnd_pixels_tab[0][0]" and
>>>> dsp.put_no_rnd_pixels_tab[1][0] causes that problem. It just works fine
>>>> with AltiVec disabled. Could someone skilled in the art help me with 
>>>> it?
>>> I have a guess:
>>>
>>> Your code does:
>>>
>>>>     luma_pitch   = FFALIGN(luma_width,   8);
>>>>     chroma_pitch = FFALIGN(chroma_width, 8);
>>> [...]
>>>
>>>>         ctx->planes[p].pitch  = (!p ? luma_pitch  : chroma_pitch);
>>>>         ctx->planes[p].width  = (!p ? luma_width  : chroma_width);
>>>>         ctx->planes[p].height = (!p ? luma_height : chroma_height);
>>> [...]
>>>
>>>>     /* copy using 16xH blocks */
>>>>     for (i = cell->width >> 2; i > 0; src += 16, dst += 16, i--)
>>>>         ctx->dsp.put_no_rnd_pixels_tab[0][0](dst, src, plane->pitch, 
>>>> h);
>>> And ppc/dsputils_altivec.c says
>>>
>>>> /* next one assumes that ((line_size % 16) == 0) */
>>>> void put_pixels16_altivec(uint8_t *block, const uint8_t *pixels, int
>>>> line_size, int h)
>>>> {
>>> [...]
>>>
>>>>     c->put_pixels_tab[0][0] = put_pixels16_altivec;
>>> So this should be the culprit. Just align to 16 instead of 8.
>>>
>>
>> Just tried it but it works still wrong on delta frames! I'll try to
>> examine that later...
> 
> I've found another problem:
> 
> ppc/dsputils_altivec.c:
> 
>> /* next one assumes that ((line_size % 8) == 0) */
>> void avg_pixels8_altivec(uint8_t * block, const uint8_t * pixels, int 
>> line_size, int h)
>> {
>> POWERPC_PERF_DECLARE(altivec_avg_pixels8_num, 1);
>>     register vector unsigned char pixelsv1, pixelsv2, pixelsv, blockv;
>>     int i;
>>
>> POWERPC_PERF_START_COUNT(altivec_avg_pixels8_num, 1);
>>
>>    for (i = 0; i < h; i++) {
>>        /* block is 8 bytes-aligned, so we're either in the
>                      ^^^^^^^^^^^^^^^
>>           left block (16 bytes-aligned) or in the right block (not) */
> 
> Your patch:
> 
>> typedef struct Plane {
>>     uint8_t         *buffers[2];
>>     uint8_t         *pixels[2]; ///< pointer to the actual pixel data 
>> of the buffers above
>>     uint32_t        width;
>>     uint32_t        height;
>>     uint32_t        pitch;
>> } Plane;
> 
> [...]
> 
>> /**
>>  *  Copy pixels of the cell(x + mv_x, y + mv_y) from the previous 
>> frame into
>>  *  the cell(x, y) in the current frame.
>>  */
>> static void copy_cell(Indeo3DecodeContext *ctx, Plane *plane, Cell *cell)
>> {
>>     int     h, i, mv_x, mv_y, offset;
>>     uint8_t *src, *dst;
>>
>>     /* setup output and reference pointers */
>>     offset  = (cell->ypos << 2) * plane->pitch + (cell->xpos << 2);
>>     dst     = plane->pixels[ctx->buf_sel] + offset;
>>     mv_y    = cell->mv_ptr[0];
>>     mv_x    = cell->mv_ptr[1];
>>     offset += mv_y * plane->pitch + mv_x;
>>     src     = plane->pixels[ctx->buf_sel ^ 1] + offset;
>>
>>     h = cell->height << 2;
>>
>>     /* copy using 16xH blocks */
>>     for (i = cell->width >> 2; i > 0; src += 16, dst += 16, i--)
>>         ctx->dsp.put_no_rnd_pixels_tab[0][0](dst, src, plane->pitch, h);
>>     /* copy using 8xH blocks */
>>     if (cell->width & 2) {
>>         ctx->dsp.put_no_rnd_pixels_tab[1][0](dst, src, plane->pitch, h);
>>         src += 8;
>>         dst += 8;
>>     }
> 
> Note that since plane->pixels[] is not aligned, dst is not aligned 
> neither. So I'd suggest something on the lines of
> 
>> typedef struct Plane {
>>     uint8_t         *buffers[2];
>>     DECLARE_ALIGNED_16(uint8_t, *pixels[2]); ///< pointer to the 
>> actual pixel data of the buffers above
>>     uint32_t        width;
>>     uint32_t        height;
>>     uint32_t        pitch;
>> } Plane;

Err, scrap that, I see that pixels[] are pointers to av_malloc'ed 
buffers, hence aligned. So no ideas here. Does anyone know the actual 
alignment requirements of dsp.put_no_rnd_pixels_tab? It is documented 
nowhere...

-Vitor