[FFmpeg-devel] need help fixing sws on PPC

[Michael Niedermayer]

On Mon, Oct 06, 2008 at 09:42:56PM +0200, Vitor Sessak wrote:
> Michael Niedermayer escreveu:
>> On Wed, Sep 10, 2008 at 01:42:44PM +0200, Vitor Sessak wrote:
>>   
>>> Michael Niedermayer escreveu:
>>>     
>>>> Hi
>>>>
>>>> Can someone who has a PPC and a x86 play a little with swscale to find
>>>> out which convertions do not match?
>>>> This is rather important as our regression tests as well as fate depend
>>>> on it.
>>>> Note -sws_flags +bitexact should be used also -sws_flags +accurate_rnd
>>>> may be needed.
>>>> Also note, some scalers still use floats to find coeffs (like spline)
>>>> iam not interrested in them, that is currently area, nearest neighbor
>>>> bilinear and bicubic are the onlx ones iam interrested in.
>>>>
>>>> And last, if something does not match, pleas explain how it does differ
>>>> +-1 rounding or red/blue exchanged or ...
>>>>         
>>> Besides rounding, there is still something wierd about swscale. For 
>>> example, I was looking at he idcin test 
>>> http://fate.multimedia.cx/index.php?test_spec=107 .
>>>     
>> [...]
>>   
>>> Unless I am missing something, there should be no calculation (and even 
>>> less rounding) doing PAL8 -> RGB conversion, but it still gives different 
>>> output with and without swscaler.
>>>     
>>
>>   
>
> [...]
>
>> if you only care about the unscaled pal->rgb24/32 case, you can add a 
>> special
>> converter for that case, which should be trivial.
>>   
>
> Here is my try at it. I really think it is important to be able to test 
> palette-based decoders without doing a full regression test of all asm 
> versions of rgb2yuv/yuv2rgb...
>
> -Vitor

> Index: libswscale/swscale.c
> ===================================================================
> --- libswscale/swscale.c	(revis?o 27716)
> +++ libswscale/swscale.c	(c?pia de trabalho)
> @@ -1711,6 +1711,40 @@
>      return srcSliceH;
>  }
>  
> +static int pal2rgbWrapper(SwsContext *c, uint8_t* src[], int srcStride[], int srcSliceY,
> +                          int srcSliceH, uint8_t* dst[], int dstStride[]){
> +    const int srcFormat= c->srcFormat;
> +    const int dstFormat= c->dstFormat;
> +    void (*conv)(const uint8_t *src, uint8_t *dst, long num_pixels,
> +		 const uint8_t *palette)=NULL;
> +

> +    if ((!(isRGB(dstFormat) || isBGR(dstFormat)) || srcFormat !=PIX_FMT_PAL8))
> +        av_log(c, AV_LOG_ERROR, "internal error %s -> %s converter\n",
> +               sws_format_name(srcFormat), sws_format_name(dstFormat));

this is redundant with the default below


> +
> +    switch(dstFormat){
> +    case PIX_FMT_RGB32: conv = palette8torgb32; break;
> +    case PIX_FMT_BGR32: conv = palette8tobgr32; break;
> +    case PIX_FMT_RGB24: conv = palette8torgb24; break;
> +    case PIX_FMT_BGR24: conv = palette8tobgr24; break;
> +    default: av_log(c, AV_LOG_ERROR, "internal error %s -> %s converter\n",
> +                    sws_format_name(srcFormat), sws_format_name(dstFormat)); break;
> +    }
> +

> +    if (conv) {

are we writing code with the assumtion of internal errors?


> +        int i;
> +        uint8_t *dstPtr= dst[0] + dstStride[0]*srcSliceY;
> +        uint8_t *srcPtr= src[0];
> +
> +        for (i=0; i<srcSliceH; i++) {
> +            conv(srcPtr, dstPtr, c->srcW, src[1]);
> +            srcPtr+= srcStride[0];
> +            dstPtr+= dstStride[0];
> +        }
> +    }
> +
> +    return srcSliceH;
> +}
>  /* {RGB,BGR}{15,16,24,32,32_1} -> {RGB,BGR}{15,16,24,32} */
>  static int rgb2rgbWrapper(SwsContext *c, uint8_t* src[], int srcStride[], int srcSliceY,
>                            int srcSliceH, uint8_t* dst[], int dstStride[]){

> @@ -2305,6 +2339,15 @@
>             && (!needsDither || (c->flags&(SWS_FAST_BILINEAR|SWS_POINT))))
>               c->swScale= rgb2rgbWrapper;
>  
> +        if ((srcFormat == PIX_FMT_PAL8 && (
> +                 dstFormat == PIX_FMT_RGB32 ||
> +                 dstFormat == PIX_FMT_RGB24 ||
> +                 dstFormat == PIX_FMT_BGR32 ||
> +                 dstFormat == PIX_FMT_BGR24)))
> +             c->swScale= pal2rgbWrapper;

this could also be used for the other byte based formats
PIX_FMT_BGR4_BYTE, PIX_FMT_BGR8, ...



> +
> +
> +
>          if (srcFormat == PIX_FMT_YUV422P)
>          {
>              if (dstFormat == PIX_FMT_YUYV422)

> @@ -2659,12 +2702,18 @@
>      int i;
>      uint8_t* src2[4]= {src[0], src[1], src[2]};
>      uint32_t pal[256];
> -    int use_pal=   c->srcFormat == PIX_FMT_PAL8
> -                || c->srcFormat == PIX_FMT_BGR4_BYTE
> +    int use_pal=   c->srcFormat == PIX_FMT_BGR4_BYTE
>                  || c->srcFormat == PIX_FMT_RGB4_BYTE
>                  || c->srcFormat == PIX_FMT_BGR8
>                  || c->srcFormat == PIX_FMT_RGB8;
>  
> +    use_pal = use_pal
> +                && c->srcFormat == PIX_FMT_PAL8
> +                && c->dstFormat != PIX_FMT_RGB32
> +                && c->dstFormat != PIX_FMT_RGB24
> +                && c->dstFormat != PIX_FMT_BGR32
> +                && c->dstFormat != PIX_FMT_BGR24;
> +
>      if (c->sliceDir == 0 && srcSliceY != 0 && srcSliceY + srcSliceH != c->srcH) {
>          av_log(c, AV_LOG_ERROR, "Slices start in the middle!\n");
>          return 0;

This code makes no sense at all
it assumes that no scaling happens which surely is not correct but it
likely is wrong in more ways than just that.

also may i suggest that you use swscale-example to test that your changes
dont break things!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is not what we do, but why we do it that matters.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081007/3b496bc5/attachment.pgp>