Re: Add ability to set control values with video(1)

2020-07-30 Thread Marcus Glocker
Hi Laurie,

Thanks for testing and feedback!

On Thu, 30 Jul 2020 08:07:56 +0100
Laurence Tratt  wrote:

> 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?

Yes, I was shortly thinking about the right way of how to handle the
white balance temperature control as well.  And I agree, probably we
should display 'auto' when auto white balance is turned on, but the
current value when it's turned off.

> However, this is not a huge deal: IMHO your diff is already good
> enough to go in tree!

Right, I also would prefer to get this diff in as is, and then
implement the white balance change on top of that.  As you already
mentioned, that should be no big deal.

> Laurie

Marcus



Re: Add ability to set control values with video(1)

2020-07-30 Thread Laurence Tratt
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)

2020-07-29 Thread Marcus Glocker
On Wed, 29 Jul 2020 07:45:52 +0200
Marcus Glocker  wrote:

> On Sat, 25 Jul 2020 18:27:45 +0100
> Laurence Tratt  wrote:
> 
> > 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 otReset all supported controls to their
> > default value and quit.her 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.  
> 
> OK - Let me try to pick this thread up once more :-)
> 
> Lets assume we leave the current behaviour of video(1) as is, which
> doesn't reset back the default control values on device close.
> 
> This adapted patch adds two new options and the ability to set
> multiple control values on the CLI with a similar interface as
> sysctl(8) has:
> 
> * c: List all current supported control values and quit
>   (-q is already occupied).
> 
> * d: Reset all supported controls to their default value and quit
>   (-r is already occupied).
>   -> This does the same thing as when pressing 'r' in the GUI.  
> 
> Some examples:
> 
>   $ doas video -c
>   brightness=128
>   contrast=32
>   saturation=64
>   hue=0
>   gamma=120
>   sharpness=2
>   white_balance_temperature=4000
>   $
> 
>   $ doas video brightness=200 sharpness=4 
>   brightness: 128 -> 200
>   sharpness: 2 -> 4
>   $
> 
>   $ doas video brightness
>   brightness: 200
>   $
> 
>   $ doas video -c
>   brightness=200
>   contrast=32
>   saturation=64
>   hue=0
>   gamma=120
>   sharpness=4
>   white_balance_temperature=4000
>   $
> 
>   $ doas video -d
>   $
> 
>   $ doas video -c
>   brightness=128
>   contrast=32
>   saturation=64
>   hue=0
>   gamma=120
>   sharpness=2
>   white_balance_temperature=4000
>   $
> 
>   $ doas video -f /dev/video1 gain=1
>   gain: 0 -> 1
>   $
> 
>   $ doas video -f /dev/video1 -c
>   brightness=128
>   contrast=128
>   saturation=128
>   gain=1
>   sharpness=128
>   white_balance_temperature=4000
>   $
> 
> What we could think about optionally is to reset to the default
> control values on device close when we did use the GUI mode, but
> leave the controls sticky when we set them through the CLI, and
> explicitly document that in the video(1) man page for the
> [control[=value]] arguments.
> 
> But entirely removing the sticky controls I think is odd since as
> already mentioned before, it will remove the ability to preset any
> controls before we enter an application where we can't change them,
> like in an browser.  And web conference calls are pretty common
> nowadays ...

Slightly adapted diff;  Negative numbers can happen on controls.


Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 -   

Re: Add ability to set control values with video(1)

2020-07-28 Thread Marcus Glocker
On Sat, 25 Jul 2020 18:27:45 +0100
Laurence Tratt  wrote:

> 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 otReset all supported controls to their default
> value and quit.her 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.

OK - Let me try to pick this thread up once more :-)

Lets assume we leave the current behaviour of video(1) as is, which
doesn't reset back the default control values on device close.

This adapted patch adds two new options and the ability to set multiple
control values on the CLI with a similar interface as sysctl(8) has:

* c: List all current supported control values and quit
  (-q is already occupied).

* d: Reset all supported controls to their default value and quit
  (-r is already occupied).
  -> This does the same thing as when pressing 'r' in the GUI.

Some examples:

$ doas video -c
brightness=128
contrast=32
saturation=64
hue=0
gamma=120
sharpness=2
white_balance_temperature=4000
$

$ doas video brightness=200 sharpness=4 
brightness: 128 -> 200
sharpness: 2 -> 4
$

$ doas video brightness
brightness: 200
$

$ doas video -c
brightness=200
contrast=32
saturation=64
hue=0
gamma=120
sharpness=4
white_balance_temperature=4000
$

$ doas video -d
$

$ doas video -c
brightness=128
contrast=32
saturation=64
hue=0
gamma=120
sharpness=2
white_balance_temperature=4000
$

$ doas video -f /dev/video1 gain=1
gain: 0 -> 1
$

$ doas video -f /dev/video1 -c
brightness=128
contrast=128
saturation=128
gain=1
sharpness=128
white_balance_temperature=4000
$

What we could think about optionally is to reset to the default control
values on device close when we did use the GUI mode, but leave the
controls sticky when we set them through the CLI, and explicitly
document that in the video(1) man page for the [control[=value]]
arguments.

But entirely removing the sticky controls I think is odd since as
already mentioned before, it will remove the ability to preset any
controls before we enter an application where we can't change them,
like in an browser.  And web conference calls are pretty common
nowadays ...


Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 -  1.15
+++ video.1 29 Jul 2020 05:02:51 -
@@ -25,7 +25,7 @@
 .Sh SYNOPSIS
 .Nm
 .Bk -words
-.Op Fl \
+.Op Fl \
 .Op Fl a Ar adaptor
 .Op Fl e Ar encoding
 .Op Fl f Ar file
@@ -34,6 +34,7 @@
 .Op Fl o Ar output
 .Op Fl r Ar rate
 .Op Fl s Ar size
+.Op Ar control Ns Op = Ns Ar value
 .Ek
 .Sh 

Re: Add ability to set control values with video(1)

2020-07-25 Thread Laurence Tratt
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)

2020-07-25 Thread Marcus Glocker
On Sat, Jul 25, 2020 at 10:27:15AM -0600, Theo de Raadt wrote:

> Matthieu Herrb  wrote:
> 
> > On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> > > Marcus Glocker  wrote:
> > > 
> > > > Instead of introducing the CLI parameter controls in video(1), we could
> > > > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > > > also gives us the possibility to only display the current video
> > > > parameters.
> > > 
> > > I must have missed something.  Why does it need to be a seperate comment.
> > > Won't this produce confusion?  Why can video do this?
> > > 
> > > Is it really correct for the video hardware to have persistant settings?
> > > Would it not be better if the required mode was always commanded when
> > > a video is being recorded?
> > 
> > I've not followed UVC hardware too closely; it seems that some of the
> > cameras are always fully automatically adjusting their parameters,
> > while others allow for manual setting that remains between device
> > open().
> > 
> > And some of the controls are not available when video(4) is used from
> > a browser (for conferencing).
> > 
> > On a 2nd thought having that integrated with video(1) allows to
> > control the values with visual feedback so it makes sense to keep only
> > one program. But ihmo it would be more user friendly if the command
> > line syntax was more regular with audio or wscons control programs.
> 
> 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?

OK - Lets put aside the audioctl(1) discussion.  Maybe the idea isn't
that good than I initially thought and would indeed over-complicate
things.

Regarding the current behavior of video(1) for the default controls;
When you change the controls today through the video(1) keys, they are
already sticky, since the reset control function only can be called
through the 'r' key-press.  If we want to go for a default mode on close,
of course that function could be easily copied to the video(1) closing
routine.

The idea of the patch on the other side was to enable somebody to
easily set his preferred controls, e.g. through a script.  At the
moment if you want to set your preferred controls on cam start-up,
that's only possible by the key-presses which is odd obviously if you
need to change a couple of controls every time by pressing keys.  How
many people would finally find that an useful feature is another
question I guess.

I have no strong opinion about the line syntax, but I agree keeping it
similar to other commands would be good.  E.g. adapting video(1) to
the audioctl(1) syntax and doing something like

$ video brightness=200 contrast=150

could be an option - If we want to follow this feature at all.



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2020/07/25 09:20, Theo de Raadt wrote:
> > The normal idiom is when last-close happens in a driver, all modal-state
> > is lost and restored to default, and when you use the driver again --
> > the new open gets you a raw configuration which is then changed via
> > ioctl, before futher use.
> 
> Isn't this a bit like volume controls for audio which do exist outside
> of a particular application's opening of the device?

That is soft state, and operate in a global experience context.

Please also consider more than volume controls.  With audio, I think
it is the only major knob, but what's being proposed is a big collection
of knobs.

I'm not sure I see the set of camera ops the same way as volume control.

Also see my message relating to 'reset', back to the default state.
No such thing exists in audio.

I think this is the difference between subsystem (soft state), versus
driver state (much more hard state)





Re: Add ability to set control values with video(1)

2020-07-25 Thread Stuart Henderson
On 2020/07/25 09:20, Theo de Raadt wrote:
> The normal idiom is when last-close happens in a driver, all modal-state
> is lost and restored to default, and when you use the driver again --
> the new open gets you a raw configuration which is then changed via
> ioctl, before futher use.

Isn't this a bit like volume controls for audio which do exist outside
of a particular application's opening of the device?



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Matthieu Herrb  wrote:

> On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> > Marcus Glocker  wrote:
> > 
> > > Instead of introducing the CLI parameter controls in video(1), we could
> > > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > > also gives us the possibility to only display the current video
> > > parameters.
> > 
> > I must have missed something.  Why does it need to be a seperate comment.
> > Won't this produce confusion?  Why can video do this?
> > 
> > Is it really correct for the video hardware to have persistant settings?
> > Would it not be better if the required mode was always commanded when
> > a video is being recorded?
> 
> I've not followed UVC hardware too closely; it seems that some of the
> cameras are always fully automatically adjusting their parameters,
> while others allow for manual setting that remains between device
> open().
> 
> And some of the controls are not available when video(4) is used from
> a browser (for conferencing).
> 
> On a 2nd thought having that integrated with video(1) allows to
> control the values with visual feedback so it makes sense to keep only
> one program. But ihmo it would be more user friendly if the command
> line syntax was more regular with audio or wscons control programs.

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?



Re: Add ability to set control values with video(1)

2020-07-25 Thread Matthieu Herrb
On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> Marcus Glocker  wrote:
> 
> > Instead of introducing the CLI parameter controls in video(1), we could
> > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > also gives us the possibility to only display the current video
> > parameters.
> 
> I must have missed something.  Why does it need to be a seperate comment.
> Won't this produce confusion?  Why can video do this?
> 
> Is it really correct for the video hardware to have persistant settings?
> Would it not be better if the required mode was always commanded when
> a video is being recorded?

I've not followed UVC hardware too closely; it seems that some of the
cameras are always fully automatically adjusting their parameters,
while others allow for manual setting that remains between device
open().

And some of the controls are not available when video(4) is used from
a browser (for conferencing).

On a 2nd thought having that integrated with video(1) allows to
control the values with visual feedback so it makes sense to keep only
one program. But ihmo it would be more user friendly if the command
line syntax was more regular with audio or wscons control programs.

-- 
Matthieu Herrb



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
  $ video -c white_balance_temperature=6500
  $ video
  $ video -c reset
  $ video

BTW, no other subsystem of ours has this "reset" type thing.

A few subsystems have persistance, but persistant modes are usually only
in the software layer, not in the hardware layer.

The normal idiom is when last-close happens in a driver, all modal-state
is lost and restored to default, and when you use the driver again --
the new open gets you a raw configuration which is then changed via
ioctl, before futher use.



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Marcus Glocker  wrote:

> Instead of introducing the CLI parameter controls in video(1), we could
> introduce videoctl(1) in base, same as we have with audioctl(1).  This
> also gives us the possibility to only display the current video
> parameters.

I must have missed something.  Why does it need to be a seperate comment.
Won't this produce confusion?  Why can video do this?

Is it really correct for the video hardware to have persistant settings?
Would it not be better if the required mode was always commanded when
a video is being recorded?



Re: Add ability to set control values with video(1)

2020-07-24 Thread Marcus Glocker
On Fri, 24 Jul 2020 15:13:19 +0100
Laurence Tratt  wrote:

> 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?

That's true and basically out of our control I would say ...

Anyway, matthieu@ came up with an idea yesterday which I like and also
think would be the better approach finally:

Instead of introducing the CLI parameter controls in video(1), we could
introduce videoctl(1) in base, same as we have with audioctl(1).  This
also gives us the possibility to only display the current video
parameters.

For things like the auto white balance temperature control we could
implement tweaks easier there, like only turning the stream on before we
set back auto white balance temperature to 1.

I also want to sound this idea here to check if somebody has objections
with that approach?

> 
> Laurie
> 

Marcus



Re: Add ability to set control values with video(1)

2020-07-24 Thread Laurence Tratt
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)

2020-07-23 Thread Marcus Glocker
Hi Laurie,

On Thu, 23 Jul 2020 21:07:26 +0100
Laurence Tratt  wrote:

> 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!

Ok, cool - I try to collect an OK therefore :-)

> 
> Laurie
> 

Thanks,
Marcus



Re: Add ability to set control values with video(1)

2020-07-23 Thread Laurence Tratt
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)

2020-07-23 Thread Marcus Glocker
On Wed, 22 Jul 2020 21:52:27 +0100
Laurence Tratt  wrote:

> 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 :)

How's that?

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've quickly tested this here and it seems to work for me ...


Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 -  1.15
+++ video.1 23 Jul 2020 19:44:21 -
@@ -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 @@ Index of
 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 are displayed during a reset 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 -p -u -p -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 -  1.31
+++ video.c 23 Jul 2020 19:44:22 -
@@ -192,7 +192,10 @@ struct video {
 #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 @@ int dev_get_ctrls(struct video *);
 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_auto_white_balance(struct video *, 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, 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 @@ void
 usage(void)
 {
fprintf(stderr, "usage: %s [-gqRv] "
-   "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
-   "   %*s [-o output] [-r rate] [-s 

Re: Add ability to set control values with video(1)

2020-07-22 Thread Laurence Tratt
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)

2020-07-22 Thread Marcus Glocker
Hello Laurie,

On Tue, 21 Jul 2020 21:18:15 +0100
Laurence Tratt  wrote:

> 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?

That makes much more sense to me.

> > 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).

Ah right, I got you wrong then, sorry.  I thought you require this for
each control setting.

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.

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?

One more inline comment regarding the man page.

> 
> Laurie

Thanks,
Marcus

> 
> 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 .

'-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."

>  .It Fl e Ar encoding
>  Lowercase FOURCC name of video encoding to use.
>  Valid arguments are
> 

Re: Add ability to set control values with video(1)

2020-07-21 Thread Laurence Tratt
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)

2020-07-21 Thread Marcus Glocker
Hi Laurie,

See my comments inline.

On Fri, 17 Jul 2020 22:51:30 +0100
Laurence Tratt  wrote:

> 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_FILE0x4
>  #define M_OUT_FILE   0x8
>  #define M_QUERY  0x10
> +#define M_RESET  0x20
> +#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);

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.

>  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 @@
>  

Re: Add ability to set control values with video(1)

2020-07-17 Thread Laurence Tratt
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)
-   dev_set_ctrl(vid, CTRL_SHARPNESS, -1);
+   dev_inc_ctrl(vid, CTRL_SHARPNESS, -1);
break;
case 'B':
if 

Add ability to set control values with video(1)

2020-07-13 Thread Laurence Tratt
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