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
I support this suggestion. I think the idea behind it is based on the
same concerns that led to "kern.audio.record" (though admittedly I
wasn't privy to the conversations leading up to it).
> 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.]
>
> * 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 built a kernel and sysctl with this and can confirm it works with my
Logitech C310. Of note, in its current form, running video(1) while
kern.video.record is 0 leads to a window showing mostly green and pink colors.
>
> Comments welcome, including "this is a terrible idea full stop"!
Again, I'm in favor of this idea.
It might be worth considering combining kern.audio.record and
kern.video.record, as they serve the same purpose (just different
modalities). I doubt that granular controls with separate switches will
be very useful at this level; maybe rather leave this differentiation
to the application?
An added benefit would be that there would be less largely duplicated
code; and probably better maintainability.
>
>
> Laurie
>
>
>
> Index: sbin/sysctl/sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.252
> diff -u -p -r1.252 sysctl.c
> --- sbin/sysctl/sysctl.c 15 Jul 2020 07:13:56 -0000 1.252
> +++ sbin/sysctl/sysctl.c 13 Sep 2020 08:08:57 -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 = { audioname, 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 -r1.209 uvideo.c
> --- sys/dev/usb/uvideo.c 31 Jul 2020 10:49:33 -0000 1.209
> +++ sys/dev/usb/uvideo.c 13 Sep 2020 08:08:58 -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 -r1.379 kern_sysctl.c
> --- sys/kern/kern_sysctl.c 1 Sep 2020 01:53:50 -0000 1.379
> +++ sys/kern/kern_sysctl.c 13 Sep 2020 08:08:58 -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 -r1.211 sysctl.h
> --- sys/sys/sysctl.h 1 Sep 2020 01:53:50 -0000 1.211
> +++ sys/sys/sysctl.h 13 Sep 2020 08:08:58 -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 }, \
> }
>
