Re: [PATCH] media: Add camera controls for the ov5642 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote:
> Hello Sakari!

Hi Bastian,

> 2011/9/6 Sakari Ailus <sakari.ailus@xxxxxx>:
> > Hi Bastian,
> >
> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote:
> >> 2011/9/1 Sakari Ailus <sakari.ailus@xxxxxx>:
> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote:
> >> >> Hi Sakari,
> >> >>
> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote:
> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote:
> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote:
> >> >> >>
> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote:
> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> >> >> >>> [clip]
> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated.
> >> >> >>>>
> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled
> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read
> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything.
> >> >> >>>
> >> >> >>> Hmm. Did you happen to check when that has been written? :)
> >> >> >>>
> >> >> >>> Please use this one instead:
> >> >> >>>
> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html>
> >> >> >>
> >> >> >> "Drivers can also implement their own custom controls using
> >> >> >> V4L2_CID_PRIVATE_BASE and higher values."
> >> >> >>
> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there?
> >> >> >
> >> >> > That was a general comment, not related to the private base. There's no
> >> >> > use for a three-year-old spec as a reference!
> >> >> >
> >> >> > The control framework does not support private controls, for example. The
> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my
> >> >> > understanding. Cc Hans.
> >> >>
> >> >> Is this really the case that we close the door for private controls in
> >> >> the mainline kernel ? Or am I misunderstanding something ?
> >> >> How about v4l2_ctrl_new_custom() ?
> >> >>
> >> >> What if there are controls applicable to single driver only ?
> >> >> Do we really want to have plenty of such in videodev2.h ?
> >> >
> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy
> >> > with this myself, considering e.g. that we could get a few truckloads of
> >> > only camera lens hardware specific controls in the near future.
> >>
> >> So in my case (as these are controls that might be used by others too)
> >> I should add something like
> >>
> >> #define V4L2_CID_BLUE_SATURATION              (V4L2_CID_CAMERA_CLASS_BASE+19)
> >> #define V4L2_CID_RED_SATURATION               (V4L2_CID_CAMERA_CLASS_BASE+20)
> >
> > What do these two controls do? Do they control gain or something else?
> 
> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V
> Saturation. To me it looks like turning up the saturation in HSV
> space, but only for either the blue or the red channel. This would
> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They
> say it is "{Red,Blue} chroma balance".
> 
> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE.
> These are gains. So in fact I should swap them in my code and the
> remaining question is, how to name the red and blue gain controls.

I think Laurent had a similar issue in his Aptina sensor driver. In my
opinion we need a class for low level controls such as the gain ones. Do I
understand correctly they control the red and blue pixel gain in the sensor
pixel matrix? Do you also have gain controls for the two greens?

> >> #define V4L2_CID_GRAY_SCALE_IMAGE             (V4L2_CID_CAMERA_CLASS_BASE+21)
> >
> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose.
> 
> Oh great! So I just take this.
> 
> >> #define V4L2_CID_SOLARIZE_EFFECT              (V4L2_CID_CAMERA_CLASS_BASE+22)
> >
> > Sounds interesting for a sensor. I wonder if this would fall under a menu
> > control, V4L2_CID_COLORFX.
> 
> When I read the the possible enums for V4L2_CID_COLORFX, it indeed
> sounds very much like my solarize effect should be added there too. I
> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color
> killer control then?

In my opinion V4L2_CID_COLORFX should never be implemented in drivers for
which the hardware doesn't implement these effects in a non-parametrisable
way. This control was originally added for the OMAP 3 ISP driver but the
driver never implemented it.

I think you have a valid case using this control. I think the main
difference between the two is that V4L2_COLORFX_BW is something that you
can't use with other effects while V4L2_CID_COLOR_KILLER can be used with
any of the effects.

Based on your original proposal the black/white should stay as a separate
control but the solarise should be configurable through V4L2_CID_COLORFX
menu control. So it boils down to the question whether you can use them at
the same time.

-- 
Sakari Ailus
e-mail: sakari.ailus@xxxxxx	jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux