[MPlayer-dev-eng] [PATCH] vf_screenshot, output filename corresponding to the played file

[Attila Kinali]

On Sun, 9 Nov 2008 22:03:07 +0300
"Ruslan Savchenko" <ruslan.savchenko at gmail.com> wrote:

> Currently mplayer saves screenshots into files named "shotNNNN.png". After
> applying attached patches one would be able to save screenshots into
> "mymovie.[timestamp].png" files. Old behaviour is default, the new one is
> turned on by passing param to vf_screenshot. "mymovie" is the filename of
> the file being played (entirely with suffix), format for timestamp could be
> either hh:mm:ss.ms (useful for mplayer options such as -endpos) or
> hh-mm-ss.ms (useful when ':' cannot appear in filename).
> So -vf screenshot (of -vf screenshot=0) saves to "shotNNNN.png", -vf
> screenshot=1 to "mymovie.[hh:mm:ss.ms].png", and -vf screenshot=2 to
> "mymovie.[hh:mm:ss.ms].png"
> 
> first patch moves "filename" variable outside of main() to global in
> mencoder.c
> second patch is for vf_screenshot
> and the third one patches the man page

Uhmm.. shouldn't be filname global to both mplayer and mencoder?
At least -vf screenshot should work as defined in the manpage
for mplayer too.

Also, it would be better to be able to specify the filename of the
screen shots instead of using the filename of the original file.

The first issue that pops up here is, that none of teh string
operations are documented and there is no comment why they are
safe. I assume most are, but that doesnt mean that there shouldnt
be any comment.

> Index: libmpcodecs/vf_screenshot.c
> ===================================================================
> --- libmpcodecs/vf_screenshot.c	(revision 27902)
> +++ libmpcodecs/vf_screenshot.c	(working copy)
> @@ -22,9 +22,15 @@
>  #include "libswscale/swscale.h"
>  #include "libavcodec/avcodec.h"
>  
> +#include "m_option.h"
> +#include "m_struct.h"
> +
> +extern char* filename;
> +
>  struct vf_priv_s {
>      int frameno;
> -    char fname[102];
> +    char fname[256];

I think, it would be better to change fname to a char* here
and allocate the needed size, as fname is now a user supplied
parameter of arbitrary size and can be very well over 256 bytes.
Also the magic number here should be replaced by a define.

> +    int fnametype;
>      /// shot stores current screenshot mode:
>      /// 0: don't take screenshots
>      /// 1: take single screenshot, reset to 0 afterwards
> @@ -36,7 +42,7 @@
>      AVCodecContext *avctx;
>      uint8_t *outbuffer;
>      int outbuffer_size;
> -};
> +} vf_priv_dlft;
>  
>  //===========================================================================//
>  
> @@ -92,15 +98,37 @@
>      else return 0;
>  }
>  
> -static void gen_fname(struct vf_priv_s* priv)
> +static void gen_fname(struct vf_priv_s* priv, double pts)
>  {
> -    do {
> -	snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno);
> -    } while (fexists(priv->fname) && priv->frameno < 100000);
> -    if (fexists(priv->fname)) {
> -	priv->fname[0] = '\0';
> -	return;
> +    char *s;
> +    int hour = (int) (pts/3600);
> +    int min = ((int) (pts/60)) % 60;
> +    int sec = ((int) pts) % 60;
> +    int msec = (int) (pts*100) % 100;

Are you sure that pts can never overflow an int?
(IIRC pts is here first converted to int, then the division/modulo
is applied). Beside, uint would be the appropriate type here,
as time values can never become negative.

> +    if ((priv->fnametype == 0) || (strnstr(filename, "://", 20) != NULL)) {

Another magic number, nobody knows where it comes from.

> +        do {
> +            snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno);
> +            } while (fexists(priv->fname) && priv->frameno < 100000);
> +        if (fexists(priv->fname)) {
> +	          priv->fname[0] = '\0';
> +	          return;
> +        }
>      }
> +    else if ((priv->fnametype == 1) || (priv->fnametype == 2)) {
> +        s = strrchr(filename, '/');
> +        if (s == NULL) 
> +            s = filename;
> +        else
> +            s++;
> +        strncpy(priv->fname, s, 235);

Don't put any magic numbers in the code, especialy not
without comments. A define (as above) would be optimal.

> +        priv->fname[235] = '\0';
> +        s = priv->fname + strlen(priv->fname);
> +        if (priv->fnametype == 1)
> +            snprintf(s, 20, ".[%02d:%02d:%02d.%02d].png", hour, min, sec, msec);
> +        else
> +            snprintf(s, 20, ".[%02d-%02d-%02d.%02d].png", hour, min, sec, msec);
> +    }

Are you sure that a trailing '\0' is written even in case snprintf
trunkates its output? (A quick look into the manpage didnt reveal anything)

>  
>      mp_msg(MSGT_VFILTER,MSGL_INFO,"*** screenshot '%s' ***\n",priv->fname);
>  
> @@ -193,7 +221,7 @@
>      if(vf->priv->shot) {
>  	if (vf->priv->shot==1)
>  	    vf->priv->shot=0;
> -	gen_fname(vf->priv);
> +	gen_fname(vf->priv, pts);
>  	if (vf->priv->fname[0]) {
>  	    if (!vf->priv->store_slices)
>  	      scale_image(vf->priv, dmpi);
> @@ -267,7 +295,10 @@
>      vf->draw_slice=draw_slice;
>      vf->get_image=get_image;
>      vf->uninit=uninit;
> -    vf->priv=malloc(sizeof(struct vf_priv_s));
> +    if (!vf->priv) {
> +        vf->priv=malloc(sizeof(struct vf_priv_s));
> +        memset(vf->priv, 0, sizeof(struct vf_priv_s));
> +    }
>      vf->priv->frameno=0;
>      vf->priv->shot=0;
>      vf->priv->store_slices=0;
> @@ -292,14 +323,26 @@
>      free(vf->priv);
>  }
>  
> +#define ST_OFF(f) M_ST_OFF(struct vf_priv_s,f)
> +static const m_option_t vf_opts_fields[] = {
> +    {"fnametype", ST_OFF(fnametype), CONF_TYPE_INT, 0, 0, 2, NULL},
> +    {NULL, NULL, 0, 0, 0, 0, NULL}
> +};

This define here doesn't make the code more readable and is used
at one place only, thus quite unnecessary.

> +static const m_struct_t vf_opts = {
> +    "screenshot",
> +    sizeof(struct vf_priv_s),
> +    &vf_priv_dlft,
> +    vf_opts_fields
> +};
> +
>  const vf_info_t vf_info_screenshot = {
>      "screenshot to file",
>      "screenshot",
>      "A'rpi, Jindrich Makovicka",
>      "",
>      screenshot_open,
> -    NULL
> +    &vf_opts
>  };
>  
>  //===========================================================================//



			Attila Kinali

-- 
If you want to walk fast, walk alone.
If you want to walk far, walk together.
		-- African proverb