[MPlayer-dev-eng] [PATCH] Fix packed YUV in dshow vo

[Sascha Sommer]

Hi,

On Freitag, 3. Oktober 2008, Laurent wrote:
> On 9/23/08, Laurent <laurent.aml at gmail.com> wrote:
> > Here is a patch to support packed yuv in DirectX when strides are
> > different between input and output buffers.
> >
> > Thanks,
>
> MPlayer crashes on Intel-based GPU (eg 945) when the Packed YUV
> colorspace is used.
>
> To reproduce the crash, start
>   mplayer.exe -vf format=YUY2 <yourvideo>
>
> Would it be possible to apply this patch, or get suggestions?
>

Thanks for the patch. There is indeed a bug.
See my remarks below.

> Index: libvo/vo_directx.c
> ===================================================================
> --- libvo/vo_directx.c	(revision 27514)
> +++ libvo/vo_directx.c	(working copy)
> @@ -1276,7 +1276,35 @@
>  	}
>  	else //packed
>  	{
> -        fast_memcpy( image, mpi->planes[0], image_height * dstride);
> +		const srcStride = mpi->stride[0];
> +		const dstStride = dstride;
> +		int srcPlane = mpi->planes[0];
> +		int dstPlane = image;

srcPlane and dstPlane are pointers and should therefore be declared as 
uint8_t*. Otherwise the code might break on 64bit systems where sizeof(void*) 
== 8


> +		int minStride;
> +		int srcH;
> +
> +		if (srcStride == dstStride)
> +		{
> +			// Same buffer size, same line alignment.
> +			fast_memcpy(dstPlane, srcPlane, image_height * srcStride);
> +		}
> +		else

I would get rid of that special case. I don't think it gives a large 
performance gain. Without it the code should be more readable and easier to 
maintain because one only needs to test one code path.

> +		{
> +			// Line by line copy is required.
> +			if (srcStride > dstStride) {
> +				mp_msg(MSGT_VO, MSGL_DBG3,"<vo_directx><DB3>Using shortest stride from
> back buffer (%d) instead of source stride.\n", dstStride, srcStride);
> +				minStride = dstStride;
> +			} else {
> +				mp_msg(MSGT_VO, MSGL_DBG3,"<vo_directx><DB3>Using shortest stride from
> source (%d) instead of back buffer stride.\n", srcStride, dstStride);
> +				minStride = srcStride;
> +			}

Not sure if MPlayer defines a MIN macro. If not you might introduce one in 
vo_directx.c. You can then simply use

			minStride = MIN(srcStride,dstStride);

instead of the if else block above.

> +			mp_msg(MSGT_VO, MSGL_DBG3 ,"<vo_directx><DB3>Packed src=%x srcstride=%d
> dst=%x dststride=%d w=%d h=%d size=%d\n", srcPlane, srcStride, dstPlane,
> dstStride, image_width, image_height, image_height * minStride);

Please get rid or comment out the mp_msg.

> +			for (srcH=0; srcH<image_height; srcH++) {
> +				fast_memcpy(dstPlane, srcPlane, minStride);
> +				srcPlane += srcStride;
> +				dstPlane += dstStride;
> +			}
> +		}
>  	}
>  	return VO_TRUE;
>  }

Once these issues are fixed I'm going to apply your patch.

Regards

Sascha