[FFmpeg-devel] need help fixing sws on PPC

Vitor Sessak vitor1001
Tue Oct 7 21:24:26 CEST 2008


Michael Niedermayer escreveu:
> 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
>   

ok

>> +
>> +    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?
>   

Removed the check, it was based in rgb2rgbWrapper().

>> +        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, ...
>   

Done (in a certain different way)

>> +
>> +
>> +
>>          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
>   

indeed, changed the logic in my patch and fixed it.

> likely is wrong in more ways than just that.
>   

Is swscale such a big mess that is that easy to break stuff?

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

I will as soon as I get a ssh from Hudo...

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pal2rgb_2.diff
Type: text/x-patch
Size: 4226 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081007/f6bf7920/attachment.bin>



More information about the ffmpeg-devel mailing list