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

[Oded Shimon]

On Tue, May 09, 2006 at 08:25:30PM +0200, Attila Kinali wrote:
> On Tue, 9 May 2006 19:33:44 +0400
> Evgeniy Stepanov <eugeni.stepanov at gmail.com> wrote:
> >  	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.

For the conversion to int* instead of void* . To avoid casts in 
the later if. Though logically, this variable should be inside the 
'if(request==VFCTRL_SCREENSHOT) {'...

> >      if(request==VFCTRL_SCREENSHOT) {
> > -	vf->priv->shot=1;
> > +	if (tmp && *tmp) { // repeated screenshot mode
> 
> Add a comment why (tmp && *tmp) denotes repeated mode.

The first 'tmp' is just paranoia, the '*tmp' actually checks the boolean 
value. I managed to figure this out just by looking at this line, I 
generaly don't believe many comments make a code more readable. whichever 
though. I do agree that the struct member needs to be commented/doxygened.

> >  .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.
> ---

BTW, regarding this whole patch, using 's pausing screenshot' would work 
just as well IMO... Am not against though.

- ods15