[Ffmpeg-devel] libswscale 8 bit decoding

[Michael Niedermayer]

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

> Index: swscale.c
> ===================================================================
> --- swscale.c	(revision 20933)
> +++ swscale.c	(working copy)
[...]
> @@ -1626,6 +1627,79 @@
>  	return srcSliceH;
>  }
>  
> +static int pal82rgbWrapper(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;
> +	const int srcBpp= (fmt_depth(srcFormat) + 7) >> 3;
> +	const int dstBpp= (fmt_depth(dstFormat) + 7) >> 3;
> +	const int dstDepth = fmt_depth(dstFormat);
> +
> +	void (*conv)(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)=NULL;
> +	void (*palconv)(const uint8_t *src, uint8_t *dst, long src_size)=NULL;
> +	uint8_t TempPalette[256*4];
> +
> +	/* PAL8 -> BGR */
> +	if((srcFormat == PIX_FMT_PAL8) && isBGR(dstFormat)) {
> +		switch(dstDepth){
> +		case 32: conv= &palette8tobgr32; 
> +			 palconv = rgb32tobgr32;
> +			 break;
> +		case 24: conv= &palette8tobgr24; 
> + 			 palconv = rgb32tobgr24;
> +			 break;
> +		case 16: conv= &palette8tobgr16; 
> + 			 palconv = rgb32tobgr16;
> +			 break;
> +		case 15: conv= &palette8tobgr15;
> + 			 palconv = rgb32tobgr15;
> +			 break;
> +		default: MSG_ERR("swScaler: internal error %s -> %s converter (for PAL8)\n", 
> +				 sws_format_name(srcFormat), sws_format_name(dstFormat)); break;
> +		}
> +	}else if((srcFormat == PIX_FMT_PAL8) && isRGB(dstFormat)){
> +		switch(dstDepth){
> +		case 32: conv= &palette8torgb32; break;
> +		case 24: conv= &palette8torgb24;
> + 			 palconv = rgb32to24;
> +			 break;
> +		case 16: conv= &palette8torgb16;
> + 			 palconv = rgb32to16;
> +			 break;
> +		case 15: conv= &palette8torgb15;
> + 			 palconv = rgb32to15;
> +			 break;
> +		default: MSG_ERR("swScaler: internal error %s -> %s converter (for PAL8)\n", 
> +				 sws_format_name(srcFormat), sws_format_name(dstFormat)); break;
> +		}

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!)


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


[...]

[...]
> 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 ...)

also the downscaling of chroma is wrong (discarding 3/4 of the samples is
not ok)

and the palette should be converted to yuv instead of doing rgb->yuv
per pixel


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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is