BUG: RoundRobin v4l2 buffers in driver when switching input

Have one card with 4chips and 16 inputs (SAA7134) and noticed that chips that only used 2 inputs needed more skip frames than chips used 4 inputs. Trying a bit I also found that I needed fewer skip frames when I say that I want to have a fps of 5 than 2 (all inputs on one chip). Looked in the code and found that there is always 4 buffers "active" in the driver. This are filled up when a frame come from the camera, and read out py the thread at fps rate. When the roundrobin code changes the input it first sends a command to the camera and after that extracts skip_frames from the driver. It is not checked if the frames recieved are before or after the input change. I have done some changes in the code that first saves the time when we change the input, extract fileds until the field timestamp is after the switch time. I attach one printout of how it can look like.

The driver is a v4l2 so motion uses it.

[3] - video standard PAL-I
[3] - video standard PAL-DK
[3] set_input_skip_frame switch_time=1183669826:487659
[3] got frame before switch timestamp=1183669826:266913
[3] got frame before switch timestamp=1183669826:306913
[3] got frame before switch timestamp=1183669826:346905
[5] v4l2_select_input: name = "Composite1", type 0x00000002, status 00000000

Patch:
Index: video2.c
===================================================================
--- video2.c    (revision 189)
+++ video2.c    (working copy)
@@ -697,6 +697,8 @@
 void v4l2_set_input(struct context *cnt, struct video_dev *viddev, unsigned char *map, int width, int height,
                    struct config *conf)
 {
+       struct timeval switchTime;
+
        int i;
        int input = conf->input;
        int norm = conf->norm;
@@ -709,6 +711,8 @@

                v4l2_select_input((src_v4l2_t *) viddev->v4l2_private, input, norm, freq, tuner_number);

+               gettimeofday(&switchTime, NULL);
+
                v4l2_picture_controls(cnt, viddev);

                viddev->input = input;
@@ -716,8 +720,23 @@
                viddev->height = height;
                viddev->freq = freq;
                viddev->tuner_number = tuner_number;
+
+               /* Skip all frames captured before switchtime, capture 1 after switchtime */
+               {
+                       src_v4l2_t *s = (src_v4l2_t *) viddev->v4l2_private;
+                       motion_log(LOG_DEBUG, 0, "set_input_skip_frame switch_time=%ld:%ld", switchTime.tv_sec, switchTime.tv_usec);
+                       while(1)
+                       {
+                               if(v4l2_next(cnt, viddev, map, width, height))
+                                       break;
+                               if (s->buf.timestamp.tv_sec > switchTime.tv_sec || (s->buf.timestamp.tv_sec == switchTime.tv_sec && s->buf.timestamp.tv_usec > switchTime.tv_usec))
+                                       break;
+                               motion_log(LOG_DEBUG, 0, "got frame before switch timestamp=%ld:%ld", s->buf.timestamp.tv_sec, s->buf.timestamp.tv_usec);
+                       }
+               }
+
                /* skip a few frames if needed */
-               for (i = 0; i < skip; i++)
+               for (i = 1; i < skip; i++)
                        v4l2_next(cnt, viddev, map, width, height);
        } else {
                /* No round robin - we only adjust picture controls */

Environment

Motion version:	SVN 189
ffmpeg version:
Shared libraries:	ffmpeg
Server OS:	Debian, kernel 2.6.18

-- DagErlandsson - 07 Jul 2007

Follow up

Seems like a very good patch.

I would like Angel to check it out also. One thing that concerns me is if the driver fails and v4l2_next never returns true the while loop will make Motion hang. We should implement some measure to make sure that it eventually finishes and cause Motion to terminate later instead of hanging so you have to kill -s 9

-- KennethLavrsen - 08 Jul 2007

The loop exits in two ways, one is that v4l2_next returns true e.g. error. The other way is that the image v4l2_next returned have a timestamp later than switchtime. So the only way to hang in the loop is if v4l2_next doesn't detect driver error and returns an invalid frame where the timestamp is befor switchtime. It is also simple to add a counter that count ddiscarded frames and if it is more than MMAP_BUFFERS break the loop.

-- DagErlandsson - 08 Jul 2007

--- video2.c    (revisión: 195)
+++ video2.c    (copia de trabajo)
@@ -707,8 +707,12 @@
        if (input != viddev->input || width != viddev->width || height != viddev->height ||
            freq != viddev->freq || tuner_number != viddev->tuner_number) {
 
+               struct timeval switchTime;
+
                v4l2_select_input((src_v4l2_t *) viddev->v4l2_private, input, norm, freq, tuner_number);
 
+               gettimeofday(&switchTime, NULL);
+
                v4l2_picture_controls(cnt, viddev);
 
                viddev->input = input;
@@ -716,8 +720,30 @@
                viddev->height = height;
                viddev->freq = freq;
                viddev->tuner_number = tuner_number;
+
+
+               /* Skip all frames captured before switchtime, capture 1 after switchtime */
+               {
+                       src_v4l2_t *s = (src_v4l2_t *) viddev->v4l2_private;
+                       unsigned int counter = 0;
+                       motion_log(LOG_DEBUG, 0, "set_input_skip_frame switch_time=%ld:%ld", switchTime.tv_sec, switchTime.tv_usec);
+
+                       /* Avoid hang using the number of mmap buffers */
+                       while(counter < s->req.count)
+                       {
+                               counter++;
+                               if (v4l2_next(cnt, viddev, map, width, height))
+                                       break;
+                               if (s->buf.timestamp.tv_sec > switchTime.tv_sec || 
+                               (s->buf.timestamp.tv_sec == switchTime.tv_sec && s->buf.timestamp.tv_usec > switchTime.tv_usec))
+                                       break;
+                               motion_log(LOG_DEBUG, 0, "got frame before switch timestamp=%ld:%ld", 
+                                       s->buf.timestamp.tv_sec, s->buf.timestamp.tv_usec);
+                       }
+               }
+
                /* skip a few frames if needed */
-               for (i = 0; i < skip; i++)
+               for (i = 1; i < skip; i++)
                        v4l2_next(cnt, viddev, map, width, height);
        } else {
                /* No round robin - we only adjust picture controls */

-- AngelCarpintero - 08 Jul 2007

Looks good now. Let us get it in 3.2.8 so please apply to SVN Angel. I think many RR users will be happy to get this performance win.

Thanks again to Dag for a great contribution. Remember to add Dag to the contributors.

-- KennethLavrsen - 09 Jul 2007

Fix record

Applied patch to SVN , i will add Dag to CREDITS and CHANGELOG files.

-- AngelCarpintero - 09 Jul 2007