How do private controls actually work?

How do private controls actually work?

[Devin Heitmueller]

This might seem like a bit of a silly question, but I've been banging
my head on the wall for a while on this.

I need to add a single private control to the saa7115 driver.
However, it's not clear to me exactly how this is supposed to work.

The v4l2-ctl program will always use the extended controls interface
for private controls, since the code only uses the g_ctrl ioctl if the
class is V4L2_CTRL_CLASS_USER (and the control class for private
controls is V4L2_CID_PRIVATE_BASE).

However, if you look at the actual code in v4l2-ioctl.c, the call for
g_ext_ctrls calls check_ext_ctrls(), which fails because
"V4L2_CID_PRIVATE_BASE cannot be used as control class when using
extended controls."

The above two behaviors would seem to be conflicting.

My original plan was to implement it as a non-extended control, but
the v4l2-ctl application always sent the get call using the extended
interface.  So then I went to convert saa7115 to use the extended
control interface, but then found out that the v4l2 core wouldn't
allow an extended control to use a private control number.

To make matters worse, the G_CTRL function that supposedly passes
through calls to vidioc_g_ext_ctrl also calls check_ext_ctrl(), so if
you want to have a private control then you would need to implement
both the extended controls interface and the older g_ctrl in the
driver (presumably the idea was that you would be able to only need to
implement an extended controls interface, but that doesn't work given
the above).

I can change v4l2-ctl to use g_ctrl for private controls if we think
that is the correct approach.  But I didn't want to do that until I
knew for sure that it is correct that you can never have a private
extended control.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: How do private controls actually work?

[Laurent Pinchart]

Hi Devin,

On Monday 01 March 2010 03:56:17 Devin Heitmueller wrote:
> This might seem like a bit of a silly question, but I've been banging
> my head on the wall for a while on this.
> 
> I need to add a single private control to the saa7115 driver.
> However, it's not clear to me exactly how this is supposed to work.
> 
> The v4l2-ctl program will always use the extended controls interface
> for private controls, since the code only uses the g_ctrl ioctl if the
> class is V4L2_CTRL_CLASS_USER (and the control class for private
> controls is V4L2_CID_PRIVATE_BASE).
> 
> However, if you look at the actual code in v4l2-ioctl.c, the call for
> g_ext_ctrls calls check_ext_ctrls(), which fails because
> "V4L2_CID_PRIVATE_BASE cannot be used as control class when using
> extended controls."
> 
> The above two behaviors would seem to be conflicting.

I don't think it should matter which API (the base one or the extended one) 
you use for controls, be they private, standard or whatever. I don't see a 
reason for disallowing some controls to be used through one or the other API.

> My original plan was to implement it as a non-extended control, but
> the v4l2-ctl application always sent the get call using the extended
> interface.  So then I went to convert saa7115 to use the extended
> control interface, but then found out that the v4l2 core wouldn't
> allow an extended control to use a private control number.
> 
> To make matters worse, the G_CTRL function that supposedly passes
> through calls to vidioc_g_ext_ctrl also calls check_ext_ctrl(), so if
> you want to have a private control then you would need to implement
> both the extended controls interface and the older g_ctrl in the
> driver (presumably the idea was that you would be able to only need to
> implement an extended controls interface, but that doesn't work given
> the above).
> 
> I can change v4l2-ctl to use g_ctrl for private controls if we think
> that is the correct approach.  But I didn't want to do that until I
> knew for sure that it is correct that you can never have a private
> extended control.

Do we have extended *controls* ? All I see today is two APIs to access 
controls, a base *control API* and an extended *control API*. G_CTRL/S_CTRL 
should always be available to userspace. If you want to set a single control, 
the extended API isn't required.

-- 
Regards,

Laurent Pinchart

Re: How do private controls actually work?

[Devin Heitmueller]

Hello Laurent,

On Mon, Mar 1, 2010 at 3:57 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> I don't think it should matter which API (the base one or the extended one)
> you use for controls, be they private, standard or whatever. I don't see a
> reason for disallowing some controls to be used through one or the other API.

I would generally agree.  My original belief was that the extended
control API was designed to be a superset of the older API and that it
could be used for both types of controls.  Imagine my surprise to find
that private controls were specifically excluded from the extended
control interface.

>> I can change v4l2-ctl to use g_ctrl for private controls if we think
>> that is the correct approach.  But I didn't want to do that until I
>> knew for sure that it is correct that you can never have a private
>> extended control.
>
> Do we have extended *controls* ? All I see today is two APIs to access
> controls, a base *control API* and an extended *control API*. G_CTRL/S_CTRL
> should always be available to userspace. If you want to set a single control,
> the extended API isn't required.

The MPEG controls count as "extended" controls, since they provide the
ability for grouping controls and modifying the values for multiple
controls in an atomic manner.

It's worth noting that I actually did track down the regression in
v4l2-ctl, and the fix ended up being pretty simple (although it took a
couple of hours to understand the code and nail down the proper fix):

http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e

It's kind of annoying that I have to tell my client that the ability
to query/update private controls using v4l2-ctl has been completely
broken for six months, but it seems like that is where we are at.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: How do private controls actually work?

[Hans Verkuil]

> Hello Laurent,
>
> On Mon, Mar 1, 2010 at 3:57 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> I don't think it should matter which API (the base one or the extended
>> one)
>> you use for controls, be they private, standard or whatever. I don't see
>> a
>> reason for disallowing some controls to be used through one or the other
>> API.
>
> I would generally agree.  My original belief was that the extended
> control API was designed to be a superset of the older API and that it
> could be used for both types of controls.  Imagine my surprise to find
> that private controls were specifically excluded from the extended
> control interface.

New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
mechanism was a really bad idea. Instead a new control should be added to
the appropriate control class and with a offset >= 0x1000. See for example
the CX2341X private controls in videodev2.h.

The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL will
support controls in that range for backwards compatibility.

Regards,

        Hans

>>> I can change v4l2-ctl to use g_ctrl for private controls if we think
>>> that is the correct approach.  But I didn't want to do that until I
>>> knew for sure that it is correct that you can never have a private
>>> extended control.
>>
>> Do we have extended *controls* ? All I see today is two APIs to access
>> controls, a base *control API* and an extended *control API*.
>> G_CTRL/S_CTRL
>> should always be available to userspace. If you want to set a single
>> control,
>> the extended API isn't required.
>
> The MPEG controls count as "extended" controls, since they provide the
> ability for grouping controls and modifying the values for multiple
> controls in an atomic manner.
>
> It's worth noting that I actually did track down the regression in
> v4l2-ctl, and the fix ended up being pretty simple (although it took a
> couple of hours to understand the code and nail down the proper fix):
>
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e
>
> It's kind of annoying that I have to tell my client that the ability
> to query/update private controls using v4l2-ctl has been completely
> broken for six months, but it seems like that is where we are at.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com
> --
> 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
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: How do private controls actually work?

[Devin Heitmueller]

On Mon, Mar 1, 2010 at 4:58 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
> mechanism was a really bad idea. Instead a new control should be added to
> the appropriate control class and with a offset >= 0x1000. See for example
> the CX2341X private controls in videodev2.h.

So, you're suggesting that the following patch then is going to be
NAK'd and that I'm going to have to go back and convert saa7115 to
support the extended controls API, extend the em28xx driver to support
the extended controls API, and retest with all the possible
applications given how they might potentially be attempting to
implement the rather poorly documented interface?

http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/a7d50db75420

And exactly what "class" of extended controls should I use for video
decoders?  It would seem that a video decoder doesn't really fall into
any of the existing categories - "Old-style user controls", "MPEG
compression controls", "Camera controls", or "FM modulator controls".
Are we saying that now I'm also going to be introducing a whole new
class of control too?

> The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
> force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL will
> support controls in that range for backwards compatibility.

While we're on the topic, do you see any problem with the proposed fix
for the regression you introduced?:

http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e

Between trying to figure out what the expected behavior is supposed to
be (given the complete lack of documentation on how private controls
are expected to be implemented in the extended controls API) and
isolating and fixing the regression, it's hard not to be a little
irritated at this situation.  This was supposed to be a very small
change - a single private control to a mature driver.  And now it
seems like I'm going to have to extend the basic infrastructure in the
decoder driver, the bridge driver, add a new class of controls, all so
I can poke one register?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: How do private controls actually work?

[Hans Verkuil]

> On Mon, Mar 1, 2010 at 4:58 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
>> mechanism was a really bad idea. Instead a new control should be added
>> to
>> the appropriate control class and with a offset >= 0x1000. See for
>> example
>> the CX2341X private controls in videodev2.h.
>
> So, you're suggesting that the following patch then is going to be
> NAK'd and that I'm going to have to go back and convert saa7115 to
> support the extended controls API, extend the em28xx driver to support
> the extended controls API, and retest with all the possible
> applications given how they might potentially be attempting to
> implement the rather poorly documented interface?

I have a lot to say on this subject, but unfortunately I'm swamped with
work at the moment. I'll get back to you on this tomorrow.

Regards,

       Hans

>
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/a7d50db75420
>
> And exactly what "class" of extended controls should I use for video
> decoders?  It would seem that a video decoder doesn't really fall into
> any of the existing categories - "Old-style user controls", "MPEG
> compression controls", "Camera controls", or "FM modulator controls".
> Are we saying that now I'm also going to be introducing a whole new
> class of control too?
>
>> The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
>> force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL
>> will
>> support controls in that range for backwards compatibility.
>
> While we're on the topic, do you see any problem with the proposed fix
> for the regression you introduced?:
>
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e
>
> Between trying to figure out what the expected behavior is supposed to
> be (given the complete lack of documentation on how private controls
> are expected to be implemented in the extended controls API) and
> isolating and fixing the regression, it's hard not to be a little
> irritated at this situation.  This was supposed to be a very small
> change - a single private control to a mature driver.  And now it
> seems like I'm going to have to extend the basic infrastructure in the
> decoder driver, the bridge driver, add a new class of controls, all so
> I can poke one register?
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: How do private controls actually work?

[Mauro Carvalho Chehab]

Hi Devin,

Devin Heitmueller wrote:
> On Mon, Mar 1, 2010 at 4:58 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
>> mechanism was a really bad idea. Instead a new control should be added to
>> the appropriate control class and with a offset >= 0x1000. See for example
>> the CX2341X private controls in videodev2.h.

The better is to create a device-specific control, if the parameter you want 
to control is really specific to just one chip family. Otherwise, just add a 
new "common" control to the API.

> 
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/a7d50db75420

FYI, v42-apps were moved to a separate tree.

-- 

Cheers,
Mauro

Re: How do private controls actually work?

[Hans Verkuil]

Hi Devin!

As promised here is the follow-up on private controls.

First I refer you to an older posting of mine which is still pretty accurate:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg07999.html

As I mention there the intention is that I provide control support in the
v4l2 framework making it much easier to work with controls in drivers.

Unfortunately, I still haven't been able to find the time to continue my
work on this. Most annoying. I know exactly what I want and how to do it,
but I can't seem to manage to have 3-4 days of quiet to actually do it.

On Monday 01 March 2010 11:20:09 Devin Heitmueller wrote:
> On Mon, Mar 1, 2010 at 4:58 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
> > mechanism was a really bad idea. Instead a new control should be added to
> > the appropriate control class and with a offset >= 0x1000. See for example
> > the CX2341X private controls in videodev2.h.
> 
> So, you're suggesting that the following patch then is going to be
> NAK'd and that I'm going to have to go back and convert saa7115 to
> support the extended controls API, extend the em28xx driver to support
> the extended controls API, and retest with all the possible
> applications given how they might potentially be attempting to
> implement the rather poorly documented interface?
> 
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/a7d50db75420

I will NAK it only because you use V4L2_CID_PRIVATE_BASE. The rest of the
code is fine. It would be sufficient to create a private user control like
this:

#define V4L2_CID_SAA7115_BASE 		(V4L2_CTRL_CLASS_USER | 0x1000)
#define V4L2_CID_SAA7115_CHROMA_GAIN	(V4L2_CID_SAA7115_BASE+0)

> 
> And exactly what "class" of extended controls should I use for video
> decoders?  It would seem that a video decoder doesn't really fall into
> any of the existing categories - "Old-style user controls", "MPEG
> compression controls", "Camera controls", or "FM modulator controls".
> Are we saying that now I'm also going to be introducing a whole new
> class of control too?

CHROMA_AGC is a user control. And setting the CHROMA_GAIN should be in that
same class. It makes no sense to use two different classes for this.

> > The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
> > force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL will
> > support controls in that range for backwards compatibility.
> 
> While we're on the topic, do you see any problem with the proposed fix
> for the regression you introduced?:
> 
> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e

No problems at all. That's the correct fix.

Acked-by: Hans Verkuil <hverkuil@xs4all.nl>

> 
> Between trying to figure out what the expected behavior is supposed to
> be (given the complete lack of documentation on how private controls
> are expected to be implemented in the extended controls API) and
> isolating and fixing the regression, it's hard not to be a little
> irritated at this situation.  This was supposed to be a very small
> change - a single private control to a mature driver.  And now it
> seems like I'm going to have to extend the basic infrastructure in the
> decoder driver, the bridge driver, add a new class of controls, all so
> I can poke one register?

As you can see it is not that bad. That said, there is one disadvantage:
the em28xx driver does not support the V4L2_CTRL_FLAG_NEXT_CTRL that is needed
to enumerate this private user control. I do not know whether you need it since
you can still get and set the control, even if you can't enumerate it.

Unfortunately implementing this flag is non-trivial. We are missing code that
can administrate all the controls, whether they are from the main host driver
or from subdev drivers. The control framework that I'm working should handle
that, but it's not there yet. There is a support function in v4l2-common.c,
though: v4l2_ctrl_next(). It works, but it requires that bridge drivers know
which controls are handled by both the bridge driver and all subdev drivers.
That's not ideal since bridge drivers shouldn't have to know that from subdev
drivers.

Looking at the em28xx driver I think that supporting V4L2_CTRL_FLAG_NEXT_CTRL
in em28xx is too much work. So for the time being I think we should support
both a CHROMA_GAIN control using the old PRIVATE_BASE offset, and the proper
SAA7115_CHROMA_GAIN control. Once we have a working framework, then the
PRIVATE_BASE variant can be removed.

It's not elegant, but I don't see a better short-term solution.

I find all this just as irritating as you, but unfortunately I cannot conjure
up the time I need to finish it out of thin air :-( This part of the V4L2 API
is actually quite complex to correctly implement in drivers. So there is little
point in modifying individual drivers. Instead we just will have to wait for
the control framework to arrive.

I hope this helps,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: How do private controls actually work?

[Devin Heitmueller]

On Tue, Mar 2, 2010 at 3:28 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> I will NAK it only because you use V4L2_CID_PRIVATE_BASE. The rest of the
> code is fine. It would be sufficient to create a private user control like
> this:
>
> #define V4L2_CID_SAA7115_BASE           (V4L2_CTRL_CLASS_USER | 0x1000)
> #define V4L2_CID_SAA7115_CHROMA_GAIN    (V4L2_CID_SAA7115_BASE+0)

Easy enough.

>> And exactly what "class" of extended controls should I use for video
>> decoders?  It would seem that a video decoder doesn't really fall into
>> any of the existing categories - "Old-style user controls", "MPEG
>> compression controls", "Camera controls", or "FM modulator controls".
>> Are we saying that now I'm also going to be introducing a whole new
>> class of control too?
>
> CHROMA_AGC is a user control. And setting the CHROMA_GAIN should be in that
> same class. It makes no sense to use two different classes for this.

Ok, I wasn't sure if "user control" was really synonymous with "video
decoder", nor was I sure if you were suggesting whether the "user
control" section was exclusively for the legacy controls (and not
accepting new controls).

>> > The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
>> > force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL will
>> > support controls in that range for backwards compatibility.
>>
>> While we're on the topic, do you see any problem with the proposed fix
>> for the regression you introduced?:
>>
>> http://kernellabs.com/hg/~dheitmueller/em28xx-test/rev/142ae5aa9e8e
>
> No problems at all. That's the correct fix.
>
> Acked-by: Hans Verkuil <hverkuil@xs4all.nl>

Great, I'll add your Ack to the patch.

>>
>> Between trying to figure out what the expected behavior is supposed to
>> be (given the complete lack of documentation on how private controls
>> are expected to be implemented in the extended controls API) and
>> isolating and fixing the regression, it's hard not to be a little
>> irritated at this situation.  This was supposed to be a very small
>> change - a single private control to a mature driver.  And now it
>> seems like I'm going to have to extend the basic infrastructure in the
>> decoder driver, the bridge driver, add a new class of controls, all so
>> I can poke one register?
>
> As you can see it is not that bad. That said, there is one disadvantage:
> the em28xx driver does not support the V4L2_CTRL_FLAG_NEXT_CTRL that is needed
> to enumerate this private user control. I do not know whether you need it since
> you can still get and set the control, even if you can't enumerate it.

It's funny though.  I haven't looked at that part of the code
specifically, but the em28xx driver does appear to show private
controls in the output of the queryctrl() command (at least it is
showing up in the output of "v4l2-ctl -l".  Are there two different
APIs for enumerating controls?

> Unfortunately implementing this flag is non-trivial. We are missing code that
> can administrate all the controls, whether they are from the main host driver
> or from subdev drivers. The control framework that I'm working should handle
> that, but it's not there yet. There is a support function in v4l2-common.c,
> though: v4l2_ctrl_next(). It works, but it requires that bridge drivers know
> which controls are handled by both the bridge driver and all subdev drivers.
> That's not ideal since bridge drivers shouldn't have to know that from subdev
> drivers.
>
> Looking at the em28xx driver I think that supporting V4L2_CTRL_FLAG_NEXT_CTRL
> in em28xx is too much work. So for the time being I think we should support
> both a CHROMA_GAIN control using the old PRIVATE_BASE offset, and the proper
> SAA7115_CHROMA_GAIN control. Once we have a working framework, then the
> PRIVATE_BASE variant can be removed.

I had some extended discussion with Mauro on this yesterday on
#linuxtv, and he is now in favor of introducing a standard user
control for chroma gain, as opposed to doing a private control at all.

> I find all this just as irritating as you, but unfortunately I cannot conjure
> up the time I need to finish it out of thin air :-( This part of the V4L2 API
> is actually quite complex to correctly implement in drivers. So there is little
> point in modifying individual drivers. Instead we just will have to wait for
> the control framework to arrive.

Yeah, I understand.  Thanks for taking the time to help clarify how
this stuff is intended to wrok.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: How do private controls actually work?

[Hans Verkuil]

On Tuesday 02 March 2010 21:42:39 Devin Heitmueller wrote:
> On Tue, Mar 2, 2010 at 3:28 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> Between trying to figure out what the expected behavior is supposed to
> >> be (given the complete lack of documentation on how private controls
> >> are expected to be implemented in the extended controls API) and
> >> isolating and fixing the regression, it's hard not to be a little
> >> irritated at this situation.  This was supposed to be a very small
> >> change - a single private control to a mature driver.  And now it
> >> seems like I'm going to have to extend the basic infrastructure in the
> >> decoder driver, the bridge driver, add a new class of controls, all so
> >> I can poke one register?
> >
> > As you can see it is not that bad. That said, there is one disadvantage:
> > the em28xx driver does not support the V4L2_CTRL_FLAG_NEXT_CTRL that is needed
> > to enumerate this private user control. I do not know whether you need it since
> > you can still get and set the control, even if you can't enumerate it.
> 
> It's funny though.  I haven't looked at that part of the code
> specifically, but the em28xx driver does appear to show private
> controls in the output of the queryctrl() command (at least it is
> showing up in the output of "v4l2-ctl -l".  Are there two different
> APIs for enumerating controls?

That's probably when enumerating the PRIVATE_BASE controls. But that will not
work for private user class controls (i.e. CLASS_USER | 0x1000).
 
> > Unfortunately implementing this flag is non-trivial. We are missing code that
> > can administrate all the controls, whether they are from the main host driver
> > or from subdev drivers. The control framework that I'm working should handle
> > that, but it's not there yet. There is a support function in v4l2-common.c,
> > though: v4l2_ctrl_next(). It works, but it requires that bridge drivers know
> > which controls are handled by both the bridge driver and all subdev drivers.
> > That's not ideal since bridge drivers shouldn't have to know that from subdev
> > drivers.
> >
> > Looking at the em28xx driver I think that supporting V4L2_CTRL_FLAG_NEXT_CTRL
> > in em28xx is too much work. So for the time being I think we should support
> > both a CHROMA_GAIN control using the old PRIVATE_BASE offset, and the proper
> > SAA7115_CHROMA_GAIN control. Once we have a working framework, then the
> > PRIVATE_BASE variant can be removed.
> 
> I had some extended discussion with Mauro on this yesterday on
> #linuxtv, and he is now in favor of introducing a standard user
> control for chroma gain, as opposed to doing a private control at all.

That will also solve the problem :-)
 
> > I find all this just as irritating as you, but unfortunately I cannot conjure
> > up the time I need to finish it out of thin air :-( This part of the V4L2 API
> > is actually quite complex to correctly implement in drivers. So there is little
> > point in modifying individual drivers. Instead we just will have to wait for
> > the control framework to arrive.
> 
> Yeah, I understand.  Thanks for taking the time to help clarify how
> this stuff is intended to wrok.

No problem.

	Hans

> 
> Devin
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: How do private controls actually work?

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Tue, Mar 2, 2010 at 3:28 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

> I had some extended discussion with Mauro on this yesterday on
> #linuxtv, and he is now in favor of introducing a standard user
                      ===
> control for chroma gain, as opposed to doing a private control at all.

To be clear: I was never against ;)

It is worthy to summarize the discussions we have and the rationale to
create another control for it.

I've checked the datasheets of some chipsets, and the chroma gain is
different than the saturation control: the gain control (chroma or luma)
are applied at the analog input (or analog input samples) before the color
decoding, while the saturation is applied to the U/V output levels (some
datasheets call it as U/V output gain - causing some mess on the interpretation
of this value).

Also, saa7134 code as already some code to control the chroma gain. The driver
currently just puts some default value there, enabling AGC for PAL/NTSC and
disabling it for SECAM - but - as we have already troubles with AGC with cx88
and saa711x, I don't doubt that we may need to add the control logic there
to solve the same kind of trouble with composite/svideo inputs and some sources
that has a very high gain at the U/V level.

So, this control is not private to saa711x chipsets, but this control is also
present on other devices as well.

The API spec patch should clearly state that Saturation is for the U/V output
level, while gain is for the analog input gain.

-- 

Cheers,
Mauro

Re: How do private controls actually work?

[Andy Walls]

On Tue, 2010-03-02 at 18:23 -0300, Mauro Carvalho Chehab wrote:
> Devin Heitmueller wrote:
> > On Tue, Mar 2, 2010 at 3:28 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > I had some extended discussion with Mauro on this yesterday on
> > #linuxtv, and he is now in favor of introducing a standard user
>                       ===
> > control for chroma gain, as opposed to doing a private control at all.
> 
> To be clear: I was never against ;)
> 
> It is worthy to summarize the discussions we have and the rationale to
> create another control for it.
> 
> I've checked the datasheets of some chipsets, and the chroma gain is
> different than the saturation control: the gain control (chroma or luma)
> are applied at the analog input (or analog input samples) before the color
> decoding, while the saturation is applied to the U/V output levels (some
> datasheets call it as U/V output gain

Yes, that is correct.


>  - causing some mess on the interpretation
> of this value).

AFAICT, the effect of chroma gain is not really different from a
saturation control that scales both the U & V components by the same
factor.

               _
A color vector A can be expressed as
	_    _   _
	A = YW + C
       _
Where YW is a white vector that has a luminance component of magnitude
Y.
_
C is the chrominace vector in a constant luuminance plane.  Its phase is
the hue, and its magnitude is the saturation.
                           _
Adjusting the magnitude of C in the analog domain will change the
saturation.
          _                                                      _
Adjusting C's U & V components will adjust only the magnitude of C, if U
& V are adjusted by the same scale factor.  (You can tweak both the hue
and saturation by adjusting U & V by different scale factors.)



> The API spec patch should clearly state that Saturation is for the U/V output
> level, while gain is for the analog input gain.

That makes sense.

Since we're thinking about what to name controls, I will note the
CX25843, doesn't quite fit the current discussion of an analog "chroma
gain" independent from "luma gain":

1. the CX25843 has at least 3 front end gains well before U/V
separation:
   a +12 dB analog boost
   a analog coarse gain (controlled by an AGC),
   a digital fine gain (also has an AGC).

The +12 dB analog boost can be applied separately for Y, C, Pb and/or
Pr, but the other analog and digital gains cannot.  They will be applied
to all video signal inputs the same.

2. the CX25843 U and V saturation scale factors can be set
independently, if desired.


Regards,
Andy

Re: How do private controls actually work?

[Mauro Carvalho Chehab]

Andy Walls wrote:
> On Tue, 2010-03-02 at 18:23 -0300, Mauro Carvalho Chehab wrote:
>> Devin Heitmueller wrote:
>>> On Tue, Mar 2, 2010 at 3:28 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> I had some extended discussion with Mauro on this yesterday on
>>> #linuxtv, and he is now in favor of introducing a standard user
>>                       ===
>>> control for chroma gain, as opposed to doing a private control at all.
>> To be clear: I was never against ;)
>>
>> It is worthy to summarize the discussions we have and the rationale to
>> create another control for it.
>>
>> I've checked the datasheets of some chipsets, and the chroma gain is
>> different than the saturation control: the gain control (chroma or luma)
>> are applied at the analog input (or analog input samples) before the color
>> decoding, while the saturation is applied to the U/V output levels (some
>> datasheets call it as U/V output gain
> 
> Yes, that is correct.
> 
> 
>>  - causing some mess on the interpretation
>> of this value).
> 
> AFAICT, the effect of chroma gain is not really different from a
> saturation control that scales both the U & V components by the same
> factor.
> 
>                _
> A color vector A can be expressed as
> 	_    _   _
> 	A = YW + C
>        _
> Where YW is a white vector that has a luminance component of magnitude
> Y.
> _
> C is the chrominace vector in a constant luuminance plane.  Its phase is
> the hue, and its magnitude is the saturation.
>                            _
> Adjusting the magnitude of C in the analog domain will change the
> saturation.
>           _                                                      _
> Adjusting C's U & V components will adjust only the magnitude of C, if U
> & V are adjusted by the same scale factor.  (You can tweak both the hue
> and saturation by adjusting U & V by different scale factors.)

Makes sense. Yet, provided that the A/D converters have a very limited range
(in general, 8 to 10 bits), and assuming that some designs may adjust the gain 
before A/D conversion, or they do some sort of quantization before adjusting
saturation, by adjusting the analog level, the quantization noise will be reduced.
> 
>> The API spec patch should clearly state that Saturation is for the U/V output
>> level, while gain is for the analog input gain.
> 
> That makes sense.
> 
> Since we're thinking about what to name controls, I will note the
> CX25843, doesn't quite fit the current discussion of an analog "chroma
> gain" independent from "luma gain":
> 
> 1. the CX25843 has at least 3 front end gains well before U/V
> separation:
>    a +12 dB analog boost
>    a analog coarse gain (controlled by an AGC),
>    a digital fine gain (also has an AGC).
> 
> The +12 dB analog boost can be applied separately for Y, C, Pb and/or
> Pr, but the other analog and digital gains cannot.  They will be applied
> to all video signal inputs the same.

CX88 seems similar to cx25843: it has the +12 dB boost and some levels to adjust
the AGC range.

The +12dB boost could be mapped as an analog gain control with just two values on its range:
0 and 12dB.

That's said, I don't think we should map all available controls to the userspace API's.
The only ones that should be exported are the ones that there are some use cases that
justifies its mapping.

> 
> 2. the CX25843 U and V saturation scale factors can be set
> independently, if desired.
> 
> 
> Regards,
> Andy
> 


-- 

Cheers,
Mauro

Re: How do private controls actually work?

[Andy Walls]

On Mon, 2010-03-01 at 10:58 +0100, Hans Verkuil wrote:
> > Hello Laurent,
> >
> > On Mon, Mar 1, 2010 at 3:57 AM, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >> I don't think it should matter which API (the base one or the extended
> >> one)
> >> you use for controls, be they private, standard or whatever. I don't see
> >> a
> >> reason for disallowing some controls to be used through one or the other
> >> API.
> >
> > I would generally agree.  My original belief was that the extended
> > control API was designed to be a superset of the older API and that it
> > could be used for both types of controls.  Imagine my surprise to find
> > that private controls were specifically excluded from the extended
> > control interface.
> 
> New private controls should not use V4L2_CID_PRIVATE_BASE at all. That
> mechanism was a really bad idea. Instead a new control should be added to
> the appropriate control class and with a offset >= 0x1000. See for example
> the CX2341X private controls in videodev2.h.

I recall doing something like this a while ago:

#define CX18_AV_CID_USER_PRIV_BASE	(V4L2_CTRL_CLASS_USER | 0x1000)
#define CX18_AV_CID_EXTRA_12DB_GAIN	(CX18_AV_CID_USER_PRIV_BASE+0)
.....

http://linuxtv.org/hg/~awalls/v4l-dvb-ctls/file/e4b2c5d550b5/linux/drivers/media/video/cx18/cx18-av-core.h#l77

(I'm not sure if it's a good example though.)

I'm still just waiting for an approved method for implementing such
video decoder controls.  I don't care how, just that there is a way.

I don't have much in the way of plans to offer, because I really don't
grok all the existing issues with controls.

Regards,
Andy

> The EXT_G/S_CTRLS ioctls do not accept PRIVATE_BASE because I want to
> force driver developers to stop using PRIVATE_BASE. So only G/S_CTRL will
> support controls in that range for backwards compatibility.
> 
> Regards,
> 
>         Hans