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

[Ruslan Savchenko]

On Mon, Nov 10, 2008 at 11:43 AM, Attila Kinali <attila at kinali.ch> wrote:

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


As Uoti said, it is already global in mplayer and even "extern char *
filename"
appears in mplayer.h. Including header now instead of declaring.
I also would appreciate any robust suggestions how to pass filename to vf.


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


Added second option for this. But I think using the filename is relevant. It
is
very useful if you collect screenshots and create them at random time.
Typing
filename two times for every playback is inconvenient (and consider
situation
when multiple files appear in command line).


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


Added one.


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


I also think so. In attached patch fname is char*


> Also the magic number here should be replaced by a define.


OK. two magic numbers now are defined and the third one explained in a
comment.

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


2^15 is 9 hours. Not enough i suppose. Switched to unsigned long.

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

I believe ISO/IEC 9899:1999 says '\0' is always written by snprintf. But
anyway,
in current patch most of snprntfs are exchanged by sprintf because enough
memory is allocated right before the operation.

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


This is just like in other video filters (vf_ass, vf_crop, etc) that use
m_option.
I don't understand usefullness of such a define eigther, but decided to
follow
the existing style.

I will update a patch to manpage later.

-- 
Regards,
Ruslan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vf_screenshot-filename.patch
Type: application/octet-stream
Size: 5405 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20081110/1c5eeac8/attachment.obj>