[Ffmpeg-devel] libswscale 8 bit decoding

[Steven Johnson]

Michael Niedermayer wrote:
> On Wed, Nov 15, 2006 at 01:08:22PM +1100, Steven Johnson wrote:
>   
>> Hello,
>>
>> Please find attached a patch to libswscale to support displaying pal8 
>> formats.
>>
>> Notes:
>>
>> I am not sure I have done everything I need to do.
>>
>> I have not worked out how to exercise the pal8 to rgb* conversion code, yet.
>>
>> I have tested the pal8toyv12 code and it seems to work (I can play FLC 
>> files with libswscale and couldn't before).
>>
>> I do notice there is a visible decrease in quality playing 8bpp FLC's 
>> through this code than through the non libswscale code in ffmpeg.  I 
>> attribute that to the yv12 conversion for display. 
>>
>> I do not claim this is perfect or ready for integration into ffmpeg,  I 
>> am posting it for review.   Thanks in advance for any comments or advice.
>>
>> Steven J
>>     
[snip]
>
> the palette8tobgr (or palette8torgb) are redundant and should be removed
> and the pal should be R<->B, actually your code halfway does that but
> its totally broken (dont submit untested code!)
>   
Sorry, it wasn't a submission.  I was trying to see if I was on the 
right track and it is difficult to do that without showing the code I am 
working on.  I apologise for not making that clear in my original post. 

How did you exercise this code?  The reason I haven't tested it is I 
haven't come up with a way to get the code path to execute.

I will remove any redundancy that exists.  I don't like redundancy either.
>
>   
>> +	}else{
>> +		MSG_ERR("swScaler: internal error %s -> %s converter (for PAL8)\n", 
>> +			 sws_format_name(srcFormat), sws_format_name(dstFormat));
>> +	}
>> +
>> +	if (palconv != NULL)
>> +		memcpy(TempPalette,src[1],256*3);
>> +	else
>> +		palconv(src[1],TempPalette,256*3);
>>     
>
> segfault
>   
Ok.
>
> [...]
>
> [...]
>   
>> Index: rgb2rgb_template.c
>> ===================================================================
>> --- rgb2rgb_template.c	(revision 20933)
>> +++ rgb2rgb_template.c	(working copy)
>> @@ -2409,6 +2409,75 @@
>>  	}
>>  }
>>  
>> +/**
>> + *
>> + * height should be a multiple of 2 and width should be a multiple of 2 (if this is a
>> + * problem for anyone then tell me, and ill fix it)
>> + * chrominance data is only taken from every secound line others are ignored in the C version FIXME write HQ version
>> + */
>> +static inline void RENAME(pal8toyv12)(const uint8_t *src, const uint8_t *pal, uint8_t *ydst, uint8_t *udst, uint8_t *vdst,
>> +	long width, long height,
>> +	long lumStride, long chromStride, long srcStride)
>> +{
>> +	long y;
>> +	const long chromWidth= width>>1;
>> +#ifdef HAVE_MMX
>> +/* NO MMX Optimised version (YET) */
>> +	y=0;
>> +#else
>> +	y=0;
>> +#endif
>> +	for(; y<height; y+=2)
>> +	{
>> +		long i;
>> +		for(i=0; i<chromWidth; i++)
>> +		{
>> +			unsigned int b= pal[(src[2*i]*4)+0];
>> +			unsigned int g= pal[(src[2*i]*4)+1];
>> +			unsigned int r= pal[(src[2*i]*4)+2];
>> +
>> +			unsigned int Y  =  ((RY*r + GY*g + BY*b)>>RGB2YUV_SHIFT) + 16;
>> +			unsigned int V  =  ((RV*r + GV*g + BV*b)>>RGB2YUV_SHIFT) + 128;
>> +			unsigned int U  =  ((RU*r + GU*g + BU*b)>>RGB2YUV_SHIFT) + 128;
>>     
>
> hardcoded conversation constants (yes i know there is already some code
> in sws which does it, it just would be nice if we could fix that instead of
> introducing more ...)
>   
OK.
> also the downscaling of chroma is wrong (discarding 3/4 of the samples is
> not ok)
>   
The code is exactly the same as the C code in rgb24toyv12, with the 
exception of palette expansion.  I noticed the image quality was 
significantly reduced using libswscale over imgresample, and mentioned 
that in my original post, that is the probably cause.  Any pointers to a 
reference for converting RGB to YV12 I can look at?

And following the thread "moving non-SIMD parts of libswscale to LGPL" I 
am confused about libswscale, imgresample and the future.  As it stands, 
imgresample does more than libswscale does (it handles palletised 
formats), and (at least for me) produces a nicer result, but it seems is 
probably slower in certain cases where the functionality overlaps.  But 
is imgresample deprecated in favour of libswscale?  Can I submit patches 
improving imgresample and ignore libswscale? Or is the focus of future 
development going to be on libswscale with an eventual aim to using it 
by default (if not actually completely obsoleting imgresample), and so 
patches to imgresample to add functionality will be unlikely to be 
accepted because that diverges its functionality further from that 
provided by libswscale?

My big concern is that there will be 2 ways to scale/convert images, 
each will have its pro's and cons, but each will not have 100% the same 
functionality so one can not assume they are interchangeable (as is 
currently the case for palletised formats).  I don't know what to do and 
what I want to do is whatever is the "right" thing to advance towards 
the future goals of the project, whatever they are.

As a suggestion and not a criticism, wouldn't a better approach be to 
use both the imgresample AND libswscale code, and when sws_getContext is 
called, any case that libswscale handles better than imgresample causes 
a subsequent call to sws_scale to call sws_scale in libswscale, 
otherwise sws_scale in imgresample is called, so you have the best of 
both worlds. Full functionality from imgresample, and for the cases 
where libswscale provides the functionality and is faster, the optimised 
version, with little or no extra overhead?  That way a command line 
option could force the use of one or the other at run time (rather than 
at config time) with default behaviour being to auto select.  The config 
time option then just allows the libswscale stuff to be dynamically 
selected at all or not (and would only work if "enable-gpl" was 
specified on configure as it is "mostly" GPL code anyway.

> and the palette should be converted to yuv instead of doing rgb->yuv
> per pixel
>   
Sure. OK. That would be a lot faster.

Steven J