[MPlayer-dev-eng] [PATCH] view stereoscopic videos

[Gordon Schmidt]

Hi,

thanks for the quick reply.

On Sat, Jan 23, 2010 at 08:21:15PM + 0100, Reimar Döffinger wrote:
> On Sat, Jan 23, 2010 at 03:34:27PM +0100, Gordon Schmidt wrote:
>> this is my first post in this list. I've created a filter to convert for
>> stereoscopic videos into other stereoscopic formats. Supported stereo
>> input contain images for the left eye and the right eye combined in one
>> video stream (e.g. side by side images as parallel and crosseye, and
>> some above/below variants). Output formats are also side by side and
>> above/below formats as well as some anaglyph modes (red/cyan,
>> green/magenta and yellow/blue) and mono video (only left or right eye
>> images). Supported image formats are IMGFMT_YV12 and IMGFMT_RGB24
>> (others may follow).
>
> I guess this probably in part duplicates what vf_down3dright.c, you should
> at least look at that and compare.
>
The filter you refer to only converts avobe/below (left eye image  
above) to parallel side by side view. (Probably even faster.) But all  
other conversions are not supported.

>> The Syntax looks like this: mplayer -vf stereo=<input_fmt>:<output_fmt>
>> video.avi
>> (e.g. stereo=slr:arc for displaying a parallel side by side video as
>> anaglyph red/cyan)
>
> I understand the laziness, however a more human-readable names probably
> still would be a good idea.
>
OK, i will add some human-readable attributes for all formats as well.

>> +//==macros==//
>> +#define MIN(X,Y)                ((X) < (Y) ? (X) : (Y))
>> +#define MAX(X,Y)                ((X) < (Y) ? (Y) : (X))
>
> FFMIN/FFMAX
>
Just out of curiosity, what do you mean by this comment? Is it about  
the name of the macros or about the fact i redefined them instead of  
including them from somewhere else? I only use them for clipping  
anyway, so it all comes down to your next remark anyway.

>> +#define YUV2R(Y,V,U)            (MIN(255,MAX(0,((1192*((Y) - 16) +  
>> 1634*((V) - 128)                   ) >> 10))))
>> +#define YUV2G(Y,V,U)            (MIN(255,MAX(0,((1192*((Y) - 16) -  
>>  833*((V) - 128) -  400*((U) - 128)) >> 10))))
>> +#define YUV2B(Y,V,U)            (MIN(255,MAX(0,((1192*((Y) - 16)    
>>                  + 2066*((U) - 128)) >> 10))))
>> +
>> +#define RGB2Y(R,G,B)            (MIN(255,MAX(0,((( 263*(R) +  
>> 516*(G) + 100*(B)) >> 10) +  16))))
>> +#define RGB2V(R,G,B)            (MIN(255,MAX(0,((( 450*(R) -  
>> 377*(G) -  73*(B)) >> 10) + 128))))
>> +#define RGB2U(R,G,B)            (MIN(255,MAX(0,(((-152*(R) -  
>> 298*(G) + 450*(B)) >> 10) + 128))))
>
> av_clip_uint8
> However there are maybe 8 different ways of doing YUV<->RGB  
> conversion, by doing
> it in this filter you'll make it even harder to support this properly and
> these conversions will be ridiculously slow compared to the ones done by
> the scale filter in addition.
>
>> +#define M4(V1,V2,V3,V4)         (MIN(255,MAX(0,(((V1) + (V2) +  
>> (V3) + (V4)) >> 2))))
>
> again av_clip_uint8
>
Ok, i will replace all (MIN(255,MAX(0, ...))) with av_clip_uint8.

>> +    case SIDE_BY_SIDE_LR:
>> +    {
>> +      int size = vf->priv->w * vf->priv->h;
>> +      memcpy_pic( vf->priv->img_l, vf->priv->img_src->planes[0],  
>> vf->priv->w, vf->priv->h, vf->priv->w, 2 * vf->priv->w );
>> +      memcpy_pic( vf->priv->img_r, vf->priv->img_src->planes[0] +  
>> vf->priv->w, vf->priv->w, vf->priv->h, vf->priv->w, 2 * vf->priv->w  
>> );
>> +
>> +      memcpy_pic( vf->priv->img_l + size,  
>> vf->priv->img_src->planes[1], vf->priv->w / 2, vf->priv->h / 2,  
>> vf->priv->w / 2, vf->priv->w );
>> +      memcpy_pic( vf->priv->img_r + size,  
>> vf->priv->img_src->planes[1] + vf->priv->w / 2, vf->priv->w / 2,  
>> vf->priv->h / 2, vf->priv->w / 2, vf->priv->w );
>> +
>> +      size = size + ( size >> 2 );
>> +      memcpy_pic( vf->priv->img_l + size,  
>> vf->priv->img_src->planes[2], vf->priv->w / 2, vf->priv->h / 2,  
>> vf->priv->w / 2, vf->priv->w );
>> +      memcpy_pic( vf->priv->img_r + size,  
>> vf->priv->img_src->planes[2] + vf->priv->w / 2, vf->priv->w / 2,  
>> vf->priv->h / 2, vf->priv->w / 2, vf->priv->w );
>> +      break;
>> +    }
>> +    case SIDE_BY_SIDE_RL:
>> +    {
>> +      int size = vf->priv->w * vf->priv->h;
>> +      memcpy_pic( vf->priv->img_l, vf->priv->img_src->planes[0] +  
>> vf->priv->w, vf->priv->w, vf->priv->h, vf->priv->w, 2 * vf->priv->w  
>> );
>> +      memcpy_pic( vf->priv->img_r, vf->priv->img_src->planes[0],  
>> vf->priv->w, vf->priv->h, vf->priv->w, 2 * vf->priv->w );
>> +
>> +      memcpy_pic( vf->priv->img_l + size,  
>> vf->priv->img_src->planes[1] + vf->priv->w / 2, vf->priv->w / 2,  
>> vf->priv->h / 2, vf->priv->w / 2, vf->priv->w );
>> +      memcpy_pic( vf->priv->img_r + size,  
>> vf->priv->img_src->planes[1], vf->priv->w / 2, vf->priv->h / 2,  
>> vf->priv->w / 2, vf->priv->w );
>> +
>> +      size = size + ( size >> 2 );
>> +      memcpy_pic( vf->priv->img_l + size,  
>> vf->priv->img_src->planes[2] + vf->priv->w / 2, vf->priv->w / 2,  
>> vf->priv->h / 2, vf->priv->w / 2, vf->priv->w );
>> +      memcpy_pic( vf->priv->img_r + size,  
>> vf->priv->img_src->planes[2], vf->priv->w / 2, vf->priv->h / 2,  
>> vf->priv->w / 2, vf->priv->w );
>> +      break;
>
> This and all the other code like it needs a very close look and everything
> possible needs to be factored out into a shared function.
> IMO anything larger than half the size of what it is currently is simply
> unmaintainable (also since huge parts of the code will remain untested
> unless you test every combination of input and output format),
> about a 1/4 of the current size is probably possible.
>
I've noticed this problem before. Most of the conversions only differ  
from each other by the params of the memcpy calls. So i should  
calculate these params in the config function already and use just one  
case for these modes in the switch.

The anaglyphs are different to the rest. I'm not sure how to reduce  
code size in this cases. They just use different macros for the  
calculation of each pixel, but i shouldn't use a switch within the  
loops. This would certainly slow down the code.


Another general question: shall i provide further patches as diffs  
from last revision or should i provide incremental patches based on my  
last patch?

Regards
Gordon