Some comments on the new autocluster patches

Some comments on the new autocluster patches

[Hans de Goede]

Hi Hans (et all),

I've been working on doing a much needed cleanup to the pwc driver,
converting it to videobuf2 and using the new ctrl framework.

I hope to be able to send a pull request for this, this weekend.

I saw your pull request and I'm looking forward to be able to
play with ctrl events. I do have some comments on your autofoo
cluster patches and related changes though.

First of all there is:
"v4l2-ctrls: fix and improve volatile control handling."

I must admit I was a bit confused about needing to set
cur.val rather then just val from the g_volatile_ctrl op
myself at first too, but I've gotten used to it now :)

With that said I'm not quite sure I like to proposed change
though, where g_ctrl will return the new value as long as the
control is volatile and then jump to the old cur val when
it turns non volatile (ie autogain is turned off) this seems
wrong to me, it will certainly surprise both driver writers
and v4l2 app users alike!

Also this requires special care taking by drivers when ie
autogain gets turned off, either they need to update cur.val
with the real current value, or they need to send the cur.val
to the device at this point to ensure the device's setting
and cur.val match.

Actually this brings me to the second point, making ie gain
non volatile as soon as autogain gets turned off, can be wrong,
as the gain may have changed between the last g_volatile_ctrl
call and the autogain getting turned off. I admit that
your solution of simply not updating cur.val and at all as long
as the ctrl is volatile and then jump back to cur.val avoids
this, but I find that less then ideal.

The entire model of having a separate manual value (stored in
cur.val) and an autocontrolled value stored in val as long
as the control is volatile, seems to assume a device with
2 separate registers for gain, one with the active gain,
and one with a manual gain preset. Where in auto mode only
the active gain is controlled and switching to manual
gain mode copies the manual gain value to the active gain.

This is IMHO a pretty limited model of reality, I know
you have experience with a device which happens to work
like that, but many do not work like that.

Actually the pwc has 2 registers, but when switching to
manual mode, it updates the manual setting register, with
the last value set in the active register by the autogain,
so the other way around then your model assumes.


But we should not be looking at existing hardware at all,
instead we should be looking at the user experience, and
build our model from there. Drivers will have to cope
with all the different variations on this theme at the
driver level IMHO.

And looking from the users perspective the right choice
is obvious IMHO. When the user turns of auto-foo, then
following the principle of least surprise, the right
thing to do is to leave the control at its current
setting, because likely the user wants to make some
adjustments to the auto-foo chosen value for foo.
Rather then to start over with some $random (from the
users perspective) value for foo. Imagine that
auto-foo has been on since driver loading, then the
value in cur.val has never been seen by the user before,
yet switching to manual all of a sudden switches
to this unseen value -> confused user

I suggest that when an auto-foo control gets turned of
the code calls g_volatile_ctrl one last time after
actually turning it off and stores the result in cur.val

Actually in my current pwc code I've done this by moving
the clearing of the volatile flag to the g_volatile_ctrl
op, when g_volatile_ctrl-foo gets called and auto-foo is
off, then g_volatile_ctrl-foo clears the volatile flag.

This avoid needlessly calling g_volatile_ctrl-foo if
auto-foo gets turned off, but no one cares about the
value of foo after that.

Note that this depends on the old behavior of
g_volatile_ctrl setting cur.val rather then just val.

Regards,

Hans

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Friday, July 01, 2011 17:06:43 Hans de Goede wrote:
> Hi Hans (et all),
> 
> I've been working on doing a much needed cleanup to the pwc driver,
> converting it to videobuf2 and using the new ctrl framework.
> 
> I hope to be able to send a pull request for this, this weekend.

Thanks!

> I saw your pull request and I'm looking forward to be able to
> play with ctrl events. I do have some comments on your autofoo
> cluster patches and related changes though.
> 
> First of all there is:
> "v4l2-ctrls: fix and improve volatile control handling."
> 
> I must admit I was a bit confused about needing to set
> cur.val rather then just val from the g_volatile_ctrl op
> myself at first too, but I've gotten used to it now :)
> 
> With that said I'm not quite sure I like to proposed change
> though, where g_ctrl will return the new value as long as the
> control is volatile and then jump to the old cur val when
> it turns non volatile (ie autogain is turned off) this seems
> wrong to me, it will certainly surprise both driver writers
> and v4l2 app users alike!
> 
> Also this requires special care taking by drivers when ie
> autogain gets turned off, either they need to update cur.val
> with the real current value, or they need to send the cur.val
> to the device at this point to ensure the device's setting
> and cur.val match.
> 
> Actually this brings me to the second point, making ie gain
> non volatile as soon as autogain gets turned off, can be wrong,
> as the gain may have changed between the last g_volatile_ctrl
> call and the autogain getting turned off. I admit that
> your solution of simply not updating cur.val and at all as long
> as the ctrl is volatile and then jump back to cur.val avoids
> this, but I find that less then ideal.
> 
> The entire model of having a separate manual value (stored in
> cur.val) and an autocontrolled value stored in val as long
> as the control is volatile, seems to assume a device with
> 2 separate registers for gain, one with the active gain,
> and one with a manual gain preset. Where in auto mode only
> the active gain is controlled and switching to manual
> gain mode copies the manual gain value to the active gain.
> 
> This is IMHO a pretty limited model of reality, I know
> you have experience with a device which happens to work
> like that, but many do not work like that.

The problem with the old approach was that cur.val was overwritten. So you
lost that information completely. The new approach keeps that value and
it is up to the driver what to do when the autogain is switched off.

In s_ctrl you can either use val (this is either a new manual gain value
set by the application, or the last manual gain value), or you can do
something like this:

if (autogain->cur.val == 1 && autogain->val == 0 && !gain->is_new)
	gain->val = read_gain();

But I also see my patch below :-)

> Actually the pwc has 2 registers, but when switching to
> manual mode, it updates the manual setting register, with
> the last value set in the active register by the autogain,
> so the other way around then your model assumes.

One possible problem with this approach BTW is that those gain values
set by the autogain on the chip are not always reliable. If I toggle the
autogain quickly from false to true to false, then the gain value after
I switch back to false from e.g. the saa7115 is usually bogus.

> 
> 
> But we should not be looking at existing hardware at all,
> instead we should be looking at the user experience, and
> build our model from there. Drivers will have to cope
> with all the different variations on this theme at the
> driver level IMHO.
> 
> And looking from the users perspective the right choice
> is obvious IMHO. When the user turns of auto-foo, then
> following the principle of least surprise, the right
> thing to do is to leave the control at its current
> setting, because likely the user wants to make some
> adjustments to the auto-foo chosen value for foo.
> Rather then to start over with some $random (from the
> users perspective) value for foo. Imagine that
> auto-foo has been on since driver loading, then the
> value in cur.val has never been seen by the user before,
> yet switching to manual all of a sudden switches
> to this unseen value -> confused user

Is this the least surprise? First of all: generally user-visible controls
like gain are not normally refreshed, so the value you see is the value the
user set the last time. Calling g_volatile_ctrl when switching back to manual
gain would actually unexpectedly change the value. Another reason for not doing
this is that the user might have painstakingly determined a great manual gain
value, which is completely undone if he switches on autogain mode and then
switches it off again.

It's not so simple...

Think of a TV: the manual values don't change when you turn on or off an
autofoo control. Frankly, I think that volatile behavior for writable controls
is dubious at best. Perhaps we should add a flag that you need to explicitly
set in order to get the volatile value. E.g. G_CTRL(V4L2_CID_GAIN) gives the
normal manual gain, and G_CTRL(V4L2_CID_GAIN | V4L2_CTRL_FLAG_VOLATILE) gives
the 'volatile' gain.

Or just add a proper read-only volatile control like AUTOGAIN_GAIN (ugly name).

> I suggest that when an auto-foo control gets turned of
> the code calls g_volatile_ctrl one last time after
> actually turning it off and stores the result in cur.val

I would prefer to make this optional by passing an extra flag or
something like that to v4l2_ctrl_autocluster. It's easy enough to do
in the control framework, but hardware varies too much to assume that
this is what you always want.

> Actually in my current pwc code I've done this by moving
> the clearing of the volatile flag to the g_volatile_ctrl
> op, when g_volatile_ctrl-foo gets called and auto-foo is
> off, then g_volatile_ctrl-foo clears the volatile flag.
> 
> This avoid needlessly calling g_volatile_ctrl-foo if
> auto-foo gets turned off, but no one cares about the
> value of foo after that.

I think I need to see the code first.
 
> Note that this depends on the old behavior of
> g_volatile_ctrl setting cur.val rather then just val.

This new behavior is definitely better and I want to keep that.

Below is a patch for v4l2-ctrls.c that changes the behavior in just the
way you want it.

It's on top of my 'core8c' branch:

http://git.linuxtv.org/hverkuil/media_tree.git?a=shortlog;h=refs/heads/core8c

Regards,

	Hans

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 37a50e5..65d9be7 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1915,6 +1915,15 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			if (master->cluster[j])
 				master->cluster[j]->is_new = 0;
 
+		if (has_op(master, g_volatile_ctrl) && !is_cur_manual(master)) {
+			for (j = 0; j < master->ncontrols; j++)
+				cur_to_new(master->cluster[j]);
+			if (!call_op(master, g_volatile_ctrl))
+				for (j = 1; j < master->ncontrols; j++)
+					if (master->cluster[j]->is_volatile)
+						master->cluster[j]->is_new = 1;
+		}
+
 		/* Copy the new caller-supplied control values.
 		   user_to_new() sets 'is_new' to 1. */
 		do {

Re: Some comments on the new autocluster patches

[Mauro Carvalho Chehab]

Em 01-07-2011 12:06, Hans de Goede escreveu:
> Hi Hans (et all),
> 
> I've been working on doing a much needed cleanup to the pwc driver,
> converting it to videobuf2 and using the new ctrl framework.
> 
> I hope to be able to send a pull request for this, this weekend.
> 
> I saw your pull request and I'm looking forward to be able to
> play with ctrl events. I do have some comments on your autofoo
> cluster patches and related changes though.
>

Hi Hans & Hans,

I've applied Hans V. patches at the tree, to allow them to be better
tested. I'm not 100% comfortable with the patches, and, from my understanding
the poll() changes are needed, in order to fix vivi behavior and add the
event patches for ivtv. I didn't have much time to test the patches (is qv4l2
already ready to test them?)

Due to that, it is probably safer to hold those patches to be merged upstream 
at 3.2, after playing with them for a while at the development tree and at -next.

So, feel free to suggest changes without being stick to the current API, as, while
they're not merging upstream, we can change/fix some things that aren't behaving
well.

Regards,
Mauro

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Saturday, July 02, 2011 02:55:10 Mauro Carvalho Chehab wrote:
> Em 01-07-2011 12:06, Hans de Goede escreveu:
> > Hi Hans (et all),
> > 
> > I've been working on doing a much needed cleanup to the pwc driver,
> > converting it to videobuf2 and using the new ctrl framework.
> > 
> > I hope to be able to send a pull request for this, this weekend.
> > 
> > I saw your pull request and I'm looking forward to be able to
> > play with ctrl events. I do have some comments on your autofoo
> > cluster patches and related changes though.
> >
> 
> Hi Hans & Hans,
> 
> I've applied Hans V. patches at the tree, to allow them to be better
> tested. I'm not 100% comfortable with the patches, and, from my understanding
> the poll() changes are needed, in order to fix vivi behavior and add the
> event patches for ivtv. I didn't have much time to test the patches (is qv4l2
> already ready to test them?)

I have a version that I use for testing in this repo:

http://git.linuxtv.org/hverkuil/v4l-utils.git?a=shortlog;h=refs/heads/core

It's pretty ugly, though. Now that the patches are merged I'll work on cleaning
it up. I do have patches for v4l2-ctl as well to easily test control events.
That's good code and I'll commit that today.

Regarding qv4l2: as long as the poll() situation is not resolved I can't commit
it to v4l-utils anyway. It is just doesn't work in that specific situation.

> Due to that, it is probably safer to hold those patches to be merged upstream 
> at 3.2, after playing with them for a while at the development tree and at -next.

Note that the only time these patches become problematic is when you want to
wait on events, and just events. None of these patches will break existing
applications. It is also not a new problem, we had this issue ever since we
introduced DQEVENT. It's just that this new control event is more likely to
be used in situations where this issue is triggered (e.g. qv4l2-like apps).

Since this control event is also going to be used in embedded applications
(flash API and the HDMI API we are working on) I am not in favor of delaying
this an extra kernel cycle. Delaying the qv4l2 changes is another matter of
course. Those will have to wait. Hmm, I could only enable the control events
in qv4l2 for drivers that do *not* support the read/write API. That would
work, I guess. I'll have to think about that a bit.

Regards,

	Hans

> So, feel free to suggest changes without being stick to the current API, as, while
> they're not merging upstream, we can change/fix some things that aren't behaving
> well.

Re: Some comments on the new autocluster patches

[Hans de Goede]

Hi,

<snip snip snip>

Ok, thinking about this some more and reading Hans V's comments
I think that the current code in Hans V's core8c branch is fine,
and should go to 3.1 (rather then be delayed to 3.2).

As for the fundamental question what to do with foo
controls when autofoo goes from auto to manual, as discussed
there are 2 options:
1) Restore the last known / previous manual setting
2) Keep foo at the current setting, iow the last setting
    configured by autofoo

Although it would be great if we could standardize on
one of these. I think that the answer here is to leave
this decision to the driver:
- In some cases this may not be under our control at all
   (ie with uvc devices),
-in other cases the hardware in question may make it
  impossible to read the setting as configured by autofoo,
  thus forcing scenario 1 so that we are sure the actual
  value for foo being used by the device matches what we
  report to the user once autofoo is in manual mode

That leaves Hans V's suggestion what to do with volatile
controls wrt reporting this to userspace. Hans V. suggested
splitting the control essentially in 2 controls, one r/w
with the manual value and a read only one with the volatile
value (*). I don't think this is a good idea, having 2
controls for one foo, will only clutter v4l2 control panels
or applets. I think we should try to keep the controls
we present to the user (and thus too userspace) to a minimum.

I suggest that instead of creating 2 controls, we add a
VOLATILE ctrl flag, which can then be set together with
the INACTIVE flag to indicate to a v4l2 control panel that
the value may change without it receiving change events. The
v4l2 control panel can then decide how it wants to deal with
this, ie poll to keep its display updated, ignore the flag,
...

Regards,

Hans


*) Either through a special flag signalling give me the
volatile value, or just outright making the 2 2 separate
controls.

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Saturday, July 02, 2011 12:28:35 Hans de Goede wrote:
> Hi,
> 
> <snip snip snip>
> 
> Ok, thinking about this some more and reading Hans V's comments
> I think that the current code in Hans V's core8c branch is fine,
> and should go to 3.1 (rather then be delayed to 3.2).
> 
> As for the fundamental question what to do with foo
> controls when autofoo goes from auto to manual, as discussed
> there are 2 options:
> 1) Restore the last known / previous manual setting
> 2) Keep foo at the current setting, iow the last setting
>     configured by autofoo

Or option 3:

Just don't report the automatic foo values at all. What possible purpose
does it serve? It is my impression that drivers implement it 'just because
they can', and not because it is meaningful.

I'm not aware of any application that actually refreshes e.g. gain values
when autogain is on, so end-users never see it anyway.

Volatile makes a lot of sense for read-only controls, but for writable
controls I really don't see why you would want it.

> Although it would be great if we could standardize on
> one of these. I think that the answer here is to leave
> this decision to the driver:
> - In some cases this may not be under our control at all
>    (ie with uvc devices),
> -in other cases the hardware in question may make it
>   impossible to read the setting as configured by autofoo,
>   thus forcing scenario 1 so that we are sure the actual
>   value for foo being used by the device matches what we
>   report to the user once autofoo is in manual mode
> 
> That leaves Hans V's suggestion what to do with volatile
> controls wrt reporting this to userspace. Hans V. suggested
> splitting the control essentially in 2 controls, one r/w
> with the manual value and a read only one with the volatile
> value (*). I don't think this is a good idea, having 2
> controls for one foo, will only clutter v4l2 control panels
> or applets. I think we should try to keep the controls
> we present to the user (and thus too userspace) to a minimum.

I agree with that.

> I suggest that instead of creating 2 controls, we add a
> VOLATILE ctrl flag, which can then be set together with
> the INACTIVE flag to indicate to a v4l2 control panel that
> the value may change without it receiving change events. The
> v4l2 control panel can then decide how it wants to deal with
> this, ie poll to keep its display updated, ignore the flag,
> ...

A volatile flag is certainly useful for read-only controls.

But I think we should stop supporting volatile writable controls.

It makes no sense from the point of view of a user. You won't see such behavior
in TVs etc. either. In rare cases you might want to export it through the
subdev API as a separate control so that advanced applications can get hold of
that value.

Does anyone know why you would want volatile writable controls?

Regards,

	Hans

> 
> Regards,
> 
> Hans
> 
> 
> *) Either through a special flag signalling give me the
> volatile value, or just outright making the 2 2 separate
> controls.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Some comments on the new autocluster patches

[Hans de Goede]

Hi,

On 07/02/2011 01:10 PM, Hans Verkuil wrote:
> On Saturday, July 02, 2011 12:28:35 Hans de Goede wrote:
>> Hi,
>>
>> <snip snip snip>
>>
>> Ok, thinking about this some more and reading Hans V's comments
>> I think that the current code in Hans V's core8c branch is fine,
>> and should go to 3.1 (rather then be delayed to 3.2).
>>
>> As for the fundamental question what to do with foo
>> controls when autofoo goes from auto to manual, as discussed
>> there are 2 options:
>> 1) Restore the last known / previous manual setting
>> 2) Keep foo at the current setting, iow the last setting
>>      configured by autofoo
>
> Or option 3:
>
> Just don't report the automatic foo values at all. What possible purpose
> does it serve?
Reporting should be seen separate of what to do with the actual
setting of for example gain as in use by the device when autogain
gets turned off, that is what I'm talking about here, when autogain
gets turned off (iow gain gets set to manual) there are 2 and only
2 options

1) leave the gain at the value last set by the devices
    autogain function (this may not be supported on all hardware)
2) restore the last known manual gain setting

What we report or not report for gain while autogain is active
is irrelevant for this choice, when switching to manual we can
either leave gain as is, or we restore the last known setting.
Independent of any values we may have reported.

 > It is my impression that drivers implement it 'just because
 > they can', and not because it is meaningful.

Well it is drivers responsibility to export hardware functionality
(in a standardized manner), then it is up to applications whether
they use it or not. And it is actually quite meaning full, you
are very much thinking TV and not webcams here, being able to
see that the autofoo is actually doing something, and what
it is doing is very useful for webcams. For example maybe it is
choosing a low exposure (to get highframerate) high gain, which
leads to more noise in the picture then the user wants

webcams are like photography, you've a shutter and a sensitivity
(iso) setting being able to see what a camera chooses in full
auto mode is quite useful.

> I'm not aware of any application that actually refreshes e.g. gain values
> when autogain is on, so end-users never see it anyway.

v4l2ucp has an option to update the ctrl readings every 1 / 2 / 5
seconds. And I use this often to track what the autofoo is doing
and / or to verify that it doing anything at all.

> But I think we should stop supporting volatile writable controls.

NACK, and note that we already don't do that, what we do is switch
a control from volatile read only (inactive) to non volatile rw-mode
and back. The only question is what to do at the transition.

Regards,

Hans

Re: Some comments on the new autocluster patches

[Mauro Carvalho Chehab]

Em 02-07-2011 06:36, Hans Verkuil escreveu:
> On Saturday, July 02, 2011 02:55:10 Mauro Carvalho Chehab wrote:
>> Em 01-07-2011 12:06, Hans de Goede escreveu:
>>> Hi Hans (et all),
>>>
>>> I've been working on doing a much needed cleanup to the pwc driver,
>>> converting it to videobuf2 and using the new ctrl framework.
>>>
>>> I hope to be able to send a pull request for this, this weekend.
>>>
>>> I saw your pull request and I'm looking forward to be able to
>>> play with ctrl events. I do have some comments on your autofoo
>>> cluster patches and related changes though.
>>>
>>
>> Hi Hans & Hans,
>>
>> I've applied Hans V. patches at the tree, to allow them to be better
>> tested. I'm not 100% comfortable with the patches, and, from my understanding
>> the poll() changes are needed, in order to fix vivi behavior and add the
>> event patches for ivtv. I didn't have much time to test the patches (is qv4l2
>> already ready to test them?)
> 
> I have a version that I use for testing in this repo:
> 
> http://git.linuxtv.org/hverkuil/v4l-utils.git?a=shortlog;h=refs/heads/core
> 
> It's pretty ugly, though. Now that the patches are merged I'll work on cleaning
> it up. I do have patches for v4l2-ctl as well to easily test control events.
> That's good code and I'll commit that today.
> 
> Regarding qv4l2: as long as the poll() situation is not resolved I can't commit
> it to v4l-utils anyway. It is just doesn't work in that specific situation.
> 
>> Due to that, it is probably safer to hold those patches to be merged upstream 
>> at 3.2, after playing with them for a while at the development tree and at -next.
> 
> Note that the only time these patches become problematic is when you want to
> wait on events, and just events. None of these patches will break existing
> applications. It is also not a new problem, we had this issue ever since we
> introduced DQEVENT. It's just that this new control event is more likely to
> be used in situations where this issue is triggered (e.g. qv4l2-like apps).
> 
> Since this control event is also going to be used in embedded applications
> (flash API and the HDMI API we are working on) I am not in favor of delaying
> this an extra kernel cycle. Delaying the qv4l2 changes is another matter of
> course. Those will have to wait. Hmm, I could only enable the control events
> in qv4l2 for drivers that do *not* support the read/write API. That would
> work, I guess. I'll have to think about that a bit.

DQEVENT is used only be one driver, currently (I never count vivi, as it is not really
a driver).

The new control event will not be used by any driver until we solve the poll issue,
so, there's no much sense to hurry and merge it before solving the polling issue, as
we might need to change something, depending on the feedback we'll got from fs people.

Cheers,
Mauro.

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Saturday, July 02, 2011 16:31:47 Hans de Goede wrote:
> Hi,
> 
> On 07/02/2011 01:10 PM, Hans Verkuil wrote:
> > On Saturday, July 02, 2011 12:28:35 Hans de Goede wrote:
> >> Hi,
> >>
> >> <snip snip snip>
> >>
> >> Ok, thinking about this some more and reading Hans V's comments
> >> I think that the current code in Hans V's core8c branch is fine,
> >> and should go to 3.1 (rather then be delayed to 3.2).
> >>
> >> As for the fundamental question what to do with foo
> >> controls when autofoo goes from auto to manual, as discussed
> >> there are 2 options:
> >> 1) Restore the last known / previous manual setting
> >> 2) Keep foo at the current setting, iow the last setting
> >>      configured by autofoo
> >
> > Or option 3:
> >
> > Just don't report the automatic foo values at all. What possible purpose
> > does it serve?
> Reporting should be seen separate of what to do with the actual
> setting of for example gain as in use by the device when autogain
> gets turned off, that is what I'm talking about here, when autogain
> gets turned off (iow gain gets set to manual) there are 2 and only
> 2 options
> 
> 1) leave the gain at the value last set by the devices
>     autogain function (this may not be supported on all hardware)
> 2) restore the last known manual gain setting
> 
> What we report or not report for gain while autogain is active
> is irrelevant for this choice, when switching to manual we can
> either leave gain as is, or we restore the last known setting.
> Independent of any values we may have reported.

It is relevant. Take an application that saves the current state of all 
controls and restores it the next time it is started. If you report the 
device's autogain value instead of the manual gain, then that manual gain 
value is lost. I consider this a major drawback.
 
>  > It is my impression that drivers implement it 'just because
>  > they can', and not because it is meaningful.
> 
> Well it is drivers responsibility to export hardware functionality
> (in a standardized manner), then it is up to applications whether
> they use it or not. And it is actually quite meaning full, you
> are very much thinking TV and not webcams here, being able to
> see that the autofoo is actually doing something, and what
> it is doing is very useful for webcams. For example maybe it is
> choosing a low exposure (to get highframerate) high gain, which
> leads to more noise in the picture then the user wants
> 
> webcams are like photography, you've a shutter and a sensitivity
> (iso) setting being able to see what a camera chooses in full
> auto mode is quite useful.

OK, but it is not useful that this means that you don't see the manual value 
anymore.

> > I'm not aware of any application that actually refreshes e.g. gain values
> > when autogain is on, so end-users never see it anyway.
> 
> v4l2ucp has an option to update the ctrl readings every 1 / 2 / 5
> seconds. And I use this often to track what the autofoo is doing
> and / or to verify that it doing anything at all.

OK, good to know.

> > But I think we should stop supporting volatile writable controls.
> 
> NACK, and note that we already don't do that, what we do is switch
> a control from volatile read only (inactive) to non volatile rw-mode
> and back. The only question is what to do at the transition.

No, the question is also what to return.

How many 'autofoo' controls are there anyway?

V4L2_CID_AUTO_WHITE_BALANCE
V4L2_CID_AUTOGAIN
V4L2_CID_EXPOSURE_AUTO
V4L2_CID_AUTOBRIGHTNESS
V4L2_CID_HUE_AUTO

Those last two are used in only two drivers (gspca and uvc respectively).

The first three would require four extra read-only volatile controls:

V4L2_CID_AUTOWB_RED_BALANCE
V4L2_CID_AUTOWB_BLUE_BALANCE
V4L2_CID_AUTOGAIN_GAIN
V4L2_CID_AUTOEXP_EXPOSURE

Simple and straightforward. Applications can show the manual value and the 
autofoo value together so you can compare them easily. No unexpected 
transitions since turning off the autofoo will restore the manual foo value.

And apps that save/restore controls will always get/set the proper manual foo 
values.

The most difficult part will be to come up with a decent description of these 
controls:

"Gain, Automatic Value"
"Gain, Computed Value"
"Gain, Current Value"
"Gain, Current"

Hmm, those last two aren't so bad since that would fit equally whether 
autogain is on or off.

That suggests better CID naming as well:

V4L2_CID_RED_BALANCE_CUR ("Red Balance, Current")
V4L2_CID_BLUE_BALANCE_CUR ("Blue Balance, Current")
V4L2_CID_GAIN_CUR ("Gain, Current")
V4L2_CID_EXPOSURE_CUR ("Exposure, Current")

Simple, straightforward, no confusion.

Regards,

	Hans

Re: Some comments on the new autocluster patches

[Hans de Goede]

Hi,

On 07/04/2011 11:43 AM, Hans Verkuil wrote:
> On Saturday, July 02, 2011 16:31:47 Hans de Goede wrote:
>> Hi,
>>
>> On 07/02/2011 01:10 PM, Hans Verkuil wrote:
>>> On Saturday, July 02, 2011 12:28:35 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> <snip snip snip>
>>>>
>>>> Ok, thinking about this some more and reading Hans V's comments
>>>> I think that the current code in Hans V's core8c branch is fine,
>>>> and should go to 3.1 (rather then be delayed to 3.2).
>>>>
>>>> As for the fundamental question what to do with foo
>>>> controls when autofoo goes from auto to manual, as discussed
>>>> there are 2 options:
>>>> 1) Restore the last known / previous manual setting
>>>> 2) Keep foo at the current setting, iow the last setting
>>>>       configured by autofoo
>>>
>>> Or option 3:
>>>
>>> Just don't report the automatic foo values at all. What possible purpose
>>> does it serve?
>> Reporting should be seen separate of what to do with the actual
>> setting of for example gain as in use by the device when autogain
>> gets turned off, that is what I'm talking about here, when autogain
>> gets turned off (iow gain gets set to manual) there are 2 and only
>> 2 options
>>
>> 1) leave the gain at the value last set by the devices
>>      autogain function (this may not be supported on all hardware)
>> 2) restore the last known manual gain setting
>>
>> What we report or not report for gain while autogain is active
>> is irrelevant for this choice, when switching to manual we can
>> either leave gain as is, or we restore the last known setting.
>> Independent of any values we may have reported.
>
> It is relevant. Take an application that saves the current state of all
> controls and restores it the next time it is started. If you report the
> device's autogain value instead of the manual gain, then that manual gain
> value is lost. I consider this a major drawback.

If autogain is on, then the gain is RO, so it should not be saved. Let alone
restored.

>
>>   >  It is my impression that drivers implement it 'just because
>>   >  they can', and not because it is meaningful.
>>
>> Well it is drivers responsibility to export hardware functionality
>> (in a standardized manner), then it is up to applications whether
>> they use it or not. And it is actually quite meaning full, you
>> are very much thinking TV and not webcams here, being able to
>> see that the autofoo is actually doing something, and what
>> it is doing is very useful for webcams. For example maybe it is
>> choosing a low exposure (to get highframerate) high gain, which
>> leads to more noise in the picture then the user wants
>>
>> webcams are like photography, you've a shutter and a sensitivity
>> (iso) setting being able to see what a camera chooses in full
>> auto mode is quite useful.
>
> OK, but it is not useful that this means that you don't see the manual value
> anymore.
>

In normal webcam use the lighting conditions are constantly changing, so
the gain value manually set 5 minutes ago is of little value, as it
is likely wrong for the current situation.

>>> I'm not aware of any application that actually refreshes e.g. gain values
>>> when autogain is on, so end-users never see it anyway.
>>
>> v4l2ucp has an option to update the ctrl readings every 1 / 2 / 5
>> seconds. And I use this often to track what the autofoo is doing
>> and / or to verify that it doing anything at all.
>
> OK, good to know.
>
>>> But I think we should stop supporting volatile writable controls.
>>
>> NACK, and note that we already don't do that, what we do is switch
>> a control from volatile read only (inactive) to non volatile rw-mode
>> and back. The only question is what to do at the transition.
>
> No, the question is also what to return.

What to return sort of follows from what you do when turning
of autogain, if you keep the last autogain set gain, then it
makes sense to return the autogain set value when reading gain, if
you switch back to the last manually set gain, then it
makes sense to just report the last manually set gain as long
as autogain is on.

I still believe that everything boils down to 2 possible scenarios,
and the rest follows from that. With the 2 scenarios being:

1) There is a manual setting which is constant until explicitly
changed, when (ie) gain switches from auto mode to manual mode
then the actual used gain is reset to this manual setting

2) There is a single gain setting / register, which is r/w when the
control is in manual mode and ro when in auto mode. When auto mode
gets switched off, the gain stays at the last value set by auto mode.

2) Is what most webcam sensors (and the pwc firmware) implement at
the hardware level, and what to me also makes the most sense for webcams.

To me this whole discussion centers around these 2 scenarios, with you
being a proponent of 1), and I guess that for video capture boards 1 makes
a lot of sense, and me being a proponent of 2.

Proposal: lets agree that these 2 methods of handling autofoo controls
both exist and both have merits in certain cases, this means letting
it be up to the driver to choose which method to implement.

If we can agree on this, then the next step would be to document both
methods, as well as how the controls should behave in either scenario.
I'm willing to write up a first draft for this.


> How many 'autofoo' controls are there anyway?
>
> V4L2_CID_AUTO_WHITE_BALANCE
> V4L2_CID_AUTOGAIN
> V4L2_CID_EXPOSURE_AUTO
> V4L2_CID_AUTOBRIGHTNESS
> V4L2_CID_HUE_AUTO
>
> Those last two are used in only two drivers (gspca and uvc respectively).
>
> The first three would require four extra read-only volatile controls:
>
> V4L2_CID_AUTOWB_RED_BALANCE
> V4L2_CID_AUTOWB_BLUE_BALANCE
> V4L2_CID_AUTOGAIN_GAIN
> V4L2_CID_AUTOEXP_EXPOSURE
>

I can see this making sense for drivers which choose to implement
scenario 1, but I see no value for drivers which choose to implement
scenario 2, it is just another control cluttering the control applet.

> Simple and straightforward. Applications can show the manual value and the
> autofoo value together so you can compare them easily. No unexpected
> transitions since turning off the autofoo will restore the manual foo value.

I would actually consider that an unexpected transition. Lets consider
auto exposure, and the last manual exposure was set during daytime, iow with
lots of daylight, so a low exposure setting. Now there is a stream active,
and being recorded in the evening. The user is not completely happy with the
autoexposure chosen value, and turns of autoexposure. The very low exposure
from the day gets restored, which is much too low, the image turns black,
and the recording is ruined, not good.

Or the exposure stays at its last automatically set value, the user can make
a small adjustment and all is well ... Note that this is exactly what most
hardware does, to avoid the scenario above.

We just seem to come at this from 2 completely different mindsets, to me
going from the autofoo foo value to some manual value which may be completely
unappropriate is an unexpected transition. IOW to me restoring the manual
foo value is the unexpected thing to do.

I think we need to agree that we disagree :)

Regards,

Hans

Re: Some comments on the new autocluster patches

[Hans Verkuil]

OK, I'm back to work after my vacation, so it's time to go through the
backlog...

On Tuesday, July 12, 2011 15:25:02 Hans de Goede wrote:
> Hi,
> 
> On 07/04/2011 11:43 AM, Hans Verkuil wrote:
> > On Saturday, July 02, 2011 16:31:47 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 07/02/2011 01:10 PM, Hans Verkuil wrote:
> >>> On Saturday, July 02, 2011 12:28:35 Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> <snip snip snip>
> >>>>
> >>>> Ok, thinking about this some more and reading Hans V's comments
> >>>> I think that the current code in Hans V's core8c branch is fine,
> >>>> and should go to 3.1 (rather then be delayed to 3.2).
> >>>>
> >>>> As for the fundamental question what to do with foo
> >>>> controls when autofoo goes from auto to manual, as discussed
> >>>> there are 2 options:
> >>>> 1) Restore the last known / previous manual setting
> >>>> 2) Keep foo at the current setting, iow the last setting
> >>>>       configured by autofoo
> >>>
> >>> Or option 3:
> >>>
> >>> Just don't report the automatic foo values at all. What possible purpose
> >>> does it serve?
> >> Reporting should be seen separate of what to do with the actual
> >> setting of for example gain as in use by the device when autogain
> >> gets turned off, that is what I'm talking about here, when autogain
> >> gets turned off (iow gain gets set to manual) there are 2 and only
> >> 2 options
> >>
> >> 1) leave the gain at the value last set by the devices
> >>      autogain function (this may not be supported on all hardware)
> >> 2) restore the last known manual gain setting
> >>
> >> What we report or not report for gain while autogain is active
> >> is irrelevant for this choice, when switching to manual we can
> >> either leave gain as is, or we restore the last known setting.
> >> Independent of any values we may have reported.
> >
> > It is relevant. Take an application that saves the current state of all
> > controls and restores it the next time it is started. If you report the
> > device's autogain value instead of the manual gain, then that manual gain
> > value is lost. I consider this a major drawback.
> 
> If autogain is on, then the gain is RO, so it should not be saved. Let alone
> restored.

Marking gain as inactive is fine, but marking it as read-only is not so clear.
Currently the RO flag is static. This allows control panels to use e.g. a text
field instead of an input field to show the value.

I would like to keep that functionality. If we make the RO flag dynamic, then
GUIs won't know whether to show it as a disabled input field or as a text field.

Whereas with the inactive flag they will know that it has to be a disabled
input field.

When the inactive flag is set, it is still allowed to set the value. However,
if we add a volatile flag as well, then we may want to have the combination
'inactive and volatile' return an error when an application attempts to set the
value.

Or is this too complex and should we just discard the value in a case like that?

> >
> >>   >  It is my impression that drivers implement it 'just because
> >>   >  they can', and not because it is meaningful.
> >>
> >> Well it is drivers responsibility to export hardware functionality
> >> (in a standardized manner), then it is up to applications whether
> >> they use it or not. And it is actually quite meaning full, you
> >> are very much thinking TV and not webcams here, being able to
> >> see that the autofoo is actually doing something, and what
> >> it is doing is very useful for webcams. For example maybe it is
> >> choosing a low exposure (to get highframerate) high gain, which
> >> leads to more noise in the picture then the user wants
> >>
> >> webcams are like photography, you've a shutter and a sensitivity
> >> (iso) setting being able to see what a camera chooses in full
> >> auto mode is quite useful.
> >
> > OK, but it is not useful that this means that you don't see the manual value
> > anymore.
> >
> 
> In normal webcam use the lighting conditions are constantly changing, so
> the gain value manually set 5 minutes ago is of little value, as it
> is likely wrong for the current situation.

True.
 
> >>> I'm not aware of any application that actually refreshes e.g. gain values
> >>> when autogain is on, so end-users never see it anyway.
> >>
> >> v4l2ucp has an option to update the ctrl readings every 1 / 2 / 5
> >> seconds. And I use this often to track what the autofoo is doing
> >> and / or to verify that it doing anything at all.
> >
> > OK, good to know.
> >
> >>> But I think we should stop supporting volatile writable controls.
> >>
> >> NACK, and note that we already don't do that, what we do is switch
> >> a control from volatile read only (inactive) to non volatile rw-mode
> >> and back. The only question is what to do at the transition.
> >
> > No, the question is also what to return.
> 
> What to return sort of follows from what you do when turning
> of autogain, if you keep the last autogain set gain, then it
> makes sense to return the autogain set value when reading gain, if
> you switch back to the last manually set gain, then it
> makes sense to just report the last manually set gain as long
> as autogain is on.
> 
> I still believe that everything boils down to 2 possible scenarios,
> and the rest follows from that. With the 2 scenarios being:
> 
> 1) There is a manual setting which is constant until explicitly
> changed, when (ie) gain switches from auto mode to manual mode
> then the actual used gain is reset to this manual setting
> 
> 2) There is a single gain setting / register, which is r/w when the
> control is in manual mode and ro when in auto mode. When auto mode
> gets switched off, the gain stays at the last value set by auto mode.
> 
> 2) Is what most webcam sensors (and the pwc firmware) implement at
> the hardware level, and what to me also makes the most sense for webcams.
> 
> To me this whole discussion centers around these 2 scenarios, with you
> being a proponent of 1), and I guess that for video capture boards 1 makes
> a lot of sense, and me being a proponent of 2.
> 
> Proposal: lets agree that these 2 methods of handling autofoo controls
> both exist and both have merits in certain cases, this means letting
> it be up to the driver to choose which method to implement.

OK.

> If we can agree on this, then the next step would be to document both
> methods, as well as how the controls should behave in either scenario.
> I'm willing to write up a first draft for this.

I can do that as well, see below.

> 
> > How many 'autofoo' controls are there anyway?
> >
> > V4L2_CID_AUTO_WHITE_BALANCE
> > V4L2_CID_AUTOGAIN
> > V4L2_CID_EXPOSURE_AUTO
> > V4L2_CID_AUTOBRIGHTNESS
> > V4L2_CID_HUE_AUTO
> >
> > Those last two are used in only two drivers (gspca and uvc respectively).
> >
> > The first three would require four extra read-only volatile controls:
> >
> > V4L2_CID_AUTOWB_RED_BALANCE
> > V4L2_CID_AUTOWB_BLUE_BALANCE
> > V4L2_CID_AUTOGAIN_GAIN
> > V4L2_CID_AUTOEXP_EXPOSURE
> >
> 
> I can see this making sense for drivers which choose to implement
> scenario 1, but I see no value for drivers which choose to implement
> scenario 2, it is just another control cluttering the control applet.

I agree with that.

> > Simple and straightforward. Applications can show the manual value and the
> > autofoo value together so you can compare them easily. No unexpected
> > transitions since turning off the autofoo will restore the manual foo value.
> 
> I would actually consider that an unexpected transition. Lets consider
> auto exposure, and the last manual exposure was set during daytime, iow with
> lots of daylight, so a low exposure setting. Now there is a stream active,
> and being recorded in the evening. The user is not completely happy with the
> autoexposure chosen value, and turns of autoexposure. The very low exposure
> from the day gets restored, which is much too low, the image turns black,
> and the recording is ruined, not good.
> 
> Or the exposure stays at its last automatically set value, the user can make
> a small adjustment and all is well ... Note that this is exactly what most
> hardware does, to avoid the scenario above.

This is certainly valid for webcams. For video capture, however, I think
scenario 1 is more likely. But I agree, this is something to leave to drivers.

> We just seem to come at this from 2 completely different mindsets, to me
> going from the autofoo foo value to some manual value which may be completely
> unappropriate is an unexpected transition. IOW to me restoring the manual
> foo value is the unexpected thing to do.
> 
> I think we need to agree that we disagree :)

Actually, I agree with much of what you wrote :-)

OK, so we have two scenarios:

1) There is a manual setting which is constant until explicitly changed, when e.g.
gain switches from auto mode to manual mode then the actual used gain is reset to
this manual setting.

In this case the e.g. gain control is *not* marked volatile, but just inactive.
If the hardware can return the gain as set by the autogain circuit, then that has
to be exported as a separate read-only control (e.g. 'Current Gain').


2) There is a single gain setting / register, which is active when the control is in
manual mode and inactive and volatile when in auto mode. When auto mode gets switched
off, the gain stays at the last value set by auto mode.

This scenario is only possible, of course, if you can obtain the gain value as set
by the autogain circuitry.

An open question is whether writing to an inactive and volatile control should return
an error or not.

Webcams should follow scenario 2 (if possible).

It is less obvious what to recommend for video capture devices. I'd leave it up to
the driver for now.

Regards,

	Hans

Re: Some comments on the new autocluster patches

[Hans de Goede]

Hi,

On 07/26/2011 11:26 AM, Hans Verkuil wrote:
> OK, I'm back to work after my vacation, so it's time to go through the
> backlog...
>

Welcome back :)

> On Tuesday, July 12, 2011 15:25:02 Hans de Goede wrote:
>> Hi,
>>
>> On 07/04/2011 11:43 AM, Hans Verkuil wrote:

<snip snip>

>>> It is relevant. Take an application that saves the current state of all
>>> controls and restores it the next time it is started. If you report the
>>> device's autogain value instead of the manual gain, then that manual gain
>>> value is lost. I consider this a major drawback.
>>
>> If autogain is on, then the gain is RO, so it should not be saved. Let alone
>> restored.
>
> Marking gain as inactive is fine, but marking it as read-only is not so clear.
> Currently the RO flag is static. This allows control panels to use e.g. a text
> field instead of an input field to show the value.
>
> I would like to keep that functionality. If we make the RO flag dynamic, then
> GUIs won't know whether to show it as a disabled input field or as a text field.
>
> Whereas with the inactive flag they will know that it has to be a disabled
> input field.
>

Agreed, where I wrote read only I meant inactive, which does make it less clear
that the control should not be saved / restored by a save / restore app.

> When the inactive flag is set, it is still allowed to set the value. However,
> if we add a volatile flag as well, then we may want to have the combination
> 'inactive and volatile' return an error when an application attempts to set the
> value.

I think that is a good solution to indicate dynamic-readonly ness (more or less),
and thus to indicate that the control should not be written (and thus not saved/
restored).

> Or is this too complex and should we just discard the value in a case like that?

I would prefer returning an error, so that things don't silently fail, also
unless we actually return an error many apps are likely to get this wrong.

<snip snip>

>> I still believe that everything boils down to 2 possible scenarios,
>> and the rest follows from that. With the 2 scenarios being:
>>
>> 1) There is a manual setting which is constant until explicitly
>> changed, when (ie) gain switches from auto mode to manual mode
>> then the actual used gain is reset to this manual setting
>>
>> 2) There is a single gain setting / register, which is r/w when the
>> control is in manual mode and ro when in auto mode. When auto mode
>> gets switched off, the gain stays at the last value set by auto mode.
>>
>> 2) Is what most webcam sensors (and the pwc firmware) implement at
>> the hardware level, and what to me also makes the most sense for webcams.
>>
>> To me this whole discussion centers around these 2 scenarios, with you
>> being a proponent of 1), and I guess that for video capture boards 1 makes
>> a lot of sense, and me being a proponent of 2.
>>
>> Proposal: lets agree that these 2 methods of handling autofoo controls
>> both exist and both have merits in certain cases, this means letting
>> it be up to the driver to choose which method to implement.
>
> OK.
>
>> If we can agree on this, then the next step would be to document both
>> methods, as well as how the controls should behave in either scenario.
>> I'm willing to write up a first draft for this.
>
> I can do that as well, see below.

Ah great, you just saved me some work I always like it when people
save me work :)

<snip snip>

>> I think we need to agree that we disagree :)
>
> Actually, I agree with much of what you wrote :-)
>

Good :)

> OK, so we have two scenarios:
>
> 1) There is a manual setting which is constant until explicitly changed, when e.g.
> gain switches from auto mode to manual mode then the actual used gain is reset to
> this manual setting.
>
> In this case the e.g. gain control is *not* marked volatile, but just inactive.
> If the hardware can return the gain as set by the autogain circuit, then that has
> to be exported as a separate read-only control (e.g. 'Current Gain').
>
>
> 2) There is a single gain setting / register, which is active when the control is in
> manual mode and inactive and volatile when in auto mode. When auto mode gets switched
> off, the gain stays at the last value set by auto mode.
>
> This scenario is only possible, of course, if you can obtain the gain value as set
> by the autogain circuitry.
>

I fully agree with the above, +1

> An open question is whether writing to an inactive and volatile control should return
> an error or not.

I would prefer an error return.

> Webcams should follow scenario 2 (if possible).
>
> It is less obvious what to recommend for video capture devices. I'd leave it up to
> the driver for now.

Sounds good to me.

Regards,

Hans

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Tuesday, July 26, 2011 15:51:58 Hans de Goede wrote:

<snip>

> > OK, so we have two scenarios:
> >
> > 1) There is a manual setting which is constant until explicitly changed, when e.g.
> > gain switches from auto mode to manual mode then the actual used gain is reset to
> > this manual setting.
> >
> > In this case the e.g. gain control is *not* marked volatile, but just inactive.
> > If the hardware can return the gain as set by the autogain circuit, then that has
> > to be exported as a separate read-only control (e.g. 'Current Gain').
> >
> >
> > 2) There is a single gain setting / register, which is active when the control is in
> > manual mode and inactive and volatile when in auto mode. When auto mode gets switched
> > off, the gain stays at the last value set by auto mode.
> >
> > This scenario is only possible, of course, if you can obtain the gain value as set
> > by the autogain circuitry.
> >
> 
> I fully agree with the above, +1
> 
> > An open question is whether writing to an inactive and volatile control should return
> > an error or not.
> 
> I would prefer an error return.

I am worried about backwards compatibility, though. Right now inactive controls
can be written safely. Suddenly you add the volatile flag and doing the same thing
causes an error.

Also, a program that saves control values will have to skip any control that:

1) Is read or write only
2) Is inactive and volatile

The first is obvious, but the second not so much.

Another reason for not returning an error is that it makes v4l2-ctrls.c more complex: if
autogain is on and I call VIDIOC_S_EXT_CTRLS to set autogain to off and gain to a new
manual value, then it is quite difficult to detect that in this case setting gain is OK
(since autogain is turned off at the same time).

The more I think about it, the more I think this should just be allowed. The value
disappears into a black hole, but at least it won't break any apps.

Regards,

	Hans

> 
> > Webcams should follow scenario 2 (if possible).
> >
> > It is less obvious what to recommend for video capture devices. I'd leave it up to
> > the driver for now.
> 
> Sounds good to me.
> 
> Regards,
> 
> Hans
> 

Re: Some comments on the new autocluster patches

[Hans de Goede]

Hi,

On 07/26/2011 04:19 PM, Hans Verkuil wrote:
> On Tuesday, July 26, 2011 15:51:58 Hans de Goede wrote:
>

<snip>

>>> An open question is whether writing to an inactive and volatile control should return
>>> an error or not.
>>
>> I would prefer an error return.
>
> I am worried about backwards compatibility, though. Right now inactive controls
> can be written safely. Suddenly you add the volatile flag and doing the same thing
> causes an error.
>
> Also, a program that saves control values will have to skip any control that:
>
> 1) Is read or write only
> 2) Is inactive and volatile
>
> The first is obvious, but the second not so much.
>
> Another reason for not returning an error is that it makes v4l2-ctrls.c more complex: if
> autogain is on and I call VIDIOC_S_EXT_CTRLS to set autogain to off and gain to a new
> manual value, then it is quite difficult to detect that in this case setting gain is OK
> (since autogain is turned off at the same time).
>
> The more I think about it, the more I think this should just be allowed. The value
> disappears into a black hole, but at least it won't break any apps.

Ok disappear into a black hole it is :)

Regards,

Hans

Re: Some comments on the new autocluster patches

[Hans Verkuil]

On Tuesday, July 26, 2011 16:38:19 Hans de Goede wrote:
> Hi,
> 
> On 07/26/2011 04:19 PM, Hans Verkuil wrote:
> > On Tuesday, July 26, 2011 15:51:58 Hans de Goede wrote:
> >
> 
> <snip>
> 
> >>> An open question is whether writing to an inactive and volatile control should return
> >>> an error or not.
> >>
> >> I would prefer an error return.
> >
> > I am worried about backwards compatibility, though. Right now inactive controls
> > can be written safely. Suddenly you add the volatile flag and doing the same thing
> > causes an error.
> >
> > Also, a program that saves control values will have to skip any control that:
> >
> > 1) Is read or write only
> > 2) Is inactive and volatile
> >
> > The first is obvious, but the second not so much.
> >
> > Another reason for not returning an error is that it makes v4l2-ctrls.c more complex: if
> > autogain is on and I call VIDIOC_S_EXT_CTRLS to set autogain to off and gain to a new
> > manual value, then it is quite difficult to detect that in this case setting gain is OK
> > (since autogain is turned off at the same time).
> >
> > The more I think about it, the more I think this should just be allowed. The value
> > disappears into a black hole, but at least it won't break any apps.
> 
> Ok disappear into a black hole it is :)

Good. Then I'll try to work on this this week.

Regards,

	Hans