public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Hans Verkuil <hansverk@cisco.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>
Subject: Re: Some comments on the new autocluster patches
Date: Tue, 26 Jul 2011 15:51:58 +0200	[thread overview]
Message-ID: <4E2EC67E.6010300@redhat.com> (raw)
In-Reply-To: <201107261126.22285.hverkuil@xs4all.nl>

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

  reply	other threads:[~2011-07-26 13:50 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-07-01 15:06 Some comments on the new autocluster patches Hans de Goede
2011-07-01 16:21 ` Hans Verkuil
2011-07-02 10:28   ` Hans de Goede
2011-07-02 11:10     ` Hans Verkuil
2011-07-02 14:31       ` Hans de Goede
2011-07-04  9:43         ` Hans Verkuil
2011-07-12 13:25           ` Hans de Goede
2011-07-26  9:26             ` Hans Verkuil
2011-07-26 13:51               ` Hans de Goede [this message]
2011-07-26 14:19                 ` Hans Verkuil
2011-07-26 14:38                   ` Hans de Goede
2011-07-26 14:39                     ` Hans Verkuil
2011-07-02  0:55 ` Mauro Carvalho Chehab
2011-07-02  9:36   ` Hans Verkuil
2011-07-02 18:33     ` Mauro Carvalho Chehab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4E2EC67E.6010300@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=hansverk@cisco.com \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-media@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox