On Sun, Sep 27, 2020 at 05:57:51PM +0100, Laurence Tratt wrote:
> On Sun, Sep 13, 2020 at 09:23:36AM +0100, Laurence Tratt wrote:
>
> > Since I recently opened my big fat mouth and suggested that
> > "kern.video.record" (analogous to kern.audio.record) might be a good idea, I
> > decided to put together a quick prototype (heavily based on the
> > kern.audio.record code). This at least roughly works for me but raises some
> > questions such as:
> >
> > * Is uvideo the only driver that can capture video? [I imagine not, but I
> > don't really know.]
utvfu(4) comes in mind.
Though, I'm not sure how much sense it would make to apply
kern.video.record on it as well since it requires an video input device
first which is playing the video stream.
> > * I've taken the same approach as kern.audio.record which is to keep on
> > handing out data even when kern.video.record=0 *but* it's harder to work
> > out what data we should hand out. At the moment we just pass on whatever
> > happened to be in the buffer beforehand (which is clearly not a good
> > idea in the long term, but proves the point for now). For uncompressed
> > video, handing over (say) an entirely black image is probably easy; for
> > compressed video we'd have to think about whether we also want to
> > manipulate frame headers etc. It would probably be easier to simply
> > intercept the "opening video" event but that would mean that if
> > something is already streaming video, setting kern.video.record=0 would
> > allow it to keep recording.
I also think this can be looked at in a second step.
For now acting same as audio(4) by only skipping the data makes sense
to me.
> > Comments welcome, including "this is a terrible idea full stop"!
>
> It seems that no-one yet thinks this is a terrible idea, and most people
> who've expressed an opinion prefer kern.audio.record and kern.video.record
> to be separate rather than combined.
I also would leave it separated.
> The two "big" questions above remain unanswered but, in the meantime, I'm
> attaching a minor update which fixes a small bug in the patch in sysctl.c
> that Edd Barrett found.
I'm basically OK with the diff, although we would need to add the
documentation peaces as well at one point. I like to have this option
in place parallel to kern.audio.record.
I would like to see more feedback from other developers if they think we
should bring this forward.
> Index: sbin/sysctl/sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.252
> diff -u -p -u -r1.252 sysctl.c
> --- sbin/sysctl/sysctl.c 15 Jul 2020 07:13:56 -0000 1.252
> +++ sbin/sysctl/sysctl.c 27 Sep 2020 16:49:54 -0000
> @@ -130,6 +130,7 @@ struct ctlname machdepname[] = CTL_MACHD
> #endif
> struct ctlname ddbname[] = CTL_DDB_NAMES;
> struct ctlname audioname[] = CTL_KERN_AUDIO_NAMES;
> +struct ctlname videoname[] = CTL_KERN_VIDEO_NAMES;
> struct ctlname witnessname[] = CTL_KERN_WITNESS_NAMES;
> char names[BUFSIZ];
> int lastused;
> @@ -219,6 +220,7 @@ void print_sensor(struct sensor *);
> int sysctl_chipset(char *, char **, int *, int, int *);
> #endif
> int sysctl_audio(char *, char **, int *, int, int *);
> +int sysctl_video(char *, char **, int *, int, int *);
> int sysctl_witness(char *, char **, int *, int, int *);
> void vfsinit(void);
>
> @@ -517,6 +519,11 @@ parse(char *string, int flags)
> if (len < 0)
> return;
> break;
> + case KERN_VIDEO:
> + len = sysctl_video(string, &bufp, mib, flags, &type);
> + if (len < 0)
> + return;
> + break;
> case KERN_WITNESS:
> len = sysctl_witness(string, &bufp, mib, flags, &type);
> if (len < 0)
> @@ -1766,6 +1773,7 @@ struct list shmlist = { shmname, KERN_SH
> struct list watchdoglist = { watchdogname, KERN_WATCHDOG_MAXID };
> struct list tclist = { tcname, KERN_TIMECOUNTER_MAXID };
> struct list audiolist = { audioname, KERN_AUDIO_MAXID };
> +struct list videolist = { videoname, KERN_VIDEO_MAXID };
> struct list witnesslist = { witnessname, KERN_WITNESS_MAXID };
>
> /*
> @@ -2813,6 +2821,25 @@ sysctl_audio(char *string, char **bufpp,
> return (-1);
> mib[2] = indx;
> *typep = audiolist.list[indx].ctl_type;
> + return (3);
> +}
> +
> +/*
> + * Handle video support
> + */
> +int
> +sysctl_video(char *string, char **bufpp, int mib[], int flags, int *typep)
> +{
> + int indx;
> +
> + if (*bufpp == NULL) {
> + listall(string, &videolist);
> + return (-1);
> + }
> + if ((indx = findname(string, "third", bufpp, &videolist)) == -1)
> + return (-1);
> + mib[2] = indx;
> + *typep = videolist.list[indx].ctl_type;
> return (3);
> }
>
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> retrieving revision 1.209
> diff -u -p -u -r1.209 uvideo.c
> --- sys/dev/usb/uvideo.c 31 Jul 2020 10:49:33 -0000 1.209
> +++ sys/dev/usb/uvideo.c 27 Sep 2020 16:49:55 -0000
> @@ -386,6 +386,12 @@ struct uvideo_devs {
> #define uvideo_lookup(v, p) \
> ((struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
>
> +/*
> + * Global flag to control if video recording is enabled when the
> + * kern.video.record setting is set to 1.
> + */
> +int video_record_enable = 0;
> +
> int
> uvideo_enable(void *v)
> {
> @@ -2210,7 +2216,8 @@ uvideo_vs_decode_stream_header(struct uv
> /* save sample */
> sample_len = frame_size - sh->bLength;
> if ((fb->offset + sample_len) <= fb->buf_size) {
> - bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
> + if (video_record_enable)
> + bcopy(frame + sh->bLength, fb->buf + fb->offset,
> sample_len);
> fb->offset += sample_len;
> }
>
> @@ -2299,7 +2306,8 @@ uvideo_vs_decode_stream_header_isight(st
> /* save sample */
> sample_len = frame_size;
> if ((fb->offset + sample_len) <= fb->buf_size) {
> - bcopy(frame, fb->buf + fb->offset, sample_len);
> + if (video_record_enable)
> + bcopy(frame, fb->buf + fb->offset, sample_len);
> fb->offset += sample_len;
> }
> }
> @@ -2327,7 +2335,8 @@ uvideo_mmap_queue(struct uvideo_softc *s
> }
>
> /* copy frame to mmap buffer and report length */
> - bcopy(buf, sc->sc_mmap[i].buf, len);
> + if (video_record_enable)
> + bcopy(buf, sc->sc_mmap[i].buf, len);
> sc->sc_mmap[i].v4l2_buf.bytesused = len;
>
> /* timestamp it */
> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.379
> diff -u -p -u -r1.379 kern_sysctl.c
> --- sys/kern/kern_sysctl.c 1 Sep 2020 01:53:50 -0000 1.379
> +++ sys/kern/kern_sysctl.c 27 Sep 2020 16:49:55 -0000
> @@ -125,6 +125,7 @@ extern long numvnodes;
> #if NAUDIO > 0
> extern int audio_record_enable;
> #endif
> +extern int video_record_enable;
>
> int allowkmem;
> int allowdt;
> @@ -141,6 +142,7 @@ int sysctl_cptime2(int *, u_int, void *,
> #if NAUDIO > 0
> int sysctl_audio(int *, u_int, void *, size_t *, void *, size_t);
> #endif
> +int sysctl_video(int *, u_int, void *, size_t *, void *, size_t);
> int sysctl_cpustats(int *, u_int, void *, size_t *, void *, size_t);
> int sysctl_utc_offset(void *, size_t *, void *, size_t);
>
> @@ -313,6 +315,7 @@ kern_sysctl(int *name, u_int namelen, vo
> case KERN_FILE:
> case KERN_WITNESS:
> case KERN_AUDIO:
> + case KERN_VIDEO:
> case KERN_CPUSTATS:
> break;
> default:
> @@ -672,6 +675,9 @@ kern_sysctl(int *name, u_int namelen, vo
> return (sysctl_audio(name + 1, namelen - 1, oldp, oldlenp,
> newp, newlen));
> #endif
> + case KERN_VIDEO:
> + return (sysctl_video(name + 1, namelen - 1, oldp, oldlenp,
> + newp, newlen));
> case KERN_CPUSTATS:
> return (sysctl_cpustats(name + 1, namelen - 1, oldp, oldlenp,
> newp, newlen));
> @@ -2462,6 +2468,19 @@ sysctl_audio(int *name, u_int namelen, v
> return (sysctl_int(oldp, oldlenp, newp, newlen, &audio_record_enable));
> }
> #endif
> +
> +int
> +sysctl_video(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> + void *newp, size_t newlen)
> +{
> + if (namelen != 1)
> + return (ENOTDIR);
> +
> + if (name[0] != KERN_VIDEO_RECORD)
> + return (ENOENT);
> +
> + return (sysctl_int(oldp, oldlenp, newp, newlen, &video_record_enable));
> +}
>
> int
> sysctl_cpustats(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> Index: sys/sys/sysctl.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/sysctl.h,v
> retrieving revision 1.211
> diff -u -p -u -r1.211 sysctl.h
> --- sys/sys/sysctl.h 1 Sep 2020 01:53:50 -0000 1.211
> +++ sys/sys/sysctl.h 27 Sep 2020 16:49:55 -0000
> @@ -189,7 +189,8 @@ struct ctlname {
> #define KERN_PFSTATUS 86 /* struct: pf status and stats
> */
> #define KERN_TIMEOUT_STATS 87 /* struct: timeout status and
> stats */
> #define KERN_UTC_OFFSET 88 /* int: adjust RTC time to UTC
> */
> -#define KERN_MAXID 89 /* number of valid kern ids */
> +#define KERN_VIDEO 89 /* struct: video properties */
> +#define KERN_MAXID 90 /* number of valid kern ids */
>
> #define CTL_KERN_NAMES { \
> { 0, 0 }, \
> @@ -281,6 +282,7 @@ struct ctlname {
> { "pfstatus", CTLTYPE_STRUCT }, \
> { "timeout_stats", CTLTYPE_STRUCT }, \
> { "utc_offset", CTLTYPE_INT }, \
> + { "video", CTLTYPE_STRUCT }, \
> }
>
> /*
> @@ -318,6 +320,17 @@ struct ctlname {
> #define KERN_AUDIO_MAXID 2
>
> #define CTL_KERN_AUDIO_NAMES { \
> + { 0, 0 }, \
> + { "record", CTLTYPE_INT }, \
> +}
> +
> +/*
> + * KERN_VIDEO
> + */
> +#define KERN_VIDEO_RECORD 1
> +#define KERN_VIDEO_MAXID 2
> +
> +#define CTL_KERN_VIDEO_NAMES { \
> { 0, 0 }, \
> { "record", CTLTYPE_INT }, \
> }
>
