[MPlayer-dev-eng] [PATCH] vf_screenshot, output filename corresponding to the played file
Michael Niedermayer
michaelni at gmx.at
Tue Nov 25 20:19:07 CET 2008
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>
More information about the MPlayer-dev-eng
mailing list