From 1660a065d3752e7b058c91b5afea7e5307fe3649 Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 3 Nov 2012 15:49:25 -0500 Subject: [PATCH 01/10] typo fix, unused variable cleanup, use type in ftp_send_type, etc ffmpeg.c "and" at the end and start of the line motion.c bad spelling, to "an image" jpegutils.c hsf was set but not used netcam_ftp.c type was converted to upper case but not used, the function says it takes an I or A, as since only I is passed the end result is the same webhttpd.c consolidate the timeout to the top of the file as I needed to change it for testing --- ffmpeg.c | 2 +- jpegutils.c | 5 ++--- motion.c | 4 ++-- netcam_ftp.c | 2 +- webhttpd.c | 9 ++++++--- 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index 57eeda4..86bdb8d 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -28,7 +28,7 @@ # warning Your version of FFmpeg is newer than version 0.4.8 # warning Newer versions of ffmpeg do not support MPEG1 with # warning non-standard framerate. MPEG1 will be disabled for -# warning normal video output. You can still use mpeg4 and +# warning normal video output. You can still use mpeg4 # warning and mpeg4ms which are both better in terms of size # warning and quality. MPEG1 is always used for timelapse. # warning Please read the Motion Guide for more information. diff --git a/jpegutils.c b/jpegutils.c index 9c9bc44..1622748 100644 --- a/jpegutils.c +++ b/jpegutils.c @@ -770,7 +770,7 @@ int decode_jpeg_gray_raw(unsigned char *jpeg_data, int len, unsigned int height, unsigned char *raw0, unsigned char *raw1, unsigned char *raw2) { - int numfields, hsf[3], field, yl, yc, xsl, xsc, xs, xd, hdown; + int numfields, field, yl, yc, xsl, xsc, xs, xd, hdown; unsigned int x, y, vsf[3]; JSAMPROW row0[16] = { buf0[0], buf0[1], buf0[2], buf0[3], @@ -818,8 +818,7 @@ int decode_jpeg_gray_raw(unsigned char *jpeg_data, int len, guarantee_huff_tables(&dinfo); jpeg_start_decompress (&dinfo); - hsf[0] = 1; hsf[1] = 1; hsf[2] = 1; - vsf[0]= 1; vsf[1] = 1; vsf[2] = 1; + vsf[0] = 1; vsf[1] = 1; vsf[2] = 1; /* Height match image height or be exact twice the image height. */ diff --git a/motion.c b/motion.c index 5666770..66cdf52 100644 --- a/motion.c +++ b/motion.c @@ -173,7 +173,7 @@ static void image_ring_destroy(struct context *cnt) /** * image_save_as_preview * - * This routine is called when we detect motion and want to save a image in the preview buffer + * This routine is called when we detect motion and want to save an image in the preview buffer * * Parameters: * @@ -2752,7 +2752,7 @@ int main (int argc, char **argv) SLEEP(1, 0); /* - * Calculate how many threads runnig or wants to run + * Calculate how many threads are running or wants to run * if zero and we want to finish, break out */ int motion_threads_running = 0; diff --git a/netcam_ftp.c b/netcam_ftp.c index 0a129b2..99f361a 100644 --- a/netcam_ftp.c +++ b/netcam_ftp.c @@ -795,7 +795,7 @@ int ftp_send_type(ftp_context_pointer ctxt, char type) utype = toupper(type); /* Assure transfer will be in "image" mode. */ - snprintf(buf, sizeof(buf), "TYPE I\r\n"); + snprintf(buf, sizeof(buf), "TYPE %c\r\n", utype); len = strlen(buf); res = send(ctxt->control_file_desc, buf, len, 0); diff --git a/webhttpd.c b/webhttpd.c index 20fe4e8..42fd7d8 100644 --- a/webhttpd.c +++ b/webhttpd.c @@ -16,6 +16,9 @@ #include #include +/* Timeout in seconds, used for read and write */ +const int NONBLOCK_TIMEOUT = 1; + pthread_mutex_t httpd_mutex; // This is a dummy variable use to kill warnings when not checking sscanf and similar functions @@ -197,7 +200,7 @@ static ssize_t write_nonblock(int fd, const void *buf, size_t size) struct timeval tm; fd_set fds; - tm.tv_sec = 1; /* Timeout in seconds */ + tm.tv_sec = NONBLOCK_TIMEOUT; tm.tv_usec = 0; FD_ZERO(&fds); FD_SET(fd, &fds); @@ -223,7 +226,7 @@ static ssize_t read_nonblock(int fd ,void *buf, ssize_t size) struct timeval tm; fd_set fds; - tm.tv_sec = 1; /* Timeout in seconds */ + tm.tv_sec = NONBLOCK_TIMEOUT; /* Timeout in seconds */ tm.tv_usec = 0; FD_ZERO(&fds); FD_SET(fd, &fds); @@ -2502,7 +2505,7 @@ void httpd_run(struct context **cnt) while ((client_sent_quit_message) && (!closehttpd)) { - client_socket_fd = acceptnonblocking(sd, 1); + client_socket_fd = acceptnonblocking(sd, NONBLOCK_TIMEOUT); if (client_socket_fd < 0) { if ((!cnt[0]) || (cnt[0]->finish)) { From 3eb5e79339d713988b337d45028eaf9518578321 Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 8 Nov 2012 22:26:17 -0600 Subject: [PATCH 02/10] set motion capture thread running in the main thread I identified this while debugging, the thread was created, but hadn't yet set its state to running before the main thread checked the running variable and started another thread for the same device. That resulted in a crash. Set running in the main thread, to avoid this race condition. --- motion.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/motion.c b/motion.c index 66cdf52..e79e7d7 100644 --- a/motion.c +++ b/motion.c @@ -1108,8 +1108,6 @@ static void *motion_loop(void *arg) * is acted upon. */ unsigned long int time_last_frame = 1, time_current_frame; - - cnt->running = 1; if (motion_init(cnt) < 0) goto err; @@ -2623,11 +2621,22 @@ static void start_motion_thread(struct context *cnt, pthread_attr_t *thread_attr /* Give the thread WATCHDOG_TMO to start */ cnt->watchdog = WATCHDOG_TMO; + /* Flag it as running outside of the thread, otherwise if the main loop + * checked if it is was running before the thread set it to 1, it would + * start another thread for this device. */ + cnt->running = 1; + /* * Create the actual thread. Use 'motion_loop' as the thread * function. */ - pthread_create(&cnt->thread_id, thread_attr, &motion_loop, cnt); + if (pthread_create(&cnt->thread_id, thread_attr, &motion_loop, cnt)) { + /* thread create failed, undo running state */ + cnt->running = 0; + pthread_mutex_lock(&global_lock); + threads_running--; + pthread_mutex_unlock(&global_lock); + } } /** From 68c671acbe67f7e4378f6a3e07113e3b599bbdec Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 8 Nov 2012 23:14:29 -0600 Subject: [PATCH 03/10] count webcontrol as a thread to avoid a crash On a SIGHUP restart the main thread waits to restart until all worker threads are finished executing, except webcontrol wasn't included. If it was still running (such as reading a web request), it would have a dangling context pointer after cnt_list was freed leading to a crash. This counts that thread in the running threads, as a termination webcontrol_finish variable to terminate it indepdent of the device thread, and creates a webcontrol_running to identify when it is running. --- CHANGELOG | 1 + CREDITS | 3 +++ motion.c | 23 ++++++++++++++++++++--- motion.h | 3 +++ webhttpd.c | 13 ++++++++++++- 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 10ca2c2..9ac2de0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -84,6 +84,7 @@ Bugfixes * Fixed leak in vloopback. * Fixed a build of motion for some kernel version with not good videodev.h * Netcam Modulo 8 + * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter (David Fries) 3.2.12 Summary of Changes diff --git a/CREDITS b/CREDITS index 89390a7..d21549b 100644 --- a/CREDITS +++ b/CREDITS @@ -473,6 +473,9 @@ Mark Feenstra Miguel Freitas * Came up with the round robing idea. +David Fries + * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter + Aaron Gage * Pointed me to the vid_mmap/int problem when calling SYNC in video.c diff --git a/motion.c b/motion.c index e79e7d7..0c7b774 100644 --- a/motion.c +++ b/motion.c @@ -340,6 +340,7 @@ static void sig_handler(int signo) while (cnt_list[++i]) { cnt_list[i]->makemovie = 1; cnt_list[i]->finish = 1; + cnt_list[i]->webcontrol_finish = 1; /* * Don't restart thread when it ends, * all threads restarts if global restart is set @@ -2747,8 +2748,21 @@ int main (int argc, char **argv) * Create a thread for the control interface if requested. Create it * detached and with 'motion_web_control' as the thread function. */ - if (cnt_list[0]->conf.webcontrol_port) - pthread_create(&thread_id, &thread_attr, &motion_web_control, cnt_list); + if (cnt_list[0]->conf.webcontrol_port) { + pthread_mutex_lock(&global_lock); + threads_running++; + /* set outside the loop to avoid thread set vs main thread check */ + cnt_list[0]->webcontrol_running = 1; + pthread_mutex_unlock(&global_lock); + if (pthread_create(&thread_id, &thread_attr, &motion_web_control, + cnt_list)) { + /* thread create failed, undo running state */ + pthread_mutex_lock(&global_lock); + threads_running--; + cnt_list[0]->webcontrol_running = 0; + pthread_mutex_unlock(&global_lock); + } + } MOTION_LOG(NTC, TYPE_ALL, NO_ERRNO, "%s: Waiting for threads to finish, pid: %d", getpid()); @@ -2770,6 +2784,9 @@ int main (int argc, char **argv) if (cnt_list[i]->running || cnt_list[i]->restart) motion_threads_running++; } + if (cnt_list[0]->conf.webcontrol_port && + cnt_list[0]->webcontrol_running) + motion_threads_running++; if (((motion_threads_running == 0) && finish) || ((motion_threads_running == 0) && (threads_running == 0))) { @@ -2829,7 +2846,7 @@ int main (int argc, char **argv) // Be sure that http control exits fine - cnt_list[0]->finish = 1; + cnt_list[0]->webcontrol_finish = 1; SLEEP(1, 0); MOTION_LOG(NTC, TYPE_ALL, NO_ERRNO, "%s: Motion terminating"); diff --git a/motion.h b/motion.h index c08d84f..567feac 100644 --- a/motion.h +++ b/motion.h @@ -370,6 +370,9 @@ struct context { volatile unsigned int restart; /* Restart the thread when it ends */ /* Is the motion thread running */ volatile unsigned int running; + /* Is the web control thread running */ + volatile unsigned int webcontrol_running; + volatile unsigned int webcontrol_finish; /* End the thread */ volatile int watchdog; pthread_t thread_id; diff --git a/webhttpd.c b/webhttpd.c index 42fd7d8..bba649e 100644 --- a/webhttpd.c +++ b/webhttpd.c @@ -2508,7 +2508,7 @@ void httpd_run(struct context **cnt) client_socket_fd = acceptnonblocking(sd, NONBLOCK_TIMEOUT); if (client_socket_fd < 0) { - if ((!cnt[0]) || (cnt[0]->finish)) { + if ((!cnt[0]) || (cnt[0]->webcontrol_finish)) { MOTION_LOG(NTC, TYPE_STREAM, NO_ERRNO, "%s: motion-httpd - Finishing"); closehttpd = 1; } @@ -2539,6 +2539,17 @@ void *motion_web_control(void *arg) { struct context **cnt = arg; httpd_run(cnt); + + /* + * Update how many threads we have running. This is done within a + * mutex lock to prevent multiple simultaneous updates to + * 'threads_running'. + */ + pthread_mutex_lock(&global_lock); + threads_running--; + cnt[0]->webcontrol_running = 0; + pthread_mutex_unlock(&global_lock); + MOTION_LOG(NTC, TYPE_STREAM, NO_ERRNO, "%s: motion-httpd thread exit"); pthread_exit(NULL); } From 088ab0c06cdab9886bbb4010f8c5608a3ed45780 Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 14 Aug 2014 20:43:19 -0500 Subject: [PATCH 04/10] redo watchdog termination logic, wait for the thread to cleanup I apparently have some marginal USB hardware and motion has been crashing every couple of days in the memcpy at the end of alg.c as both cnt->imgs.ref and cnt->imgs.image_virgin were NULL pointers. This was just after the main thread watchdog timer called pthread_cancel on that thread. The problem is pthread_cancel can't possibly kill a thread running on another CPU atomically, although in this case it's a single core processor and I think pthread_cancel was releasing it from the ioctl and it would then run to completition. This modifies the code to not cleanup that content's resources until that thread is no longer running. If it runs to completition a normal restart will happen, if it doesn't the main thread will check once a second until it no longer is running, cleanup and restart, but it can't cleanup with that thread running. --- motion.c | 49 ++++++++++++++++++++++++++++++++----------------- motion.h | 1 + 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/motion.c b/motion.c index 0c7b774..bd4fc2d 100644 --- a/motion.c +++ b/motion.c @@ -2804,24 +2804,39 @@ int main (int argc, char **argv) } if (cnt_list[i]->watchdog > WATCHDOG_OFF) { - cnt_list[i]->watchdog--; - - if (cnt_list[i]->watchdog == 0) { - MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, trying to do " - "a graceful restart", cnt_list[i]->threadnr); - cnt_list[i]->finish = 1; - } + if (cnt_list[i]->watchdog == WATCHDOG_KILL) { + /* if 0 then it finally did clean up (and will restart without any further action here) + * kill(, 0) == ESRCH means the thread is no longer running + * if it is no longer running with running set, then cleanup here so it can restart + */ + if(cnt_list[i]->running && pthread_kill(cnt_list[i]->thread_id, 0) == ESRCH) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: cleaning Thread %d", cnt_list[i]->threadnr); + pthread_mutex_lock(&global_lock); + threads_running--; + pthread_mutex_unlock(&global_lock); + motion_cleanup(cnt_list[i]); + cnt_list[i]->running = 0; + cnt_list[i]->finish = 0; + } + } else { + cnt_list[i]->watchdog--; + + + if (cnt_list[i]->watchdog == 0) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, trying to do " + "a graceful restart", cnt_list[i]->threadnr); + cnt_list[i]->finish = 1; + } - if (cnt_list[i]->watchdog == -60) { - MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, did NOT restart graceful," - "killing it!", cnt_list[i]->threadnr); - pthread_cancel(cnt_list[i]->thread_id); - pthread_mutex_lock(&global_lock); - threads_running--; - pthread_mutex_unlock(&global_lock); - motion_cleanup(cnt_list[i]); - cnt_list[i]->running = 0; - cnt_list[i]->finish = 0; + if (cnt_list[i]->watchdog == WATCHDOG_KILL) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, did NOT restart graceful," + "killing it!", cnt_list[i]->threadnr); + /* The problem is pthead_cancel might just wake up the thread so it runs to completition + * or it might not. In either case don't rip the carpet out under it by + * doing motion_cleanup until it no longer is running. + */ + pthread_cancel(cnt_list[i]->thread_id); + } } } } diff --git a/motion.h b/motion.h index 567feac..65d2698 100644 --- a/motion.h +++ b/motion.h @@ -149,6 +149,7 @@ */ #define WATCHDOG_TMO 30 /* 30 sec max motion_loop interval */ +#define WATCHDOG_KILL -60 /* -60 sec grace period before calling thread cancel */ #define WATCHDOG_OFF -127 /* Turn off watchdog, used when we wants to quit a thread */ #define CONNECTION_KO "Lost connection" From a39ccc5e6e63a00eef600ececef7a0d2bd6ce493 Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 16 Aug 2014 00:27:28 -0500 Subject: [PATCH 05/10] get the thread out of the xioctl loop As long as errno is EINTR (and that must be the case when the USB device is still there, but the transfers are failing), the thread keeps looping running the ioctl when it isn't going to succeed, so give it access to the finish variable to only loop if the thread isn't supposed to be going away, and then keep sendig SIGVTALRM to break out of the ioctl when the thread is supposed to be exiting. With this fix the webcam was no longer crashing, which let the webcam without a problem continue to stream. --- motion.c | 10 ++++++++++ video2.c | 56 +++++++++++++++++++++++++++++--------------------------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/motion.c b/motion.c index bd4fc2d..df81ae9 100644 --- a/motion.c +++ b/motion.c @@ -356,6 +356,9 @@ static void sig_handler(int signo) break; case SIGSEGV: exit(0); + case SIGVTALRM: + printf("SIGVTALRM went off\n"); + break; } } @@ -2553,6 +2556,10 @@ static void setup_signals(struct sigaction *sig_handler_action, struct sigaction sigaction(SIGQUIT, sig_handler_action, NULL); sigaction(SIGTERM, sig_handler_action, NULL); sigaction(SIGUSR1, sig_handler_action, NULL); + + /* use SIGVTALRM as a way to break out of the ioctl, don't restart */ + sig_handler_action->sa_flags = 0; + sigaction(SIGVTALRM, sig_handler_action, NULL); } /** @@ -2817,6 +2824,9 @@ int main (int argc, char **argv) motion_cleanup(cnt_list[i]); cnt_list[i]->running = 0; cnt_list[i]->finish = 0; + } else { + /* keep sending signals so it doesn't get stuck in a blocking call */ + pthread_kill(cnt_list[i]->thread_id, SIGVTALRM); } } else { cnt_list[i]->watchdog--; diff --git a/video2.c b/video2.c index 9896edb..32d80c1 100644 --- a/video2.c +++ b/video2.c @@ -173,19 +173,20 @@ typedef struct { u32 ctrl_flags; struct v4l2_queryctrl *controls; + volatile unsigned int *finish; /* End the thread */ } src_v4l2_t; /** * xioctl */ -static int xioctl(int fd, int request, void *arg) +static int xioctl(src_v4l2_t *vid_source, int request, void *arg) { int ret; do - ret = ioctl(fd, request, arg); - while (-1 == ret && EINTR == errno); + ret = ioctl(vid_source->fd, request, arg); + while (-1 == ret && EINTR == errno && !vid_source->finish); return ret; } @@ -195,7 +196,7 @@ static int xioctl(int fd, int request, void *arg) */ static int v4l2_get_capability(src_v4l2_t * vid_source) { - if (xioctl(vid_source->fd, VIDIOC_QUERYCAP, &vid_source->cap) < 0) { + if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) { MOTION_LOG(ERR, TYPE_VIDEO, NO_ERRNO, "%s: Not a V4L2 device?"); return -1; } @@ -258,7 +259,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, input.index = IN_TV; else input.index = in; - if (xioctl(vid_source->fd, VIDIOC_ENUMINPUT, &input) == -1) { + if (xioctl(vid_source, VIDIOC_ENUMINPUT, &input) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Unable to query input %d." " VIDIOC_ENUMINPUT, if you use a WEBCAM change input value in conf by -1", input.index); @@ -274,7 +275,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, if (input.type & V4L2_INPUT_TYPE_CAMERA) MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: - CAMERA"); - if (xioctl(vid_source->fd, VIDIOC_S_INPUT, &input.index) == -1) { + if (xioctl(vid_source, VIDIOC_S_INPUT, &input.index) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error selecting input %d" " VIDIOC_S_INPUT", input.index); return -1; @@ -286,7 +287,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, * Set video standard usually webcams doesn't support the ioctl or * return V4L2_STD_UNKNOWN */ - if (xioctl(vid_source->fd, VIDIOC_G_STD, &std_id) == -1) { + if (xioctl(vid_source, VIDIOC_G_STD, &std_id) == -1) { MOTION_LOG(WRN, TYPE_VIDEO, NO_ERRNO, "%s: Device doesn't support VIDIOC_G_STD"); norm = std_id = 0; // V4L2_STD_UNKNOWN = 0 } @@ -295,7 +296,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, memset(&standard, 0, sizeof(standard)); standard.index = 0; - while (xioctl(vid_source->fd, VIDIOC_ENUMSTD, &standard) == 0) { + while (xioctl(vid_source, VIDIOC_ENUMSTD, &standard) == 0) { if (standard.id & std_id) MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: - video standard %s", standard.name); @@ -314,7 +315,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, std_id = V4L2_STD_PAL; } - if (xioctl(vid_source->fd, VIDIOC_S_STD, &std_id) == -1) + if (xioctl(vid_source, VIDIOC_S_STD, &std_id) == -1) MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error selecting standard" " method %d VIDIOC_S_STD", (int)std_id); @@ -334,7 +335,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, memset(&tuner, 0, sizeof(struct v4l2_tuner)); tuner.index = input.tuner; - if (xioctl(vid_source->fd, VIDIOC_G_TUNER, &tuner) == -1) { + if (xioctl(vid_source, VIDIOC_G_TUNER, &tuner) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: tuner %d VIDIOC_G_TUNER", tuner.index); return 0; @@ -349,7 +350,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, freq.type = V4L2_TUNER_ANALOG_TV; freq.frequency = (freq_ / 1000) * 16; - if (xioctl(vid_source->fd, VIDIOC_S_FREQUENCY, &freq) == -1) { + if (xioctl(vid_source, VIDIOC_S_FREQUENCY, &freq) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: freq %ul VIDIOC_S_FREQUENCY", freq.frequency); return 0; @@ -401,7 +402,7 @@ static int v4l2_do_set_pix_format(u32 pixformat, src_v4l2_t * vid_source, vid_source->dst_fmt.fmt.pix.pixelformat = pixformat; vid_source->dst_fmt.fmt.pix.field = V4L2_FIELD_ANY; - if (xioctl(vid_source->fd, VIDIOC_TRY_FMT, &vid_source->dst_fmt) != -1 && + if (xioctl(vid_source, VIDIOC_TRY_FMT, &vid_source->dst_fmt) != -1 && vid_source->dst_fmt.fmt.pix.pixelformat == pixformat) { MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: Testing palette %c%c%c%c (%dx%d)", pixformat >> 0, pixformat >> 8, @@ -419,7 +420,7 @@ static int v4l2_do_set_pix_format(u32 pixformat, src_v4l2_t * vid_source, *height = vid_source->dst_fmt.fmt.pix.height; } - if (xioctl(vid_source->fd, VIDIOC_S_FMT, &vid_source->dst_fmt) == -1) { + if (xioctl(vid_source, VIDIOC_S_FMT, &vid_source->dst_fmt) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error setting pixel " "format.\nVIDIOC_S_FMT: "); return -1; @@ -498,7 +499,7 @@ static int v4l2_set_pix_format(struct context *cnt, src_v4l2_t * vid_source, MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: Supported palettes:"); - while (xioctl(vid_source->fd, VIDIOC_ENUM_FMT, &fmtd) != -1) { + while (xioctl(vid_source, VIDIOC_ENUM_FMT, &fmtd) != -1) { int i; @@ -552,7 +553,7 @@ static void v4l2_set_fps(src_v4l2_t * vid_source) { setfpvid_source->parm.capture.timeperframe.numerator = 1; setfpvid_source->parm.capture.timeperframe.denominator = vid_source->fps; - if (xioctl(vid_source->fd, VIDIOC_S_PARM, setfps) == -1) + if (xioctl(vid_source, VIDIOC_S_PARM, setfps) == -1) MOTION_LOG(ERR, 1, "%s: v4l2_set_fps VIDIOC_S_PARM"); @@ -577,7 +578,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) vid_source->req.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; vid_source->req.memory = V4L2_MEMORY_MMAP; - if (xioctl(vid_source->fd, VIDIOC_REQBUFS, &vid_source->req) == -1) { + if (xioctl(vid_source, VIDIOC_REQBUFS, &vid_source->req) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error requesting buffers" " %d for memory map. VIDIOC_REQBUFS", vid_source->req.count); @@ -609,7 +610,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) buf.memory = V4L2_MEMORY_MMAP; buf.index = buffer_index; - if (xioctl(vid_source->fd, VIDIOC_QUERYBUF, &buf) == -1) { + if (xioctl(vid_source, VIDIOC_QUERYBUF, &buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error querying buffer" " %i\nVIDIOC_QUERYBUF: ", buffer_index); free(vid_source->buffers); @@ -638,7 +639,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) vid_source->buf.memory = V4L2_MEMORY_MMAP; vid_source->buf.index = buffer_index; - if (xioctl(vid_source->fd, VIDIOC_QBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_QBUF, &vid_source->buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: VIDIOC_QBUF"); return -1; } @@ -646,7 +647,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - if (xioctl(vid_source->fd, VIDIOC_STREAMON, &type) == -1) { + if (xioctl(vid_source, VIDIOC_STREAMON, &type) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error starting stream." " VIDIOC_STREAMON"); return -1; @@ -667,7 +668,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) for (i = 0, count = 0; queried_ctrls[i]; i++) { queryctrl.id = queried_ctrls[i]; - if (xioctl(vid_source->fd, VIDIOC_QUERYCTRL, &queryctrl)) + if (xioctl(vid_source, VIDIOC_QUERYCTRL, &queryctrl)) continue; count++; @@ -688,7 +689,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) struct v4l2_control control; queryctrl.id = queried_ctrls[i]; - if (xioctl(vid_source->fd, VIDIOC_QUERYCTRL, &queryctrl)) + if (xioctl(vid_source, VIDIOC_QUERYCTRL, &queryctrl)) continue; memcpy(ctrl, &queryctrl, sizeof(struct v4l2_queryctrl)); @@ -700,7 +701,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) memset(&control, 0, sizeof (control)); control.id = queried_ctrls[i]; - xioctl(vid_source->fd, VIDIOC_G_CTRL, &control); + xioctl(vid_source, VIDIOC_G_CTRL, &control); MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: \t\"%s\", default %d, current %d", ctrl->name, ctrl->default_value, control.value); @@ -736,12 +737,12 @@ static int v4l2_set_control(src_v4l2_t * vid_source, u32 cid, int value) case V4L2_CTRL_TYPE_INTEGER: value = control.value = (value * (ctrl->maximum - ctrl->minimum) / 256) + ctrl->minimum; - ret = xioctl(vid_source->fd, VIDIOC_S_CTRL, &control); + ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); break; case V4L2_CTRL_TYPE_BOOLEAN: value = control.value = value ? 1 : 0; - ret = xioctl(vid_source->fd, VIDIOC_S_CTRL, &control); + ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); break; default: @@ -818,6 +819,7 @@ unsigned char *v4l2_start(struct context *cnt, struct video_dev *viddev, int wid vid_source->fd = viddev->fd; vid_source->fps = cnt->conf.frame_limit; vid_source->pframe = -1; + vid_source->finish = &cnt->finish; struct config *conf = &cnt->conf; if (v4l2_get_capability(vid_source)) @@ -962,7 +964,7 @@ int v4l2_next(struct context *cnt, struct video_dev *viddev, unsigned char *map, vid_source->pframe); if (vid_source->pframe >= 0) { - if (xioctl(vid_source->fd, VIDIOC_QBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_QBUF, &vid_source->buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: VIDIOC_QBUF"); pthread_sigmask(SIG_UNBLOCK, &old, NULL); return -1; @@ -974,7 +976,7 @@ int v4l2_next(struct context *cnt, struct video_dev *viddev, unsigned char *map, vid_source->buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; vid_source->buf.memory = V4L2_MEMORY_MMAP; - if (xioctl(vid_source->fd, VIDIOC_DQBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_DQBUF, &vid_source->buf) == -1) { int ret; /* * Some drivers return EIO when there is no signal, @@ -1079,7 +1081,7 @@ void v4l2_close(struct video_dev *viddev) enum v4l2_buf_type type; type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - xioctl(vid_source->fd, VIDIOC_STREAMOFF, &type); + xioctl(vid_source, VIDIOC_STREAMOFF, &type); close(vid_source->fd); vid_source->fd = -1; } From d2a9d34acbc016563115fffc9177ebcd345f415b Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 23 Aug 2014 14:04:22 -0500 Subject: [PATCH 06/10] fix dangling pointer cnt->current_image after resize cnt->current_image because a dangling pointer after image_ring_resize because it is pointing to cnt->imgs.image_ring which is reallocated in that routine. motion_loop will then store cnt->current_image in old_image which it can then read from. Reallocations are rare, once in init to size 1, then once to the final size. I apparently have a bad USB link and I was seeing a crash pointing to bad data, after that camera started, then had an error and crashed in process_image_ring(cnt, IMAGE_BUFFER_FLUSH); it hadn't yet resized to the normal ring buffer size. That got me trying valgrind with a ring buffer size limit of 1 which found this bug. --- motion.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/motion.c b/motion.c index df81ae9..83c11e2 100644 --- a/motion.c +++ b/motion.c @@ -133,6 +133,7 @@ static void image_ring_resize(struct context *cnt, int new_size) /* Point to the new ring */ cnt->imgs.image_ring = tmp; + cnt->current_image = NULL; cnt->imgs.image_ring_size = new_size; } @@ -167,6 +168,7 @@ static void image_ring_destroy(struct context *cnt) free(cnt->imgs.image_ring); cnt->imgs.image_ring = NULL; + cnt->current_image = NULL; cnt->imgs.image_ring_size = 0; } From 4e4aefed3e1d9e7bd1f772a47c208168f6811c3e Mon Sep 17 00:00:00 2001 From: David Fries Date: Mon, 28 Jul 2014 23:27:53 -0500 Subject: [PATCH 07/10] alg_labeling give diffs feedback even when not over the threshold This is especially useful when picking a threshold to see how close to the threshold it was. --- alg.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/alg.c b/alg.c index 93c260f..8890940 100644 --- a/alg.c +++ b/alg.c @@ -526,6 +526,8 @@ static int alg_labeling(struct context *cnt) int height = imgs->height; int labelsize = 0; int current_label = 2; + /* Keep track of the area just under the threshold. */ + int max_under = 0; cnt->current_image->total_labels = 0; imgs->labelsize_max = 0; @@ -561,7 +563,8 @@ static int alg_labeling(struct context *cnt) labelsize = iflood(ix, iy, width, height, out, labels, current_label + 32768, current_label); imgs->labelgroup_max += labelsize; imgs->labels_above++; - } + } else if(max_under < labelsize) + max_under = labelsize; if (imgs->labelsize_max < labelsize) { imgs->labelsize_max = labelsize; @@ -579,8 +582,11 @@ static int alg_labeling(struct context *cnt) "Largest Label: %i", imgs->largest_label, imgs->labelsize_max, cnt->current_image->total_labels); - /* Return group of significant labels. */ - return imgs->labelgroup_max; + /* Return group of significant labels or if that's none, the next largest + * group (which is under the threshold, but especially for setup gives an + * idea how close it was). + */ + return imgs->labelgroup_max ? imgs->labelgroup_max : max_under; } /** From 385df115fbf322a68ef6b11f6bb8f022e46c1d13 Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 8 Nov 2012 23:59:32 -0600 Subject: [PATCH 08/10] accept a width specifier to mystrftime Allow the user to specify field widths to keep the strings from jumping around as the width changes. This makes the values easier to read. --- CHANGELOG | 1 + CREDITS | 1 + motion.c | 42 +++++++++++++++++++++++++++--------------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 9ac2de0..fe31cc4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -49,6 +49,7 @@ Features http://www.lavrsen.dk/foswiki/bin/view/Motion/OggTimelapse (Michael Luich) * Added support for ffmpeg 0.11 new API. * Added RSTP support for netcam ( merge https://github.com/hyperbolic2346/motion ) + * Allow text format specifiers to take a width like printf would. (David Fries) Bugfixes * Avoid segfault detecting strerror_r() version GNU or SUSv3. (Angel Carpintero) diff --git a/CREDITS b/CREDITS index d21549b..a656adb 100644 --- a/CREDITS +++ b/CREDITS @@ -475,6 +475,7 @@ Miguel Freitas David Fries * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter + * Allow text format specifiers to take a width like printf would. Aaron Gage * Pointed me to the vid_mmap/int problem when calling SYNC in diff --git a/motion.c b/motion.c index 83c11e2..7bcdcb3 100644 --- a/motion.c +++ b/motion.c @@ -3176,6 +3176,7 @@ size_t mystrftime(const struct context *cnt, char *s, size_t max, const char *us char tempstring[PATH_MAX] = ""; char *format, *tempstr; const char *pos_userformat; + int width; format = formatstring; @@ -3195,6 +3196,12 @@ size_t mystrftime(const struct context *cnt, char *s, size_t max, const char *us */ tempstr = tempstring; tempstr[0] = '\0'; + width = 0; + while ('0' <= pos_userformat[1] && pos_userformat[1] <= '9') { + width *= 10; + width += pos_userformat[1] - '0'; + ++pos_userformat; + } switch (*++pos_userformat) { case '\0': // end of string @@ -3202,73 +3209,78 @@ size_t mystrftime(const struct context *cnt, char *s, size_t max, const char *us break; case 'v': // event - sprintf(tempstr, "%02d", cnt->event_nr); + sprintf(tempstr, "%0*d", width ? width : 2, cnt->event_nr); break; case 'q': // shots - sprintf(tempstr, "%02d", cnt->current_image->shot); + sprintf(tempstr, "%0*d", width ? width : 2, + cnt->current_image->shot); break; case 'D': // diffs - sprintf(tempstr, "%d", cnt->current_image->diffs); + sprintf(tempstr, "%*d", width, cnt->current_image->diffs); break; case 'N': // noise - sprintf(tempstr, "%d", cnt->noise); + sprintf(tempstr, "%*d", width, cnt->noise); break; case 'i': // motion width - sprintf(tempstr, "%d", cnt->current_image->location.width); + sprintf(tempstr, "%*d", width, + cnt->current_image->location.width); break; case 'J': // motion height - sprintf(tempstr, "%d", cnt->current_image->location.height); + sprintf(tempstr, "%*d", width, + cnt->current_image->location.height); break; case 'K': // motion center x - sprintf(tempstr, "%d", cnt->current_image->location.x); + sprintf(tempstr, "%*d", width, cnt->current_image->location.x); break; case 'L': // motion center y - sprintf(tempstr, "%d", cnt->current_image->location.y); + sprintf(tempstr, "%*d", width, cnt->current_image->location.y); break; case 'o': // threshold - sprintf(tempstr, "%d", cnt->threshold); + sprintf(tempstr, "%*d", width, cnt->threshold); break; case 'Q': // number of labels - sprintf(tempstr, "%d", cnt->current_image->total_labels); + sprintf(tempstr, "%*d", width, + cnt->current_image->total_labels); break; case 't': // thread number - sprintf(tempstr, "%d",(int)(unsigned long) + sprintf(tempstr, "%*d", width, (int)(unsigned long) pthread_getspecific(tls_key_threadnr)); break; case 'C': // text_event if (cnt->text_event_string && cnt->text_event_string[0]) - snprintf(tempstr, PATH_MAX, "%s", cnt->text_event_string); + snprintf(tempstr, PATH_MAX, "%*s", width, + cnt->text_event_string); else ++pos_userformat; break; case 'f': // filename -- or %fps if ((*(pos_userformat+1) == 'p') && (*(pos_userformat+2) == 's')) { - sprintf(tempstr, "%d", cnt->movie_fps); + sprintf(tempstr, "%*d", width, cnt->movie_fps); pos_userformat += 2; break; } if (filename) - snprintf(tempstr, PATH_MAX, "%s", filename); + snprintf(tempstr, PATH_MAX, "%*s", width, filename); else ++pos_userformat; break; case 'n': // sqltype if (sqltype) - sprintf(tempstr, "%d", sqltype); + sprintf(tempstr, "%*d", width, sqltype); else ++pos_userformat; break; From fb73273c17502bd32205bb2bda0437c22299124b Mon Sep 17 00:00:00 2001 From: David Fries Date: Fri, 9 Nov 2012 23:54:12 -0600 Subject: [PATCH 09/10] add power_line_frequency configuration item The USB webcam I have defaults to the wrong setting causing visible flickering degrading the image quality. This allows setting the value to any of the currently available options or -1 to by default not modify the value. --- CHANGELOG | 1 + CREDITS | 1 + conf.c | 17 +++++++++++++++++ conf.h | 1 + video.h | 1 + video2.c | 19 +++++++++++++++++++ video_common.c | 2 ++ 7 files changed, 42 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index fe31cc4..c4e7d9e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -50,6 +50,7 @@ Features * Added support for ffmpeg 0.11 new API. * Added RSTP support for netcam ( merge https://github.com/hyperbolic2346/motion ) * Allow text format specifiers to take a width like printf would. (David Fries) + * Add power_line_frequency configuration item to improve image quality. (David Fries) Bugfixes * Avoid segfault detecting strerror_r() version GNU or SUSv3. (Angel Carpintero) diff --git a/CREDITS b/CREDITS index a656adb..02434b1 100644 --- a/CREDITS +++ b/CREDITS @@ -476,6 +476,7 @@ Miguel Freitas David Fries * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter * Allow text format specifiers to take a width like printf would. + * Add power_line_frequency configuration item to improve image quality. Aaron Gage * Pointed me to the vid_mmap/int problem when calling SYNC in diff --git a/conf.c b/conf.c index 2da0da6..f39f448 100644 --- a/conf.c +++ b/conf.c @@ -71,6 +71,7 @@ struct config conf_template = { contrast: 0, saturation: 0, hue: 0, + power_line_frequency: -1, roundrobin_frames: 1, roundrobin_skip: 1, pre_capture: 0, @@ -460,6 +461,22 @@ config_param config_params[] = { print_int }, { + "power_line_frequency", + "# Set the power line frequency to help cancel flicker by compensating\n" + "# for light intensity ripple. (default: -1).\n" + "# This can help reduce power line light flicker.\n" + "# Valuse :\n" + "# do not modify the device setting : -1\n" + "# V4L2_CID_POWER_LINE_FREQUENCY_DISABLED : 0\n" + "# V4L2_CID_POWER_LINE_FREQUENCY_50HZ : 1\n" + "# V4L2_CID_POWER_LINE_FREQUENCY_60HZ : 2\n" + "# V4L2_CID_POWER_LINE_FREQUENCY_AUTO : 3", + 0, + CONF_OFFSET(power_line_frequency), + copy_int, + print_int + }, + { "roundrobin_frames", "\n############################################################\n" "# Round Robin (multiple inputs on same video device name)\n" diff --git a/conf.h b/conf.h index b03397e..455311c 100644 --- a/conf.h +++ b/conf.h @@ -53,6 +53,7 @@ struct config { int contrast; int saturation; int hue; + int power_line_frequency; int roundrobin_frames; int roundrobin_skip; int pre_capture; diff --git a/video.h b/video.h index 12f2928..ee6aa05 100644 --- a/video.h +++ b/video.h @@ -61,6 +61,7 @@ struct video_dev { int contrast; int saturation; int hue; + int power_line_frequency; unsigned long freq; int tuner_number; int fps; diff --git a/video2.c b/video2.c index 32d80c1..68f2de7 100644 --- a/video2.c +++ b/video2.c @@ -144,6 +144,10 @@ static const u32 queried_ctrls[] = { V4L2_CID_CONTRAST, V4L2_CID_SATURATION, V4L2_CID_HUE, +/* first added in Linux kernel v2.6.26 */ +#ifdef V4L2_CID_POWER_LINE_FREQUENCY + V4L2_CID_POWER_LINE_FREQUENCY, +#endif V4L2_CID_RED_BALANCE, V4L2_CID_BLUE_BALANCE, @@ -745,6 +749,13 @@ static int v4l2_set_control(src_v4l2_t * vid_source, u32 cid, int value) ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); break; + case V4L2_CTRL_TYPE_MENU: + /* set as is, no adjustments */ + control.value = value; + ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); + break; + + default: MOTION_LOG(WRN, TYPE_VIDEO, NO_ERRNO, "%s: control type not supported yet"); return -1; @@ -785,6 +796,14 @@ static void v4l2_picture_controls(struct context *cnt, struct video_dev *viddev) v4l2_set_control(vid_source, V4L2_CID_HUE, viddev->hue); } +#ifdef V4L2_CID_POWER_LINE_FREQUENCY + /* -1 is don't modify as 0 is an option to disable the power line filter */ + if (cnt->conf.power_line_frequency != -1 && cnt->conf.power_line_frequency != viddev->power_line_frequency) { + viddev->power_line_frequency = cnt->conf.power_line_frequency; + v4l2_set_control(vid_source, V4L2_CID_POWER_LINE_FREQUENCY, viddev->power_line_frequency); + } +#endif + if (cnt->conf.autobright) { if (vid_do_autobright(cnt, viddev)) { if (v4l2_set_control(vid_source, V4L2_CID_BRIGHTNESS, viddev->brightness)) diff --git a/video_common.c b/video_common.c index 9f8d6a0..79a10a4 100644 --- a/video_common.c +++ b/video_common.c @@ -760,6 +760,8 @@ static int vid_v4lx_start(struct context *cnt) dev->contrast = 0; dev->saturation = 0; dev->hue = 0; + /* -1 is don't modify, (0 is a valid value) */ + dev->power_line_frequency = -1; dev->owner = -1; dev->v4l_fmt = VIDEO_PALETTE_YUV420P; dev->fps = 0; From 2496e21b8a1d4fe10b6cbf2f3b50861384e98dfc Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 5 Jan 2013 16:42:13 -0600 Subject: [PATCH 10/10] add ffmpeg_duplicate_frames option This lets the user decide which video problems they would rather see. In my case a Raspberry Pi 2 with a 640x480 webcam sometimes can keep up with 10fps so I don't want to set the framerate any lower, but then sometimes it can't and with duplicate frames it looks like the video output freezes every second. --- conf.c | 9 +++++++++ conf.h | 1 + motion.c | 7 ++++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/conf.c b/conf.c index f39f448..5fed2da 100644 --- a/conf.c +++ b/conf.c @@ -668,6 +668,15 @@ config_param config_params[] = { print_string }, { + "ffmpeg_duplicate_frames", + "# True to duplicate frames to achieve \"framerate\" fps, but enough\n" + "duplicated frames and the video appears to freeze once a second.", + 0, + CONF_OFFSET(ffmpeg_duplicate_frames), + copy_bool, + print_bool + }, + { "output_debug_pictures", "# Output pictures with only the pixels moving object (ghost images) (default: off)", 0, diff --git a/conf.h b/conf.h index 455311c..04040db 100644 --- a/conf.h +++ b/conf.h @@ -30,6 +30,7 @@ struct config { int max_changes; int threshold_tune; const char *output_pictures; + int ffmpeg_duplicate_frames; int motion_img; int emulate_motion; int event_gap; diff --git a/motion.c b/motion.c index 7bcdcb3..5f0695f 100644 --- a/motion.c +++ b/motion.c @@ -570,8 +570,13 @@ static void process_image_ring(struct context *cnt, unsigned int max_images) /* * Check if we must add any "filler" frames into movie to keep up fps * Only if we are recording videos ( ffmpeg or extenal pipe ) + * While the overall elapsed time might be correct, if there are + * many duplicated frames, say 10 fps, 5 duplicated, the video will + * look like it is frozen every second for half a second. */ - if ((cnt->imgs.image_ring[cnt->imgs.image_ring_out].shot == 0) && + if (!cnt->conf.ffmpeg_duplicate_frames) { + /* don't duplicate frames */ + } else if ((cnt->imgs.image_ring[cnt->imgs.image_ring_out].shot == 0) && #ifdef HAVE_FFMPEG (cnt->ffmpeg_output || (cnt->conf.useextpipe && cnt->extpipe))) { #else