[PATCH] vf_screenshot, output filename corresponding to the played file
Hello, Currently mplayer saves screenshots into files named "shotNNNN.png". After applying attached patches one would be able to save screenshots into "mymovie.[timestamp].png" files. Old behaviour is default, the new one is turned on by passing param to vf_screenshot. "mymovie" is the filename of the file being played (entirely with suffix), format for timestamp could be either hh:mm:ss.ms (useful for mplayer options such as -endpos) or hh-mm-ss.ms (useful when ':' cannot appear in filename). So -vf screenshot (of -vf screenshot=0) saves to "shotNNNN.png", -vf screenshot=1 to "mymovie.[hh:mm:ss.ms].png", and -vf screenshot=2 to "mymovie.[hh:mm:ss.ms].png" first patch moves "filename" variable outside of main() to global in mencoder.c second patch is for vf_screenshot and the third one patches the man page Regards, Ruslan
On Sun, Nov 9, 2008 at 10:03 PM, Ruslan Savchenko < ruslan.savchenko@gmail.com> wrote:
and -vf screenshot=2 to "mymovie.[hh:mm:ss.ms].png"
Sorry for mistake, there should be "mymovie.[hh-mm-ss.ms].png". Regards, Ruslan
On Sun, 9 Nov 2008 22:03:07 +0300 "Ruslan Savchenko" <ruslan.savchenko@gmail.com> wrote:
Currently mplayer saves screenshots into files named "shotNNNN.png". After applying attached patches one would be able to save screenshots into "mymovie.[timestamp].png" files. Old behaviour is default, the new one is turned on by passing param to vf_screenshot. "mymovie" is the filename of the file being played (entirely with suffix), format for timestamp could be either hh:mm:ss.ms (useful for mplayer options such as -endpos) or hh-mm-ss.ms (useful when ':' cannot appear in filename). So -vf screenshot (of -vf screenshot=0) saves to "shotNNNN.png", -vf screenshot=1 to "mymovie.[hh:mm:ss.ms].png", and -vf screenshot=2 to "mymovie.[hh:mm:ss.ms].png"
first patch moves "filename" variable outside of main() to global in mencoder.c second patch is for vf_screenshot and the third one patches the man page
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. 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. 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.
Index: libmpcodecs/vf_screenshot.c =================================================================== --- libmpcodecs/vf_screenshot.c (revision 27902) +++ libmpcodecs/vf_screenshot.c (working copy) @@ -22,9 +22,15 @@ #include "libswscale/swscale.h" #include "libavcodec/avcodec.h"
+#include "m_option.h" +#include "m_struct.h" + +extern char* filename; + struct vf_priv_s { int frameno; - char fname[102]; + char fname[256];
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. Also the magic number here should be replaced by a define.
+ int fnametype; /// shot stores current screenshot mode: /// 0: don't take screenshots /// 1: take single screenshot, reset to 0 afterwards @@ -36,7 +42,7 @@ AVCodecContext *avctx; uint8_t *outbuffer; int outbuffer_size; -}; +} vf_priv_dlft;
//===========================================================================//
@@ -92,15 +98,37 @@ else return 0; }
-static void gen_fname(struct vf_priv_s* priv) +static void gen_fname(struct vf_priv_s* priv, double pts) { - do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); - } while (fexists(priv->fname) && priv->frameno < 100000); - if (fexists(priv->fname)) { - priv->fname[0] = '\0'; - return; + char *s; + 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.
+ if ((priv->fnametype == 0) || (strnstr(filename, "://", 20) != NULL)) {
Another magic number, nobody knows where it comes from.
+ do { + snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + } while (fexists(priv->fname) && priv->frameno < 100000); + if (fexists(priv->fname)) { + priv->fname[0] = '\0'; + return; + } } + else if ((priv->fnametype == 1) || (priv->fnametype == 2)) { + s = strrchr(filename, '/'); + if (s == NULL) + s = filename; + else + s++; + strncpy(priv->fname, s, 235);
Don't put any magic numbers in the code, especialy not without comments. A define (as above) would be optimal.
+ priv->fname[235] = '\0'; + s = priv->fname + strlen(priv->fname); + if (priv->fnametype == 1) + snprintf(s, 20, ".[%02d:%02d:%02d.%02d].png", hour, min, sec, msec); + else + snprintf(s, 20, ".[%02d-%02d-%02d.%02d].png", hour, min, sec, msec); + }
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)
mp_msg(MSGT_VFILTER,MSGL_INFO,"*** screenshot '%s' ***\n",priv->fname);
@@ -193,7 +221,7 @@ if(vf->priv->shot) { if (vf->priv->shot==1) vf->priv->shot=0; - gen_fname(vf->priv); + gen_fname(vf->priv, pts); if (vf->priv->fname[0]) { if (!vf->priv->store_slices) scale_image(vf->priv, dmpi); @@ -267,7 +295,10 @@ vf->draw_slice=draw_slice; vf->get_image=get_image; vf->uninit=uninit; - vf->priv=malloc(sizeof(struct vf_priv_s)); + if (!vf->priv) { + vf->priv=malloc(sizeof(struct vf_priv_s)); + memset(vf->priv, 0, sizeof(struct vf_priv_s)); + } vf->priv->frameno=0; vf->priv->shot=0; vf->priv->store_slices=0; @@ -292,14 +323,26 @@ free(vf->priv); }
+#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.
+static const m_struct_t vf_opts = { + "screenshot", + sizeof(struct vf_priv_s), + &vf_priv_dlft, + vf_opts_fields +}; + const vf_info_t vf_info_screenshot = { "screenshot to file", "screenshot", "A'rpi, Jindrich Makovicka", "", screenshot_open, - NULL + &vf_opts };
//===========================================================================//
Attila Kinali -- If you want to walk fast, walk alone. If you want to walk far, walk together. -- African proverb
On Mon, 2008-11-10 at 09:43 +0100, Attila Kinali wrote:
On Sun, 9 Nov 2008 22:03:07 +0300 "Ruslan Savchenko" <ruslan.savchenko@gmail.com> wrote:
first patch moves "filename" variable outside of main() to global in mencoder.c second patch is for vf_screenshot and the third one patches the man page
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.
There already is a global "filename" in mplayer.c. However IMO it's a bad idea for individual filters to rely on such globals existing and being set to any particular value. Especially when the patch adds a private "extern char *filename;" declaration in a filter instead of including a header (but I still wouldn't like it even if it included a header).
On Mon, Nov 10, 2008 at 11:43 AM, Attila Kinali <attila@kinali.ch> wrote:
On Sun, 9 Nov 2008 22:03:07 +0300 "Ruslan Savchenko" <ruslan.savchenko@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
On Mon, Nov 10, 2008 at 03:01:50PM +0300, Ruslan Savchenko wrote:
On Mon, Nov 10, 2008 at 11:43 AM, Attila Kinali <attila@kinali.ch> wrote:
On Sun, 9 Nov 2008 22:03:07 +0300 "Ruslan Savchenko" <ruslan.savchenko@gmail.com> wrote: [...] + 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.
int in posix is at least 32 bit
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.
i dont think this is a good idea [...]
@@ -92,15 +100,69 @@ else return 0; }
-static void gen_fname(struct vf_priv_s* priv) +#define TIMESTAMP_LENGTH 12 +#define SHOT_FNAME_LENGTH 13 + +static void gen_fname(struct vf_priv_s* priv, double pts) { - do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); - } while (fexists(priv->fname) && priv->frameno < 100000); - if (fexists(priv->fname)) { - priv->fname[0] = '\0'; - return; [...] + char *s; + char tstamp[TIMESTAMP_LENGTH]; + unsigned long hour = (unsigned long) (pts/3600); + unsigned long min = (unsigned long) (pts/60) % 60; + unsigned long sec = (unsigned long) pts % 60; + unsigned long msec = (unsigned long) (pts*100) % 100; + + if (priv->fnametype == 0) { + /* allocate space to handle "shotXXXX.png" filenames */ + priv->fname = malloc(SHOT_FNAME_LENGTH); + do { + sprintf (priv->fname, "shot%04d.png", ++priv->frameno); + } while (fexists(priv->fname) && priv->frameno < 100000);
what is the < 100000 check supposed to be? but i see now this code was already there before, but you replace the snprintf() by sprintf and VERY significantly reduce the array size so the code will write over the allocated space. This is a nice example why reindenting code in functional patches IS a bad idea.
+ if (fexists(priv->fname)) { + priv->fname[0] = '\0'; + return; + } } + else if ((priv->fnametype == 1) || (priv->fnametype == 2)) { + if (priv->fnametype == 1) + snprintf(tstamp, TIMESTAMP_LENGTH, "%02lu:%02lu:%02lu.%02lu", hour, min, sec, msec); + else if (priv->fnametype == 2) + snprintf(tstamp, TIMESTAMP_LENGTH, "%02lu-%02lu-%02lu.%02lu", hour, min, sec, msec);
TIMESTAMP_LENGTH is clearly too small for more han 99 hours
+ + if (!priv->fnamebase) { + if (strstr(filename, "://") != NULL) {
+ priv->fnamebase = malloc(SHOT_FNAME_LENGTH); + strcpy(priv->fnamebase, "shot");
strdup()
+ } + else { + s = strrchr(filename, '/'); + if (s == NULL) + s = filename; + else + s++; + priv->fnamebase = malloc(strlen(s) + 1); + strcpy(priv->fnamebase, s); + } + } + /* 8 is length of ".[].png" plus '\0' */
+ priv->fname = malloc(strlen(priv->fnamebase) + strlen(tstamp) + 8); + sprintf(priv->fname, "%s.[%s].png", priv->fnamebase, tstamp);
interger oveflows leading to segfaults or exploits [...]
+ vf->priv=malloc(sizeof(struct vf_priv_s)); + memset(vf->priv, 0, sizeof(struct vf_priv_s));
calloc() besides these, i seriously douubt all the *alloc() in this patch is needed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Tue, Nov 11, 2008 at 6:01 PM, Michael Niedermayer <michaelni@gmx.at>wrote:
On Mon, Nov 10, 2008 at 03:01:50PM +0300, Ruslan Savchenko wrote:
On Mon, Nov 10, 2008 at 11:43 AM, Attila Kinali <attila@kinali.ch> wrote:
On Sun, 9 Nov 2008 22:03:07 +0300 "Ruslan Savchenko" <ruslan.savchenko@gmail.com> wrote: [...]
+ priv->fname = malloc(strlen(priv->fnamebase) + strlen(tstamp) + 8); + sprintf(priv->fname, "%s.[%s].png", priv->fnamebase, tstamp);
interger oveflows leading to segfaults or exploits
priv->filename and tstamp occupied the whole address space? I don't believe it. Maybe I misunderstand you?
besides these, i seriously douubt all the *alloc() in this patch is needed <https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng>
Use fixed-length array? What length? Thank you very much for comments. Attached new patch (merged it with the patch for mencoder). -- Regards, Ruslan
Sorry, patch in previous mail has wrong comment about string operations in gen_fname and includes stdint.h manually while inttypes.h is also included. Fixed. -- Regards, Ruslan
Any comments? (sorry if I didn't wait long enough) Attached patch also updates man page, but the code part is the same. -- Regards, savrus
On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote:
Any comments? (sorry if I didn't wait long enough) Attached patch also updates man page, but the code part is the same.
[...]
+ 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;
+ size_t n; + + if (priv->fnametype == 0) { + /* allocate space to handle "shotXXXX.png" filenames */ + priv->fname = malloc(SHOT_FNAME_LENGTH); do { snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); ^^^ should be replaced by SHOT_FNAME_LENGTH, and actually ideally
the sec would look nicer with 1 more space the 102->SHOT_FNAME_LENGTH could be a seperate patch
} while (fexists(priv->fname) && priv->frameno < 100000); @@ -101,6 +133,32 @@ priv->fname[0] = '\0'; return; } + } + else if ((priv->fnametype == 1) || (priv->fnametype == 2)) {
maybe a switch/case would be slightly cleaner than this if, though its more an issue if more cases get added.
+ /* TIMESTAMP_LENGTH is enough for 2^32 seconds */ + if (priv->fnametype == 1) + snprintf(tstamp, TIMESTAMP_LENGTH, "%02" PRIu32 ":%02" PRIu32 ":%02" PRIu32 ".%02" PRIu32, hour, min, sec, msec); + else if (priv->fnametype == 2) + snprintf(tstamp, TIMESTAMP_LENGTH, "%02" PRIu32 "-%02" PRIu32 "-%02" PRIu32 ".%02" PRIu32, hour, min, sec, msec); + + if (!priv->fnamebase) { + if (strstr(filename, "://") != NULL) + priv->fnamebase = strdup("shot"); + else { + s = strrchr(filename, '/'); + if (s == NULL) + s = filename; + else + s++;
+ priv->fnamebase = malloc(strlen(s) + 1); + strcpy(priv->fnamebase, s);
strdup() also does this code work when there are several files on the command line? or does it always end up using the first filename?
+ } + } + /* 8 is length of ".[].png" plus '\0' */ + n = strlen(priv->fnamebase) + strlen(tstamp) + 8;
+ priv->fname = malloc(n); + snprintf(priv->fname, n, "%s.[%s].png", priv->fnamebase, tstamp);
did i miss the free() or is this a memleak? [...]
Index: mencoder.c =================================================================== --- mencoder.c (revision 27909) +++ mencoder.c (working copy) @@ -115,6 +115,7 @@ char* audio_lang=NULL; char* dvdsub_lang=NULL; static char* spudec_ifo=NULL; +char* filename=NULL;
static char** audio_codec_list=NULL; // override audio codec static char** video_codec_list=NULL; // override video codec @@ -397,7 +398,6 @@ double v_timer_corr=0;
m_entry_t* filelist = NULL; -char* filename=NULL;
int decoded_frameno=0; int next_frameno=-1;
why? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is not what we do, but why we do it that matters.
On Sat, Nov 15, 2008 at 1:16 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote:
Any comments? (sorry if I didn't wait long enough) Attached patch also updates man page, but the code part is the same.
+ size_t n; + + if (priv->fnametype == 0) { + /* allocate space to handle "shotXXXX.png" filenames */ + priv->fname = malloc(SHOT_FNAME_LENGTH); do { snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); ^^^ should be replaced by SHOT_FNAME_LENGTH, and actually ideally the 102->SHOT_FNAME_LENGTH could be a seperate patch
What should i use instead svn diff to create a patch? I did one with diff -Naur and attached, is it ok?
also does this code work when there are several files on the command line? or does it always end up using the first filename?
It works with several files. You can even specify different options as in here: mplayer video1.mkv -vf screenshot=1 video2.mkv video3.mkv -vf screenshot I entered 's' once in each file, this produced: *** screenshot 'video1.mkv.[00:00:00.46].png' *** *** screenshot 'video2.mkv.[00:02:30.38].png' *** *** screenshot 'shot0002.png' *** (shot0002.png is because i've already had shot0001.png in my directory)
+ /* 8 is length of ".[].png" plus '\0' */ + n = strlen(priv->fnamebase) + strlen(tstamp) + 8;
+ priv->fname = malloc(n); + snprintf(priv->fname, n, "%s.[%s].png", priv->fnamebase, tstamp);
did i miss the free() or is this a memleak?
I believe i do free() in uninit(): @@ -288,18 +346,33 @@ 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); + if (vf->priv->fnamebase) free(vf->priv->fnamebase); free(vf->priv->outbuffer); free(vf->priv);
[...]
Index: mencoder.c =================================================================== --- mencoder.c (revision 27909) +++ mencoder.c (working copy) @@ -115,6 +115,7 @@ char* audio_lang=NULL; char* dvdsub_lang=NULL; static char* spudec_ifo=NULL; +char* filename=NULL;
static char** audio_codec_list=NULL; // override audio codec static char** video_codec_list=NULL; // override video codec @@ -397,7 +398,6 @@ double v_timer_corr=0;
m_entry_t* filelist = NULL; -char* filename=NULL;
int decoded_frameno=0; int next_frameno=-1;
why?
Because in mencoder.c "filename" is declared inside main(). In mplayer.c "filename" is global and I moved it to global in mencoder.c. It is needed, because vf_screenshot is linked with mencoder. In the thread opening message there was a separate patch for mencoder.c doing this slip. Sorry if i distracted you by sending incomplete patch (only for vf_screenshot.c file) in the middle of discussion. I don't quite understand if i should split them or no (but i see now i should always send all parts). Thanks again for comments, fixed patch attached. -- Regards, savrus
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@gmx.at> wrote:
On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote: [...]
also does this code work when there are several files on the command line? or does it always end up using the first filename?
It works with several files. You can even specify different options as in here: mplayer video1.mkv -vf screenshot=1 video2.mkv video3.mkv -vf screenshot I entered 's' once in each file, this produced: *** screenshot 'video1.mkv.[00:00:00.46].png' *** *** screenshot 'video2.mkv.[00:02:30.38].png' *** *** screenshot 'shot0002.png' ***
(shot0002.png is because i've already had shot0001.png in my directory)
Why shot000?.png instead of video3.mkv.[00:XX:YY.ZZ].png ?
+ /* 8 is length of ".[].png" plus '\0' */ + n = strlen(priv->fnamebase) + strlen(tstamp) + 8;
+ priv->fname = malloc(n); + snprintf(priv->fname, n, "%s.[%s].png", priv->fnamebase, tstamp);
did i miss the free() or is this a memleak?
I believe i do free() in uninit():
is uninit() called after each screenshot ? [...]
[...]
Index: mencoder.c =================================================================== --- mencoder.c (revision 27909) +++ mencoder.c (working copy) @@ -115,6 +115,7 @@ char* audio_lang=NULL; char* dvdsub_lang=NULL; static char* spudec_ifo=NULL; +char* filename=NULL;
static char** audio_codec_list=NULL; // override audio codec static char** video_codec_list=NULL; // override video codec @@ -397,7 +398,6 @@ double v_timer_corr=0;
m_entry_t* filelist = NULL; -char* filename=NULL;
int decoded_frameno=0; int next_frameno=-1;
why?
Because in mencoder.c "filename" is declared inside main(). In mplayer.c "filename" is global and I moved it to global in mencoder.c.
right, mplayer doesnt use indention, how could i forget, sorry. the filename moving is ok IMHO, mplayer & mencoder should either both or none have a global filename. also spliting is always better than not spliting. And the more independant patches are the quicker some will be applied [...]
--- libmpcodecs/vf_screenshot.c 2008-11-15 01:50:22.000000000 +0300 +++ ../mplayerb/libmpcodecs/vf_screenshot.c 2008-11-15 02:09:52.000000000 +0300 @@ -157,7 +157,7 @@ /* allocate space to handle "shotXXXX.png" filenames */ priv->fname = malloc(SHOT_FNAME_LENGTH); do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + snprintf (priv->fname, SHOT_FNAME_LENGTH, "shot%04d.png", ++priv->frameno); } while (fexists(priv->fname) && priv->frameno < 100000); if (fexists(priv->fname)) { priv->fname[0] = '\0';
if that didnt depend on the other patch, it would be ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML.
On Sat, Nov 15, 2008 at 2:47 AM, Michael Niedermayer <michaelni@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@gmx.at> wrote:
On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote: [...]
also does this code work when there are several files on the command line? or does it always end up using the first filename?
It works with several files. You can even specify different options as in here: mplayer video1.mkv -vf screenshot=1 video2.mkv video3.mkv -vf screenshot I entered 's' once in each file, this produced: *** screenshot 'video1.mkv.[00:00:00.46].png' *** *** screenshot 'video2.mkv.[00:02:30.38].png' *** *** screenshot 'shot0002.png' ***
(shot0002.png is because i've already had shot0001.png in my directory)
Why shot000?.png instead of video3.mkv.[00:XX:YY.ZZ].png ?
Because of the last -vf screenshot. Without it "video3.mkv.[00:XX:YY.ZZ].png" comes out.
+ /* 8 is length of ".[].png" plus '\0' */ + n = strlen(priv->fnamebase) + strlen(tstamp) + 8;
+ priv->fname = malloc(n); + snprintf(priv->fname, n, "%s.[%s].png", priv->fnamebase, tstamp);
did i miss the free() or is this a memleak?
I believe i do free() in uninit():
is uninit() called after each screenshot ?
Oops, my bad. I will start fixing this memleak right now.
right, mplayer doesnt use indention, how could i forget, sorry. the filename moving is ok IMHO, mplayer & mencoder should either both or none have a global filename.
also spliting is always better than not spliting. And the more independant patches are the quicker some will be applied
Attached filename moving patch.
[...]
--- libmpcodecs/vf_screenshot.c 2008-11-15 01:50:22.000000000 +0300 +++ ../mplayerb/libmpcodecs/vf_screenshot.c 2008-11-15 02:09:52.000000000 +0300 @@ -157,7 +157,7 @@ /* allocate space to handle "shotXXXX.png" filenames */ priv->fname = malloc(SHOT_FNAME_LENGTH); do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + snprintf (priv->fname, SHOT_FNAME_LENGTH, "shot%04d.png", ++priv->frameno); } while (fexists(priv->fname) && priv->frameno < 100000); if (fexists(priv->fname)) { priv->fname[0] = '\0';
if that didnt depend on the other patch, it would be ok
Sorry I don't understand you. Should I send this path before the main one? Attached in such case. -- Regards, savrus
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@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@gmx.at> wrote:
On Fri, Nov 14, 2008 at 10:24:47PM +0300, Ruslan Savchenko wrote: [...]
also does this code work when there are several files on the command line? or does it always end up using the first filename?
It works with several files. You can even specify different options as in here: mplayer video1.mkv -vf screenshot=1 video2.mkv video3.mkv -vf screenshot I entered 's' once in each file, this produced: *** screenshot 'video1.mkv.[00:00:00.46].png' *** *** screenshot 'video2.mkv.[00:02:30.38].png' *** *** screenshot 'shot0002.png' ***
(shot0002.png is because i've already had shot0001.png in my directory)
Why shot000?.png instead of video3.mkv.[00:XX:YY.ZZ].png ?
Because of the last -vf screenshot. Without it "video3.mkv.[00:XX:YY.ZZ].png" comes out.
+ /* 8 is length of ".[].png" plus '\0' */ + n = strlen(priv->fnamebase) + strlen(tstamp) + 8;
+ priv->fname = malloc(n); + snprintf(priv->fname, n, "%s.[%s].png", priv->fnamebase, tstamp);
did i miss the free() or is this a memleak?
I believe i do free() in uninit():
is uninit() called after each screenshot ?
Oops, my bad. I will start fixing this memleak right now.
right, mplayer doesnt use indention, how could i forget, sorry. the filename moving is ok IMHO, mplayer & mencoder should either both or none have a global filename.
also spliting is always better than not spliting. And the more independant patches are the quicker some will be applied
Attached filename moving patch.
[...]
--- libmpcodecs/vf_screenshot.c 2008-11-15 01:50:22.000000000 +0300 +++ ../mplayerb/libmpcodecs/vf_screenshot.c 2008-11-15 02:09:52.000000000 +0300 @@ -157,7 +157,7 @@ /* allocate space to handle "shotXXXX.png" filenames */ priv->fname = malloc(SHOT_FNAME_LENGTH); do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + snprintf (priv->fname, SHOT_FNAME_LENGTH, "shot%04d.png", ++priv->frameno); } while (fexists(priv->fname) && priv->frameno < 100000); if (fexists(priv->fname)) { priv->fname[0] = '\0';
if that didnt depend on the other patch, it would be ok
Sorry I don't understand you. Should I send this path before the main one? Attached in such case.
yes thats what ive meant both attached patches look ok IMHO [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus
2008/11/15 Michael Niedermayer <michaelni@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@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@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? -- Regards, savrus
On Mon, Nov 17, 2008 at 9:10 PM, Ruslan Savchenko <ruslan.savchenko@gmail.com> wrote:
2008/11/15 Michael Niedermayer <michaelni@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@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@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. -- Regards, savrus
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@gmail.com> wrote:
2008/11/15 Michael Niedermayer <michaelni@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@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@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. [...]
+ break; + default: + case 0: + /* Enough memory for "shotXXXXX.png" filename */ + priv->fname = malloc(SHOT_FNAME_LENGTH); + if (!priv->fname) { + mp_msg(MSGT_VFILTER,MSGL_ERR,"Unable to allocate memory in vf_screenshot.c\n"); + return; + } do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + snprintf (priv->fname, SHOT_FNAME_LENGTH, "shot%04d.png", ++priv->frameno); } while (fexists(priv->fname) && priv->frameno < 100000);
please avoid mixing tabs and spaces in indention also a patch changing all tabs in vf_screenshot to spaces is probably welcome, at least id approve it if noone objects. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
On Sat, Nov 22, 2008 at 12:56 AM, Michael Niedermayer <michaelni@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@gmail.com> wrote:
2008/11/15 Michael Niedermayer <michaelni@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@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@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 ...
Please do not misunderstand. I just wanted to show what I want to do next in case those two one-line patches will be applied.
Actually i would suggest that you split the filename and timestamp in name things in 2 patches as well.
I don't understand what do you want me to do. Split patch into many little ones and send them all in one email? I can do this but using diff, not svn diff.
[...]
+ break; + default: + case 0: + /* Enough memory for "shotXXXXX.png" filename */ + priv->fname = malloc(SHOT_FNAME_LENGTH); + if (!priv->fname) { + mp_msg(MSGT_VFILTER,MSGL_ERR,"Unable to allocate memory in vf_screenshot.c\n"); + return; + } do { - snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno); + snprintf (priv->fname, SHOT_FNAME_LENGTH, "shot%04d.png", ++priv->frameno); } while (fexists(priv->fname) && priv->frameno < 100000);
please avoid mixing tabs and spaces in indention also a patch changing all tabs in vf_screenshot to spaces is probably welcome, at least id approve it if noone objects.
Well, I'll try to make this one against the current svn. -- Regards, savrus
On Sat, Nov 22, 2008 at 12:56 AM, Michael Niedermayer <michaelni@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@gmail.com> wrote:
2008/11/15 Michael Niedermayer <michaelni@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@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@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. 0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers. 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. 0004-Add-m_option-structure-for-new-options-in-further-patches.patch Initialize options parser. Options will be introduced in further patches 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" 0006-cosmetics-in-gen_fname.patch Move old code to appropriate indentation inside switch. 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. 0008-Update-screenshot-section-in-man-page.patch Describe "style" and "basename" options to vf_screenshot in man page. -- Regards, savrus
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@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@gmail.com> wrote:
2008/11/15 Michael Niedermayer <michaelni@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@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@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
On Tue, Nov 25, 2008 at 10:19 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Sun, Nov 23, 2008 at 07:59:26PM +0300, Ruslan Savchenko wrote:
@@ -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
I don't know mplayer design so i was cautious if control would be taken off from gen_fname() right after priv->fname allocation and then given to vf uninitialisze code. But if you say this never happens then that free() is futile.
[...]
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
Is the following code ok? ipts handles 2^32 msecs which is more than 490 days video length. long int ipts; uint32_t hour, min, sec, msec; ipts = lrint(pts * 100); hour = (uint32_t) (ipts/360000); min = (uint32_t) (ipts/6000) % 60; sec = (uint32_t) (ipts/100) % 60; msec = (uint32_t) (ipts) % 100;
[...]
@@ -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
OK. My bad, didn't notice this. -- Regards, savrus
On Tue, Nov 25, 2008 at 11:26:49PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:19 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Sun, Nov 23, 2008 at 07:59:26PM +0300, Ruslan Savchenko wrote: [...]
[...]
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
Is the following code ok? ipts handles 2^32 msecs which is more than 490 days video length.
long int ipts; uint32_t hour, min, sec, msec; ipts = lrint(pts * 100); hour = (uint32_t) (ipts/360000); min = (uint32_t) (ipts/6000) % 60; sec = (uint32_t) (ipts/100) % 60; msec = (uint32_t) (ipts) % 100;
i would use a int64_t in there except that it does look ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Sat, Nov 29, 2008 at 12:47 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Tue, Nov 25, 2008 at 11:26:49PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:19 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Sun, Nov 23, 2008 at 07:59:26PM +0300, Ruslan Savchenko wrote: Is the following code ok? ipts handles 2^32 msecs which is more than 490 days video length.
long int ipts; uint32_t hour, min, sec, msec; ipts = lrint(pts * 100); hour = (uint32_t) (ipts/360000); min = (uint32_t) (ipts/6000) % 60; sec = (uint32_t) (ipts/100) % 60; msec = (uint32_t) (ipts) % 100;
i would use a int64_t in there except that it does look ok
Updated patches attached. Since Uoti objected the usage of the global filename, only "shot" and user-specified basename are left. I will highly appreciate if someone point me how passing the filename into vf should be implemented. -- Regards, savrus
On Sun, Nov 30, 2008 at 03:18:03AM +0300, Ruslan Savchenko wrote:
On Sat, Nov 29, 2008 at 12:47 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Tue, Nov 25, 2008 at 11:26:49PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:19 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Sun, Nov 23, 2008 at 07:59:26PM +0300, Ruslan Savchenko wrote: Is the following code ok? ipts handles 2^32 msecs which is more than 490 days video length.
long int ipts; uint32_t hour, min, sec, msec; ipts = lrint(pts * 100); hour = (uint32_t) (ipts/360000); min = (uint32_t) (ipts/6000) % 60; sec = (uint32_t) (ipts/100) % 60; msec = (uint32_t) (ipts) % 100;
i would use a int64_t in there except that it does look ok
Updated patches attached. Since Uoti objected the usage of the global filename, only "shot" and user-specified basename are left. I will highly appreciate if someone point me how passing the filename into vf should be implemented.
somehow by adding a field to mp_image_t, this might be a pointer to some other struct though or filename directly or even better by helping us finish libavfilter :) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote:
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.
I still think this is a bad idea. A filter shouldn't rely on such globals existing. What if the "file" is an URL? Standard input? You'd need some kind of policy for naming the output, and it does not really belong in a filter. If functionality like this is really required then I think there should be top-level code deciding what subdirectory or prefix to use for all output related to a file, whatever filter or other part generates it.
In mplayer.c 'filename' is already global.
I've already removed that global in my git tree.
0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
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.
Seems the filename is only used immediately after being generated and there is no need to allocate permanent storage for it. It should be possible to store it in a local variable with much simpler code.
On Tue, Nov 25, 2008 at 10:44 PM, Uoti Urpala <uoti.urpala@pp1.inet.fi> wrote:
On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote:
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.
I still think this is a bad idea. A filter shouldn't rely on such globals existing. What if the "file" is an URL? Standard input? You'd need some kind of policy for naming the output, and it does not really belong in a filter.
If "://" appears in the global filename, then "shot" is used, not filename.
If functionality like this is really required then I think there should be top-level code deciding what subdirectory or prefix to use for all output related to a file, whatever filter or other part generates it.
I'll look into such solution, but I think the design (at least idea) should be done by more experienced people than me.
In mplayer.c 'filename' is already global.
I've already removed that global in my git tree.
0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
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.
Seems the filename is only used immediately after being generated and there is no need to allocate permanent storage for it. It should be possible to store it in a local variable with much simpler code.
Yes, but the question is what length should be priv->fname? I used 256 in very the first version of vf_screenshot patch but it seemed this number didn't satisfy everyone. Even I once had a video file with file name length more than 256 chars. -- Regards, savrus
On Tue, Nov 25, 2008 at 11:40:09PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:44 PM, Uoti Urpala <uoti.urpala@pp1.inet.fi> wrote:
On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote: [...]
In mplayer.c 'filename' is already global.
I've already removed that global in my git tree.
0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
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.
Seems the filename is only used immediately after being generated and there is no need to allocate permanent storage for it. It should be possible to store it in a local variable with much simpler code.
Yes, but the question is what length should be priv->fname? I used 256 in very the first version of vf_screenshot patch but it seemed this number didn't satisfy everyone. Even I once had a video file with file name length more than 256 chars.
question is why is it in the context instead of a local var? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
On Mon, Dec 1, 2008 at 1:19 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Tue, Nov 25, 2008 at 11:40:09PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:44 PM, Uoti Urpala <uoti.urpala@pp1.inet.fi> wrote:
On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote: [...]
In mplayer.c 'filename' is already global.
I've already removed that global in my git tree.
0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
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.
Seems the filename is only used immediately after being generated and there is no need to allocate permanent storage for it. It should be possible to store it in a local variable with much simpler code.
Yes, but the question is what length should be priv->fname? I used 256 in very the first version of vf_screenshot patch but it seemed this number didn't satisfy everyone. Even I once had a video file with file name length more than 256 chars.
question is why is it in the context instead of a local var?
That's how it was originally. But I don't see how storing it in a local var would simplify the code. Do you mean just by changing malloc() to alloca() (but man pages say it's usage is discouraged)? Maybe I don't understand something again. -- Regards, savrus
On Mon, Dec 01, 2008 at 07:42:34AM +0300, Ruslan Savchenko wrote:
On Mon, Dec 1, 2008 at 1:19 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Tue, Nov 25, 2008 at 11:40:09PM +0300, Ruslan Savchenko wrote:
On Tue, Nov 25, 2008 at 10:44 PM, Uoti Urpala <uoti.urpala@pp1.inet.fi> wrote:
On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote: [...]
In mplayer.c 'filename' is already global.
I've already removed that global in my git tree.
0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
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.
Seems the filename is only used immediately after being generated and there is no need to allocate permanent storage for it. It should be possible to store it in a local variable with much simpler code.
Yes, but the question is what length should be priv->fname? I used 256 in very the first version of vf_screenshot patch but it seemed this number didn't satisfy everyone. Even I once had a video file with file name length more than 256 chars.
question is why is it in the context instead of a local var?
That's how it was originally. But I don't see how storing it in a local var would simplify the code.
putting stuff that is just local semantically in the context is confusing even if not simpler i think it would be more readable
Do you mean just by changing malloc() to alloca() (but man pages say it's usage is discouraged)? Maybe I don't understand something again.
i did not mean alloca(), if i suggested something like that it would be a variable sized ISO C array, but iam a little afraid of these with user controlable sizes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is not what we do, but why we do it that matters.
participants (4)
-
Attila Kinali -
Michael Niedermayer -
Ruslan Savchenko -
Uoti Urpala