[MPlayer-dev-eng] [PATCH] screenshoot every frame

[Attila Kinali]

On Tue, 9 May 2006 19:33:44 +0400
Evgeniy Stepanov <eugeni.stepanov at gmail.com> wrote:

Following are comments on how the code should be commented.
Please read also DOCS/tech/code-documentation.txt.

> [screenshot-switch.patch  text/x-diff (3.3KB)]
> Index: libmpcodecs/vf_screenshot.c
> ===================================================================
> --- libmpcodecs/vf_screenshot.c	(revision 366)
> +++ libmpcodecs/vf_screenshot.c	(working copy)
> @@ -210,7 +210,8 @@
>      }
>  
>      if(vf->priv->shot) {
> -	vf->priv->shot=0;
> +	if (vf->priv->shot==1)
> +	    vf->priv->shot=0;

Please add a doxygen comment in the priv struct to
clarify that shot isnt anymore a "boolean" but
carries a different meaning based on its value.
Also list all those meanings.

>  	gen_fname(vf->priv);
>  	if (vf->priv->fname[0]) {
>  	    if (!vf->priv->store_slices)
> @@ -225,8 +226,17 @@
>  
>  int control (vf_instance_t *vf, int request, void *data)
>  {
> +    const int *const tmp = data;

Just a nitpick, but why do you create another
variable? Ok, const * const, but still.
Beside giving it a generic name like "tmp" isn't
a good idea.

>      if(request==VFCTRL_SCREENSHOT) {
> -	vf->priv->shot=1;
> +	if (tmp && *tmp) { // repeated screenshot mode

Add a comment why (tmp && *tmp) denotes repeated mode.

> +	    if (vf->priv->shot==2)
> +		vf->priv->shot=0;
> +	    else
> +		vf->priv->shot=2;
> +	} else { // single screenshot
> +	    if (!vf->priv->shot)
> +		vf->priv->shot=1;
> +	}
>          return CONTROL_TRUE;
>      }
>      return vf_next_control (vf, request, data);
[...]
> Index: DOCS/man/en/mplayer.1
> ===================================================================
> --- DOCS/man/en/mplayer.1	(revision 366)
> +++ DOCS/man/en/mplayer.1	(working copy)
> @@ -307,6 +307,8 @@
>  Set EDL mark.
>  .IPs "s (\-vf screenshot only)"
>  Take a screenshot.
> +.IPs "S (\-vf screenshot only)"
> +Start/stop taking screenshots.
>  .IPs "I"
>  Show filename on the OSD.
>  .RE
> @@ -6145,7 +6147,8 @@
>  .TP
>  .B screenshot
>  Allows acquiring screenshots of the movie using the screenshot command
> -(bound to the 's' key by default).
> +(bound to the 's' key by default). 'S' key triggers automatic shooting of
> +every frame.

I don't understand what you write here.
i guess it should read more like:
---
Pressing the 's' key takes a single screenshot.
When 'S' is pressed, consecutive screenshots are taken
with each frame until 'S' is pressed again.
---

>  Files named 'shotNNNN.png' will be saved in the working directory,
>  using the first available number - no files will be overwritten.
>  The filter has no overhead when not used and accepts an arbitrary


				Attila Kinali

-- 
心をこめて聞け心をこめて話せ