[MPlayer-dev-eng] [Patch] Cropdetect Aspect Ratio display & True Speed

[Reimar Döffinger]

On Thu, May 14, 2009 at 09:18:23AM +0200, Alexander Roalter wrote:
> +if ((speed24 == 1) && (((int)(1.0/mpctx->sh_video->frametime)) == 25))

That be written a bit simpler (less () in particular), also comparing
the frame rate against 25 without rounding seems a bit questionable -
any rounding error and it won't work.

> +    playback_speed = 0.96;
> +    mpctx->sh_video->frametime = (1.0/24.0);

Huh? Why both? setiing playback_speed should already fix up the frame
rate.

> Index: cfg-mplayer.h
> ===================================================================
> --- cfg-mplayer.h	(revision 29308)
> +++ cfg-mplayer.h	(working copy)
> @@ -162,6 +162,9 @@
>  #endif
>  #endif
>  
> +	// force speed on PAL source to 24fps (0.96%)
> +        {"speed24", &speed24, CONF_TYPE_FLAG, 0, 0, 1, NULL},
> +
>  	// force window width/height or resolution (with -vm)
>  	{"x", &opt_screen_size_x, CONF_TYPE_INT, CONF_RANGE, 0, 4096, NULL},

Other options use tabs.
Now the main problem I have with that:
on a file-by-file basis -speed 24/25 works just fine (if not that's a
bug I think).
For autodetection, the code you have is simply horribly unreliable:
if a single frame has a duration of 1/25 s it will enable this feature
and it won't disable it again.
If the frame rate is slightly off from 1/25 s it possibly will fail to
enable it. If you avoid that, it might get enabled for files where it
should not be.
Overall I just think this isn't a feature that is appropriate for SVN.

> @@ -50,11 +51,17 @@
>      vf->priv->x2=0;
>      vf->priv->y2=0;
>      vf->priv->fno=0;
> +    if (d_height == 0)
> +        vf->priv->aspect = 1;
> +    else
> +        vf->priv->aspect = (float)d_width/(float)d_height;

Does aspect == 0 make any sense?
IMO it should either be
if (!d_height) vf->priv->aspect = 0;
...

or

if (!d_height || !d_width) vf->priv->aspect = 1;
...

> +    ratio = (((float)(vf->priv->x2 - vf->priv->x1 + 1))/
> +             ((float)(vf->priv->y2 - vf->priv->y1 + 1)));

You make C look like LISP :-P
ratio = (float)(vf->priv->x2 - vf->priv->x1 + 1) / (vf->priv->y2 - vf->priv->y1 + 1);
e.g. is enough. Though I'd suggest just using double, this is not
performance critical so we need to risk more rounding errors.

> +    if ((mpi->height == 0) || (mpi->width == 0))
> +      stretch = 1;
> +    else
> +      stretch = vf->priv->aspect / ((float)mpi->width/ (float)mpi->height);

Not sure if 1 or 0 is better as "fallback" (though it should be consistent with above
code setting vf->priv->aspect).
Also it should be simply "if (!mpi->width || !mpi->height)" and
stretch = vf->priv->aspect * mpi->height / mpi->width;