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

[Michael Niedermayer]

On Sun, Nov 23, 2008 at 07:59:26PM +0300, Ruslan Savchenko wrote:
> On Sat, Nov 22, 2008 at 12:56 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Nov 21, 2008 at 02:17:32PM +0300, Ruslan Savchenko wrote:
> >> On Mon, Nov 17, 2008 at 9:10 PM, Ruslan Savchenko
> >> <ruslan.savchenko at gmail.com> wrote:
> >> > 2008/11/15 Michael Niedermayer <michaelni at gmx.at>:
> >> >> On Sat, Nov 15, 2008 at 03:03:41AM +0300, Ruslan Savchenko wrote:
> >> >>> On Sat, Nov 15, 2008 at 2:47 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >>> > On Sat, Nov 15, 2008 at 02:28:03AM +0300, Ruslan Savchenko wrote:
> >> >>> >> On Sat, Nov 15, 2008 at 1:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >>> >> > On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote:
> >> >>
> >> >> both attached patches look ok IMHO
> >> >>
> >> >
> >> > Any news?
> >> > I think I should wait until those will be applied/rejected before
> >> > proposing next patches. Right?
> >>
> >> Since I get no response, I send the whole updated patch once again.
> >
> > well and i have to reject it as it contains changes that should be seperate
> > patches. And actually are but the patch now contains them too.
> > Do you think if noone applied a 2 line patch that someone will apply a much
> > larger patch that contains these 2 lines as well ...
> >
> > Actually i would suggest that you split the filename and timestamp in name
> > things in 2 patches as well.
> 
> I split the patch into 8 parts. I hope this is what you want. Also,
> "fnametype" from previous patches is now "style" and "famebase" is
> "basename".
> All attached patches except of 0001 and 0008 refer to vf_screenshot.c.
> Here is a brief explanation about each patch:
> 

>  0001-Move-filename-variable-to-global-in-mencoder.c.patch
>     Move 'filename' variable outside main() to global in mencoder.c.
> In mplayer.c 'filename' is already global.

as noone said anything against this its ok IMHO


> 
> 0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch
>     Define SHOT_FNAME_LENGTH and use it instead of magic numbers.

looks ok


> 
> 0003-Change-priv-fname-allocation-from-static-to-dynamic.patch
>     Allocate priv->fname dynamically in gen_fname() and free it in
> put_image(). Dynamic allocation is required by further patches.

see below


> 
> 0004-Add-m_option-structure-for-new-options-in-further-patches.patch
>     Initialize options parser. Options will be introduced in further patches

probably ok, iam no expect in these though


> 
> 0005-Added-a-style-option-to-print-timestamp-in-screens.patch
>     If style is 0, then old-style screenshots "shotXXXX.png" are
> emited. If style is 1, then screenshot file name will be like
> "shot.[hh:mm:ss.ms].png", and if style is 2, screenshot name will be
> like "shot.[hh-mm-ss.ms].png"
> 

see below


> 0006-cosmetics-in-gen_fname.patch
>      Move old code to appropriate indentation inside switch.

ok


> 
> 0007-Added-a-basename-option-to-use-instead-of-shot-whe.patch
>     Added a basename option to use instead of "shot" when style is 1
> or 2 and mplayer's playing a file. If mplayer's playing a stream,
> still "shot" will be used.

see below

[...]

> @@ -265,6 +273,7 @@ static void uninit(vf_instance_t *vf)
>      av_freep(&vf->priv->avctx);
>      if(vf->priv->ctx) sws_freeContext(vf->priv->ctx);
>      if (vf->priv->buffer) free(vf->priv->buffer);
> +    if (vf->priv->fname) free(vf->priv->fname);
>      free(vf->priv->outbuffer);
>      free(vf->priv);
>  }

i dont see how it can still be allocated here



[...]

> diff --git a/libmpcodecs/vf_screenshot.c b/libmpcodecs/vf_screenshot.c
> index afc6770..eb122bb 100644
> --- a/libmpcodecs/vf_screenshot.c
> +++ b/libmpcodecs/vf_screenshot.c
> @@ -26,10 +26,12 @@
>  #include "m_struct.h"
>  
>  #define SHOT_FNAME_LENGTH 102
> +#define TIMESTAMP_LENGTH 20
>  
>  struct vf_priv_s {
>      int frameno;
>      char *fname;
> +    int style;
>      /// shot stores current screenshot mode:
>      /// 0: don't take screenshots
>      /// 1: take single screenshot, reset to 0 afterwards
> @@ -97,8 +99,37 @@ static int fexists(char *fname)
>      else return 0;
>  }
>  
> -static void gen_fname(struct vf_priv_s* priv)
> +static void gen_fname(struct vf_priv_s* priv, double pts)
>  {
> +    char *base = "shot";
> +    char tstamp[TIMESTAMP_LENGTH];
> +    char ts_sep;

> +    uint32_t hour = (uint32_t) (pts/3600);
> +    uint32_t min  = (uint32_t) (pts/60) % 60;
> +    uint32_t sec  = (uint32_t) pts % 60;
> +    uint32_t msec = (uint32_t) (pts*100) % 100;

this code does not look like it could work reliably
double is not a real number in the mathematical sense its not exact,
the way it rounds can be different in each calculation

[...]
> @@ -119,6 +122,20 @@ static void gen_fname(struct vf_priv_s* priv, double pts)
>                    "%02" PRIu32 "%c%02" PRIu32 "%c%02" PRIu32 ".%02" PRIu32,
>                     hour, ts_sep, min, ts_sep, sec, msec);
>  
> +        if (priv->basename)
> +           base = priv->basename;
> +        else {
> +            if (strstr(filename, "://"))

please dont indent things randomly, even if this is mplayer code


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20081125/b81bbe84/attachment.pgp>