On Mon, May 30, 2016 at 02:29:08PM +0200, Marcus Glocker wrote:
> On Mon, May 30, 2016 at 10:49:10AM +0200, Theo Buehler wrote:
>
> > On Mon, May 30, 2016 at 10:02:23AM +0200, Marcus Glocker wrote:
> > > I was in the middle of testing a uvideo(4) mmap queue diff when I
> > > noticed that our video(1) tool doesn't support the mmap method to grab
> > > frames. This diff adds it and also makes mmap the default method.
> > >
> > > Some test reports with different uvideo(4) devices would be welcome.
> > > It would be good if you could switch between different resolutions and
> > > also regression test the read method. E.g.:
> > >
> > > $ video
> > > $ video -s 800x600
> > > $ video -s 1600x1200
> > > $ video -g
> > > $ video -g -s 1024x768
> > > ...
> > >
> > > Thanks,
> > > Marcus
> > >
> >
> > A quick test of the built-in camera of my T420, shows no regression when
> > directly compared to output of video(1) without your patch. I tested
> > each of the supported frame sizes and rates for about 10 seconds both
> > with and without -g.
>
> Thanks for testing! Attached an updated diff after more feedback from
> Patrick Keshishian; Fix frame buffer processing order to dequeue,
> process buffer, requeue to avoid a potential buffer overwriting by
> driver before we can copy away the buffer. And added munmap(2) once done.
>
The same tests went fine with these changes. Diff reads ok to me, it's
really easy to follow what is happening.
Very minor comments inline.
>
> Index: video.1
> ===================================================================
> RCS file: /cvs/xenocara/app/video/video.1,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 video.1
> --- video.1 30 Nov 2014 01:40:26 -0000 1.11
> +++ video.1 30 May 2016 12:24:06 -0000
> @@ -25,7 +25,7 @@
> .Sh SYNOPSIS
> .Nm
> .Bk -words
> -.Op Fl \&Rv
> +.Op Fl \&gRv
> .Op Fl a Ar adaptor
> .Op Fl e Ar encoding
> .Op Fl f Ar file
> @@ -101,6 +101,11 @@ will be used by default.
> device from which frames will be read.
> The default is
> .Pa /dev/video .
> +.It Fl g
> +Use
> +.Xr read 2
> +method to grab frames instead of
> +.Xr mmap 2 .
> .It Fl i Ar input
> File from which frames will be read.
> If
> Index: video.c
> ===================================================================
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.12
> diff -u -p -u -p -r1.12 video.c
> --- video.c 23 Oct 2014 07:36:06 -0000 1.12
> +++ video.c 30 May 2016 12:24:06 -0000
> @@ -20,6 +20,7 @@
> #include <sys/videoio.h>
> #include <sys/time.h>
> #include <sys/limits.h>
> +#include <sys/mman.h>
>
> #include <err.h>
> #include <errno.h>
> @@ -145,6 +146,9 @@ struct encodings {
> struct video {
> struct xdsp xdsp;
> struct dev dev;
> +#define MMAP_NUM_BUFS 4
> + uint8_t mmap_on;
> + void *mmap_buffer[MMAP_NUM_BUFS];
> uint8_t *frame_buffer;
> size_t frame_bufsz;
> uint8_t *conv_buffer;
> @@ -189,8 +193,11 @@ void dev_reset_ctrls(struct video *);
> int parse_size(struct video *);
> int choose_size(struct video *);
> int choose_enc(struct video *);
> +int mmap_init(struct video *);
> +int mmap_stop(struct video *);
> int setup(struct video *);
> void cleanup(struct video *, int);
> +int ioctl_input(struct video *);
> int poll_input(struct video *);
> int grab_frame(struct video *);
> int stream(struct video *);
> @@ -206,7 +213,7 @@ extern char *__progname;
> void
> usage(void)
> {
> - fprintf(stderr, "usage: %s [-Rv] "
> + fprintf(stderr, "usage: %s [-gRv] "
> "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
> " %*s [-o output] [-r rate] [-s size]\n", __progname,
> (int)strlen(__progname), "");
> @@ -668,10 +675,14 @@ dev_check_caps(struct video *vid)
> warnx("%s is not a capture device", d->path);
> return 0;
> }
> - if (!(cap.capabilities & V4L2_CAP_READWRITE)) {
> + if (!(cap.capabilities & V4L2_CAP_READWRITE) && !vid->mmap_on) {
> warnx("%s does not support read(2)", d->path);
> return 0;
> }
> + if (!(cap.capabilities & V4L2_CAP_STREAMING) && vid->mmap_on) {
> + warnx("%s does not support mmap(2)");
This should be:
warnx("%s does not support read(2)", d->path);
> + return 0;
> + }
> d->buf_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>
> return 1;
> @@ -1231,6 +1242,85 @@ choose_enc(struct video *vid)
> }
>
> int
> +mmap_init(struct video *vid)
> +{
> + struct v4l2_requestbuffers rb;
> + struct v4l2_buffer buf;
> + int i, r, type;
> +
> + /* request buffers */
> + rb.count = MMAP_NUM_BUFS;
> + r = ioctl(vid->dev.fd, VIDIOC_REQBUFS, &rb);
> + if (r == -1) {
> + warn("ioctl VIDIOC_REQBUFS");
> + return 0;
> + }
> +
> + /* mmap the buffers */
> + for (i = 0; i < MMAP_NUM_BUFS; i++) {
> + buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + buf.memory = V4L2_MEMORY_MMAP;
> + buf.index = i;
> + r = ioctl(vid->dev.fd, VIDIOC_QUERYBUF, &buf);
> + if (r == -1) {
> + warn("ioctl VIDIOC_QUERYBUF");
> + return 0;
> + }
> + vid->mmap_buffer[i] = mmap(0, buf.length, PROT_READ,
> + MAP_SHARED, vid->dev.fd, buf.m.offset);
> + if (vid->mmap_buffer[i] == NULL) {
> + warn("mmap");
> + return 0;
> + }
> + }
> +
> + /* initial buffer queueing */
> + for (i = 0; i < MMAP_NUM_BUFS; i++) {
> + buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + buf.memory = V4L2_MEMORY_MMAP;
> + buf.index = i;
> + r = ioctl(vid->dev.fd, VIDIOC_QBUF, &buf);
> + if (r == -1) {
> + warn("ioctl VIDIOC_QBUF");
> + return 0;
> + }
> + }
> +
> + /* start video stream */
> + r = ioctl(vid->dev.fd, VIDIOC_STREAMON, &type);
> + if (r == -1) {
> + warn("ioctl VIDIOC_STREAMON");
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +int
> +mmap_stop(struct video *vid)
> +{
> + int i, r, type;
> +
> + /* stop video stream */
> + r = ioctl(vid->dev.fd, VIDIOC_STREAMOFF, &type);
> + if (r == -1) {
> + warn("ioctl STREAMOFF");
> + return 0;
> + }
> +
> + /* unmap the buffers */
> + for (i = 0; i < MMAP_NUM_BUFS; i++) {
> + r = munmap(vid->mmap_buffer[i], vid->bpf);
> + if (r == -1) {
> + warn("munmap");
> + return 0;
> + }
> + }
> +
> + return 1;
> +}
> +
> +int
> setup(struct video *vid)
> {
> if (vid->mode & M_IN_FILE) {
> @@ -1315,6 +1405,43 @@ setup(struct video *vid)
> if (vid->sz_str && !strcmp(vid->sz_str, "full"))
> resize_window(vid, 1);
>
> + if (vid->mmap_on) {
> + if (!mmap_init(vid))
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +int
> +ioctl_input(struct video *vid)
> +{
> + struct v4l2_buffer buf;
> + int r;
> +
> + /* dequeue buffer */
> + buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + buf.memory = V4L2_MEMORY_MMAP;
> + r = ioctl(vid->dev.fd, VIDIOC_DQBUF, &buf);
> + if (r == -1) {
> + warn("mmap ioctl VIDIOC_DQBUF");
Why "mmap ioctl" here?
> + return 0;
> + }
> +
> + /* copy frame buffer */
> + if (buf.length > vid->bpf)
> + return 0;
> + memcpy(vid->frame_buffer, vid->mmap_buffer[buf.index], buf.length);
> +
> + /* requeue buffer */
> + buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + buf.memory = V4L2_MEMORY_MMAP;
> + r = ioctl(vid->dev.fd, VIDIOC_QBUF, &buf);
> + if (r == -1) {
> + warn("mmap ioctl VIDIOC_QBUF");
and here?
> + return 0;
> + }
> +
> return 1;
> }
>
> @@ -1431,11 +1558,16 @@ stream(struct video *vid)
>
> while (!shutdown) {
> err = 0;
> - ret = poll_input(vid);
> + if (vid->mmap_on) {
> + if (!(ret = ioctl_input(vid)))
> + return 0;
> + } else
> + ret = poll_input(vid);
> if (ret == 1) {
> if ((vid->mode & M_IN_DEV) ||
> frames_grabbed - 1 == frames_played) {
> - ret = grab_frame(vid);
> + if (!vid->mmap_on)
> + ret = grab_frame(vid);
> if (ret == 1) {
> frames_grabbed++;
> if (vid->nofps)
> @@ -1559,6 +1691,8 @@ stream(struct video *vid)
> __dead void
> cleanup(struct video *vid, int excode)
> {
> + int type;
> +
> if (vid->xdsp.xv_image != NULL)
> XFree(vid->xdsp.xv_image);
>
> @@ -1574,8 +1708,11 @@ cleanup(struct video *vid, int excode)
> if (vid->xdsp.dpy != NULL)
> XCloseDisplay(vid->xdsp.dpy);
>
> - if (vid->dev.fd >= 0)
> + if (vid->dev.fd >= 0) {
> + if (vid->mmap_on)
> + mmap_stop(vid);
> close(vid->dev.fd);
> + }
>
> if (vid->iofile_fd >= 0)
> close(vid->iofile_fd);
> @@ -1608,9 +1745,10 @@ main(int argc, char *argv[])
> vid.dev.fd = vid.iofile_fd = -1;
> vid.mode = M_IN_DEV | M_OUT_XV;
> vid.enc = -1;
> + vid.mmap_on = 1; /* mmap method is default */
> wout = 1;
>
> - while ((ch = getopt(argc, argv, "vRa:e:f:i:O:o:r:s:")) != -1) {
> + while ((ch = getopt(argc, argv, "gvRa:e:f:i:O:o:r:s:")) != -1) {
> switch (ch) {
> case 'a':
> x->cur_adap = strtonum(optarg, 0, 4, &errstr);
> @@ -1628,6 +1766,10 @@ main(int argc, char *argv[])
> break;
> case 'f':
> snprintf(d->path, sizeof(d->path), optarg);
> + break;
> + case 'g':
> + vid.mmap_on = 0;
> + printf("Using read instead of mmap to grab frames\n");
Seems odd to print this by default, especially since the program is
rather silent. You invoke the command with -g, so you requested the
read method, you don't need to be told. How about only printing it
when -v was given?
> break;
> case 'i':
> if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
>
