Re: [xenocara] xenodm.man fix
On Thu, Feb 16, 2023 at 09:29:53PM +0300, Mikhail wrote: Hello Mikhail, > /etc/X11/xenodm/Xsession file has a check for x bit Yes, this one caught me out a few years back: https://marc.info/?l=openbsd-bugs=162737223625768=2 In subsequent discussions with Matthieu and Theo, I *think* the eventual thought was that it would be better to get rid of the `-x` check. I might be misremembering that, though. Laurie
Re: AMD pcidevs updates
On Tue, Nov 29, 2022 at 10:42:36PM +, Laurence Tratt wrote: > The diff below adds some newish AMD elements to pcidevs. As Mike Larkin kindly pointed out off-list, I sent a diff to the generated file. Sorry! Laurie diff --git sys/dev/pci/pcidevs sys/dev/pci/pcidevs index 2a395ab413a..158a3cc7640 100644 --- sys/dev/pci/pcidevs +++ sys/dev/pci/pcidevs @@ -780,6 +780,16 @@ product AMD 19_4X_IOMMU0x14b6 19h/4xh IOMMU product AMD 19_4X_HB_1 0x14b7 19h/4xh Host product AMD 19_4X_PCIE_1 0x14b9 19h/4xh PCIE product AMD 19_4X_PCIE_2 0x14ba 19h/4xh PCIE +product AMD 19_6X_RC 0x14d8 19h/6xh Root Complex +product AMD 19_6X_IOMMU0x14d9 19h/6xh IOMMU +product AMD 19_6X_DF_1 0x14e0 19h Data Fabric +product AMD 19_6X_DF_2 0x14e1 19h Data Fabric +product AMD 19_6X_DF_3 0x14e2 19h Data Fabric +product AMD 19_6X_DF_4 0x14e3 19h Data Fabric +product AMD 19_6X_DF_5 0x14e4 19h Data Fabric +product AMD 19_6X_DF_6 0x14e5 19h Data Fabric +product AMD 19_6X_DF_7 0x14e6 19h Data Fabric +product AMD 19_6X_DF_8 0x14e7 19h Data Fabric product AMD 14_HB 0x1510 14h Host product AMD 14_PCIE_1 0x1512 14h PCIE product AMD 14_PCIE_2 0x1513 14h PCIE
AMD pcidevs updates
The diff below adds some newish AMD elements to pcidevs. Here's the diff of them on my MSI board: -pchb0 at pci0 dev 0 function 0 vendor "AMD", unknown product 0x14d8 rev 0x00 -vendor "AMD", unknown product 0x14d9 (class system subclass IOMMU, rev 0x00) at pci0 dev 0 function 2 not configured +pchb0 at pci0 dev 0 function 0 "AMD 19h/6xh Root Complex" rev 0x00 +"AMD 19h/6xh IOMMU" rev 0x00 at pci0 dev 0 function 2 not configured -pchb6 at pci0 dev 24 function 0 vendor "AMD", unknown product 0x14e0 rev 0x00 -pchb7 at pci0 dev 24 function 1 vendor "AMD", unknown product 0x14e1 rev 0x00 -pchb8 at pci0 dev 24 function 2 vendor "AMD", unknown product 0x14e2 rev 0x00 -pchb9 at pci0 dev 24 function 3 vendor "AMD", unknown product 0x14e3 rev 0x00 -pchb10 at pci0 dev 24 function 4 vendor "AMD", unknown product 0x14e4 rev 0x00 -pchb11 at pci0 dev 24 function 5 vendor "AMD", unknown product 0x14e5 rev 0x00 -pchb12 at pci0 dev 24 function 6 vendor "AMD", unknown product 0x14e6 rev 0x00 -pchb13 at pci0 dev 24 function 7 vendor "AMD", unknown product 0x14e7 rev 0x00 +pchb6 at pci0 dev 24 function 0 "AMD 19h Data Fabric" rev 0x00 +pchb7 at pci0 dev 24 function 1 "AMD 19h Data Fabric" rev 0x00 +pchb8 at pci0 dev 24 function 2 "AMD 19h Data Fabric" rev 0x00 +pchb9 at pci0 dev 24 function 3 "AMD 19h Data Fabric" rev 0x00 +pchb10 at pci0 dev 24 function 4 "AMD 19h Data Fabric" rev 0x00 +pchb11 at pci0 dev 24 function 5 "AMD 19h Data Fabric" rev 0x00 +pchb12 at pci0 dev 24 function 6 "AMD 19h Data Fabric" rev 0x00 +pchb13 at pci0 dev 24 function 7 "AMD 19h Data Fabric" rev 0x00 As the dmesg shows, there are several remaining unknown elements, and several unconfigured, on this machine. The machine is, unfortunately, not really usable as it loses wireless access and/or input to X (mouse/keyboard stop working, but the display is fine) after about 15-45 minutes, at which point only a hard reboot will restore them. Still, fewer "unknown products" is a start! Laurie diff --git sys/dev/pci/pcidevs.h sys/dev/pci/pcidevs.h index cb1f344909a..7df027538cd 100644 --- sys/dev/pci/pcidevs.h +++ sys/dev/pci/pcidevs.h @@ -785,6 +785,16 @@ #definePCI_PRODUCT_AMD_19_4X_HB_1 0x14b7 /* 19h/4xh Host */ #definePCI_PRODUCT_AMD_19_4X_PCIE_10x14b9 /* 19h/4xh PCIE */ #definePCI_PRODUCT_AMD_19_4X_PCIE_20x14ba /* 19h/4xh PCIE */ +#definePCI_PRODUCT_AMD_19_6X_RC0x14d8 /* 19h/6xh Root Complex */ +#definePCI_PRODUCT_AMD_19_6X_IOMMU 0x14d9 /* 19h/6xh IOMMU */ +#definePCI_PRODUCT_AMD_19_6X_DF_1 0x14e0 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_2 0x14e1 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_3 0x14e2 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_4 0x14e3 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_5 0x14e4 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_6 0x14e5 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_7 0x14e6 /* 19h Data Fabric */ +#definePCI_PRODUCT_AMD_19_6X_DF_8 0x14e7 /* 19h Data Fabric */ #definePCI_PRODUCT_AMD_14_HB 0x1510 /* 14h Host */ #definePCI_PRODUCT_AMD_14_PCIE_1 0x1512 /* 14h PCIE */ #definePCI_PRODUCT_AMD_14_PCIE_2 0x1513 /* 14h PCIE */ OpenBSD 7.2-current (GENERIC.MP) #15: Tue Nov 29 22:26:24 GMT 2022 ltr...@phase.tratt.net:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 33390379008 (31843MB) avail mem = 32360992768 (30861MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.5 @ 0xa9b2a000 (43 entries) bios0: vendor American Megatrends International, LLC. version "1.30" date 10/12/2022 bios0: Micro-Star International Co., Ltd. MS-7D67 efi0 at bios0: UEFI 2.8 efi0: American Megatrends rev 0x5001a acpi0 at bios0: ACPI 6.4Undefined scope: \\_SB_.PCI0.GPP7.UP00.DP40.UP00.DP68 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP IVRS SSDT SSDT FIDT MCFG HPET WDRT UEFI FPDT VFCT TPM2 SSDT CRAT CDIT BGRT SSDT SSDT SSDT SSDT WSMT APIC SSDT SSDT SSDT SSDT SSDT SSDT SSDT SSDT acpi0: wakeup devices GPP3(S4) GPP4(S4) GPP5(S4) GPP6(S4) GP17(S4) XHC0(S4) XHC1(S4) XHC2(S4) GPP0(S4) GPP1(S4) GPP2(S4) GPP7(S4) GPP8(S4) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpimcfg0 at acpi0 acpimcfg0: addr 0xf000, bus 0-127 acpihpet0 at acpi0: 14318180 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD Ryzen 9 7900X 12-Core Processor, 4700.00 MHz, 19-61-02 cpu0:
Re: hidmt: clickpad detection
On Tue, Sep 13, 2022 at 11:06:33PM +0200, Ulf Brosziewski wrote: Hello Ulf, > The diff below improves the way hidmt identifies clickpads, and > addresses the problems described in > https://marc.info/?l=openbsd-tech=165296530618617=2 > and > https://marc.info/?l=openbsd-tech=165980534415586=2 > > If devices don't report a HUD_BUTTON_TYPE property, the driver checks > whether they claim to have an external left button, and if they do, > hidmt doesn't treat them as clickpads. > > I think this part of the logic should be turned around: hidmt should > treat everything as clickpad except if there is no "clickpad button" > (HUP_BUTTON 1) *and* both an external left and an external right button > (HUP BUTTON 2 and 3) are being reported. Touchpads without the internal > button are still usable with the clickpad configuration, it does less > harm to err on this side. > > Tests and OKs would be welcome. Thanks for doing this! This fixes the Framework touchpad and also seems like a better heuristic than we currently have (certainly it seems better to me than the Framework-specific quirk I added!). I think it would be great to try this in snapshots for a little while to check whether it causes any issues, though I'm optimistic that it will Just Work! Laurie
Re: libsoup2/3 conflicts and gstreamer1 [Re: audio/quodlibet & devel/libsoup]
On Mon, Aug 15, 2022 at 01:00:22PM +0200, Jeremie Courreges-Anglas wrote: Hello Jeremie, [RTLD_NOLOAD from Stuart] > I think so. The code looks good, would support a valid (IMO) use case and > would get us rid of a few patches and possibly blocking problems in ports. > > ok jca@ > > (I'm not sure it would help in the audio/quodlibet case, even after > patching the hardcoded sonames.) I can confirm that it makes quodlibet work reliably again! I've also used it to build (with a few other minor patches, some of which I've started upstreaming) the mold linker on OpenBSD. So it definitely makes porting some other partly-or-wholly-unportable software possible. I certainly support getting this patch in! Laurie
Re: Framework/PixArt clickpad quirk
On Sat, Aug 06, 2022 at 06:05:30PM +0100, Laurence Tratt wrote:
> The Framework clickpad (a PixArt PIXA3854) announces that it has 4 buttons
> which defeats the normal heuristic of "2 or more buttons means it's a
> touchpad". When it's identified as a touchpad, right hand mouse clicks don't
> work (apart from that, I can't tell any difference between clickpad and
> touchpad in operation!). Linux/libinput also have a quirk for this device
> [1], although they simply disable the second button.
>
> The patch at the end of this mail adds a quirk that detects the PIXA3854 and
> forces it to be identified as a clickpad. To say that I am unfamiliar with
> these parts of the kernel is an understatement: this patch works for me, but
> I don't know whether it's the best, or even an acceptable, way of dealing
> with "right hand mouse clicks don't work" on this particular device!
Not only to bump this, but also because I later realised that this addresses
the same issue jcs@ tried fixing for the Framework clickpad in a different
way in:
https://marc.info/?l=openbsd-tech=165296530618617=2
though that ran into problems and the eventual conclusion was either "add a
quirk" (like my patch does) or a wscontctl knob.
Since we currently know of only one device with this problem, and since a
wsconsctl knob is not something many people (OK, me...) would think of
trying, I tend to prefer a quirk. Whether that should be hard-coded in, as
I've done, or as a table, I'm happy to take advice on!
Laurie
Index: hid/hidmt.c
===
RCS file: /cvs/src/sys/dev/hid/hidmt.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 hidmt.c
--- hid/hidmt.c 9 Jul 2020 21:01:08 - 1.12
+++ hid/hidmt.c 6 Aug 2022 17:00:18 -
@@ -150,7 +150,9 @@ hidmt_setup(struct device *self, struct
}
/* find whether this is a clickpad or not */
- if (hid_locate(desc, dlen, HID_USAGE2(HUP_DIGITIZERS, HUD_BUTTON_TYPE),
+ if (mt->sc_flags & HIDMT_REALLY_CLICKPAD)
+ mt->sc_clickpad = 1;
+ else if (hid_locate(desc, dlen, HID_USAGE2(HUP_DIGITIZERS,
HUD_BUTTON_TYPE),
mt->sc_rep_cap, hid_feature, , NULL)) {
d = hid_get_udata(rep, capsize, );
mt->sc_clickpad = (d == 0);
Index: hid/hidmtvar.h
===
RCS file: /cvs/src/sys/dev/hid/hidmtvar.h,v
retrieving revision 1.8
diff -u -p -u -r1.8 hidmtvar.h
--- hid/hidmtvar.h 8 Nov 2019 01:20:22 - 1.8
+++ hid/hidmtvar.h 6 Aug 2022 17:00:18 -
@@ -37,6 +37,7 @@ struct hidmt {
int sc_enabled;
uint32_tsc_flags;
#define HIDMT_REVY 0x0001 /* Y-axis is reversed ("natural" scrolling) */
+#define HIDMT_REALLY_CLICKPAD 0x0002 /* incorrectly identified as touchpad */
struct device *sc_device;
int (*hidev_report_type_conv)(int);
Index: i2c/imt.c
===
RCS file: /cvs/src/sys/dev/i2c/imt.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 imt.c
--- i2c/imt.c 9 Jul 2020 21:01:56 - 1.5
+++ i2c/imt.c 6 Aug 2022 17:00:18 -
@@ -159,6 +159,11 @@ imt_attach(struct device *parent, struct
/* assume everything has "natural scrolling" where Y axis is reversed */
mt->sc_flags = HIDMT_REVY;
+ struct i2c_hid_desc *hid_desc = >sc_hdev.sc_parent->hid_desc;
+ /* The PixArt PIXA3854 clickpad offers 4 buttons, causing hidmt to
+ recognise it as a trackpad, not a clickpad. */
+ if (hid_desc->wVendorID == 0x093A && hid_desc->wProductID == 0x274)
+ mt->sc_flags |= HIDMT_REALLY_CLICKPAD;
mt->hidev_report_type_conv = ihidev_report_type_conv;
mt->hidev_get_report = imt_hidev_get_report;
Framework/PixArt clickpad quirk
The Framework clickpad (a PixArt PIXA3854) announces that it has 4 buttons
which defeats the normal heuristic of "2 or more buttons means it's a
touchpad". When it's identified as a touchpad, right hand mouse clicks don't
work (apart from that, I can't tell any difference between clickpad and
touchpad in operation!). Linux/libinput also have a quirk for this device
[1], although they simply disable the second button.
The patch at the end of this mail adds a quirk that detects the PIXA3854 and
forces it to be identified as a clickpad. To say that I am unfamiliar with
these parts of the kernel is an understatement: this patch works for me, but
I don't know whether it's the best, or even an acceptable, way of dealing
with "right hand mouse clicks don't work" on this particular device!
Laurie
[1] https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/665
Index: hid/hidmt.c
===
RCS file: /cvs/src/sys/dev/hid/hidmt.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 hidmt.c
--- hid/hidmt.c 9 Jul 2020 21:01:08 - 1.12
+++ hid/hidmt.c 6 Aug 2022 17:00:18 -
@@ -150,7 +150,9 @@ hidmt_setup(struct device *self, struct
}
/* find whether this is a clickpad or not */
- if (hid_locate(desc, dlen, HID_USAGE2(HUP_DIGITIZERS, HUD_BUTTON_TYPE),
+ if (mt->sc_flags & HIDMT_REALLY_CLICKPAD)
+ mt->sc_clickpad = 1;
+ else if (hid_locate(desc, dlen, HID_USAGE2(HUP_DIGITIZERS,
HUD_BUTTON_TYPE),
mt->sc_rep_cap, hid_feature, , NULL)) {
d = hid_get_udata(rep, capsize, );
mt->sc_clickpad = (d == 0);
Index: hid/hidmtvar.h
===
RCS file: /cvs/src/sys/dev/hid/hidmtvar.h,v
retrieving revision 1.8
diff -u -p -u -r1.8 hidmtvar.h
--- hid/hidmtvar.h 8 Nov 2019 01:20:22 - 1.8
+++ hid/hidmtvar.h 6 Aug 2022 17:00:18 -
@@ -37,6 +37,7 @@ struct hidmt {
int sc_enabled;
uint32_tsc_flags;
#define HIDMT_REVY 0x0001 /* Y-axis is reversed ("natural" scrolling) */
+#define HIDMT_REALLY_CLICKPAD 0x0002 /* incorrectly identified as touchpad */
struct device *sc_device;
int (*hidev_report_type_conv)(int);
Index: i2c/imt.c
===
RCS file: /cvs/src/sys/dev/i2c/imt.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 imt.c
--- i2c/imt.c 9 Jul 2020 21:01:56 - 1.5
+++ i2c/imt.c 6 Aug 2022 17:00:18 -
@@ -159,6 +159,11 @@ imt_attach(struct device *parent, struct
/* assume everything has "natural scrolling" where Y axis is reversed */
mt->sc_flags = HIDMT_REVY;
+ struct i2c_hid_desc *hid_desc = >sc_hdev.sc_parent->hid_desc;
+ /* The PixArt PIXA3854 clickpad offers 4 buttons, causing hidmt to
+ recognise it as a trackpad, not a clickpad. */
+ if (hid_desc->wVendorID == 0x093A && hid_desc->wProductID == 0x274)
+ mt->sc_flags |= HIDMT_REALLY_CLICKPAD;
mt->hidev_report_type_conv = ihidev_report_type_conv;
mt->hidev_get_report = imt_hidev_get_report;
Re: atomic read write
On Fri, Mar 11, 2022 at 09:33:29AM +0100, Mark Kettenis wrote: Hello Mark, Unfortunately this transformation almost certainly isn't safe: for example, the non-atomic load can return values that were never written by any thread (e.g. due to load/store tearing amongst other fun effects). >>> is that true even when care is taken to only use int/long sized variables >>> that are naturally aligned? are compilers that pathological now? >> Yes and yes, I'm afraid -- though rare, there are real examples of >> compilers doing this (pity the poor person who debugs it!). My working >> guess is that they'll be increasingly likely to occur as compilers become >> ever more aggressive at taking advantage of undefined behaviour. > Such a compiler is broken beyond repair. Unfortunately, the compiler is correct with respect to C's semantics. We might wish that it did not choose to take advantage of this, but it does. As much as I dislike this, I've given up hoping that the situation will change. To be specific, since we're using such a compiler in clang 13 (I have no idea if gcc 4.* is as aggressive, though more recent gcc versions definitely are), we've got little choice but to fit into its mould. For example, if an integer can be shared between threads, it must either always be read/written atomically or synchronised via some sort of guard (e.g. a mutex): failing to do so can lead to the horrors that the Alglave et al. article details. Laurie
Re: atomic read write
On Fri, Mar 11, 2022 at 09:00:57AM +1000, David Gwynne wrote: Hello David, >> Unfortunately this transformation almost certainly isn't safe: for >> example, the non-atomic load can return values that were never written by >> any thread (e.g. due to load/store tearing amongst other fun effects). > is that true even when care is taken to only use int/long sized variables > that are naturally aligned? are compilers that pathological now? Yes and yes, I'm afraid -- though rare, there are real examples of compilers doing this (pity the poor person who debugs it!). My working guess is that they'll be increasingly likely to occur as compilers become ever more aggressive at taking advantage of undefined behaviour. Jade Alglave et al. did an approachable intro to many (all?) of the horrors one might observe in the context of the Linux kernel [1], which is worth a read IMHO. Laurie [1] https://lwn.net/Articles/793253/
Re: atomic read write
On Thu, Mar 10, 2022 at 09:52:27PM +0100, Mark Kettenis wrote: Hello Mark, > If you think about it, the invariants being tested by those KASSERTs should > not depend on whether the old or the new value is read if another CPU is > modifying that variable at the same time. Unless of course there is a > refcounting bug. But even with the barrier we're not guaranteed to catch > that bug. ... > > - KASSERT(atomic_load_int(>task_refs.r_refs) == 0); > > + KASSERT(sc->task_refs.r_refs == 0); Unfortunately this transformation almost certainly isn't safe: for example, the non-atomic load can return values that were never written by any thread (e.g. due to load/store tearing amongst other fun effects). Laurie
Re: atomic read write
On Thu, Mar 10, 2022 at 09:05:54AM +, Visa Hankala wrote: Hello Visa, > In general, atomic_* functions have not provided implicit memory > barriers on OpenBSD. I've used atomics fairly extensively in other settings. Forgive me if I'm explaining the obvious, but I had a devil of a job making sense of this stuff a few years back, and so perhaps others might find it useful to expand on this point. Quick background: modern CPUs come in two main flavours, weakly ordered (e.g. most Arm systems) and strongly ordered (e.g. x86), which determine the rules of when multiple cores can see the reads/writes of other cores. Weakly ordered systems can move/optimise loads/stores around more than strongly ordered systems (so code that seems to work fine on x86 can then fail on Arm). There are in a sense two "safe" ways to use atomics: to assume that each atomic is isolated and that reading/writing to it tells you nothing about any other location in memory; or that every atomic is fully ordered with respect to every other atomic (i.e. no reorderings of atomic operations are allowed). The former is fast but (without additional operations) can't even express a mutex safely. The latter doesn't have very good performance. C11 thus allows you to do various atomic operations with different memory orderings [1] so that you can choose on a case-by-case basis what you're prepared to tolerate. "relaxed" is the most efficient but has the least guarantees; "seq_cst" is the least efficient but has the most guarantees. I would be very nervous about adding further atomic functions (as in the original diff) to OpenBSD that don't allow the user to specify what ordering they want: it's impossible to pick a memory ordering that suits all use cases. For example, neither READ_ONCE nor the existing atomic_* instructions define an ordering: I suppose I'd have to to assume they're relaxed. I worry that people might assume these atomic operations provide greater guarantees than they actually do. Fortunately since, AFAICT, we already use C11 (or C17?!) for base, and LLVM includes all of the relevant functions (e.g. the compare_exchange family [2]) I don't think we need to add any functions of our own? It might not even be a bad idea to deprecate the current atomic_* functions in base and migrate to the C11 alternatives? Laurie [1] https://en.cppreference.com/w/c/atomic/memory_order [2] https://en.cppreference.com/w/c/atomic/atomic_compare_exchange
Re: possible fix for Xorg 21.1.1 crashes
On Wed, Nov 17, 2021 at 09:25:22AM -0500, David Hill wrote: Hello David, >>> I think I found the bug that causes crashes in X for some people. > The first diff did not help, but this one has kept my X stable for 12 hours > so far... I haven't got quite as far as 12 hours with it, yet, but I've spent a while deliberately trying the things that caused it to crash very frequently before -- and haven't experienced a crash yet! Laurie
Re: httpd.conf grammar
On Sun, Mar 21, 2021 at 10:58:02PM +, Raf Czlonka wrote: Hello Raf, > I'd simply use the existing wording, without getting into details. > > While there, "braces" dominate the manual page, with a single occurrence of > "brackets" so let's change that too, for consistency. That works for me! Laurie
httpd.conf grammar
I wanted to use httpd's fastcgi "socket" and "strip" options and based upon
the man page's brief text:
[no] fastcgi [option]
Enable FastCGI instead of serving files. Valid options are:
tried "obvious" permutations such as:
fastcgi strip 1 socket "..."
fastcgi socket "..." strip 1
fastcgi socket "...", strip 1
but with each was greeted by a terse "syntax error".
After hunting around in the relevant parse.y file, it transpires that the
grammar allows, roughly speaking, the following:
fastcgi
fastcgi option
fastcgi { option ((',' '\n'? | '\n') option)* }
In other words, if you want to use more than one option you *have* to use
the {...} notation, but there's more than one way for options inside curly
brackets to be separated. In my case I can specify:
fastcgi {
socket "..."
strip 1
}
or:
fastcgi {
socket "...", strip 1
}
or:
fastcgi {
socket "...",
strip 1
}
This raised a couple of questions in my mind.
First, stylistically, I'm not quite sure if having three slightly different
ways of separating multiple options is useful or not. That said, I assume
that some people might already be taking advantage of this flexibility, so
perhaps worrying about it now is pointless.
Second, is it worthwhile giving users a hint about what to do when multiple
options need to be specified? For example, something like:
[no] fastcgi [option]
Enable FastCGI instead of serving files. If more than option
is specified, they must be included inside { ... }, with each
option separated by a comma or newline. Valid options are:
I'm happy to raise a patch if other people think this is worth fixing,
although I'm not entirely sure if we want to make people aware of the full
extent of the grammar, or something a little less complete such as the
suggestion above.
Laurie
Re: kern.video.record - part 2
On Wed, Dec 16, 2020 at 03:50:54PM +0100, Marcus Glocker wrote: Hello Marcus, > I reviewed this again, and I think this implementation should be consistent > on a video(4) level now. I've just tried this, and it works, and works better than my original (and uvideo-only) patch. I've been flipping it on and off and video(1), ffplay, Firefox, and Chrome all work fine with it even when they're midway through using the stream. [Of mild interest, it seems that you turn kern.video.record to 0, ffplay and Firefox keep displaying the last frame while video(1) and Chrome display a solid green picture.] This certainly supersedes my patch and has my OK, not that it needs it :) Laurie
Re: RFC: kern.video.record
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.]
>
> * 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.
>
> 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.
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.
Laurie
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.c15 Jul 2020 07:13:56 - 1.252
+++ sbin/sysctl/sysctl.c27 Sep 2020 16:49:54 -
@@ -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, , mib, flags, );
+ if (len < 0)
+ return;
+ break;
case KERN_WITNESS:
len = sysctl_witness(string, , mib, flags, );
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, );
+ return (-1);
+ }
+ if ((indx = findname(string, "third", bufpp, )) == -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.c31 Jul 2020 10:49:33 - 1.209
+++ sys/dev/usb/uvideo.c27 Sep 2020 16:49:55 -
@@ -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
Re: RFC: kern.video.record
On Fri, Sep 18, 2020 at 06:50:33AM -0600, Thomas Frohwein wrote: Hello Thomas, Thanks for your comments! > 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. I agree that it would simplify the code. The reason that I didn't merge them is because I know that sometimes people want to record audio but not video (I doubt that many people record video without audio). Now, admittedly, this isn't necessarily a super-common use case, so it might not be worth having two knobs for it, but it might be worth considering. Personally I'm completely comfortable with whatever the general consensus is for merging/not-merging! Laurie
RFC: kern.video.record
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.]
* 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.
Comments welcome, including "this is a terrible idea full stop"!
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.c15 Jul 2020 07:13:56 - 1.252
+++ sbin/sysctl/sysctl.c13 Sep 2020 08:08:57 -
@@ -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, , mib, flags, );
+ if (len < 0)
+ return;
+ break;
case KERN_WITNESS:
len = sysctl_witness(string, , mib, flags, );
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, );
+ return (-1);
+ }
+ if ((indx = findname(string, "third", bufpp, )) == -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.c31 Jul 2020 10:49:33 - 1.209
+++ sys/dev/usb/uvideo.c13 Sep 2020 08:08:58 -
@@ -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) {
-
Re: [WIP FAQ13] New section for webcam usage
On Tue, Sep 08, 2020 at 07:13:10PM +0200, Stefan Hagen wrote: Hello Stefan, > An audio device is special in a way that it has playback and recording > capabilities in one device. The sysctl is used to allow playback (by > default) but not allow recording. > > Video (as in webcam) is always a recording device, which shouldn't be > allowed to access in a default install (in contrast to audio playback). Personally I only set kern.audio.record to 1 immediately before I want to record from my microphone: I turn it back to 0 immediately afterwards, as I don't want a program to record audio when I'm not expecting it to. It would be impractical to do this if it was at a group level, as I would have to log out and back in to make the equivalent change. [I approximate this change by chown'ing a-rwx /dev/video after I've used my webcam though, of course, any other program can chown it back afterwards, so this is gives only very limited security.] Laurie
Re: [WIP FAQ13] New section for webcam usage
On Sun, Sep 06, 2020 at 07:45:32PM +0200, Stefan Hagen wrote: Hello Stefan, Thanks for this! I'll leave others who are more familiar with the website guidelines to comment on your patch, but I hope something like it can go in, as I found it a bit difficult at first to work out how to use webcams under OpenBSD. I have one tangential comment: > Using a webcam as user > > To use the webcam as regular user, you need to change the device > permissions. This made me wonder if uvideo should have an equivalent sysctl to audio (i.e. kern.audio.record)? Most, but I'm not sure all, webcams do display a light to tell you they're recording, but I can see that there is a security concern here. Laurie
Re: video -c: showing auto white balance temperature
On Sun, Aug 23, 2020 at 09:17:57AM +0200, Marcus Glocker wrote: Hello Marcus, > Sorry for the delay - I'm back to business mode and need to take care > about naive project managers with crazy requirements the most of the day Rather you than me :) > See inline some (nitpicking) feedback and adapted diff. Ok for you? This is fine except: > @@ -1684,7 +1705,7 @@ setup(struct video *vid) >* after the video stream has been started since some cams only >* process this control while the video stream is on. >*/ > - dev_set_ctrl_auto_white_balance(vid, 0, 1); > + dev_set_ctrl_auto(vid, V4L2_CID_AUTO_WHITE_BALANCE, 0, 1); > > if (vid->mode & M_OUT_XV) > net_wm_supported(vid); This segfaults because we're passing the wrong constant in. This chunk should be: @@ -1684,7 +1707,7 @@ setup(struct video *vid) * after the video stream has been started since some cams only * process this control while the video stream is on. */ - dev_set_ctrl_auto_white_balance(vid, 0, 1); + dev_set_ctrl_auto(vid, CTRL_WHITE_BALANCE_TEMPERATURE, 0, 1); if (vid->mode & M_OUT_XV) net_wm_supported(vid); Laurie
Re: video -c: showing auto white balance temperature
On Sat, Aug 08, 2020 at 11:29:41PM +0200, Marcus Glocker wrote:
Hello Marcus,
One other thing has occurred to me -- but can be done in a future patch
-- is that we probably want to be able to do:
$ video white_balance_temperature=auto
Please find attached a patch which does this. It also makes setting auto
controls more generic for if/when we gain more. If you set (say)
"saturation=auto" we warn, but don't error. So for example:
$ video -d
$ video white_balance_temperature=3000
white_balance_temperature: auto -> 3000
$ video white_balance_temperature=auto
white_balance_temperature: 3000 -> auto
$ video white_balance_temperature=auto
white_balance_temperature: auto -> auto
$ video white_balance_temperature=3000 saturation=auto
white_balance_temperature: auto -> 3000
saturation: no automatic control found
Laurie
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.35
diff -u -r1.35 video.c
--- video.c 9 Aug 2020 06:51:04 - 1.35
+++ video.c 9 Aug 2020 11:58:35 -
@@ -219,8 +219,8 @@
int dev_init(struct video *);
int dev_set_ctrl_abs(struct video *vid, int, int);
void dev_set_ctrl_rel(struct video *, int, int);
-void dev_set_ctrl_auto_white_balance(struct video *, int, int);
int dev_get_ctrl_auto(struct video *, int);
+void dev_set_ctrl_auto(struct video *, int, int, int);
void dev_reset_ctrls(struct video *);
int parse_ctrl(struct video *, int, char **);
@@ -1037,7 +1037,7 @@
* The spec requires auto-white balance to be off before
* we can set the white balance temperature.
*/
- dev_set_ctrl_auto_white_balance(vid, 0, 0);
+ dev_set_ctrl_auto(vid, ctrl, 0, 0);
}
if (val > ctrls[ctrl].max)
val = ctrls[ctrl].max;
@@ -1082,12 +1082,15 @@
}
void
-dev_set_ctrl_auto_white_balance(struct video *vid, int value, int reset)
+dev_set_ctrl_auto(struct video *vid, int ctrl, int value, int reset)
{
struct dev *d = >dev;
struct v4l2_control control;
- control.id = V4L2_CID_AUTO_WHITE_BALANCE;
+ if (!ctrls[ctrl].id_auto)
+ return;
+
+ control.id = ctrls[ctrl].id_auto;
if (ioctl(d->fd, VIDIOC_G_CTRL, ) != 0) {
warn("VIDIOC_G_CTRL");
return;
@@ -1134,8 +1137,7 @@
if (!ctrls[i].supported)
continue;
dev_set_ctrl_abs(vid, i, ctrls[i].def);
- if (i == CTRL_WHITE_BALANCE_TEMPERATURE)
- dev_set_ctrl_auto_white_balance(vid, 1, 0);
+ dev_set_ctrl_auto(vid, i, 1, 0);
}
}
@@ -1303,7 +1305,7 @@
}
}
if (i == CTRL_LAST)
- warnx("%s: unknown control", *argv);
+ warnx("%s: unknown control", *argv);
continue;
}
@@ -1315,21 +1317,34 @@
warnx("%s: no value", *argv);
break;
}
- val_new = strtonum(p, -32768, 32768, );
- if (errstr != NULL) {
- warnx("%s: %s", *argv, errstr);
- return 0;
- }
- val_old = ctrls[i].cur;
auto_old = dev_get_ctrl_auto(vid, i);
- if (dev_set_ctrl_abs(vid, i, val_new) == 0) {
- if (auto_old) {
- fprintf(stderr, "%s: auto -> %d\n",
- ctrls[i].name, ctrls[i].cur);
+ val_old = ctrls[i].cur;
+ if (strcmp(p, "auto") == 0) {
+ if (ctrls[i].id_auto == 0) {
+ fprintf(stderr, "%s: no automatic
control found\n",
+ ctrls[i].name);
+ } else if (!auto_old) {
+ fprintf(stderr, "%s: %d -> auto\n",
+ ctrls[i].name, val_old);
+ dev_set_ctrl_auto(vid, i, 1, 0);
} else {
- fprintf(stderr, "%s: %d -> %d\n",
- ctrls[i].name, val_old,
- ctrls[i].cur);
+ fprintf(stderr, "%s: auto -> auto\n",
ctrls[i].name);
+ }
+ } else {
+ val_new = strtonum(p, -32768, 32768, );
+ if (errstr != NULL)
Re: video -c: showing auto white balance temperature
On Sat, Aug 08, 2020 at 09:30:18PM +0200, Marcus Glocker wrote:
Hello Marcus,
>> I like your patch, which is better than my original! My only very minor
>> comment is whether we might want to document that it's safe to put "0" in
>> a "V4L2_CID_AUTO" settings, since the lowest value one of those can be is
>> 0x900 (judging by videoio.h)? At least, I had to look to make sure it was
>> safe :)
> Sorry, I lost you. Which control value exactly can be lowest 0x900?
In this bit of your diff:
#define CTRL_SHARPNESS 6
- { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
+ { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0, 0 },
#define CTRL_WHITE_BALANCE_TEMPERATURE 7
{ "white_balance_temperature",
- 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0, 0, 0, 0 },
+ 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 0, 0, 0, 0 },
it wasn't immediately obvious to me that the first "0" after
V4L2_CID_SHARPNESS wouldn't conflict with some other V4L2 setting. But it
turns out that all the V4L2_CID values are defined relative to this:
#define V4L2_CID_BASE (V4L2_CTRL_CLASS_USER | 0x900)
so "0" can't accidentally clash with anything. It's not a big deal :)
>> One other thing has occurred to me -- but can be done in a future
>> patch -- is that we probably want to be able to do:
>>
>> $ video white_balance_temperature=auto
> I was thinking about that as well but just not decided yet if we should
> introduce this or for now be compliant with the GUI where you would
> require to reset all settings to get back to auto.
In a sense the command-line is already more expressive (you can, for
example, express arbitrary white balance temperatures when in the GUI you
have to use increments of 10), so I'm inclined to think that allowing them
to be more separate is no bad thing.
[I personally think the GUI controls are very hard to use in practise: they
change so slowly that I find it almost impossible to A/B anything. So I'm a
bit biased against them!]
Laurie
Re: video -c: showing auto white balance temperature
On Sat, Aug 08, 2020 at 02:45:16PM +0200, Marcus Glocker wrote: Hello Marcus, > For now how about adding the according auto control id to our dev_ctrls > structure? In a next step we could change > dev_set_ctrl_auto_white_balance() to become a generic function > dev_set_ctrl_auto() available for all auto controls. Agreed. I like your patch, which is better than my original! My only very minor comment is whether we might want to document that it's safe to put "0" in a "V4L2_CID_AUTO" settings, since the lowest value one of those can be is 0x900 (judging by videoio.h)? At least, I had to look to make sure it was safe :) One other thing has occurred to me -- but can be done in a future patch -- is that we probably want to be able to do: $ video white_balance_temperature=auto Laurie
video -c: showing auto white balance temperature
Following Marcus's commit of video(1) changes, the attached patch crudely
solves the "-c output is misleading for white_balance_temperature" because
we conflate auto_white_balance_temperature and white_balance_temperature
(which are two separate UVC controls) into one control in video(1).
With this patch we can tell people if white_balance_temperature is being
automatically controlled or not:
$ video white_balance_temperature=6500
white_balance_temperature: 4000 -> 6500
$ obj/video -c
brightness=128
contrast=32
saturation=64
hue=0
gamma=120
sharpness=2
white_balance_temperature=6500
$ obj/video -d
$ obj/video -c
brightness=128
contrast=32
saturation=64
hue=0
gamma=120
sharpness=2
white_balance_temperature=auto
This patch raises several questions:
1) At the moment the only "auto" control we have is
white_balance_temperature. If we gain control of zoom/pan/exposure
(etc), it might be worth breaking out the common "auto" functionality?
2) Arguably the first command should look like:
$ video white_balance_temperature=6500
white_balance_temperature: auto -> 6500
3) The output of "video -dv" is very different to "video -c": I suspect
the former should look more like the latter for consistency.
4) "video -dc" doesn't seem to reset auto_white_balance_temperature?
Laurie
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.33
diff -u -r1.33 video.c
--- video.c 5 Aug 2020 11:34:00 - 1.33
+++ video.c 5 Aug 2020 20:33:56 -
@@ -1187,6 +1187,8 @@
void
dev_dump_query_ctrls(struct video *vid)
{
+ struct dev *d = >dev;
+ struct v4l2_control control;
int i;
if (!dev_check_caps(vid))
@@ -1195,8 +1197,21 @@
return;
for (i = 0; i < CTRL_LAST; i++) {
- if (ctrls[i].supported)
- fprintf(stderr, "%s=%d\n", ctrls[i].name, ctrls[i].cur);
+ if (!ctrls[i].supported)
+ continue;
+
+ if (i == CTRL_WHITE_BALANCE_TEMPERATURE) {
+ control.id = V4L2_CID_AUTO_WHITE_BALANCE;
+ if (ioctl(d->fd, VIDIOC_G_CTRL, ) != 0) {
+ warn("VIDIOC_G_CTRL");
+ return;
+ }
+ if (control.value == 1) {
+ fprintf(stderr, "%s=auto\n", ctrls[i].name);
+ continue;
+ }
+ }
+ fprintf(stderr, "%s=%d\n", ctrls[i].name, ctrls[i].cur);
}
}
Re: Add ability to set control values with video(1)
On Wed, Jul 29, 2020 at 10:52:31PM +0200, Marcus Glocker wrote: Hello Marcus, > Slightly adapted diff; Negative numbers can happen on controls. This looks really good to me, and a significant improvement on my original. I've tested everything I can think of and it all looks good, with one mild exception. UVC has separate controls for white_balance_temperature [int] and white_balance_temperature_auto [bool]. The former is only active if the latter is false. This probably makes devices about 30 seconds easier to develop, but it's not a nice UI for end users, so we've conflated them into one control. However, I now think we might need to display this conflated control slightly differently to others. For example consider this: $ video -d $ video -c brightness=128 contrast=128 saturation=128 gain=0 sharpness=128 white_balance_temperature=4000 That last line, I think, should probably be: white_balance_temperature=auto because although the device's white_balance_temperature is set to 4000K, we've turned auto white balance back on, so the device will choose whatever white_balance_temperature it feels like. If/when we get controls like zoom/exposure, they'll also probably want to be treated in the same way, so if there's a generic way of handling these in the code, it might be a nice bit of future proofing? However, this is not a huge deal: IMHO your diff is already good enough to go in tree! Laurie
Re: Add ability to set control values with video(1)
On Sat, Jul 25, 2020 at 10:27:15AM -0600, Theo de Raadt wrote: Hello Theo, > My primary concern is about a user changing settings which then persist > past close. > > Upon re-open, how do they know what mode they are in? > > I understand the default mode for a camera might not be nice. But at least > it is a default. If the previous use of the camera put it into a nasty > mode, does this mean all new users must first force default? > > Now you don't know what mode it is in. As a result, you must *always* > demand change into a specific mode. Rather than making things simpler, > doesn't use of a camera become potentially more complicated? >From what I can tell, there are two ways to control a uvideo device: 1) The "semi-persistent" state changes that video(1) can make that affects subsequent apps which access the device. My patch simply makes those state changes possible from the command-line instead of forcing the user to open a video and hold down keys until they reach the desired state. In other words, it doesn't change how you control the device: "-c reset" is equivalent to running video(1) and pressing "r", for example. 2) Control via a loopback device. For example, on Windows, Logitech allow you to change controls in their app where you can see video; they then expose a second internal device which other apps can use; I think controls are reset when the Logitech app is closed. On Linux I believe v4l2-ctl works as video(1) does (semi-persistent state changes) but Linux also has video loopback devices. Presumably they could do something similar to the Logitech Windows app, but I don't know if they do so or not. Unless we develop a loopback facility (or, perhaps, some sort of uvideo daemon roughly equivalent to sndiod), I don't think we have much choice but to continue with the semi-persistent state changes that video(1) has always been capable of. It is a bit icky, but it's the only way, for example, to change a webcam's brightness before taking a video call in a web browser. Laurie
Re: Add ability to set control values with video(1)
On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote: Hello Marcus, > We read the current value of the white balance temperature auto control > (since this gets set correctly during the reset), and just reset it again > on the next cam start up after the video stream has been started? > By that we avoid the video stream on/off dance when using '-c'. I missed an obvious scenario :/ "video -c reset" then "use the webcam in a browser" (or ffplay or whatever). Depending on your webcam, this won't appear to have done a full reset in such a scenario? Laurie
Re: Add ability to set control values with video(1)
On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote: Hello Marcus, > We read the current value of the white balance temperature auto control > (since this gets set correctly during the reset), and just reset it again > on the next cam start up after the video stream has been started? Aha, very clever -- I hadn't thought of doing that! I've tried your version and it works correctly in all the scenarios I can think of. I think this is now ready to go in! Laurie
Re: Add ability to set control values with video(1)
On Wed, Jul 22, 2020 at 10:23:19PM +0200, Marcus Glocker wrote: Hello Marcus, > I've tested this here as well in the meantime by leaving mmap_init() on > its original location (doesn't get involved for '-c reset') with three > different cams: > > * Logitech C930e: I see the same problem like you do with your C920 > finally. > * Logitech QuickCam Pro 9000: reset works fine. > * SunplusIT Inc Integrated Camera: reset works fine. Hmph, that's slightly annoying of Logitech :/ > This seems to be a problem only with some cams when turning the > auto white balance temperature back on while the stream is off, the > setting doesn't get recognized by the cam afterwards. > > I'm basically OK with your last diff, except the mmap_init() location > change: I don't like to turn the cam stream on only for setting this > parameter because some cams can't handle the obvious. > > I tried out some things with the C930e to get the auto white balance > temperature back on without having the stream on, but no luck so far. > > I would aim to get your diff in without the mmap_init() change. Maybe > we'll find a solution/workaround for this partial problem later? If we can find a change that allows this, I'd be happy! When I briefly tested things on Windows, the Logitech app there activated the stream before changing settings, so it's quite possible that they never tested changing some settings with the stream off. v4l2-ctl might have some clues in it, but their model is subtly different than ours and forces the user to understand a lot more about their camera (e.g. they force the user to turn off auto white balance before they allow them to set manual white balance, a differentiation which IMHO only makes sense if you've read the UVC spec). That makes me think that if/when uvideo is extended to deal with terminal control requests (e.g. zoom, exposure, focus) we'll find that other settings with "auto" options have similar problems. Honestly, if the price of controlling exposure and focus is having to turn the camera stream on, I think I will consider that an acceptable trade-off, given how useful those settings are :) >> +.It Fl c Ar reset | control=value >> +Set control value (e.g. brightness) and exit. The special name >> +.Ql reset >> +resets all values to their default. The available controls can be >> found +with >> +.Fl q >> +and the default values with >> +.Fl c Ar reset >> +.Fl v . > '-c reset -v' will not only display the default values, but also do > reset the cam to them. Shouldn't the sentence be more something like > the following since this sounds like '-c reset -v' can only display the > default values? > > "The available controls can be found with -q and the default values > are displayed during a reset with -c reset -v." Works for me! Laurie
Re: Add ability to set control values with video(1)
On Tue, Jul 21, 2020 at 09:01:26PM +0200, Marcus Glocker wrote:
Hello Marcus,
Thanks for the comments! Again, I agree with all of them with a couple of
comments:
> I'm a bit confused by the dev_set_ctrl() function renaming. Does 'inc'
> stand for increment? And if yes, it makes not a lot sense to me since we
> use this function to increase and decrease values. Leaving the
> dev_set_ctrl() as is and introduce dev_set_ctrl_abs() would make more
> sense to me then. Or rename the dev_inc_ctrl() to dev_chg_ctrl() instead.
I think it needs renaming because I managed to misread "dev_set_ctrl" as
"set absolute value" at first (hence the subtly incorrect first patch I sent
out). Finding good names is hard, but you're right that "inc" is confusing
here. How about dev_set_ctrl_rel, which I think gets the intent across
clearly, and makes the relation to dev_set_ctrl_abs clear?
> Ouch! It really shouldn't be required to initialize the mmap buffers
> and turn on the cams stream thereby each time you set a control
> parameter. Think of a larger script which sets multiple control
> parameters, turning on and off the stream each time, and there are cams
> which are slow in turning on their streams. With my C930e setting the
> controls works without turning on the stream first, and that's what I
> would expect.
>
> Can you figure out why this is required for your C920? There must be
> another solution to set controls without turning on the stream first ...
On my C920 I only need to do this to turn auto white_balance_temperature
back on. I tested this with:
$ video -c white_balance_temperature=6500
$ video
$ video -c reset
$ video
If pressing "r" in the final "video" call doesn't visibly change anything,
then (assuming auto white balance doesn't set things to 6500K!) I conclude
that resetting has worked. I wonder if it's the same for your C930e and
auto white_balance_temperature or not?
Unfortunately when I try messing with mmap_init, it seems that to turn on
auto white_balance_temperature, the final ioctl ("start video stream") has
to be executed. For any other control, we don't need to call mmap_init.
I agree that this is less than ideal, but I don't know if we can do anything
about it (other than, perhaps, only calling mmap_init for "-c reset", but
that feels rather fragile).
Laurie
Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 - 1.15
+++ video.1 21 Jul 2020 20:15:01 -
@@ -27,6 +27,7 @@
.Bk -words
.Op Fl \
.Op Fl a Ar adaptor
+.Op Fl c Ar reset | control=value
.Op Fl e Ar encoding
.Op Fl f Ar file
.Op Fl i Ar input
@@ -81,6 +82,15 @@
adaptor to use.
The default is 0, the first adaptor reported by
.Xr X 7 .
+.It Fl c Ar reset | control=value
+Set control value (e.g. brightness) and exit. The special name
+.Ql reset
+resets all values to their default. The available controls can be found
+with
+.Fl q
+and the default values with
+.Fl c Ar reset
+.Fl v .
.It Fl e Ar encoding
Lowercase FOURCC name of video encoding to use.
Valid arguments are
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.31
diff -u -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 - 1.31
+++ video.c 21 Jul 2020 20:15:01 -
@@ -192,7 +192,10 @@
#define M_IN_FILE 0x4
#define M_OUT_FILE 0x8
#define M_QUERY0x10
+#define M_RESET0x20
+#define M_SET_CTRL 0x40
int mode;
+ char*set_ctrl_str;
int verbose;
};
@@ -212,10 +215,12 @@
void dev_dump_info(struct video *);
void dev_dump_query(struct video *);
int dev_init(struct video *);
-void dev_set_ctrl(struct video *, int, int);
+void dev_set_ctrl_abs(struct video *vid, int, int);
+void dev_set_ctrl_rel(struct video *, int, int);
void dev_set_ctrl_auto_white_balance(struct video *, int);
void dev_reset_ctrls(struct video *);
+int parse_ctrl(struct video *, int *, int *);
int parse_size(struct video *);
int choose_size(struct video *);
int choose_enc(struct video *);
@@ -241,9 +246,9 @@
usage(void)
{
fprintf(stderr, "usage: %s [-gqRv] "
- "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
- " %*s [-o output] [-r rate] [-s size]\n", __progname,
- (int)strlen(__progname), "");
+ "[-a adaptor] [-c reset|control=value] [-e encoding] [-f file]\n"
+ " %*s [-i input] [-O output] [-o output] [-r rate] [-s
size]\n",
+ __progname, (int)strlen(__progname), "");
}
int
@@ -657,46 +662,46 @@
switch (str) {
case 'A':
if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
Re: Add ability to set control values with video(1)
On Mon, Jul 13, 2020 at 07:39:41PM +0100, Laurence Tratt wrote:
> video(1) allows users to adjust controls such as brightness, saturation
> (etc.) depending on the input device in question. These values persist even
> after video(1) has quit, allowing you to e.g. increase the brightness of a
> webcam before connecting to a video call. However, the only way to adjust
> values is to hold down keys in the GUI, which is slow, error prone, and
> can't easily be scripted.
>
> This patch adds a "-c" option to video(1) which either takes the special
> value "reset" or a "control=value" pair. For example:
>
> $ video -c reset
>
> resets all the controls to their default values. Assuming the input device
> in question supports brightness one can set that as follows:
>
> $ video -c brightness=200
>
> Note that the available controls, and their min/max values, will vary from
> device to device.
>
> To keep the patch simple, only one "-c" option can be passed to video(1) at
> a time. Note that passing this option causes video(1) to quit before
> displaying video (in identical fashion to "-q") which makes it useful for
> scripting purposes.
The attached patch reworks things a bit. First, it now works with
white_balance_temperature, which (at least on my C920) requires mmap_init to
be called first. Second, the previous patch sometimes set controls to
surprising values because it called what is (in effect) a "change control by
X" function. This patch now renames the "old" function to "dev_inc_ctrl" and
introduces a new "dev_set_ctrl_abs". This then provides an obvious
opportunity to simplify the reset function.
With this patch I can do things like:
$ video -c white_balance_temperature=6500
$ video -c brightness=200
$ video && video -c reset && video
and see changes being made as appropriate.
Laurie
Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 - 1.15
+++ video.1 17 Jul 2020 21:44:49 -
@@ -27,6 +27,7 @@
.Bk -words
.Op Fl \
.Op Fl a Ar adaptor
+.Op Fl c Ar reset | control=value
.Op Fl e Ar encoding
.Op Fl f Ar file
.Op Fl i Ar input
@@ -81,6 +82,15 @@
adaptor to use.
The default is 0, the first adaptor reported by
.Xr X 7 .
+.It Fl c Ar reset | control=value
+Set control value (e.g. brightness) and exit. The special name
+.Ql reset
+resets all values to their default. The available controls can be found
+with
+.Fl q
+and the default values with
+.Fl c Ar reset
+.Fl v .
.It Fl e Ar encoding
Lowercase FOURCC name of video encoding to use.
Valid arguments are
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.31
diff -u -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 - 1.31
+++ video.c 17 Jul 2020 21:44:49 -
@@ -192,7 +192,10 @@
#define M_IN_FILE 0x4
#define M_OUT_FILE 0x8
#define M_QUERY0x10
+#define M_RESET0x20
+#define M_SET_CTRL 0x40
int mode;
+ char*set_ctrl_str;
int verbose;
};
@@ -212,10 +215,12 @@
void dev_dump_info(struct video *);
void dev_dump_query(struct video *);
int dev_init(struct video *);
-void dev_set_ctrl(struct video *, int, int);
+void dev_inc_ctrl(struct video *, int, int);
+void dev_set_ctrl_abs(struct video *vid, int, int);
void dev_set_ctrl_auto_white_balance(struct video *, int);
void dev_reset_ctrls(struct video *);
+int parse_ctrl(struct video *, int *, int *);
int parse_size(struct video *);
int choose_size(struct video *);
int choose_enc(struct video *);
@@ -241,9 +246,9 @@
usage(void)
{
fprintf(stderr, "usage: %s [-gqRv] "
- "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
- " %*s [-o output] [-r rate] [-s size]\n", __progname,
- (int)strlen(__progname), "");
+ "[-a adaptor] [-c reset|control=value] [-e encoding] [-f file]\n"
+ " %*s [-i input] [-O output] [-o output] [-r rate] [-s
size]\n",
+ __progname, (int)strlen(__progname), "");
}
int
@@ -657,46 +662,46 @@
switch (str) {
case 'A':
if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SHARPNESS, 1);
+ dev_inc_ctrl(vid, CTRL_SHARPNESS, 1);
break;
case 'a':
if (vid->mode & M_IN_DEV)
-
Re: Add white temperature balance adjustability to video(1)
On Wed, Jul 15, 2020 at 08:33:54PM +0200, Marcus Glocker wrote:
Hello Marcus,
Thanks for your comments. I agree with all but one, and attach a new diff.
>> +#define CTRL_WHITE_BALANCE_TEMPERATURE 7
>> +{ "white_balance_temperature",
> I think we should replace '_' with ' ' just to make the output look more
> aligned when we run video -v.
In the next patch [1], I allow people to adjust controls via a command-line
switch to video(1). Having controls with spaces in would be really awkward
for that and I can't see an obvious solution -- but I'm open to suggestions!
Laurie
[1] https://marc.info/?l=openbsd-tech=159466570510992=2
Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.14
diff -u -r1.14 video.1
--- video.1 25 Feb 2019 12:34:35 - 1.14
+++ video.1 15 Jul 2020 20:10:25 -
@@ -66,8 +66,8 @@
.Ar output
and displayed via
.Xr Xv 3 .
-The acutance, brightness, contrast, gain, gamma, hue and saturation
-controls of
+The acutance, brightness, contrast, gain, gamma, hue, saturation, and white
+balance temperature controls of
.Ar file
can also be adjusted if
.Ar file
@@ -293,6 +293,12 @@
.Ar file .
.It Ic s
Decrease saturation control of
+.Ar file .
+.It Ic W
+Increase white balance temperature control of
+.Ar file .
+.It Ic w
+Decrease white balance temperature control of
.Ar file .
.El
.Sh EXAMPLES
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.30
diff -u -r1.30 video.c
--- video.c 1 Jul 2020 06:45:24 - 1.30
+++ video.c 15 Jul 2020 20:10:25 -
@@ -114,7 +114,10 @@
{ "gamma", 0, V4L2_CID_GAMMA, 0, 0, 0, 0, 0 },
#define CTRL_SHARPNESS 6
{ "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
-#define CTRL_LAST 7
+#define CTRL_WHITE_BALANCE_TEMPERATURE 7
+ { "white_balance_temperature",
+ 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0, 0, 0, 0 },
+#define CTRL_LAST 8
{ NULL, 0, 0, 0, 0, 0, 0, 0 }
};
@@ -210,6 +213,7 @@
void dev_dump_query(struct video *);
int dev_init(struct video *);
void dev_set_ctrl(struct video *, int, int);
+void dev_set_ctrl_auto_white_balance(struct video *, int);
void dev_reset_ctrls(struct video *);
int parse_size(struct video *);
@@ -730,6 +734,16 @@
if (vid->mode & M_IN_DEV)
dev_set_ctrl(vid, CTRL_SATURATION, -1);
break;
+ case 'W':
+ if (vid->mode & M_IN_DEV)
+ dev_set_ctrl(vid,
+ CTRL_WHITE_BALANCE_TEMPERATURE, 10);
+ break;
+ case 'w':
+ if (vid->mode & M_IN_DEV)
+ dev_set_ctrl(vid,
+ CTRL_WHITE_BALANCE_TEMPERATURE,
-10);
+ break;
default:
break;
}
@@ -1011,6 +1025,13 @@
ctrls[ctrl].name, d->path);
return;
}
+ if (ctrl == CTRL_WHITE_BALANCE_TEMPERATURE) {
+ /*
+* The spec requires auto-white balance to be off before
+* we can set the white balance temperature.
+*/
+ dev_set_ctrl_auto_white_balance(vid, 0);
+ }
val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
if (val > ctrls[ctrl].max)
val = ctrls[ctrl].max;
@@ -1034,6 +1055,23 @@
}
void
+dev_set_ctrl_auto_white_balance(struct video *vid, int toggle)
+{
+ struct dev *d = >dev;
+ struct v4l2_control control;
+
+ control.id = V4L2_CID_AUTO_WHITE_BALANCE;
+ if (ioctl(d->fd, VIDIOC_G_CTRL, ) != 0)
+ warn("VIDIOC_G_CTRL");
+ if (control.value == toggle)
+ return;
+
+ control.value = toggle;
+ if (ioctl(d->fd, VIDIOC_S_CTRL, ) != 0)
+ warn("VIDIOC_S_CTRL");
+}
+
+void
dev_reset_ctrls(struct video *vid)
{
struct dev *d = >dev;
@@ -1043,6 +1081,14 @@
for (i = 0; i < CTRL_LAST; i++) {
if (!ctrls[i].supported)
continue;
+ if (i == CTRL_WHITE_BALANCE_TEMPERATURE) {
+ /*
+* We might be asked to reset before the white balance
+* temperature has been adjusted, so we need to make
+* sure that auto-white balance really is off.
+*/
+ dev_set_ctrl_auto_white_balance(vid, 0);
+ }
control.id = ctrls[i].id;
Add ability to set control values with video(1)
video(1) allows users to adjust controls such as brightness, saturation
(etc.) depending on the input device in question. These values persist even
after video(1) has quit, allowing you to e.g. increase the brightness of a
webcam before connecting to a video call. However, the only way to adjust
values is to hold down keys in the GUI, which is slow, error prone, and
can't easily be scripted.
This patch adds a "-c" option to video(1) which either takes the special
value "reset" or a "control=value" pair. For example:
$ video -c reset
resets all the controls to their default values. Assuming the input device
in question supports brightness one can set that as follows:
$ video -c brightness=200
Note that the available controls, and their min/max values, will vary from
device to device.
To keep the patch simple, only one "-c" option can be passed to video(1) at
a time. Note that passing this option causes video(1) to quit before
displaying video (in identical fashion to "-q") which makes it useful for
scripting purposes.
Laurie
Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.14
diff -u -r1.14 video.1
--- video.1 25 Feb 2019 12:34:35 - 1.14
+++ video.1 13 Jul 2020 18:33:51 -
@@ -27,6 +27,7 @@
.Bk -words
.Op Fl \
.Op Fl a Ar adaptor
+.Op Fl c Ar reset | control=value
.Op Fl e Ar encoding
.Op Fl f Ar file
.Op Fl i Ar input
@@ -81,6 +82,15 @@
adaptor to use.
The default is 0, the first adaptor reported by
.Xr X 7 .
+.It Fl c Ar reset | control=value
+Set control value (e.g. brightness) and exit. The special name
+.Ql reset
+resets all values to their default. The available controls can be found
+with
+.Fl q
+and the default values with
+.Fl c Ar reset
+.Fl v .
.It Fl e Ar encoding
Lowercase FOURCC name of video encoding to use.
Valid arguments are
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.30
diff -u -r1.30 video.c
--- video.c 1 Jul 2020 06:45:24 - 1.30
+++ video.c 13 Jul 2020 18:33:51 -
@@ -189,7 +189,10 @@
#define M_IN_FILE 0x4
#define M_OUT_FILE 0x8
#define M_QUERY0x10
+#define M_RESET0x20
+#define M_SET_CTRL 0x40
int mode;
+ char*set_ctrl_str;
int verbose;
};
@@ -212,6 +215,7 @@
void dev_set_ctrl(struct video *, int, int);
void dev_reset_ctrls(struct video *);
+int parse_ctrl(struct video *, int *, int *);
int parse_size(struct video *);
int choose_size(struct video *);
int choose_enc(struct video *);
@@ -237,9 +241,9 @@
usage(void)
{
fprintf(stderr, "usage: %s [-gqRv] "
- "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
- " %*s [-o output] [-r rate] [-s size]\n", __progname,
- (int)strlen(__progname), "");
+ "[-a adaptor] [-c reset|control=value] [-e encoding] [-f file]\n"
+ " %*s [-i input] [-O output] [-o output] [-r rate] [-s
size]\n",
+ __progname, (int)strlen(__progname), "");
}
int
@@ -1172,6 +1176,38 @@
return 1;
}
+
+int
+parse_ctrl(struct video *vid, int *ctrl_id, int *ctrl_val)
+{
+ char *valp;
+ const char *errstr;
+
+ if (!vid->set_ctrl_str) {
+ return 0;
+ }
+
+ valp = strsep(>set_ctrl_str, "=");
+ if (*valp == '\0' || vid->set_ctrl_str == '\0') {
+ return 0;
+ }
+ for (*ctrl_id = 0; *ctrl_id < CTRL_LAST; (*ctrl_id)++) {
+ if (strcmp(valp, ctrls[*ctrl_id].name) == 0) {
+ break;
+ }
+ }
+ if (*ctrl_id == CTRL_LAST) {
+ warnx("Unknown control '%s'", valp);
+ return 0;
+ }
+ *ctrl_val = strtonum(vid->set_ctrl_str, ctrls[*ctrl_id].min,
ctrls[*ctrl_id].max, );
+ if (errstr != NULL) {
+ warnx("control value '%s' is %s", valp, errstr);
+ return 0;
+ }
+ return 1;
+}
+
int
parse_size(struct video *vid)
{
@@ -1432,6 +1468,8 @@
int
setup(struct video *vid)
{
+ int ctrl_id, ctrl_val;
+
if (vid->mode & M_IN_FILE) {
if (!strcmp(vid->iofile, "-"))
vid->iofile_fd = STDIN_FILENO;
@@ -1471,6 +1509,19 @@
if (!parse_size(vid) || !choose_size(vid))
return 0;
+ if (vid->mode & M_RESET) {
+ dev_reset_ctrls(vid);
+ return 1;
+ }
+
+ if (vid->mode & M_SET_CTRL) {
+ if (!parse_ctrl(vid, _id, _val)) {
+ return 0;
+ }
+ dev_set_ctrl(vid, ctrl_id, ctrl_val);
+ return 1;
+ }
+
vid->bpf = vid->width * vid->height * encs[vid->enc].bpp / NBBY;
if (vid->verbose > 0) {
@@ -1900,7
Add white temperature balance adjustability to video(1)
This patch adds the ability to control the white balance temperature on
webcams in video(1). This is bound to keys W/w in steps of +/-10K (because
changing the temperature 1K at a time is tortuously slow given a typical
range of 2800-6500K, and +/-10 seems to roughly be the minimum difference
that my eyes can detect).
At least on my Logitech C920, one can only change the white balance
temperature once auto-white balance is turned off (if it's not off, the C920
refuses to change anything and the ioctl returns an error). This patch thus
turns auto-white balance on/off when you change/reset the white balance.
This is required by the USB video spec which doesn't seem too surprising
until you realise that the spec also says that the contrast value can't be
changed without auto-contrast being turned off. However, I can't see any
code anywhere which does this: perhaps auto-contrast is off on some, or all,
webcams by default?
Laurie
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.30
diff -u -r1.30 video.c
--- video.c 1 Jul 2020 06:45:24 - 1.30
+++ video.c 13 Jul 2020 16:08:05 -
@@ -114,7 +114,10 @@
{ "gamma", 0, V4L2_CID_GAMMA, 0, 0, 0, 0, 0 },
#define CTRL_SHARPNESS 6
{ "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
-#define CTRL_LAST 7
+#define CTRL_WHITE_BALANCE_TEMPERATURE 7
+ { "white_balance_temperature",
+ 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0, 0, 0, 0 },
+#define CTRL_LAST 8
{ NULL, 0, 0, 0, 0, 0, 0, 0 }
};
@@ -730,6 +733,14 @@
if (vid->mode & M_IN_DEV)
dev_set_ctrl(vid, CTRL_SATURATION, -1);
break;
+ case 'W':
+ if (vid->mode & M_IN_DEV)
+ dev_set_ctrl(vid,
CTRL_WHITE_BALANCE_TEMPERATURE, 10);
+ break;
+ case 'w':
+ if (vid->mode & M_IN_DEV)
+ dev_set_ctrl(vid,
CTRL_WHITE_BALANCE_TEMPERATURE, -10);
+ break;
default:
break;
}
@@ -1011,6 +1022,15 @@
ctrls[ctrl].name, d->path);
return;
}
+ /* The spec requires auto-white balance to be off before we can set the
+* white balance temperature. */
+ if (ctrl == CTRL_WHITE_BALANCE_TEMPERATURE) {
+ control.id = V4L2_CID_AUTO_WHITE_BALANCE;
+ control.value = 0;
+ if (ioctl(d->fd, VIDIOC_S_CTRL, ) != 0) {
+ warn("VIDIOC_S_CTRL(auto_white_balance)");
+ }
+ }
val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
if (val > ctrls[ctrl].max)
val = ctrls[ctrl].max;
@@ -1054,6 +1074,15 @@
if (vid->verbose > 0)
fprintf(stderr, "%s now %d\n", ctrls[i].name,
ctrls[i].cur);
+ /* After resetting the temperature, also turn auto-white
+* balance back on. */
+ if (i == CTRL_WHITE_BALANCE_TEMPERATURE) {
+ control.id = V4L2_CID_AUTO_WHITE_BALANCE;
+ control.value = 1;
+ if (ioctl(d->fd, VIDIOC_S_CTRL, ) != 0) {
+ warn("VIDIOC_S_CTRL(auto_white_balance)");
+ }
+ }
}
}
Re: Document that openlog's first paramater must continue to point to valid data
On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote: Hello Ingo, > The parameter > .Fa ident > -is a string that will be prepended to every message. > +points to a string that will be prepended to every message; > +its storage must persist until > +.Fn closelog > +or the corresponding > +.Fn closelog_r . > +If the content of the string is changed, behaviour is unspecified. This works well for me and would, I think, have stopped me making the mistake that started this whole thread -- thanks! Laurie
Re: Document that openlog's first paramater must continue to point to valid data
On Sun, Feb 02, 2020 at 04:45:53PM -0700, Theo de Raadt wrote: Hello Theo, >> When I passed that to openlog, and later called syslog, well, I was >> confused. I then distilled the example down to C. The Debian & GNU man >> pages suggest that I'm not the first person to try doing this. > That would only be true if you've looked at the history of how that text > ended up in their pages, as it is, I think either way overspecifies it, I > think openlog() could plausibly be coded to cache a copy of the whole > string. The standard may have left this open-ended intentionally. Is there > a historian watching this thread? I wasn't able to find anything out when I looked into this a bit, but I'm not a Posix or GNU historian, so I might have missed something obvious. The good news is that the altered text I'm suggesting would still guarantee identical observed behaviour should a given syslog implementation choose to copy 'ident' into a buffer. Laurie
Re: Document that openlog's first paramater must continue to point to valid data
On Sun, Feb 02, 2020 at 04:20:13PM -0700, Theo de Raadt wrote: Hello Theo, > What is the difference between storage and contents You can't free() the backing memory *and* (according to GNU's man page, at least) you can't safely change the string contents either on some OSs. I've tried something slightly lengthier in the patch below. > (actually contents could change. As long as there is always a terminating > NUL, for the purpose of traversal. But that's not something someone should > code to) > > You still didn't say how you ran into this. While writing a daemon in Rust, I discovered that, AFAICT, one doesn't have sensible access to __progname and has to figure out the executable name dynamically (AFAICT), so it ends up in malloc'd storage. When I passed that to openlog, and later called syslog, well, I was confused. I then distilled the example down to C. The Debian & GNU man pages suggest that I'm not the first person to try doing this. Laurie Index: syslog.3 === RCS file: /cvs/src/lib/libc/gen/syslog.3,v retrieving revision 1.35 diff -u -r1.35 syslog.3 --- syslog.330 Aug 2019 20:27:25 - 1.35 +++ syslog.32 Feb 2020 23:32:50 - @@ -216,7 +216,9 @@ .Fn vsyslog . The parameter .Fa ident -is a string that will be prepended to every message. +is a string that will be prepended to every message; the memory pointed to +must remain valid, and the string's contents unchanged, until +.Fn closelog . The .Fa logopt argument
Re: Document that openlog's first paramater must continue to point to valid data
On Sun, Feb 02, 2020 at 03:22:12PM -0700, Theo de Raadt wrote: Hello Theo, >> OpenBSD's documentation for openlog's first paramater 'ident' is less >> clear than Debian [1] or GNU [2] that the memory pointed to must remain >> valid for as long as syslog is called (which I'm assuming without hard >> evidence is equivalent to "until closelog is called"). [...] > If we are going to document it, about 6 word adjustment speaking about > "storage", "lifetime", or "persisting" "until closelog()" should be enough, Keeping it simple works for me. Patch at the bottom of this email. Laurie Index: syslog.3 === RCS file: /cvs/src/lib/libc/gen/syslog.3,v retrieving revision 1.35 diff -u -r1.35 syslog.3 --- syslog.330 Aug 2019 20:27:25 - 1.35 +++ syslog.32 Feb 2020 23:13:30 - @@ -216,7 +216,9 @@ .Fn vsyslog . The parameter .Fa ident -is a string that will be prepended to every message. +is a string that will be prepended to every message; both storage and contents +must persist unchanged until +.Fn closelog . The .Fa logopt argument
Document that openlog's first paramater must continue to point to valid data
OpenBSD's documentation for openlog's first paramater 'ident' is less clear
than Debian [1] or GNU [2] that the memory pointed to must remain valid for
as long as syslog is called (which I'm assuming without hard evidence is
equivalent to "until closelog is called").
Although this isn't specified by POSIX, on both OpenBSD and Debian passing a
pointer to memory that is then free'd causes random bytes to be written to
syslog. This tiny program demonstrates the problem:
#include
#include
#include
int main() {
char *n = malloc(128);
strcpy(n, "name");
openlog(n, LOG_CONS, LOG_DAEMON);
free(n);
syslog(LOG_ERR, "msg");
return 0;
}
Most of the time this leads to rubbish being sent to /var/log/daemon: once
in a while it will segfault. Removing the free() call fixes the problem.
The patch at the end of this email is one possible suggestion for making this
clear, but it's difficult to do so succinctly.
Laurie
[1] https://manpages.debian.org/testing/manpages-dev/openlog.3.en.html
[2] https://www.gnu.org/software/libc/manual/html_node/openlog.html
Index: syslog.3
===
RCS file: /cvs/src/lib/libc/gen/syslog.3,v
retrieving revision 1.35
diff -u -r1.35 syslog.3
--- syslog.330 Aug 2019 20:27:25 - 1.35
+++ syslog.32 Feb 2020 21:15:40 -
@@ -216,7 +216,17 @@
.Fn vsyslog .
The parameter
.Fa ident
-is a string that will be prepended to every message.
+is a pointer to a string that will be prepended to every message. Note that
+.Fn openlog
+stores the
+.Fa ident
+pointer itself: it does not copy the string that
+.Fa ident
+points to, and does not guarantee safety if the contents of the string change
+later. You should thus ensure the memory pointed to by
+.Fa ident
+does not change until and unless you call
+.Fn closelog .
The
.Fa logopt
argument
Re: Patch for column number in doas error messages
On Mon, Jul 09, 2018 at 10:04:35AM +0200, Otto Moerbeek wrote:
Hello Otto,
>> I still don't see the point. In 30 years, I've gotten by with parsers
>> that say "syntax error", and had very bad experiences with programs that
>> do a poor job anticipating where the parse error is. Of course there are
>> programs which do a good job of detecting the error, but generally those
>> don't use yacc...
> I'm not trying to guess anything. yacc uses one-token lookahead. If it
> cannot continue, the lexical value of the token that could not be processed
> gives a pretty clear indication of the error spot.
We can state something even a little stronger than that. An LR parser (such
as Yacc) will always stop at the first possible point an error can be
guaranteed to have occurred. Often that point is the same as where the error
was made (e.g. "x = 1 + ;" will report an error at the semi-colon), although
sometimes it isn't (a simple example is "fi (x) { ... }": the error will be
reported at the open curly, even though a human would consider the error to
have occurred at "fi"). In my opinion, when the error location is correct
it's a genuine help (and, in practise, most errors seem to fall into this
category); when it's wrong, it tends to be very obvious, and you're in no
worse a situation than you were if it just says "syntax error".
[My personal opinion is that a lot of the ill feeling towards error recovery
is to do with when it gets it wrong, tries to keep on parsing, and fills the
terminal up with incorrect errors -- the cascading error problem. Anyone with
too much time on their hands can read my attempted solutions to that at
https://arxiv.org/abs/1804.07133]
Laurie
--
Personal http://tratt.net/laurie/
Software Development Teamhttp://soft-dev.org/
https://github.com/ltratt http://twitter.com/laurencetratt
Re: readdir man page
On Thu, Feb 02, 2012 at 09:50:45PM -0700, Philip Guenther wrote: The current man page doesn't claim that errno is always set but one might reasonably assume that it is: certainly, it seems a horribly easy way of introducing bugs (at least for idiots such as myself). [...] [Christiano] Although I like your intention, I think the problem is in the later part: The readdir() and readdir_r() functions may also fail and set errno for any of the errors specified for the routine getdirentries(2). Maybe this could be clarified ? Lets see what others think. I'll propose a third option: I think the directory(3) manpage would be clearer if the DESCRIPTION section was unwoven to have separate DESCRIPTION and RETURN VALUES section. Explicit wording that errno is not set when end of directory is reached and that programs that want to tell apart end-of-directory and errors must set errno to zero should be added. I think this would make a lot of sense. I also think readdir() should set errno if it detects an invalid seekdir(). EINVAL seems correct. This seems like a good idea overall, but I have one minor worry. Does any other Unix do this? If not, it may introduce another subtle portability bug, if a program thinks it can detect an invalid seekdir by checking whether errno was set or not by readdir. This is purely hypothetical though. Laurie -- Personal http://tratt.net/laurie/ The Converge programming language http://convergepl.org/ https://github.com/ltratt http://twitter.com/laurencetratt
apmd setperf on MP machines
18 months ago I posted a patch to make apmd's -C and -A modes work
half-sensibly on multi-processor machines:
http://marc.info/?l=openbsd-techm=123315164930014w=2
The patch went into the tree but was backed out because on some very slow,
very old Sparc machines it apparently couldn't react quickly enough to
changes in processor utilisation. Since I had no access to such a machine, I
couldn't debug it, and the patch died. That means that apmd -C has been
close to useless on MP machines, since it needs all CPUs to be working hard
before it increases hw.setperf.
One day hopefully this functionality will move into the kernel (I know Ted
and others are working towards that), but in the meantime someone asked me
if I could resurrect the backed-out patch which I attach to the end of this
e-mail. This patch cranks hw.setperf to 100 as soon as increased activity is
noticed and then gradually backs it off it can (in contrast to the old patch
which ramped up gradually; the new behaviour should increase battery life).
It also reflects recent changes to apmd.c. Apart from that it's largely the
same as before. It's been working for me over the last few days, but hasn't
been extensively tested so YMMV. However, if you work off battery on an MP
machine, you might find it helpful.
Laurie
--
http://tratt.net/laurie/ -- Personal
http://fetegeo.org/ -- Free text geocoding
http://convergepl.org/ -- The Converge programming language
Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.56
diff -u -r1.56 apmd.c
--- apmd.c 2 Apr 2010 04:12:46 - 1.56
+++ apmd.c 27 Jun 2010 12:05:11 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: apmd.c,v 1.56 2010/04/02 04:12:46 deraadt Exp $ */
+/* $OpenBSD: apmd.c,v 1.51 2009/02/04 05:22:58 tedu Exp $ */
/*
* Copyright (c) 1995, 1996 John T. Kohl
@@ -61,12 +61,14 @@
int debug = 0;
int doperf = PERF_NONE;
-#define PERFINC 50
-#define PERFDEC 20
+#define PERFDEC 15
#define PERFMIN 0
#define PERFMAX 100
#define PERFINCTHRES 10
#define PERFDECTHRES 30
+#define PERFTIMEOUTERR 9
+#define PERFTIMEOUTFAST 1000
+#define PERFTIMEOUTSLOW 2500
extern char *__progname;
@@ -74,9 +76,8 @@
int power_status(int fd, int force, struct apm_power_info *pinfo);
int bind_socket(const char *sn);
enum apm_state handle_client(int sock_fd, int ctl_fd);
-int get_avg_idle_mp(int ncpu);
-int get_avg_idle_up(void);
-void perf_status(struct apm_power_info *pinfo, int ncpu);
+int get_min_idle_mp(int ncpu);
+useconds_t perf_status(struct apm_power_info *pinfo, int ncpu);
void suspend(int ctl_fd);
void stand_by(int ctl_fd);
void setperf(int new_perf);
@@ -195,13 +196,12 @@
/* multi- and uni-processor case */
int
-get_avg_idle_mp(int ncpu)
+get_min_idle_mp(int ncpu)
{
static int64_t **cp_time_old;
static int64_t **cp_time;
- static int *avg_idle;
int64_t change, sum, idle;
- int i, cpu, min_avg_idle;
+ int i, cpu, min_idle;
size_t cp_time_sz = CPUSTATES * sizeof(int64_t);
if (!cp_time_old)
@@ -212,11 +212,7 @@
if ((cp_time = calloc(sizeof(int64_t *), ncpu)) == NULL)
return -1;
- if (!avg_idle)
- if ((avg_idle = calloc(sizeof(int), ncpu)) == NULL)
- return -1;
-
- min_avg_idle = 0;
+ min_idle = 100;
for (cpu = 0; cpu ncpu; cpu++) {
int cp_time_mib[] = {CTL_KERN, KERN_CPTIME2, cpu};
@@ -230,14 +226,14 @@
calloc(sizeof(int64_t), CPUSTATES)) == NULL)
return -1;
- if (sysctl(cp_time_mib, 3, cp_time[cpu], cp_time_sz, NULL, 0)
-0)
+ if (sysctl(cp_time_mib, 3, cp_time[cpu], cp_time_sz, NULL, 0))
syslog(LOG_INFO, cannot read kern.cp_time2);
sum = 0;
+ idle = 100;
for (i = 0; i CPUSTATES; i++) {
- if ((change = cp_time[cpu][i] - cp_time_old[cpu][i])
-0) {
+ change = cp_time[cpu][i] - cp_time_old[cpu][i];
+ if (change 0) {
/* counter wrapped */
change = ((uint64_t)cp_time[cpu][i] -
(uint64_t)cp_time_old[cpu][i]);
@@ -249,72 +245,28 @@
if (sum == 0)
sum = 1;
- /* smooth data */
- avg_idle[cpu] = (avg_idle[cpu] + (100 * idle) / sum) / 2;
-
- if (cpu == 0)
- min_avg_idle = avg_idle[cpu];
-
- if (avg_idle[cpu] min_avg_idle)
- min_avg_idle = avg_idle[cpu];
+ if ((100 * idle) / sum min_idle)
+ min_idle = (100 * idle) / sum;
