Adding a control for Sensor Orientation

Adding a control for Sensor Orientation

[Adam Baker]

Hi all,

Hans Verkuil put forward a convincing argument that sensor orientation 
shouldn't be part of the buffer flags as then it would be unavailable to 
clients that use read() so it looks like implementing a read only control is 
the only appropriate option.

It seems that Sensor Orientation is an attribute that many cameras may want to 
expose so it shouldn't be a private control. Olivier Lorin's example patch 
created a new CAMERA_PROPERTIES class. I'm not sure that a new class is 
really justified so would like to hear other views on where the control 
should live (and also if everyone is happy with Hans Verkuil's suggested name 
of SENSOR_ORIENTATION which I prefer to Olivier Lorin's SENSOR_UPSIDE_DOWN as 
we want to represent HFLIP and VFLIP as well as upside down (which as 
currently implemented means 180 degree rotation.))

Assuming that it is considered inappropriate to add a new control as 
an "Old-style 'user' control" then it is also, I presume, necessary to extend 
gspca to support VIDIOC_G_EXT_CTRLS as at the moment it requires all control 
access to use VIDIOC_G_CTRL. Would doing this conflict with anything anyone 
else may be working on such as conversion to use v4l2_device.

Thoughts please.

Adam Baker

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

On Saturday 14 February 2009 21:48:51 Adam Baker wrote:
> Hi all,
>
> Hans Verkuil put forward a convincing argument that sensor orientation
> shouldn't be part of the buffer flags as then it would be unavailable to
> clients that use read() so it looks like implementing a read only control
> is the only appropriate option.
>
> It seems that Sensor Orientation is an attribute that many cameras may
> want to expose so it shouldn't be a private control. Olivier Lorin's
> example patch created a new CAMERA_PROPERTIES class. I'm not sure that a
> new class is really justified so would like to hear other views on where
> the control should live (and also if everyone is happy with Hans
> Verkuil's suggested name of SENSOR_ORIENTATION which I prefer to Olivier
> Lorin's SENSOR_UPSIDE_DOWN as we want to represent HFLIP and VFLIP as
> well as upside down (which as currently implemented means 180 degree
> rotation.))
>
> Assuming that it is considered inappropriate to add a new control as
> an "Old-style 'user' control" then it is also, I presume, necessary to
> extend gspca to support VIDIOC_G_EXT_CTRLS as at the moment it requires
> all control access to use VIDIOC_G_CTRL. Would doing this conflict with
> anything anyone else may be working on such as conversion to use
> v4l2_device.
>
> Thoughts please.

This is definitely a camera control, so I guess you need to add this new 
camera control and the g_ext_ctrls ioctl. It's a bit overkill, and I have 
lots of ideas on how to drastically improve control handling in drivers, 
but that's a few months in the future once all drivers are converted to 
v4l2_device. Hmm, if you are working in gspca anyway, are you interested in 
adding v4l2_device to it? It's trivial for that driver.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Adam Baker wrote:
> Hi all,
> 
> Hans Verkuil put forward a convincing argument that sensor orientation 
> shouldn't be part of the buffer flags as then it would be unavailable to 
> clients that use read()

Yes and this is a bogus argument, clients using read also do not get things 
like timestamps, and vital information like which field is in the read buffer 
when dealing with interleaved sources. read() is a simple interface for simple 
applications. Given that the only user of these flags will likely be libv4l I 
*strongly* object to having this info in some control, it is not a control, it 
is a per frame (on some cams) information about how to interpret that frame, 
the buffer flags is a very logical place, *the* logical place even for this!

The fact that there is no way to transport metadata about a frame like flags, 
but also timestamp and field! Is a problem with the read interface in general, 
iow read() is broken wrt to this. If people care add some ioctl or something 
which users of read() can use to get the buffer metadata from the last read() 
buffer, stuffing buffer metadata in a control (barf), because of read() 
brokenness is a very *bad* idea, and won't work in general due to 
synchronization problems.

Doing this as a control will be a pain to implement both at the driver level, 
see the discussion this is causing, and in libv4l. For libv4l this will basicly 
mean polling the control. And hello polling is lame and something from the 
1980-ies.

Please just make this a buffer flag.

Thank you,

Hans

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
> Adam Baker wrote:
> > Hi all,
> >
> > Hans Verkuil put forward a convincing argument that sensor orientation
> > shouldn't be part of the buffer flags as then it would be unavailable
> > to clients that use read()
>
> Yes and this is a bogus argument, clients using read also do not get
> things like timestamps, and vital information like which field is in the
> read buffer when dealing with interleaved sources. read() is a simple
> interface for simple applications. Given that the only user of these
> flags will likely be libv4l I *strongly* object to having this info in
> some control, it is not a control, it is a per frame (on some cams)
> information about how to interpret that frame, the buffer flags is a very
> logical place, *the* logical place even for this!
>
> The fact that there is no way to transport metadata about a frame like
> flags, but also timestamp and field! Is a problem with the read interface
> in general, iow read() is broken wrt to this. If people care add some
> ioctl or something which users of read() can use to get the buffer
> metadata from the last read() buffer, stuffing buffer metadata in a
> control (barf), because of read() brokenness is a very *bad* idea, and
> won't work in general due to synchronization problems.
>
> Doing this as a control will be a pain to implement both at the driver
> level, see the discussion this is causing, and in libv4l. For libv4l this
> will basicly mean polling the control. And hello polling is lame and
> something from the 1980-ies.
>
> Please just make this a buffer flag.

OK, make it a buffer flag. I've got to agree that it makes more sense to do 
it that way.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[kilgota]

On Sat, 14 Feb 2009, Hans Verkuil wrote:

> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
>> Adam Baker wrote:
>>> Hi all,
>>>
>>> Hans Verkuil put forward a convincing argument that sensor orientation
>>> shouldn't be part of the buffer flags as then it would be unavailable
>>> to clients that use read()
>>
>> Yes and this is a bogus argument, clients using read also do not get
>> things like timestamps, and vital information like which field is in the
>> read buffer when dealing with interleaved sources. read() is a simple
>> interface for simple applications. Given that the only user of these
>> flags will likely be libv4l I *strongly* object to having this info in
>> some control, it is not a control, it is a per frame (on some cams)
>> information about how to interpret that frame, the buffer flags is a very
>> logical place, *the* logical place even for this!
>>
>> The fact that there is no way to transport metadata about a frame like
>> flags, but also timestamp and field! Is a problem with the read interface
>> in general, iow read() is broken wrt to this. If people care add some
>> ioctl or something which users of read() can use to get the buffer
>> metadata from the last read() buffer, stuffing buffer metadata in a
>> control (barf), because of read() brokenness is a very *bad* idea, and
>> won't work in general due to synchronization problems.
>>
>> Doing this as a control will be a pain to implement both at the driver
>> level, see the discussion this is causing, and in libv4l. For libv4l this
>> will basicly mean polling the control. And hello polling is lame and
>> something from the 1980-ies.
>>
>> Please just make this a buffer flag.
>
> OK, make it a buffer flag. I've got to agree that it makes more sense to do
> it that way.
>
> Regards,
>
> 	Hans
>
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>

Let me take a moment to remind everyone what the problem is that brought 
this discussion up. Adam Baker and I are working on a driver for a certain 
camera. Or, better stated, for a set of various cameras, which all have 
the same USB Vendor:Product number. Various cameras which all have this ID 
have different capabilities and need different treatment of the frame 
data.

The most particular problem is that some of the cameras require byte 
reversal of the frame data string, which would rotate the image 180 
degrees around its center. Others of these cameras require reversal of the 
horizontal lines in the image (vertical 180 degree rotation of the image 
across a horizontal axis).

The point is, one can not tell from the Vendor:Product number which of 
these actions is required. However, one *is* able to tell immediately 
after the camera is initialized, which of these actions is required. 
Namely, one reads and parses the response to the first USB command sent to 
the camera.

So, for us (Adam and me) the question is simply to know how everyone will 
agree that the information about the image orientation can be sent from 
the module to V4L. When this issue is resolved, we can finish writing the 
sq905 camera driver. From this rather narrow point of view, the issue is 
not which method ought to be adopted. Rather, the issue is that no method 
has been adopted. It is rather difficult to write module code which will 
obey a non-existent standard.

Theodore Kilgore

Re: Adding a control for Sensor Orientation

[Hans de Goede]

kilgota@banach.math.auburn.edu wrote:
> 
> 
> On Sat, 14 Feb 2009, Hans Verkuil wrote:
> 
>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
>>> Adam Baker wrote:
>>>> Hi all,
>>>>
>>>> Hans Verkuil put forward a convincing argument that sensor orientation
>>>> shouldn't be part of the buffer flags as then it would be unavailable
>>>> to clients that use read()
>>>
>>> Yes and this is a bogus argument, clients using read also do not get
>>> things like timestamps, and vital information like which field is in the
>>> read buffer when dealing with interleaved sources. read() is a simple
>>> interface for simple applications. Given that the only user of these
>>> flags will likely be libv4l I *strongly* object to having this info in
>>> some control, it is not a control, it is a per frame (on some cams)
>>> information about how to interpret that frame, the buffer flags is a 
>>> very
>>> logical place, *the* logical place even for this!
>>>
>>> The fact that there is no way to transport metadata about a frame like
>>> flags, but also timestamp and field! Is a problem with the read 
>>> interface
>>> in general, iow read() is broken wrt to this. If people care add some
>>> ioctl or something which users of read() can use to get the buffer
>>> metadata from the last read() buffer, stuffing buffer metadata in a
>>> control (barf), because of read() brokenness is a very *bad* idea, and
>>> won't work in general due to synchronization problems.
>>>
>>> Doing this as a control will be a pain to implement both at the driver
>>> level, see the discussion this is causing, and in libv4l. For libv4l 
>>> this
>>> will basicly mean polling the control. And hello polling is lame and
>>> something from the 1980-ies.
>>>
>>> Please just make this a buffer flag.
>>
>> OK, make it a buffer flag. I've got to agree that it makes more sense 
>> to do
>> it that way.
>>
>> Regards,
>>
>>     Hans
>>
>> -- 
>> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>
> 
> Let me take a moment to remind everyone what the problem is that brought 
> this discussion up. Adam Baker and I are working on a driver for a 
> certain camera. Or, better stated, for a set of various cameras, which 
> all have the same USB Vendor:Product number. Various cameras which all 
> have this ID have different capabilities and need different treatment of 
> the frame data.
> 
> The most particular problem is that some of the cameras require byte 
> reversal of the frame data string, which would rotate the image 180 
> degrees around its center. Others of these cameras require reversal of 
> the horizontal lines in the image (vertical 180 degree rotation of the 
> image across a horizontal axis).
> 
> The point is, one can not tell from the Vendor:Product number which of 
> these actions is required. However, one *is* able to tell immediately 
> after the camera is initialized, which of these actions is required. 
> Namely, one reads and parses the response to the first USB command sent 
> to the camera.
> 
> So, for us (Adam and me) the question is simply to know how everyone 
> will agree that the information about the image orientation can be sent 
> from the module to V4L. When this issue is resolved, we can finish 
> writing the sq905 camera driver. From this rather narrow point of view, 
> the issue is not which method ought to be adopted. Rather, the issue is 
> that no method has been adopted. It is rather difficult to write module 
> code which will obey a non-existent standard.
> 

Ack, but the problem later was extended by the fact that it turns out some cams 
have a rotation detection (gravity direction) switch, which means you can flip 
the cam on its socket while streaming, and then the cam will tell you its 
rotation has changed, that makes this a per frame property rather then a static 
property of the cam. Which lead to this discussion, but we (the 2 Hans 's) 
agree now that using the flags field in the buffer struct is the best way 
forward. So there is a standard now, simply add 2 buffer flags to videodev2.h, 
one for content is h-flipped and one for content is v-flipped and you are done.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
> kilgota@banach.math.auburn.edu wrote:
> > On Sat, 14 Feb 2009, Hans Verkuil wrote:
> >> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
> >>> Adam Baker wrote:
> >>>> Hi all,
> >>>>
> >>>> Hans Verkuil put forward a convincing argument that sensor
> >>>> orientation shouldn't be part of the buffer flags as then it would
> >>>> be unavailable to clients that use read()
> >>>
> >>> Yes and this is a bogus argument, clients using read also do not get
> >>> things like timestamps, and vital information like which field is in
> >>> the read buffer when dealing with interleaved sources. read() is a
> >>> simple interface for simple applications. Given that the only user of
> >>> these flags will likely be libv4l I *strongly* object to having this
> >>> info in some control, it is not a control, it is a per frame (on some
> >>> cams) information about how to interpret that frame, the buffer flags
> >>> is a very
> >>> logical place, *the* logical place even for this!
> >>>
> >>> The fact that there is no way to transport metadata about a frame
> >>> like flags, but also timestamp and field! Is a problem with the read
> >>> interface
> >>> in general, iow read() is broken wrt to this. If people care add some
> >>> ioctl or something which users of read() can use to get the buffer
> >>> metadata from the last read() buffer, stuffing buffer metadata in a
> >>> control (barf), because of read() brokenness is a very *bad* idea,
> >>> and won't work in general due to synchronization problems.
> >>>
> >>> Doing this as a control will be a pain to implement both at the
> >>> driver level, see the discussion this is causing, and in libv4l. For
> >>> libv4l this
> >>> will basicly mean polling the control. And hello polling is lame and
> >>> something from the 1980-ies.
> >>>
> >>> Please just make this a buffer flag.
> >>
> >> OK, make it a buffer flag. I've got to agree that it makes more sense
> >> to do
> >> it that way.
> >>
> >> Regards,
> >>
> >>     Hans
> >>
> >> --
> >> Hans Verkuil - video4linux developer - sponsored by TANDBERG
> >
> > Let me take a moment to remind everyone what the problem is that
> > brought this discussion up. Adam Baker and I are working on a driver
> > for a certain camera. Or, better stated, for a set of various cameras,
> > which all have the same USB Vendor:Product number. Various cameras
> > which all have this ID have different capabilities and need different
> > treatment of the frame data.
> >
> > The most particular problem is that some of the cameras require byte
> > reversal of the frame data string, which would rotate the image 180
> > degrees around its center. Others of these cameras require reversal of
> > the horizontal lines in the image (vertical 180 degree rotation of the
> > image across a horizontal axis).
> >
> > The point is, one can not tell from the Vendor:Product number which of
> > these actions is required. However, one *is* able to tell immediately
> > after the camera is initialized, which of these actions is required.
> > Namely, one reads and parses the response to the first USB command sent
> > to the camera.
> >
> > So, for us (Adam and me) the question is simply to know how everyone
> > will agree that the information about the image orientation can be sent
> > from the module to V4L. When this issue is resolved, we can finish
> > writing the sq905 camera driver. From this rather narrow point of view,
> > the issue is not which method ought to be adopted. Rather, the issue is
> > that no method has been adopted. It is rather difficult to write module
> > code which will obey a non-existent standard.
>
> Ack, but the problem later was extended by the fact that it turns out
> some cams have a rotation detection (gravity direction) switch, which
> means you can flip the cam on its socket while streaming, and then the
> cam will tell you its rotation has changed, that makes this a per frame
> property rather then a static property of the cam. Which lead to this
> discussion, but we (the 2 Hans 's) agree now that using the flags field
> in the buffer struct is the best way forward. So there is a standard now,
> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
> and one for content is v-flipped and you are done.
>
> Regards,
>
> Hans

I think we should also be able to detect 90 and 270 degree rotations. Or at 
the very least prepare for it. It's a safe bet to assume that webcams will 
arrive that can detect portrait vs landscape orientation.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Hans Verkuil wrote:
> On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
>> kilgota@banach.math.auburn.edu wrote:
>>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
>>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
>>>>> Adam Baker wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Hans Verkuil put forward a convincing argument that sensor
>>>>>> orientation shouldn't be part of the buffer flags as then it would
>>>>>> be unavailable to clients that use read()
>>>>> Yes and this is a bogus argument, clients using read also do not get
>>>>> things like timestamps, and vital information like which field is in
>>>>> the read buffer when dealing with interleaved sources. read() is a
>>>>> simple interface for simple applications. Given that the only user of
>>>>> these flags will likely be libv4l I *strongly* object to having this
>>>>> info in some control, it is not a control, it is a per frame (on some
>>>>> cams) information about how to interpret that frame, the buffer flags
>>>>> is a very
>>>>> logical place, *the* logical place even for this!
>>>>>
>>>>> The fact that there is no way to transport metadata about a frame
>>>>> like flags, but also timestamp and field! Is a problem with the read
>>>>> interface
>>>>> in general, iow read() is broken wrt to this. If people care add some
>>>>> ioctl or something which users of read() can use to get the buffer
>>>>> metadata from the last read() buffer, stuffing buffer metadata in a
>>>>> control (barf), because of read() brokenness is a very *bad* idea,
>>>>> and won't work in general due to synchronization problems.
>>>>>
>>>>> Doing this as a control will be a pain to implement both at the
>>>>> driver level, see the discussion this is causing, and in libv4l. For
>>>>> libv4l this
>>>>> will basicly mean polling the control. And hello polling is lame and
>>>>> something from the 1980-ies.
>>>>>
>>>>> Please just make this a buffer flag.
>>>> OK, make it a buffer flag. I've got to agree that it makes more sense
>>>> to do
>>>> it that way.
>>>>
>>>> Regards,
>>>>
>>>>     Hans
>>>>
>>>> --
>>>> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>> Let me take a moment to remind everyone what the problem is that
>>> brought this discussion up. Adam Baker and I are working on a driver
>>> for a certain camera. Or, better stated, for a set of various cameras,
>>> which all have the same USB Vendor:Product number. Various cameras
>>> which all have this ID have different capabilities and need different
>>> treatment of the frame data.
>>>
>>> The most particular problem is that some of the cameras require byte
>>> reversal of the frame data string, which would rotate the image 180
>>> degrees around its center. Others of these cameras require reversal of
>>> the horizontal lines in the image (vertical 180 degree rotation of the
>>> image across a horizontal axis).
>>>
>>> The point is, one can not tell from the Vendor:Product number which of
>>> these actions is required. However, one *is* able to tell immediately
>>> after the camera is initialized, which of these actions is required.
>>> Namely, one reads and parses the response to the first USB command sent
>>> to the camera.
>>>
>>> So, for us (Adam and me) the question is simply to know how everyone
>>> will agree that the information about the image orientation can be sent
>>> from the module to V4L. When this issue is resolved, we can finish
>>> writing the sq905 camera driver. From this rather narrow point of view,
>>> the issue is not which method ought to be adopted. Rather, the issue is
>>> that no method has been adopted. It is rather difficult to write module
>>> code which will obey a non-existent standard.
>> Ack, but the problem later was extended by the fact that it turns out
>> some cams have a rotation detection (gravity direction) switch, which
>> means you can flip the cam on its socket while streaming, and then the
>> cam will tell you its rotation has changed, that makes this a per frame
>> property rather then a static property of the cam. Which lead to this
>> discussion, but we (the 2 Hans 's) agree now that using the flags field
>> in the buffer struct is the best way forward. So there is a standard now,
>> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
>> and one for content is v-flipped and you are done.
>>
>> Regards,
>>
>> Hans
> 
> I think we should also be able to detect 90 and 270 degree rotations. Or at 
> the very least prepare for it. It's a safe bet to assume that webcams will 
> arrive that can detect portrait vs landscape orientation.
> 

Handling those (esp on the fly) will be rather hard as width and height then 
get swapped. So lets worry about those when we need to. We will need an 
additional flag for those cases anyways.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Trent Piepho]

On Sun, 15 Feb 2009, Hans de Goede wrote:
> Hans Verkuil wrote:
> > On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
> >> kilgota@banach.math.auburn.edu wrote:
> >>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
> >>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
> >>>>> Adam Baker wrote:
> >>>> OK, make it a buffer flag. I've got to agree that it makes more sense
> >>>> to do
> >>>> it that way.
> >>>>
> >>> The most particular problem is that some of the cameras require byte
> >>> reversal of the frame data string, which would rotate the image 180
> >>> degrees around its center. Others of these cameras require reversal of
> >>> the horizontal lines in the image (vertical 180 degree rotation of the
> >>> image across a horizontal axis).
> >>>
> >>> The point is, one can not tell from the Vendor:Product number which of
> >>> these actions is required. However, one *is* able to tell immediately
> >>> after the camera is initialized, which of these actions is required.
> >>> Namely, one reads and parses the response to the first USB command sent
> >>> to the camera.

> >> Ack, but the problem later was extended by the fact that it turns out
> >> some cams have a rotation detection (gravity direction) switch, which
> >> means you can flip the cam on its socket while streaming, and then the
> >> cam will tell you its rotation has changed, that makes this a per frame
> >> property rather then a static property of the cam. Which lead to this
> >> discussion, but we (the 2 Hans 's) agree now that using the flags field
> >> in the buffer struct is the best way forward. So there is a standard now,
> >> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
> >> and one for content is v-flipped and you are done.
> >
> > I think we should also be able to detect 90 and 270 degree rotations. Or at
> > the very least prepare for it. It's a safe bet to assume that webcams will
> > arrive that can detect portrait vs landscape orientation.
>
> Handling those (esp on the fly) will be rather hard as width and height then
> get swapped. So lets worry about those when we need to. We will need an
> additional flag for those cases anyways.

Why would you need to worry about width and height getting swapped?
Meta-data about the frame would indicate it's now in portrait mode vs
landscape mode, but the dimentions would be unchanged.

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Trent Piepho wrote:
> On Sun, 15 Feb 2009, Hans de Goede wrote:
>> Hans Verkuil wrote:
>>> On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
>>>> kilgota@banach.math.auburn.edu wrote:
>>>>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
>>>>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
>>>>>>> Adam Baker wrote:
>>>>>> OK, make it a buffer flag. I've got to agree that it makes more sense
>>>>>> to do
>>>>>> it that way.
>>>>>>
>>>>> The most particular problem is that some of the cameras require byte
>>>>> reversal of the frame data string, which would rotate the image 180
>>>>> degrees around its center. Others of these cameras require reversal of
>>>>> the horizontal lines in the image (vertical 180 degree rotation of the
>>>>> image across a horizontal axis).
>>>>>
>>>>> The point is, one can not tell from the Vendor:Product number which of
>>>>> these actions is required. However, one *is* able to tell immediately
>>>>> after the camera is initialized, which of these actions is required.
>>>>> Namely, one reads and parses the response to the first USB command sent
>>>>> to the camera.
> 
>>>> Ack, but the problem later was extended by the fact that it turns out
>>>> some cams have a rotation detection (gravity direction) switch, which
>>>> means you can flip the cam on its socket while streaming, and then the
>>>> cam will tell you its rotation has changed, that makes this a per frame
>>>> property rather then a static property of the cam. Which lead to this
>>>> discussion, but we (the 2 Hans 's) agree now that using the flags field
>>>> in the buffer struct is the best way forward. So there is a standard now,
>>>> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
>>>> and one for content is v-flipped and you are done.
>>> I think we should also be able to detect 90 and 270 degree rotations. Or at
>>> the very least prepare for it. It's a safe bet to assume that webcams will
>>> arrive that can detect portrait vs landscape orientation.
>> Handling those (esp on the fly) will be rather hard as width and height then
>> get swapped. So lets worry about those when we need to. We will need an
>> additional flag for those cases anyways.
> 
> Why would you need to worry about width and height getting swapped?
> Meta-data about the frame would indicate it's now in portrait mode vs
> landscape mode, but the dimentions would be unchanged.

Yes, unless ofcourse you want to display a proper picture and not one on its 
side, when the camera is rotated 90 degrees, so somewere you need to rotate the 
picture 90 degrees, and the lower down in the stack you do that, the bigger the 
chance you do not need to duplicate the rotation code in every single app. 
however the app will mostlikely become unhappy when you start out pushing 
frames whith a changed width / height.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Trent Piepho]

On Sun, 15 Feb 2009, Hans de Goede wrote:
> Trent Piepho wrote:
> > On Sun, 15 Feb 2009, Hans de Goede wrote:
> >> Hans Verkuil wrote:
> >>> On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
> >>>> kilgota@banach.math.auburn.edu wrote:
> >>>>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
> >>>>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
> >>>>>>> Adam Baker wrote:
> >>>>>> OK, make it a buffer flag. I've got to agree that it makes more sense
> >>>>>> to do
> >>>>>> it that way.
> >>>>>>
> >>>>> The most particular problem is that some of the cameras require byte
> >>>>> reversal of the frame data string, which would rotate the image 180
> >>>>> degrees around its center. Others of these cameras require reversal of
> >>>>> the horizontal lines in the image (vertical 180 degree rotation of the
> >>>>> image across a horizontal axis).
> >>>>>
> >>>>> The point is, one can not tell from the Vendor:Product number which of
> >>>>> these actions is required. However, one *is* able to tell immediately
> >>>>> after the camera is initialized, which of these actions is required.
> >>>>> Namely, one reads and parses the response to the first USB command sent
> >>>>> to the camera.
> >
> >>>> Ack, but the problem later was extended by the fact that it turns out
> >>>> some cams have a rotation detection (gravity direction) switch, which
> >>>> means you can flip the cam on its socket while streaming, and then the
> >>>> cam will tell you its rotation has changed, that makes this a per frame
> >>>> property rather then a static property of the cam. Which lead to this
> >>>> discussion, but we (the 2 Hans 's) agree now that using the flags field
> >>>> in the buffer struct is the best way forward. So there is a standard now,
> >>>> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
> >>>> and one for content is v-flipped and you are done.
> >>> I think we should also be able to detect 90 and 270 degree rotations. Or at
> >>> the very least prepare for it. It's a safe bet to assume that webcams will
> >>> arrive that can detect portrait vs landscape orientation.
> >> Handling those (esp on the fly) will be rather hard as width and height then
> >> get swapped. So lets worry about those when we need to. We will need an
> >> additional flag for those cases anyways.
> >
> > Why would you need to worry about width and height getting swapped?
> > Meta-data about the frame would indicate it's now in portrait mode vs
> > landscape mode, but the dimentions would be unchanged.
>
> Yes, unless ofcourse you want to display a proper picture and not one on its
> side, when the camera is rotated 90 degrees, so somewere you need to rotate the
> picture 90 degrees, and the lower down in the stack you do that, the bigger the
> chance you do not need to duplicate the rotation code in every single app.
> however the app will mostlikely become unhappy when you start out pushing
> frames whith a changed width / height.

It seems that image rotation, like format conversion, is something that is
best done in userspace.  It could be done in hardware with opengl or faster
software using MMX or SSE based code that can't be used in the kernel.

Re: Adding a control for Sensor Orientation

[kilgota]

On Sun, 15 Feb 2009, Trent Piepho wrote:

> On Sun, 15 Feb 2009, Hans de Goede wrote:
>> Trent Piepho wrote:
>>> On Sun, 15 Feb 2009, Hans de Goede wrote:
>>>> Hans Verkuil wrote:
>>>>> On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
>>>>>> kilgota@banach.math.auburn.edu wrote:
>>>>>>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
>>>>>>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
>>>>>>>>> Adam Baker wrote:
>>>>>>>> OK, make it a buffer flag. I've got to agree that it makes more sense
>>>>>>>> to do
>>>>>>>> it that way.
>>>>>>>>
>>>>>>> The most particular problem is that some of the cameras require byte
>>>>>>> reversal of the frame data string, which would rotate the image 180
>>>>>>> degrees around its center. Others of these cameras require reversal of
>>>>>>> the horizontal lines in the image (vertical 180 degree rotation of the
>>>>>>> image across a horizontal axis).
>>>>>>>
>>>>>>> The point is, one can not tell from the Vendor:Product number which of
>>>>>>> these actions is required. However, one *is* able to tell immediately
>>>>>>> after the camera is initialized, which of these actions is required.
>>>>>>> Namely, one reads and parses the response to the first USB command sent
>>>>>>> to the camera.
>>>
>>>>>> Ack, but the problem later was extended by the fact that it turns out
>>>>>> some cams have a rotation detection (gravity direction) switch, which
>>>>>> means you can flip the cam on its socket while streaming, and then the
>>>>>> cam will tell you its rotation has changed, that makes this a per frame
>>>>>> property rather then a static property of the cam. Which lead to this
>>>>>> discussion, but we (the 2 Hans 's) agree now that using the flags field
>>>>>> in the buffer struct is the best way forward. So there is a standard now,
>>>>>> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
>>>>>> and one for content is v-flipped and you are done.
>>>>> I think we should also be able to detect 90 and 270 degree rotations. Or at
>>>>> the very least prepare for it. It's a safe bet to assume that webcams will
>>>>> arrive that can detect portrait vs landscape orientation.
>>>> Handling those (esp on the fly) will be rather hard as width and height then
>>>> get swapped. So lets worry about those when we need to. We will need an
>>>> additional flag for those cases anyways.
>>>
>>> Why would you need to worry about width and height getting swapped?
>>> Meta-data about the frame would indicate it's now in portrait mode vs
>>> landscape mode, but the dimentions would be unchanged.
>>
>> Yes, unless ofcourse you want to display a proper picture and not one on its
>> side, when the camera is rotated 90 degrees, so somewere you need to rotate the
>> picture 90 degrees, and the lower down in the stack you do that, the bigger the
>> chance you do not need to duplicate the rotation code in every single app.
>> however the app will mostlikely become unhappy when you start out pushing
>> frames whith a changed width / height.
>
> It seems that image rotation, like format conversion, is something that is
> best done in userspace.  It could be done in hardware with opengl or faster
> software using MMX or SSE based code that can't be used in the kernel.
>

My impression is that nobody here disagrees with this. Certainly, I do 
not. As I understand, there is a general intention to avoid writing new 
modules which do such things and to try to rewrite old ones. The reasons 
are, presumably, very similar to the reasons you give.

Therefore,

1. Everyone seems to agree that the kernel module itself is not going to 
do things like rotate or flip data even if a given supported device 
always needs that done.

However, this decision has a consequence:

2. Therefore, the module must send the information about what is needed 
out of the module, to whatever place is going to deal with it. Information 
which is known to the module but unknown anywere else must be transmitted 
somehow.

Now there is a further consequence:

3. In view of (1) and (2) there has to be a way agreed upon for the module 
to pass the relevant information onward.

It is precisely on item 3 that we are stuck right now. There is an 
immediate need, not a theoretical need but an immediate need. However, 
there is no agreed-upon method or convention for communication.

Some might argue that it is sufficient to know some ID of the device (USB 
Vendor:Product number, for example), but it is not:

4. The idea of relying on the USB ID of the supported device to decide 
what should be done with the frame data is inadequate. Sometimes, 
preliminary communication with the device is the only possible way to 
learn what is needed. Again, this is not a theoretical problem. It is an 
actual problem. A known device exists. Go back to item (3).


There are of course related problems. But it strikes me that the related 
problems are not so very related at all. As I understand, it is visualized 
that a camera could be put on a pivot, with control mechanism which would 
permit various rotations and then the question becomes how to support a 
camera and to make the stream come out "right" no matter which way the 
camera is pointed. A far-seeing project design will certainly think of 
things like that before they happen and will try to anticipate what to do.

Why do I say then that these problems are not related at all to the 
present problem? Because we are dealing with cameras that always present 
the data upside-down or mirrored, unless corrective action is taken. 
So, again, either the module has to do the correction inside itself (and 
everyone agrees that it should not!) or it has to have a standard protocol 
to pass that information onward. to be dealt with appropriately. It would 
seem to me best to separate a problem like this from discussions about 
tilting or physically rotating the camera.

Theodore Kilgore

Re: Adding a control for Sensor Orientation

[Mauro Carvalho Chehab]

On Sun, 15 Feb 2009 10:29:03 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> > I think we should also be able to detect 90 and 270 degree rotations. Or at 
> > the very least prepare for it. It's a safe bet to assume that webcams will 
> > arrive that can detect portrait vs landscape orientation.
> > 
> 
> Handling those (esp on the fly) will be rather hard as width and height then 
> get swapped. So lets worry about those when we need to. We will need an 
> additional flag for those cases anyways.
> 
The camera rotation is something that is already needed, at least on some
embedded devices, like those cellular phones whose display changes when you
rotate the device.

By looking at the v4l2_buffer struct, we currently have 4 reserved bytes. It
has also one flags field, with several bits not used. I can see 2 possibilities to extend the API:

1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This would work
for 90, 180 and 270 rotation;

2) spend a few bytes for storing the camera angle. If a camera have a precise
sensor, this could be used by some software to reduce the angle movements of a
moving camera. However, if we consider this case, we would also need to
consider other camera movements.

So, IMO, the two flags could be more appropriate.


Cheers,
Mauro

Re: Adding a control for Sensor Orientation

[hermann pitton]

Hi guys,

Am Sonntag, den 15.02.2009, 19:46 -0600 schrieb
kilgota@banach.math.auburn.edu:
> 
> On Sun, 15 Feb 2009, Trent Piepho wrote:
> 
> > On Sun, 15 Feb 2009, Hans de Goede wrote:
> >> Trent Piepho wrote:
> >>> On Sun, 15 Feb 2009, Hans de Goede wrote:
> >>>> Hans Verkuil wrote:
> >>>>> On Sunday 15 February 2009 10:08:04 Hans de Goede wrote:
> >>>>>> kilgota@banach.math.auburn.edu wrote:
> >>>>>>> On Sat, 14 Feb 2009, Hans Verkuil wrote:
> >>>>>>>> On Saturday 14 February 2009 22:55:39 Hans de Goede wrote:
> >>>>>>>>> Adam Baker wrote:
> >>>>>>>> OK, make it a buffer flag. I've got to agree that it makes more sense
> >>>>>>>> to do
> >>>>>>>> it that way.
> >>>>>>>>
> >>>>>>> The most particular problem is that some of the cameras require byte
> >>>>>>> reversal of the frame data string, which would rotate the image 180
> >>>>>>> degrees around its center. Others of these cameras require reversal of
> >>>>>>> the horizontal lines in the image (vertical 180 degree rotation of the
> >>>>>>> image across a horizontal axis).
> >>>>>>>
> >>>>>>> The point is, one can not tell from the Vendor:Product number which of
> >>>>>>> these actions is required. However, one *is* able to tell immediately
> >>>>>>> after the camera is initialized, which of these actions is required.
> >>>>>>> Namely, one reads and parses the response to the first USB command sent
> >>>>>>> to the camera.
> >>>
> >>>>>> Ack, but the problem later was extended by the fact that it turns out
> >>>>>> some cams have a rotation detection (gravity direction) switch, which
> >>>>>> means you can flip the cam on its socket while streaming, and then the
> >>>>>> cam will tell you its rotation has changed, that makes this a per frame
> >>>>>> property rather then a static property of the cam. Which lead to this
> >>>>>> discussion, but we (the 2 Hans 's) agree now that using the flags field
> >>>>>> in the buffer struct is the best way forward. So there is a standard now,
> >>>>>> simply add 2 buffer flags to videodev2.h, one for content is h-flipped
> >>>>>> and one for content is v-flipped and you are done.
> >>>>> I think we should also be able to detect 90 and 270 degree rotations. Or at
> >>>>> the very least prepare for it. It's a safe bet to assume that webcams will
> >>>>> arrive that can detect portrait vs landscape orientation.
> >>>> Handling those (esp on the fly) will be rather hard as width and height then
> >>>> get swapped. So lets worry about those when we need to. We will need an
> >>>> additional flag for those cases anyways.
> >>>
> >>> Why would you need to worry about width and height getting swapped?
> >>> Meta-data about the frame would indicate it's now in portrait mode vs
> >>> landscape mode, but the dimentions would be unchanged.
> >>
> >> Yes, unless ofcourse you want to display a proper picture and not one on its
> >> side, when the camera is rotated 90 degrees, so somewere you need to rotate the
> >> picture 90 degrees, and the lower down in the stack you do that, the bigger the
> >> chance you do not need to duplicate the rotation code in every single app.
> >> however the app will mostlikely become unhappy when you start out pushing
> >> frames whith a changed width / height.
> >
> > It seems that image rotation, like format conversion, is something that is
> > best done in userspace.  It could be done in hardware with opengl or faster
> > software using MMX or SSE based code that can't be used in the kernel.
> >
> 
> My impression is that nobody here disagrees with this. Certainly, I do 
> not. As I understand, there is a general intention to avoid writing new 
> modules which do such things and to try to rewrite old ones. The reasons 
> are, presumably, very similar to the reasons you give.
> 
> Therefore,
> 
> 1. Everyone seems to agree that the kernel module itself is not going to 
> do things like rotate or flip data even if a given supported device 
> always needs that done.
> 
> However, this decision has a consequence:
> 
> 2. Therefore, the module must send the information about what is needed 
> out of the module, to whatever place is going to deal with it. Information 
> which is known to the module but unknown anywere else must be transmitted 
> somehow.
> 
> Now there is a further consequence:
> 
> 3. In view of (1) and (2) there has to be a way agreed upon for the module 
> to pass the relevant information onward.
> 
> It is precisely on item 3 that we are stuck right now. There is an 
> immediate need, not a theoretical need but an immediate need. However, 
> there is no agreed-upon method or convention for communication.
> 
> Some might argue that it is sufficient to know some ID of the device (USB 
> Vendor:Product number, for example), but it is not:
> 
> 4. The idea of relying on the USB ID of the supported device to decide 
> what should be done with the frame data is inadequate. Sometimes, 
> preliminary communication with the device is the only possible way to 
> learn what is needed. Again, this is not a theoretical problem. It is an 
> actual problem. A known device exists. Go back to item (3).
> 
> 
> There are of course related problems. But it strikes me that the related 
> problems are not so very related at all. As I understand, it is visualized 
> that a camera could be put on a pivot, with control mechanism which would 
> permit various rotations and then the question becomes how to support a 
> camera and to make the stream come out "right" no matter which way the 
> camera is pointed. A far-seeing project design will certainly think of 
> things like that before they happen and will try to anticipate what to do.
> 
> Why do I say then that these problems are not related at all to the 
> present problem? Because we are dealing with cameras that always present 
> the data upside-down or mirrored, unless corrective action is taken. 
> So, again, either the module has to do the correction inside itself (and 
> everyone agrees that it should not!) or it has to have a standard protocol 
> to pass that information onward. to be dealt with appropriately. It would 
> seem to me best to separate a problem like this from discussions about 
> tilting or physically rotating the camera.
> 
> Theodore Kilgore

we are still lacking women here.

About cams, I vote for to have them on a separate list until they have
something ready for API inclusion.

But to come to a point, we are making some sort of cam history and can't
excuse anymore with that others do it already anyway since long.

I recommend to read an already old book about pictures and video.

It is by Paul Virilio and in German titled "Rasender Stillstand".

Original "L'inertie polaire" by editor Christian Bourgois, Paris 1990.

When I joined this sort of stuff here, it was common to point to
something eventually related, but seems old fashioned these days ...

Cheers,
Hermann

Re: Adding a control for Sensor Orientation

[Trent Piepho]

On Sun, 15 Feb 2009 kilgota@banach.math.auburn.edu wrote:
> On Sun, 15 Feb 2009, Trent Piepho wrote:
> > On Sun, 15 Feb 2009, Hans de Goede wrote:
> >>>>> I think we should also be able to detect 90 and 270 degree rotations. Or at
> >>>>> the very least prepare for it. It's a safe bet to assume that webcams will
> >>>>> arrive that can detect portrait vs landscape orientation.
> >>>> Handling those (esp on the fly) will be rather hard as width and height then
> >>>> get swapped. So lets worry about those when we need to. We will need an
> >>>> additional flag for those cases anyways.
> >>>
> >>> Why would you need to worry about width and height getting swapped?
> >>> Meta-data about the frame would indicate it's now in portrait mode vs
> >>> landscape mode, but the dimentions would be unchanged.
> >>
> >> Yes, unless ofcourse you want to display a proper picture and not one on its
> >> side, when the camera is rotated 90 degrees, so somewere you need to rotate the
> >> picture 90 degrees, and the lower down in the stack you do that, the bigger the
> >> chance you do not need to duplicate the rotation code in every single app.
> >> however the app will mostlikely become unhappy when you start out pushing
> >> frames whith a changed width / height.
> >
> > It seems that image rotation, like format conversion, is something that is
> > best done in userspace.  It could be done in hardware with opengl or faster
> > software using MMX or SSE based code that can't be used in the kernel.
>
> 1. Everyone seems to agree that the kernel module itself is not going to
> do things like rotate or flip data even if a given supported device
> always needs that done.
>
> However, this decision has a consequence:
>
> 2. Therefore, the module must send the information about what is needed
> out of the module, to whatever place is going to deal with it. Information
> which is known to the module but unknown anywere else must be transmitted
> somehow.
>
> Now there is a further consequence:
>
> 3. In view of (1) and (2) there has to be a way agreed upon for the module
> to pass the relevant information onward.
>
> It is precisely on item 3 that we are stuck right now. There is an
> immediate need, not a theoretical need but an immediate need. However,
> there is no agreed-upon method or convention for communication.

There are already controls being added for HFLIP, VFLIP, and rotation, so
that's certainly one way.  They're being added as writable controls for
someone's hardware that can do these image manipulations.  But it seems
like a different driver could just as easily provide them as read-only
controls to inform software that it has a non-standard image layout.

It sounds like per frame rotation metadata would be useful for cameras that
have an orientation sensor.

> problems are not so very related at all. As I understand, it is visualized
> that a camera could be put on a pivot, with control mechanism which would
> permit various rotations and then the question becomes how to support a
> camera and to make the stream come out "right" no matter which way the

I think we already have controls for camera motors, i.e. pan, tilt, and
rotation.  That's totally different.

Re: Adding a control for Sensor Orientation

[Trent Piepho]

On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
> On Sun, 15 Feb 2009 10:29:03 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> > > I think we should also be able to detect 90 and 270 degree rotations. Or at
> > > the very least prepare for it. It's a safe bet to assume that webcams will
> > > arrive that can detect portrait vs landscape orientation.
> > >
> >
> > Handling those (esp on the fly) will be rather hard as width and height then
> > get swapped. So lets worry about those when we need to. We will need an
> > additional flag for those cases anyways.
> >
> The camera rotation is something that is already needed, at least on some
> embedded devices, like those cellular phones whose display changes when you
> rotate the device.
>
> By looking at the v4l2_buffer struct, we currently have 4 reserved bytes. It
> has also one flags field, with several bits not used. I can see 2 possibilities to extend the API:
>
> 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This would work
> for 90, 180 and 270 rotation;

HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't the
same as flipping.

The problem I'm seeing here is that as people are using v4l2 for digital
cameras instead of tv capture there is more and more meta-data available.
Things like shutter speed, aperture, focus distance, and so on.  Just look
at all the EXIF data a digital camera provides.  Four bytes and two flags
are going to run out very quickly at this rate.

It's a shame there are not 8 bytes left, as then they could be used for a
pointer to an extended meta-data structure.

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

On Monday 16 February 2009 05:04:40 Trent Piepho wrote:
> On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
> > On Sun, 15 Feb 2009 10:29:03 +0100
> >
> > Hans de Goede <hdegoede@redhat.com> wrote:
> > > > I think we should also be able to detect 90 and 270 degree
> > > > rotations. Or at the very least prepare for it. It's a safe bet to
> > > > assume that webcams will arrive that can detect portrait vs
> > > > landscape orientation.
> > >
> > > Handling those (esp on the fly) will be rather hard as width and
> > > height then get swapped. So lets worry about those when we need to.
> > > We will need an additional flag for those cases anyways.
> >
> > The camera rotation is something that is already needed, at least on
> > some embedded devices, like those cellular phones whose display changes
> > when you rotate the device.
> >
> > By looking at the v4l2_buffer struct, we currently have 4 reserved
> > bytes. It has also one flags field, with several bits not used. I can
> > see 2 possibilities to extend the API:
> >
> > 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This would
> > work for 90, 180 and 270 rotation;
>
> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't
> the same as flipping.
>
> The problem I'm seeing here is that as people are using v4l2 for digital
> cameras instead of tv capture there is more and more meta-data available.
> Things like shutter speed, aperture, focus distance, and so on.  Just
> look at all the EXIF data a digital camera provides.  Four bytes and two
> flags are going to run out very quickly at this rate.
>
> It's a shame there are not 8 bytes left, as then they could be used for a
> pointer to an extended meta-data structure.

I think we have to distinguish between two separate types of data: fixed 
('the sensor is mounted upside-down', or 'the sensor always requires a 
hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').

The first is static data and I think we can just reuse the existing 
HFLIP/VFLIP controls: just make them READONLY to tell libv4l that libv4l 
needs to do the flipping.

The second is dynamic data and should be passed through v4l2_buffer since 
this can change on a per-frame basis. In this case add two bits to the 
v4l2_buffer's flags field:

V4L2_BUF_FLAG_ROTATION_MSK	0x0c00
V4L2_BUF_FLAG_ROTATION_0	0x0000
V4L2_BUF_FLAG_ROTATION_90	0x0400
V4L2_BUF_FLAG_ROTATION_180	0x0800
V4L2_BUF_FLAG_ROTATION_270	0x0c00

No need to use the reserved field.

This makes a lot more sense to me: static (or rarely changing) data does not 
belong to v4l2_buffer, that's what controls are for. And something dynamic 
like pivoting belongs to v4l2_buffer. This seems like a much cleaner API to 
me.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans de Goede]

kilgota@banach.math.auburn.edu wrote:
> 

<huge snip>

> Therefore,
> 
> 1. Everyone seems to agree that the kernel module itself is not going to 
> do things like rotate or flip data even if a given supported device 
> always needs that done.
> 
> However, this decision has a consequence:
> 
> 2. Therefore, the module must send the information about what is needed 
> out of the module, to whatever place is going to deal with it. 
> Information which is known to the module but unknown anywere else must 
> be transmitted somehow.
> 
> Now there is a further consequence:
> 
> 3. In view of (1) and (2) there has to be a way agreed upon for the 
> module to pass the relevant information onward.
> 
> It is precisely on item 3 that we are stuck right now. There is an 
> immediate need, not a theoretical need but an immediate need. However, 
> there is no agreed-upon method or convention for communication.
> 

We are no longer stuck here, the general agreement is adding 2 new buffer 
flags, one to indicate the driver knows the data in the buffer is
vflipped and one for hflip. Then we can handle v-flipped, h-flipped and 180 
degrees cameras

This is agreed up on, Trent is arguing we may need more flags in the future, 
but that is something for the future, all we need know is these 2 flags and 
Hans Verkuil who AFAIK was the only one objecting to doing this with buffer 
flags has agreed this is the best solution.

So Adam, kilgota, please ignore the rest of this thread and move forward with 
the driver, just add the necessary buffer flags to videodev2.h as part of your 
patch (It is usually to submit new API stuff with the same patch which 
introduces the first users of this API.

I welcome libv4l patches to use these flags.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Hans Verkuil wrote:
> On Monday 16 February 2009 05:04:40 Trent Piepho wrote:
>> On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
>>> On Sun, 15 Feb 2009 10:29:03 +0100
>>>
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> I think we should also be able to detect 90 and 270 degree
>>>>> rotations. Or at the very least prepare for it. It's a safe bet to
>>>>> assume that webcams will arrive that can detect portrait vs
>>>>> landscape orientation.
>>>> Handling those (esp on the fly) will be rather hard as width and
>>>> height then get swapped. So lets worry about those when we need to.
>>>> We will need an additional flag for those cases anyways.
>>> The camera rotation is something that is already needed, at least on
>>> some embedded devices, like those cellular phones whose display changes
>>> when you rotate the device.
>>>
>>> By looking at the v4l2_buffer struct, we currently have 4 reserved
>>> bytes. It has also one flags field, with several bits not used. I can
>>> see 2 possibilities to extend the API:
>>>
>>> 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This would
>>> work for 90, 180 and 270 rotation;
>> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't
>> the same as flipping.
>>
>> The problem I'm seeing here is that as people are using v4l2 for digital
>> cameras instead of tv capture there is more and more meta-data available.
>> Things like shutter speed, aperture, focus distance, and so on.  Just
>> look at all the EXIF data a digital camera provides.  Four bytes and two
>> flags are going to run out very quickly at this rate.
>>
>> It's a shame there are not 8 bytes left, as then they could be used for a
>> pointer to an extended meta-data structure.
> 
> I think we have to distinguish between two separate types of data: fixed 
> ('the sensor is mounted upside-down', or 'the sensor always requires a 
> hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').
> 
> The first is static data and I think we can just reuse the existing 
> HFLIP/VFLIP controls: just make them READONLY to tell libv4l that libv4l 
> needs to do the flipping.
> 
> The second is dynamic data and should be passed through v4l2_buffer since 
> this can change on a per-frame basis. In this case add two bits to the 
> v4l2_buffer's flags field:
> 
> V4L2_BUF_FLAG_ROTATION_MSK	0x0c00
> V4L2_BUF_FLAG_ROTATION_0	0x0000
> V4L2_BUF_FLAG_ROTATION_90	0x0400
> V4L2_BUF_FLAG_ROTATION_180	0x0800
> V4L2_BUF_FLAG_ROTATION_270	0x0c00
> 
> No need to use the reserved field.
> 
> This makes a lot more sense to me: static (or rarely changing) data does not 
> belong to v4l2_buffer, that's what controls are for. And something dynamic 
> like pivoting belongs to v4l2_buffer. This seems like a much cleaner API to 
> me.

I agree that we have static and dynamic camera properties, and that we may want 
  to have 2 API's for them. I disagree the control API is the proper API to 
expose static properties, many existing applications will not handle this well.

More over in the case we are discussing now, we have one type of data (sensor 
orientation) which can be both static or dynamic depending on the camera having 
2 API's for this is just plain silly. Thus unnecessarily complicates things if 
some camera property can be static in some cases and dynamic in others then we 
should just always treat it as dynamic. This way we will only have one code 
path to deal with instead of two (with very interesting interactions also, what 
if both API's sat rotate 180 degrees, should we then not rotate at all?). This 
way lies madness.

My conclusion:
1) Since rotation can be dynamic store it in the buffer flags
2) In the future we will most likely need an API to be able to query camera
    properties

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Adam Baker]

On Tuesday 17 February 2009, Hans Verkuil wrote:
<snip>
> The tentative conclusion was that putting it in the v4l2_input
> struct was a good idea.

I'm not sure I'd go as far as to call it even a tentative conclusion.

I think the biggest stumbling block for now is how to handle Olivier Lorin's 
bi-directional camera case.

<snip>
>
> Actually, support for a lot of the cheap webcams is relatively new to the
> kernel. For a long time it was maintained outside the tree. So I'm not
> surprised that came up fairly late in the game.
>

Yes, it's only the advent of libv4l that finally makes pulling all these 
drivers into the kernel viable as before that they all had to break the rules 
and have in kernel format translation from their obscure formats to something 
widely recognized to be useful. (I speak here as someone who has been 
maintaining such a driver out of kernel for a few years)

> And everyone agrees with the need to solve the issue you have. There was
> just the question of were to store that information.
>
> Whoever is pushing this change (you? Adam Baker? I must admit I'm not sure)
> should write a small RFC with possible solutions and pros and cons, post
> it, and when a consensus is reached make a test implementation, see if it
> works and then post the patches with the change. This RFC should only
> address the mount position, not pivoting or USB tables. Those are separate
> issues.

Hans de Geode and Olivier Lorin both posted RFCs on the subject which failed 
to attract the interest that this thread has so I was hoping to reach a 
conclusion here while there is still some momentum to peoples thoughts but 
if you think a new thread that starts with a summary of this one is the best 
way to proceed then I'm happy to start it.

>
> I find it much more productive to use RFCs for API changes/additions, it
> keeps things more focused.

Adam

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

On Tuesday 17 February 2009 03:00:34 kilgota@banach.math.auburn.edu wrote:
> On Mon, 16 Feb 2009, Adam Baker wrote:
> > Lots of snipping below so I hope I get the attributions correct.
> >
> > On Monday 16 February 2009, Hans Verkuil wrote:
> >> We are talking about a core change, so some careful thought should go
> >> into this.
> >
> > Agreed, a few days or even weeks spent getting the right solution is
> > far better than having to update lots of drivers and apps if we get it
> > wrong.
>
> Also agreed. But please permit me to express surprise that such a problem
> never came up before, and does not seem to have been envisioned.
>
> >>> So Adam, kilgota, please ignore the rest of this thread and move
> >>> forward with the driver, just add the necessary buffer flags to
> >>> videodev2.h as part of  your patch (It is usually to submit new API
> >>> stuff with the same patch which introduces the first users of this
> >>> API.
> >>
> >> Don't ignore it yet :-)
>
> Yeah ...
>
> > I've tried twice to write some code when I thought the discussion had
> > died down
>
> I didn't. And now one sees why.
>
> - I'll wait a while before attempting version 3.
>
> Indeed.
>
> >> Hans de Goede <hdegoede@redhat.com> wrote:
> >>> I welcome libv4l patches to use these flags.
> >
> > Olivier Lorin submitted a patch to use the flags to support the 180
> > degree rotation - it was pretty trivial but
> >
> > a) didn't allow v4lconvert_flags to over-ride it to support kernels
> > that don't specify behaviour for those cameras
>
> Eeps. So all those legacy kernels out there need to be supported, too.
>
> > b) only coped with HFLIP and VFLIP both being set
>
> Won't fit the present case.
>
> > Given an agreed solution I intend to fix both of those problems.
>
> [...]
>
> > I certainly agree that re-using the existing controls doesn't seem like
> > a good idea - it seems to combine the case of "the user made a creative
> > decision to produce flipped video" with "this hardware always creates
> > flipped video so please fix it" [...]
>
> Well put.
>
> > Where does all of that leave us?
>
> Immobilized, apparently.
>
>
> Sorry, I would be more patient and less flippant if only everything said
> had addressed the point instead of flying off on tangents and discussing
> things which will not solve the problem, no matter how they are decided.
> As an egregious example, it was brought up that there is/is not/should
> be/should not be a table of devices which behave this way or that way,
> according to USB Vendor:Product number. Now, perhaps it is possible to
> construct an ontological argument for the existence of the Perfect Table,
> or one could argue in some neo-Platonist sense that the Perfect Table
> already exists, in the Realm of Ideals, and we mere mortals only need to
> decide where to keep our imperfect copy. But after seeing that discussion
> I feel forced to point out -- again -- right here and right now there
> sure as hell is a device that can't fit in that table, even if said table
> exists and is Perfect, and no matter where it is kept.
>
> Again, the problem is that here is a set of devices all of which have the
> same USB Vendor:Product number, namely 0x2770:0x9120, and some of them
> require that one does A with the output and others require B. How do you
> tell by the Vendor:Product number whether A is required. or B? You don't.
> What you have to do is to ask the device, and it will answer your
> question. Since nothing else in the kernel or in userspace can do that
> asking other than code internal to the module, the only entity which can
> put the question to the device is the module itself. So, I ask everybody,
> please, this is the actual situation. Deal with it.

We were. There were three discussions going on at the same time: device 
table, pivoting and sensor mount information. The latter is the important 
one for you. The tentative conclusion was that putting it in the v4l2_input 
struct was a good idea.

> I am also quite puzzled that no one seems to have anticipated such a
> situation. I am a bit new to the business of writing a kernel module. But
> I have, in recent years, dealt with a lot of the shit hardware that comes
> our way over at Gphoto, the really cheap $10 to $20 cameras which
> sometimes are even used as prizes in boxes of breakfast cereal The SQ905
> cameras are but one example of these. One thing I have definitely learned
> is that hardware can destroy all "reasonable" expectations. One has to
> adjust. We still have to support the hardware.
>
> Returning to the present discussion, what is wrong with a manufacturer
> producing several devices with minor variations and putting the same
> Vendor:Product number on all of them, and providing a way to query the
> specific capabilities and needs of each of them? Nothing, actually. So
> why does it create such a tailspin? Why do I get the impression that
> nobody else here has ever confronted, envisioned, or anticipated such a
> scenario? I confess that I am surprised.

Actually, support for a lot of the cheap webcams is relatively new to the 
kernel. For a long time it was maintained outside the tree. So I'm not 
surprised that came up fairly late in the game.

And everyone agrees with the need to solve the issue you have. There was 
just the question of were to store that information.

Whoever is pushing this change (you? Adam Baker? I must admit I'm not sure) 
should write a small RFC with possible solutions and pros and cons, post 
it, and when a consensus is reached make a test implementation, see if it 
works and then post the patches with the change. This RFC should only 
address the mount position, not pivoting or USB tables. Those are separate 
issues.

I find it much more productive to use RFCs for API changes/additions, it 
keeps things more focused.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[kilgota]

On Mon, 16 Feb 2009, Adam Baker wrote:

> Lots of snipping below so I hope I get the attributions correct.
>
> On Monday 16 February 2009, Hans Verkuil wrote:
>>
>> We are talking about a core change, so some careful thought should go into
>> this.
>
> Agreed, a few days or even weeks spent getting the right solution is far
> better than having to update lots of drivers and apps if we get it wrong.

Also agreed. But please permit me to express surprise that such a problem
never came up before, and does not seem to have been envisioned.

>
>>
>>> So Adam, kilgota, please ignore the rest of this thread and move forward
>>> with the driver, just add the necessary buffer flags to videodev2.h as
>>> part of  your patch (It is usually to submit new API stuff with the same
>>> patch which introduces the first users of this API.
>>
>> Don't ignore it yet :-)
>>

Yeah ...


> I've tried twice to write some code when I thought the discussion had died
> down

I didn't. And now one sees why.

- I'll wait a while before attempting version 3.

Indeed.

>
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>> I welcome libv4l patches to use these flags.
>
> Olivier Lorin submitted a patch to use the flags to support the 180 degree
> rotation - it was pretty trivial but
>
> a) didn't allow v4lconvert_flags to over-ride it to support kernels that don't
> specify behaviour for those cameras

Eeps. So all those legacy kernels out there need to be supported, too.

> b) only coped with HFLIP and VFLIP both being set

Won't fit the present case.

>
> Given an agreed solution I intend to fix both of those problems.

[...]

> I certainly agree that re-using the existing controls doesn't seem like a good
> idea - it seems to combine the case of "the user made a creative decision to
> produce flipped video" with "this hardware always creates flipped video so
> please fix it" [...]

Well put.

> Where does all of that leave us?

Immobilized, apparently.


Sorry, I would be more patient and less flippant if only everything said 
had addressed the point instead of flying off on tangents and discussing 
things which will not solve the problem, no matter how they are decided. 
As an egregious example, it was brought up that there is/is not/should 
be/should not be a table of devices which behave this way or that way, 
according to USB Vendor:Product number. Now, perhaps it is possible to 
construct an ontological argument for the existence of the Perfect Table, 
or one could argue in some neo-Platonist sense that the Perfect Table 
already exists, in the Realm of Ideals, and we mere mortals only need to 
decide where to keep our imperfect copy. But after seeing that discussion 
I feel forced to point out -- again -- right here and right now there sure 
as hell is a device that can't fit in that table, even if said table 
exists and is Perfect, and no matter where it is kept.

Again, the problem is that here is a set of devices all of which have the 
same USB Vendor:Product number, namely 0x2770:0x9120, and some of them 
require that one does A with the output and others require B. How do you 
tell by the Vendor:Product number whether A is required. or B? You don't. 
What you have to do is to ask the device, and it will answer your 
question. Since nothing else in the kernel or in userspace can do that 
asking other than code internal to the module, the only entity which can 
put the question to the device is the module itself. So, I ask everybody, 
please, this is the actual situation. Deal with it.

I am also quite puzzled that no one seems to have anticipated such a 
situation. I am a bit new to the business of writing a kernel module. But 
I have, in recent years, dealt with a lot of the shit hardware that comes 
our way over at Gphoto, the really cheap $10 to $20 cameras which 
sometimes are even used as prizes in boxes of breakfast cereal The SQ905 
cameras are but one example of these. One thing I have definitely learned 
is that hardware can destroy all "reasonable" expectations. One has to 
adjust. We still have to support the hardware.

Returning to the present discussion, what is wrong with a manufacturer 
producing several devices with minor variations and putting the same 
Vendor:Product number on all of them, and providing a way to query the 
specific capabilities and needs of each of them? Nothing, actually. So why 
does it create such a tailspin? Why do I get the impression that nobody 
else here has ever confronted, envisioned, or anticipated such a scenario?
I confess that I am surprised.

Theodore Kilgore

Re: Adding a control for Sensor Orientation

[Adam Baker]

Lots of snipping below so I hope I get the attributions correct.

On Monday 16 February 2009, Hans Verkuil wrote:
>
> We are talking about a core change, so some careful thought should go into
> this.

Agreed, a few days or even weeks spent getting the right solution is far 
better than having to update lots of drivers and apps if we get it wrong.

>
> > So Adam, kilgota, please ignore the rest of this thread and move forward
> > with the driver, just add the necessary buffer flags to videodev2.h as
> > part of  your patch (It is usually to submit new API stuff with the same
> > patch which introduces the first users of this API.
>
> Don't ignore it yet :-)
>

I've tried twice to write some code when I thought the discussion had died 
down - I'll wait a while before attempting version 3.

> Hans de Goede <hdegoede@redhat.com> wrote:
> > I welcome libv4l patches to use these flags.

Olivier Lorin submitted a patch to use the flags to support the 180 degree 
rotation - it was pretty trivial but 

a) didn't allow v4lconvert_flags to over-ride it to support kernels that don't 
specify behaviour for those cameras
b) only coped with HFLIP and VFLIP both being set

Given an agreed solution I intend to fix both of those problems.


> Hans Verkuil wrote:
> > I think we have to distinguish between two separate types of data: fixed
> > ('the sensor is mounted upside-down', or 'the sensor always requires a
> > hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').
> >

Agreed they are different cases that potentially need different handling

> > The first is static data and I think we can just reuse the existing
> > HFLIP/VFLIP controls: just make them READONLY to tell libv4l that libv4l
> > needs to do the flipping.
> >
> > The second is dynamic data and should be passed through v4l2_buffer since
> > this can change on a per-frame basis. In this case add two bits to the
> > v4l2_buffer's flags field:

I'm not sure how Olivier Lorin's Genesys gl860 case should be handled in this 
scenario - It feels to me that this should be treated as the sensor being 
mounted upside down when it is turned away from the user as it is due to a 
hardware limitation that the picture is upside down in that case and the user 
would want libv4l to fix it - pivoting I see as being more a case of a user 
creative activity and automatically correcting it isn't necessarily good. The 
gl860 case is clearly dynamic data though.

On Monday 16 February 2009, Trent Piepho wrote:
> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't the
> same as flipping.

Agreed - but I think 90 and 270 will only apply to the user pivot case and 
HFLIP / VFLIP only to the sensor mounting. The fact that HFLIP + VFLIP == 
pivot 180 should probably be ignored. Some of the sq905 camera variants 
provide examples of the sensor data being VFLIPed but not HFLIPed.

On Monday 16 February 2009, Hans de Goede wrote:
>
> I agree that we have static and dynamic camera properties, and that we may
> want to have 2 API's for them. I disagree the control API is the proper API
> to expose static properties, many existing applications will not handle
> this well.

I certainly agree that re-using the existing controls doesn't seem like a good 
idea - it seems to combine the case of "the user made a creative decision to 
produce flipped video" with "this hardware always creates flipped video so 
please fix it" If the sensor mounting is going to go in a control then it 
ought to be a new one and I rather see just 1 control with 2 bits as I 
wouldn't want to see a camera be able to tell us only part of the info, that 
just complicates the code unnecessarily. Also having the info possibly 
available via 2 different routes is bound to cause problems.

Where does all of that leave us?

We need to decide if sensor mounting should be considered static info - if it 
should then putting it in a new control seems reasonable. The presence of 
that control then definitively indicates if the driver is providing this 
info. If we say that the gl860 case means this is dynamic info (which is the 
way I'm leaning at the moment) then using 2 bits of buffer flags seems the 
best option as dynamic info shouldn't be in controls.

Unless anyone has evidence to the contrary we don't yet know of any cameras 
that provide pivot info. If any do it is likely that they are in embedded 
devices which may well make the info available via the input mechanism rather 
than as part of the camera. If we do ever get pivot info it might even be 
from some fancy camera mount that provides pitch, yaw and roll so it would be 
premature to attempt to design for it now. Currently we know neither what 
data might be available or how it might be used and the supply of suitable 
crystal balls is limited.

Re: Adding a control for Sensor Orientation

[Trent Piepho]

On Mon, 16 Feb 2009, Hans Verkuil wrote:
> >> If you want to add two bits with
> >> mount information, feel free. But don't abuse them for pivot
> >> information.
> >> If you want that, then add another two bits for the rotation:
> >
> > Ok, this seems good. But if we want to distinguish between static sensor
> > mount
> > information, and dynamic sensor orientation changing due to pivotting,
> > then I
> > think we should only put the pivot flags in the buffer flags, and the
> > static
> > flags should be in the VIDIOC_QUERYCAP capabilities flag, what do you
> > think of
> > that?
>
> That's for driver global information. This type of information is
> input-specific (e.g. input 1 might be from an S-Video input and does not
> require v/hflip, and input 2 is from a sensor and does require v/hflip).
> So struct v4l2_input seems a good place for it.

A control could the return configuration of the current input.

I don't see this as a new and novel use of controls.  If V4L2_CID_VFLIP is
set to 1, then the image is vertically flipped.  What else would it mean?
What does it mean now that's different?

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Mauro Carvalho Chehab wrote:
> On Mon, 16 Feb 2009 13:19:47 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Hans,
> 
>> Mauro Carvalho Chehab wrote:
>>> On Mon, 16 Feb 2009 10:44:03 +0100
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
>>>> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
>>>> and design an API to tell userspace to sensor is upside down and have code for 
>>>> all this both in the driver and in userspace makes no sense. Esp since such a 
>>>> table will probably be more easy to update in userspace too. So the conclusion 
>>>> was to just put the entire table of cams with known upside down mounted sensors 
>>>> in userspace. This is currently in libv4l and making many philips webcam users 
>>>> happy (philips has a tendency to mount the sensor upside down).
>>> Are you saying that you have a table at libv4l for what cameras have sensors
>>> flipped? 
>> Yes.
>>
>>> This is really ugly and proofs that the api is broken. No userspace
>>> application or library should need to do any special hack based on usb id,
>>> driver name or querycap names.
>> Well libv4l is already pretty full of cam specific knowledge in the form of 
>> decompression algorithm's etc.
> 
> That's bad. Not all approaches use libv4l, unfortunately. It will take time to
> port all userspace apps to use it

Quite a bit of work has been done there in F-10, all applications are ported 
except for kopete. And kopete is being worked on.

> and maybe some driver authors will never
> accept libv4l.

So far all I've had contact with have been cheering about it. They are all very 
happy there is a solution to move decompression out of kernelspace. Remember 
this all started to move decompression out of userspace.

 > We are too late with the userspace library. This should have been
> released together with the first V4L2 API, in order to have a broad acceptance.

As we do more and more stuff in libv4l applications will have to use libv4l, 
for example to work with a lot of the webcams supported by the new gspca, they 
need to either reimplement the decompression code for pac207 pac73100 spca501 
spca505 spca507 spca561 and others I forget, or use libv4l.

Also currently I've patches pending to add support for software whitebalance 
correction for cams which need this.

> Due to that, instead of having the info on just one place (at kernel), we will
> split this info on other places. This will lead to inconsistent support,
> depending on what app you're using.

Apps not using libv4l will in general not work with many many cams. For example 
quite a few cheaper UVC cams produce YUYV packed pixel format data many apps 
cannot handle this.

> Since those info are about the hardware characteristics, IMO, kernel driver
> should provide such info.
> 

That is something I agree with, and was my first way of looking at this too, 
but Laurent managed to convince me that there is little use of having a table 
in kernelspace if all that is done with it is export it to userspace and 
kernelspace does nothing with it. Thats just obfuscation for no good reasons 
and added (kernel!) code for no good reasons.

<snip>

>> I've discussed this with Laurent Pinchart, and it really makes the most sense 
>> to do this in userspace.
>>
>> Userspace approach:
>> 1 table is in userspace, libv4l reads it directly, done.
>>
>> Kernelspace approach:
>> 1 add a (smaller) table to *each* driver (which the driver has 0 use for)
>> 2 add code to *each* driver to export this info
>> 3 add code to libv4l to read this
>>
>> You've just created a kernel round trip for no good reason at all, and added a 
>> significant amount of code to the kernel, which can live in userspace just as 
>> well. The userspace approach is the KISS way. Also it is far easier for people 
>> to upgrade libv4l, then it is to upgrade a kernel. Given that this table will 
>> most likely change regulary the ease of updating is another argument for doing 
>> this in userspace.
> 
> I don't agree. Having an userspace library so closely bound to the kernelspace
> counterpart just increases overall support troubles. 
> 
> For example, consider adding support for camera FOO, that is mounted with 180
> degrees at the kernel driver, on the trivial case where the new cam is just a
> new USB ID to an existing driver/chipset.
> 

The trivial case now a days is its a UVC cam, which works by USB class so no 
changes to the kernel are needed at all, unless the table of upside down 
devices lives in the kernel. See why it is a bad idea to have this in the kernel ?

> With a combined userspace/kernelspace, you will need to upgrade both
> kernelspace AND userspace, in order to properly support this cam.
> 

Nope, only libv4l will need to be updated. Now one can argue that the table 
should move from libv4l to a config file, then only a config file would need to 
be updated.

> This also means more work to distro, since libv4l should depend on the kernel
> version, and it will need to check, at runtime, for each driver specific
> version, complaining if libv4l finds a kernel driver newer than libv4l or a
> unknown kernel driver, and providing backport support for older kernels.
> 

Huh? The only match between libv4l and kernel there needs to be is that if the 
webcam produces exotic pixel format foo, you need a libv4l which supports 
exotic pixel format foo, and that is something which we cannot change. Besides 
that there is little need to match libv4l and kernel versions.

> With a kernel only approach, you only need to set the rotation flag at the
> kernel driver. Userspace will work fine with the older and newer kernel
> versions, since all info that userspace needs are the capability flags (or
> controls) for that device.
> 

And support to decompress the data if compressed, which is a much bigger issue.

>> Also can we please STOP with coming up of new and novel ways of abusing the 
>> control API, the control API's purpose is for userspace to control v4l device 
>> settings. It is way overkill for things like communicating a few simple flags 
>> to userspace (and is a pain to use for things like that both on the kernel and 
>> the userspace side).
> 
> In the case of sensors mounted rotated or flipped, I agree that the control API
> is not the better approach. This is better fitted by a capability flag. Hans
> proposal to connect it to the input information seems interesting.
> 
> What would be the other capabilities flag that we need, in order to provide
> enough info to libv4l for it to not need to check for USB ID codes?

At the moment, nothing only rotation info is used, in the future a lot, things 
like "could benefit from software whitebalance" and other flags for future 
image enhancement algorithms come to mind.

Note that this is another argument for having the table in libv4l, as libv4l 
grows new (much asked for) functions, it is very beneficial to have the table 
with flags deciding which functions to enable on which cams inside the lib, 
this way I can push an update to Fedora of just libv4l and all users all of a 
sudden stop getting very blue or green ish pictures from their (cheap) webcam.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Mauro Carvalho Chehab]

On Mon, 16 Feb 2009 13:19:47 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

Hans,

> Mauro Carvalho Chehab wrote:
> > On Mon, 16 Feb 2009 10:44:03 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> >> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
> >> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
> >> and design an API to tell userspace to sensor is upside down and have code for 
> >> all this both in the driver and in userspace makes no sense. Esp since such a 
> >> table will probably be more easy to update in userspace too. So the conclusion 
> >> was to just put the entire table of cams with known upside down mounted sensors 
> >> in userspace. This is currently in libv4l and making many philips webcam users 
> >> happy (philips has a tendency to mount the sensor upside down).
> > 
> > Are you saying that you have a table at libv4l for what cameras have sensors
> > flipped? 
> 
> Yes.
> 
> > This is really ugly and proofs that the api is broken. No userspace
> > application or library should need to do any special hack based on usb id,
> > driver name or querycap names.
> 
> Well libv4l is already pretty full of cam specific knowledge in the form of 
> decompression algorithm's etc.

That's bad. Not all approaches use libv4l, unfortunately. It will take time to
port all userspace apps to use it and maybe some driver authors will never
accept libv4l. We are too late with the userspace library. This should have been
released together with the first V4L2 API, in order to have a broad acceptance.

Due to that, instead of having the info on just one place (at kernel), we will
split this info on other places. This will lead to inconsistent support,
depending on what app you're using.

Since those info are about the hardware characteristics, IMO, kernel driver
should provide such info.

> Quirk tables like this are best kept in userspace, esp. when userspace is the 
> only consumer of the information, why store information in the kernel if the 
> kernel never uses it at all? Take a look at HAL quirks for suspend resume, 
> wireless on/off buttons, etc. for example.

Take a look on how those subsystems work in practice. IMO, they need to work
more on those subsystems.

For example, the desktop machine I'm writing this email has troubles with both
wireless and suspend subsystems.

In the case of suspend, it only suspends with certain kernel/userspace
combinations. With 2.6.28.2 kernel I'm currently running, suspend is broken.
With the kernel that comes with distro, suspend generally works, but
hibernation hangs. On my laptop (that uses the same HAL version and has the same
distro) suspend generally works fine with both the original and the new
kernels (but hibernation fails).

Wireless is another source of mess that starts with their libraries complaining
that kernel has wireless extension FOO, but the library supports only BAR
extension version. It never works fine. 

Here, on all machines/distros/kernel versions I ever tried with wireless, I
always need to do some hacking to have my wireless card to work with WPA. This
generally require things like starting wpa_supplicant manually after machine
reboot, and even sometimes rebooting my access point or disabling dhcp.

So, I don't think that the current status of those subsystems are good
examples, at least currently. I know the maintainers of those subsystems are
working hard to fix the troubles they're facing with, although I'm not
following all the discussions about those subsystems.

> > In the case of flipping, kernel should provide this info for userspace, at
> > least for the cameras it knows it is flipped (based on USB ID or any other
> > method). In the case of DMI, it seems ok to let userspace to use the kernel DMI
> > support to read this info and detect if the sensor were mounted flipped on a
> > notebook, but for those cams where such info is known based on USB ID, we need
> > to have an interface to read this information. I can see some ways for doing it:
> > 
> > 1) via VIDIOC_QUERYCAP capabilities flag;
> > 2) via VIDIOC_*CNTL read-only interfaces;
> > 3) another ioctl for querying the webcam capabilities;
> > 4) some info via sysfs interface;
> > 
> > IMO, the easier and more adequate way for this case is creating an enumbered
> > control. Something like:
> > 
> > #define V4L2_CID_MOUNTED_ANGLE    (V4L2_CID_CAMERA_CLASS_BASE+17)
> > 
> > enum v4l2_mounted_angle {
> > 	V4L2_CID_MOUNTED_ANGLE_0_DEGREES = 0,
> > 	V4L2_CID_MOUNTED_ANGLE_90_DEGREES = 1,
> > 	V4L2_CID_MOUNTED_ANGLE_180_DEGREES = 2,
> > 	V4L2_CID_MOUNTED_ANGLE_270_DEGREES = 3,
> > 	V4L2_CID_MOUNTED_ANGLE_VIA_DMI = 4,
> > };
> > 
> 
> Here you are making things nice and inconsistent, so the information is in the 
> kernel, except where it is not (the DMI case). If we move this in to the 
> kernel, we should move it *completely* in to the kernel.
> 
> I've discussed this with Laurent Pinchart, and it really makes the most sense 
> to do this in userspace.
> 
> Userspace approach:
> 1 table is in userspace, libv4l reads it directly, done.
> 
> Kernelspace approach:
> 1 add a (smaller) table to *each* driver (which the driver has 0 use for)
> 2 add code to *each* driver to export this info
> 3 add code to libv4l to read this
> 
> You've just created a kernel round trip for no good reason at all, and added a 
> significant amount of code to the kernel, which can live in userspace just as 
> well. The userspace approach is the KISS way. Also it is far easier for people 
> to upgrade libv4l, then it is to upgrade a kernel. Given that this table will 
> most likely change regulary the ease of updating is another argument for doing 
> this in userspace.

I don't agree. Having an userspace library so closely bound to the kernelspace
counterpart just increases overall support troubles. 

For example, consider adding support for camera FOO, that is mounted with 180
degrees at the kernel driver, on the trivial case where the new cam is just a
new USB ID to an existing driver/chipset.

With a combined userspace/kernelspace, you will need to upgrade both
kernelspace AND userspace, in order to properly support this cam.

This also means more work to distro, since libv4l should depend on the kernel
version, and it will need to check, at runtime, for each driver specific
version, complaining if libv4l finds a kernel driver newer than libv4l or a
unknown kernel driver, and providing backport support for older kernels.

With a kernel only approach, you only need to set the rotation flag at the
kernel driver. Userspace will work fine with the older and newer kernel
versions, since all info that userspace needs are the capability flags (or
controls) for that device.

> Also can we please STOP with coming up of new and novel ways of abusing the 
> control API, the control API's purpose is for userspace to control v4l device 
> settings. It is way overkill for things like communicating a few simple flags 
> to userspace (and is a pain to use for things like that both on the kernel and 
> the userspace side).

In the case of sensors mounted rotated or flipped, I agree that the control API
is not the better approach. This is better fitted by a capability flag. Hans
proposal to connect it to the input information seems interesting.

What would be the other capabilities flag that we need, in order to provide
enough info to libv4l for it to not need to check for USB ID codes?

-

In the case of the dynamic camera rotation, I'm not sure if we should provide
this info at the frame info. Why do we need such info 60 times per second?
Let's imagine a case of a cam inclined around 45 degrees. You may have bouncing
troubles, like:
frame odd - 0 degrees
frame even - 90 degrees
frame odd - 0 degrees
frame even - 90 degrees

So, I don't think it would be a good idea to have this at v4l2-buffer. This
seems like what we have with tuner strength and mono/stereo detection: we have
tuner ioctl to get such status, and the driver polls this info, from time to
time. The debounce should be done at userspace.

Cheers,
Mauro

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Hans Verkuil wrote:
>>> If you want to add two bits with
>>> mount information, feel free. But don't abuse them for pivot
>>> information.
>>> If you want that, then add another two bits for the rotation:
>>>
>>> #define V4L2_BUF_FLAG_VFLIP     0x0400
>>> #define V4L2_BUF_FLAG_HFLIP     0x0800
>>>
>>> #define V4L2_BUF_FLAG_PIVOT_0   0x0000
>>> #define V4L2_BUF_FLAG_PIVOT_90  0x1000
>>> #define V4L2_BUF_FLAG_PIVOT_180 0x2000
>>> #define V4L2_BUF_FLAG_PIVOT_270 0x3000
>>> #define V4L2_BUF_FLAG_PIVOT_MSK 0x3000
>>>
>> Ok, this seems good. But if we want to distinguish between static sensor
>> mount
>> information, and dynamic sensor orientation changing due to pivotting,
>> then I
>> think we should only put the pivot flags in the buffer flags, and the
>> static
>> flags should be in the VIDIOC_QUERYCAP capabilities flag, what do you
>> think of
>> that?
> 
> That's for driver global information. This type of information is
> input-specific (e.g. input 1 might be from an S-Video input and does not
> require v/hflip, and input 2 is from a sensor and does require v/hflip).
> So struct v4l2_input seems a good place for it.
> 
> Looking at v4l2_input there is a status field, but the status information
> is valid for the current input only. We can:
> 
> 1) add flags here and only set the mounting information for the current
> input,
> 
> 2) add flags here and document that these flags are valid for any input,
> not just the current, or:
> 
> 3) take one of the reserved fields and turn that into a 'flags' field that
> can be used for static info about the input.
> 
> To be honest, I prefer 3.
> 
> The same can be done for v4l2_output should it become necessary in the
> future.
> 
> Actually, pivot information could be stored here as well. Doing that makes
> it possible to obtain the orientation without needing to start a capture,
> and makes it possible to be used (although awkwardly) with the read()
> interface.
> 
> You still want to report this information in v4l2_buffer as well since it
> can change on the fly.
> 

Sounds good to me.

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Mauro Carvalho Chehab]

On Mon, 16 Feb 2009 13:19:47 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

Hans,

> Mauro Carvalho Chehab wrote:
> > On Mon, 16 Feb 2009 10:44:03 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> >> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
> >> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
> >> and design an API to tell userspace to sensor is upside down and have code for 
> >> all this both in the driver and in userspace makes no sense. Esp since such a 
> >> table will probably be more easy to update in userspace too. So the conclusion 
> >> was to just put the entire table of cams with known upside down mounted sensors 
> >> in userspace. This is currently in libv4l and making many philips webcam users 
> >> happy (philips has a tendency to mount the sensor upside down).
> > 
> > Are you saying that you have a table at libv4l for what cameras have sensors
> > flipped? 
> 
> Yes.
> 
> > This is really ugly and proofs that the api is broken. No userspace
> > application or library should need to do any special hack based on usb id,
> > driver name or querycap names.
> 
> Well libv4l is already pretty full of cam specific knowledge in the form of 
> decompression algorithm's etc.

That's bad. Not all approaches use libv4l, unfortunately. It will take time to
port all userspace apps to use it and maybe some driver authors will never
accept libv4l. We are too late with the userspace library. This should be
released together with the first V4L2 API, in order to have a broad acceptance.

Due to that, instead of having the info on just one place (at kernel), we will
split this info on other places. This will lead to inconsistent support,
depending on what app you're using.

Since those info are about the hardware characteristics, IMO, kernel driver
should provide such info.

> Quirk tables like this are best kept in userspace, esp. when userspace is the 
> only consumer of the information, why store information in the kernel if the 
> kernel never uses it at all? Take a look at HAL quirks for suspend resume, 
> wireless on/off buttons, etc. for example.

Take a look on how those subsystems work in practice. For example, the desktop
machine I'm writing this email has troubles with both wireless and suspend
subsystems.

In the case of suspend, it only suspends with some kernel/userspace
combinations. Since I'm running here 2.6.28 kernel, and HAL is prepared for
other kernel versions, suspend is broken. With the kernel that comes with
distro, suspend works. My laptop (that uses the same HAL version and has the
same distro) works fine with both the original and the new kernels.

Wireless is another source of mess that starts with their libraries complaining
that kernel has wireless extension FOO, but the library supports only BAR
extension version. It never works fine. 

Here, on all machines I ever had with wireless, I always need to do some
hacking to have my wireless card to work with WPA. This generally require
things like starting wpa_supplicant manually after machine reboot, and even
sometimes rebooting my access point or disabling dhcp.

So, I don't think that the current status of those subsystems are good examples
of a good implementation. I'm positive that they'll find some solution for
fixing such issues.

> > In the case of flipping, kernel should provide this info for userspace, at
> > least for the cameras it knows it is flipped (based on USB ID or any other
> > method). In the case of DMI, it seems ok to let userspace to use the kernel DMI
> > support to read this info and detect if the sensor were mounted flipped on a
> > notebook, but for those cams where such info is known based on USB ID, we need
> > to have an interface to read this information. I can see some ways for doing it:
> > 
> > 1) via VIDIOC_QUERYCAP capabilities flag;
> > 2) via VIDIOC_*CNTL read-only interfaces;
> > 3) another ioctl for querying the webcam capabilities;
> > 4) some info via sysfs interface;
> > 
> > IMO, the easier and more adequate way for this case is creating an enumbered
> > control. Something like:
> > 
> > #define V4L2_CID_MOUNTED_ANGLE    (V4L2_CID_CAMERA_CLASS_BASE+17)
> > 
> > enum v4l2_mounted_angle {
> > 	V4L2_CID_MOUNTED_ANGLE_0_DEGREES = 0,
> > 	V4L2_CID_MOUNTED_ANGLE_90_DEGREES = 1,
> > 	V4L2_CID_MOUNTED_ANGLE_180_DEGREES = 2,
> > 	V4L2_CID_MOUNTED_ANGLE_270_DEGREES = 3,
> > 	V4L2_CID_MOUNTED_ANGLE_VIA_DMI = 4,
> > };
> > 
> 
> Here you are making things nice and inconsistent, so the information is in the 
> kernel, except where it is not (the DMI case). If we move this in to the 
> kernel, we should move it *completely* in to the kernel.
> 
> I've discussed this with Laurent Pinchart, and it really makes the most sense 
> to do this in userspace.
> 
> Userspace approach:
> 1 table is in userspace, libv4l reads it directly, done.
> 
> Kernelspace approach:
> 1 add a (smaller) table to *each* driver (which the driver has 0 use for)
> 2 add code to *each* driver to export this info
> 3 add code to libv4l to read this
> 
> You've just created a kernel round trip for no good reason at all, and added a 
> significant amount of code to the kernel, which can live in userspace just as 
> well. The userspace approach is the KISS way. Also it is far easier for people 
> to upgrade libv4l, then it is to upgrade a kernel. Given that this table will 
> most likely change regulary the ease of updating is another argument for doing 
> this in userspace.

I don't agree. Having an userspace library so closely bound to the kernelspace
counterpart just increases support troubles. For example, consider adding support
for camera FOO, that is mounted with 180 degrees at the kernel driver, on
the trivial case where the new cam is just a new USB ID to an existing
driver/chipset.

With a combined userspace/kernelspace, you will need to upgrade both
kernelspace AND userspace.

This also means more work to distro, since libv4l should depend on the kernel
version, and it will need to check, at runtime, for each driver specific
version, complaining if libv4l finds a kernel driver newer than libv4l or a
unknown kernel driver, and providing backport support for older kernels.

With a kernel only approach, you only need to set the rotation flag at the
kernel driver. Userspace will work fine with the older and newer kernel
versions, since all userspace need are the capability flags from kernel.

> Also can we please STOP with coming up of new and novel ways of abusing the 
> control API, the control API's purpose is for userspace to control v4l device 
> settings. It is way overkill for things like communicating a few simple flags 
> to userspace (and is a pain to use for things like that both on the kernel and 
> the userspace side).

In the case of sensors mounted rotated or flipped, I agree that the control API
is not the better approach. This is better fitted by a capability flag. We are
currently using 16 of the 32 available bits of the querycap flags field. There
are still 16 bytes available there, so, we may use this to report the
capabilities of the webcam devices.

What are the capabilities that are needed to remove all those hacks at libv4l?

Cheers,
Mauro

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

>> If you want to add two bits with
>> mount information, feel free. But don't abuse them for pivot
>> information.
>> If you want that, then add another two bits for the rotation:
>>
>> #define V4L2_BUF_FLAG_VFLIP     0x0400
>> #define V4L2_BUF_FLAG_HFLIP     0x0800
>>
>> #define V4L2_BUF_FLAG_PIVOT_0   0x0000
>> #define V4L2_BUF_FLAG_PIVOT_90  0x1000
>> #define V4L2_BUF_FLAG_PIVOT_180 0x2000
>> #define V4L2_BUF_FLAG_PIVOT_270 0x3000
>> #define V4L2_BUF_FLAG_PIVOT_MSK 0x3000
>>
>
> Ok, this seems good. But if we want to distinguish between static sensor
> mount
> information, and dynamic sensor orientation changing due to pivotting,
> then I
> think we should only put the pivot flags in the buffer flags, and the
> static
> flags should be in the VIDIOC_QUERYCAP capabilities flag, what do you
> think of
> that?

That's for driver global information. This type of information is
input-specific (e.g. input 1 might be from an S-Video input and does not
require v/hflip, and input 2 is from a sensor and does require v/hflip).
So struct v4l2_input seems a good place for it.

Looking at v4l2_input there is a status field, but the status information
is valid for the current input only. We can:

1) add flags here and only set the mounting information for the current
input,

2) add flags here and document that these flags are valid for any input,
not just the current, or:

3) take one of the reserved fields and turn that into a 'flags' field that
can be used for static info about the input.

To be honest, I prefer 3.

The same can be done for v4l2_output should it become necessary in the
future.

Actually, pivot information could be stored here as well. Doing that makes
it possible to obtain the orientation without needing to start a capture,
and makes it possible to be used (although awkwardly) with the read()
interface.

You still want to report this information in v4l2_buffer as well since it
can change on the fly.

Regards,

       Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Mauro Carvalho Chehab wrote:
> On Mon, 16 Feb 2009 10:44:03 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
>> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
>> and design an API to tell userspace to sensor is upside down and have code for 
>> all this both in the driver and in userspace makes no sense. Esp since such a 
>> table will probably be more easy to update in userspace too. So the conclusion 
>> was to just put the entire table of cams with known upside down mounted sensors 
>> in userspace. This is currently in libv4l and making many philips webcam users 
>> happy (philips has a tendency to mount the sensor upside down).
> 
> Are you saying that you have a table at libv4l for what cameras have sensors
> flipped? 

Yes.

> This is really ugly and proofs that the api is broken. No userspace
> application or library should need to do any special hack based on usb id,
> driver name or querycap names.

Well libv4l is already pretty full of cam specific knowledge in the form of 
decompression algorithm's etc.

Quirk tables like this are best kept in userspace, esp. when userspace is the 
only consumer of the information, why store information in the kernel if the 
kernel never uses it at all? Take a look at HAL quirks for suspend resume, 
wireless on/off buttons, etc. for example.

> In the case of flipping, kernel should provide this info for userspace, at
> least for the cameras it knows it is flipped (based on USB ID or any other
> method). In the case of DMI, it seems ok to let userspace to use the kernel DMI
> support to read this info and detect if the sensor were mounted flipped on a
> notebook, but for those cams where such info is known based on USB ID, we need
> to have an interface to read this information. I can see some ways for doing it:
> 
> 1) via VIDIOC_QUERYCAP capabilities flag;
> 2) via VIDIOC_*CNTL read-only interfaces;
> 3) another ioctl for querying the webcam capabilities;
> 4) some info via sysfs interface;
> 
> IMO, the easier and more adequate way for this case is creating an enumbered
> control. Something like:
> 
> #define V4L2_CID_MOUNTED_ANGLE    (V4L2_CID_CAMERA_CLASS_BASE+17)
> 
> enum v4l2_mounted_angle {
> 	V4L2_CID_MOUNTED_ANGLE_0_DEGREES = 0,
> 	V4L2_CID_MOUNTED_ANGLE_90_DEGREES = 1,
> 	V4L2_CID_MOUNTED_ANGLE_180_DEGREES = 2,
> 	V4L2_CID_MOUNTED_ANGLE_270_DEGREES = 3,
> 	V4L2_CID_MOUNTED_ANGLE_VIA_DMI = 4,
> };
> 

Here you are making things nice and inconsistent, so the information is in the 
kernel, except where it is not (the DMI case). If we move this in to the 
kernel, we should move it *completely* in to the kernel.

I've discussed this with Laurent Pinchart, and it really makes the most sense 
to do this in userspace.

Userspace approach:
1 table is in userspace, libv4l reads it directly, done.

Kernelspace approach:
1 add a (smaller) table to *each* driver (which the driver has 0 use for)
2 add code to *each* driver to export this info
3 add code to libv4l to read this

You've just created a kernel round trip for no good reason at all, and added a 
significant amount of code to the kernel, which can live in userspace just as 
well. The userspace approach is the KISS way. Also it is far easier for people 
to upgrade libv4l, then it is to upgrade a kernel. Given that this table will 
most likely change regulary the ease of updating is another argument for doing 
this in userspace.

Also can we please STOP with coming up of new and novel ways of abusing the 
control API, the control API's purpose is for userspace to control v4l device 
settings. It is way overkill for things like communicating a few simple flags 
to userspace (and is a pain to use for things like that both on the kernel and 
the userspace side).

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Hi Hans,

Hans Verkuil wrote:
> Hi Hans,
> 

<snip>

> Case 3 *is* pivoting. That's a separate piece of information from the
> mount position. All I want is that that is administrated in separate bits.
> And if we do this, do it right and support the reporting of 0, 90, 180 and
> 270 degrees. No one expects libv4l to handle the portrait modes, and apps
> that can handle this will probably not use libv4l at all.
> 
>> Now can we please stop this color of the bikeshed discussion, add the 2
>> damn
>> flags and move forward?
> 
> Anyone can add an API in 5 seconds. It's modifying or removing a bad API
> that worries me as that can take years.

I understand.

> If you want to add two bits with
> mount information, feel free. But don't abuse them for pivot information.
> If you want that, then add another two bits for the rotation:
> 
> #define V4L2_BUF_FLAG_VFLIP     0x0400
> #define V4L2_BUF_FLAG_HFLIP     0x0800
> 
> #define V4L2_BUF_FLAG_PIVOT_0   0x0000
> #define V4L2_BUF_FLAG_PIVOT_90  0x1000
> #define V4L2_BUF_FLAG_PIVOT_180 0x2000
> #define V4L2_BUF_FLAG_PIVOT_270 0x3000
> #define V4L2_BUF_FLAG_PIVOT_MSK 0x3000
> 

Ok, this seems good. But if we want to distinguish between static sensor mount 
information, and dynamic sensor orientation changing due to pivotting, then I 
think we should only put the pivot flags in the buffer flags, and the static 
flags should be in the VIDIOC_QUERYCAP capabilities flag, what do you think of 
that?

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

> On Mon, 16 Feb 2009 12:01:14 +0100 (CET)
> "Hans Verkuil" <hverkuil@xs4all.nl> wrote:
>
>> Anyone can add an API in 5 seconds. It's modifying or removing a bad
>> API that worries me as that can take years. If you want to add two
>> bits with mount information, feel free. But don't abuse them for
>> pivot information. If you want that, then add another two bits for
>> the rotation:
>>
>> #define V4L2_BUF_FLAG_VFLIP     0x0400
>> #define V4L2_BUF_FLAG_HFLIP     0x0800
>>
>> #define V4L2_BUF_FLAG_PIVOT_0   0x0000
>> #define V4L2_BUF_FLAG_PIVOT_90  0x1000
>> #define V4L2_BUF_FLAG_PIVOT_180 0x2000
>> #define V4L2_BUF_FLAG_PIVOT_270 0x3000
>> #define V4L2_BUF_FLAG_PIVOT_MSK 0x3000
>
> Hi,
>
> HFLIP + VFLIP = PIVOT_180
>
> then
>
> #define V4L2_BUF_FLAG_PIVOT_180 0x0c00

This makes it impossible for an application to see the difference between
an upside-down mounted sensor and a 180-degrees pivoted camera. In
addition, there may be sensors that are not upside-down, but only
V-flipped (or only H-flipped).

All this seems so simple, but it really isn't. This is something for an
RFC, posted and discussed both here and at least the linux-omap list, as
the input from the people who are working on those embedded systems will
be very valuable.

And what about output devices? E.g. LCDs? These too can be mounted
upside-down or rotated. You won't encounter this situation on a PC, but it
is another matter in the embedded space. Any API should by preference be
usable both for capture and output.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Jean-Francois Moine]

On Mon, 16 Feb 2009 12:01:14 +0100 (CET)
"Hans Verkuil" <hverkuil@xs4all.nl> wrote:

> Anyone can add an API in 5 seconds. It's modifying or removing a bad
> API that worries me as that can take years. If you want to add two
> bits with mount information, feel free. But don't abuse them for
> pivot information. If you want that, then add another two bits for
> the rotation:
> 
> #define V4L2_BUF_FLAG_VFLIP     0x0400
> #define V4L2_BUF_FLAG_HFLIP     0x0800
> 
> #define V4L2_BUF_FLAG_PIVOT_0   0x0000
> #define V4L2_BUF_FLAG_PIVOT_90  0x1000
> #define V4L2_BUF_FLAG_PIVOT_180 0x2000
> #define V4L2_BUF_FLAG_PIVOT_270 0x3000
> #define V4L2_BUF_FLAG_PIVOT_MSK 0x3000

Hi,

HFLIP + VFLIP = PIVOT_180

then

#define V4L2_BUF_FLAG_PIVOT_180 0x0c00

Cheers.

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: Adding a control for Sensor Orientation

[Mauro Carvalho Chehab]

On Mon, 16 Feb 2009 10:44:03 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
> and design an API to tell userspace to sensor is upside down and have code for 
> all this both in the driver and in userspace makes no sense. Esp since such a 
> table will probably be more easy to update in userspace too. So the conclusion 
> was to just put the entire table of cams with known upside down mounted sensors 
> in userspace. This is currently in libv4l and making many philips webcam users 
> happy (philips has a tendency to mount the sensor upside down).

Are you saying that you have a table at libv4l for what cameras have sensors
flipped? This is really ugly and proofs that the api is broken. No userspace
application or library should need to do any special hack based on usb id,
driver name or querycap names. All needed info should be provided via the
kernel to userspace API. Is there any other case where you need to do such
hacks at userspace?

In the case of flipping, kernel should provide this info for userspace, at
least for the cameras it knows it is flipped (based on USB ID or any other
method). In the case of DMI, it seems ok to let userspace to use the kernel DMI
support to read this info and detect if the sensor were mounted flipped on a
notebook, but for those cams where such info is known based on USB ID, we need
to have an interface to read this information. I can see some ways for doing it:

1) via VIDIOC_QUERYCAP capabilities flag;
2) via VIDIOC_*CNTL read-only interfaces;
3) another ioctl for querying the webcam capabilities;
4) some info via sysfs interface;

IMO, the easier and more adequate way for this case is creating an enumbered
control. Something like:

#define V4L2_CID_MOUNTED_ANGLE    (V4L2_CID_CAMERA_CLASS_BASE+17)

enum v4l2_mounted_angle {
	V4L2_CID_MOUNTED_ANGLE_0_DEGREES = 0,
	V4L2_CID_MOUNTED_ANGLE_90_DEGREES = 1,
	V4L2_CID_MOUNTED_ANGLE_180_DEGREES = 2,
	V4L2_CID_MOUNTED_ANGLE_270_DEGREES = 3,
	V4L2_CID_MOUNTED_ANGLE_VIA_DMI = 4,
};

Of curse, the mounted angle is read-only.

This solves the static case.

We need to discuss more the dynamic case, if this is needed by some device.

Cheers,
Mauro

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

Hi Hans,

> Agreed, and that is not what we are doing, we are only talking mount
> information here.
>
> Lets please keep pivot out of the discussion entirely. Esp the whole
> handheld
> case, notice that the handheld case will have to be handled at the
> application
> level anyways, as the portrait versus landscape detection sensor will not
> be
> part of the cam, but will be handled by the kernel input subsystem.
>
> The problems we are *currently* trying to deal with are 3 things:
>
>
> 1) Some (web)cams have their sensors mounted upside down and some laptops
> have
>     their webcam module entirely mounted upside down
>
> This is a known and solved problem. These webcams have well known usb-ID's
> and
> in the laptop case this get complemented by DMI info as one module type
> (so one
> USB-id) may be mounted correctly in one laptop model and upside down in
> another.
>
> I've discussed this with Laurent Pinchart (and other webcam driver
> authors) and
> the conclusion was that having a table of USB-ID's + DMI strings in the
> driver,
> and design an API to tell userspace to sensor is upside down and have code
> for
> all this both in the driver and in userspace makes no sense. Esp since
> such a
> table will probably be more easy to update in userspace too. So the
> conclusion
> was to just put the entire table of cams with known upside down mounted
> sensors
> in userspace. This is currently in libv4l and making many philips webcam
> users
> happy (philips has a tendency to mount the sensor upside down).
>
> A special case here is if the driver can do flipping in hardware, in this
> case
> (which is rarer then we would like), the driver keeps the table itself and
> simply inverts the meaning of the v and h flip controls so userspace will
> never
> notice anything, this is what the m5602 driver does.

Very nice. I agree completely.

> 2) The new sq905X driver being worked on has the problem that all devices
> have
> the same USB-ID, and some model string or id is read in a sq905 specific
> way
> over usb and the sensor orientation differs from model.
>
> So we need an API to relay this information to userspace, and specifically
> to
> libv4l, so it can correct the orientation in software.
>
> If you look at the amount of code needed here to relay this information
> using a
> control at both the kernel and userspace side this is ridiculous we are
> talking
> about a shitload of code to transport 2 bits from kernel space to
> userspace.
> Adding a new ioctl just for this would be less code.
>
> Also read-only versions of existing controls are definitively not the
> answer
> here. read-only already has a well defined meaning, lets not overload that
> and
> add all kind of vagueness to our API. API's need to be clear, well and
> precisely defined!

OK.

> The discussion on solving the sq905X case was wide open until 3 came
> along:
>
>
> 3) There is a cam which can be clicked on for example the top of a
> notebook
> screen, and then can be flipped over the screen to film someone who is not
> behind the keyboard, but on the other (back) side of the notebook screen.
> This
> flipping happens over the X axis, causing the image to be upside down,
> this cam
> has a built-in sensor to detect this (and only this, its a 1 bit sensor).
>
> Since we want to be able to correct this in software (in libv4l) on the
> fly,
> the idea was born to add vflip and hflip flags to the v4l buffer flags, as
> polling some control for this is too ugly for words. This also gave us a
> nice
> simple KISS solution for problem 2.

OK, I'm fine with using two hflip/vflip bits to tell the mount position.

> So all this has nothing to do with pivotting, case 3 could be seen as a
> very
> special case of pivotting, but it is really just a case of the becoming
> mounted
> upside down on the fly. and there certainly is no 90 degrees rotation (or
> any
> rotation other then 180 degrees) involved in this anywhere. That is a
> different
>   (much harder) problem, which needs to be solved at the application level
> as
> width and height will change.

Case 3 *is* pivoting. That's a separate piece of information from the
mount position. All I want is that that is administrated in separate bits.
And if we do this, do it right and support the reporting of 0, 90, 180 and
270 degrees. No one expects libv4l to handle the portrait modes, and apps
that can handle this will probably not use libv4l at all.

> Now can we please stop this color of the bikeshed discussion, add the 2
> damn
> flags and move forward?

Anyone can add an API in 5 seconds. It's modifying or removing a bad API
that worries me as that can take years. If you want to add two bits with
mount information, feel free. But don't abuse them for pivot information.
If you want that, then add another two bits for the rotation:

#define V4L2_BUF_FLAG_VFLIP     0x0400
#define V4L2_BUF_FLAG_HFLIP     0x0800

#define V4L2_BUF_FLAG_PIVOT_0   0x0000
#define V4L2_BUF_FLAG_PIVOT_90  0x1000
#define V4L2_BUF_FLAG_PIVOT_180 0x2000
#define V4L2_BUF_FLAG_PIVOT_270 0x3000
#define V4L2_BUF_FLAG_PIVOT_MSK 0x3000

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans de Goede]

Hans Verkuil wrote:
>>>
>>> Hans Verkuil wrote:
>>>> On Monday 16 February 2009 05:04:40 Trent Piepho wrote:
>>>>> On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
>>>>>> On Sun, 15 Feb 2009 10:29:03 +0100
>>>>>>
>>>>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>> I think we should also be able to detect 90 and 270 degree
>>>>>>>> rotations. Or at the very least prepare for it. It's a safe bet to
>>>>>>>> assume that webcams will arrive that can detect portrait vs
>>>>>>>> landscape orientation.
>>>>>>> Handling those (esp on the fly) will be rather hard as width and
>>>>>>> height then get swapped. So lets worry about those when we need to.
>>>>>>> We will need an additional flag for those cases anyways.
>>>>>> The camera rotation is something that is already needed, at least on
>>>>>> some embedded devices, like those cellular phones whose display
>>>>>> changes
>>>>>> when you rotate the device.
>>>>>>
>>>>>> By looking at the v4l2_buffer struct, we currently have 4 reserved
>>>>>> bytes. It has also one flags field, with several bits not used. I can
>>>>>> see 2 possibilities to extend the API:
>>>>>>
>>>>>> 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This
>>>>>> would
>>>>>> work for 90, 180 and 270 rotation;
>>>>> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't
>>>>> the same as flipping.
>>>>>
>>>>> The problem I'm seeing here is that as people are using v4l2 for
>>>>> digital
>>>>> cameras instead of tv capture there is more and more meta-data
>>>>> available.
>>>>> Things like shutter speed, aperture, focus distance, and so on.  Just
>>>>> look at all the EXIF data a digital camera provides.  Four bytes and
>>>>> two
>>>>> flags are going to run out very quickly at this rate.
>>>>>
>>>>> It's a shame there are not 8 bytes left, as then they could be used
>>>>> for
>>>>> a
>>>>> pointer to an extended meta-data structure.
>>>> I think we have to distinguish between two separate types of data:
>>>> fixed
>>>> ('the sensor is mounted upside-down', or 'the sensor always requires a
>>>> hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').
>>>>
>>>> The first is static data and I think we can just reuse the existing
>>>> HFLIP/VFLIP controls: just make them READONLY to tell libv4l that
>>>> libv4l
>>>> needs to do the flipping.
>>>>
>>>> The second is dynamic data and should be passed through v4l2_buffer
>>>> since
>>>> this can change on a per-frame basis. In this case add two bits to the
>>>> v4l2_buffer's flags field:
>>>>
>>>> V4L2_BUF_FLAG_ROTATION_MSK	0x0c00
>>>> V4L2_BUF_FLAG_ROTATION_0	0x0000
>>>> V4L2_BUF_FLAG_ROTATION_90	0x0400
>>>> V4L2_BUF_FLAG_ROTATION_180	0x0800
>>>> V4L2_BUF_FLAG_ROTATION_270	0x0c00
>>>>
>>>> No need to use the reserved field.
>>>>
>>>> This makes a lot more sense to me: static (or rarely changing) data
>>>> does
>>>> not
>>>> belong to v4l2_buffer, that's what controls are for. And something
>>>> dynamic
>>>> like pivoting belongs to v4l2_buffer. This seems like a much cleaner
>>>> API
>>>> to
>>>> me.
>>> I agree that we have static and dynamic camera properties, and that we
>>> may
>>> want
>>>   to have 2 API's for them. I disagree the control API is the proper API
>>> to
>>> expose static properties, many existing applications will not handle
>>> this
>>> well.
>> ??? And they will when exposed through v4l2_buffer? It's all new
>> functionality, so that is a non-argument. The point is that libv4l has to
>> be able to detect and handle oddly mounted sensors. It can do that easily
>> through the already existing HFLIP/VFLIP controls. It's a one time check
>> when the device is opened (does it have read-only H/VFLIP controls? If so,
>> then libv4l knows it has to correct).
>>
>> Completely independent from that is the camera pivot: this is dynamic and
>> while by default libv4l may be called upon to handle this, it should also
>> be possible to disable this in libv4l by the application.
>>
>> You should definitely not mix pivoting information with sensor mount
>> information: e.g. if you see hflip and vflip bits set, does that mean that
>> the sensor is mounted upside down? Or that the camera is pivoted 180
>> degrees? That's two different things.
>>
>>> More over in the case we are discussing now, we have one type of data
>>> (sensor
>>> orientation) which can be both static or dynamic depending on the camera
>>> having
>>> 2 API's for this is just plain silly. Thus unnecessarily complicates
>>> things if
>>> some camera property can be static in some cases and dynamic in others
>>> then we
>>> should just always treat it as dynamic. This way we will only have one
>>> code
>>> path to deal with instead of two (with very interesting interactions
>>> also,
>>> what
>>> if both API's sat rotate 180 degrees, should we then not rotate at
>>> all?).
>>> This
>>> way lies madness.
>> I strongly disagree. Yes, if both sensor mount info and pivot info is
>> handled completely inside libv4l, then indeed it doesn't have to rotate at
>> all. But the application probably still wants to know that the user
>> rotated the camera 180 degrees, if only to be able to report this
>> situation. And this is of course even more important for the 90 and 270
>> degree rotations (think handhelds).
>>
>>> My conclusion:
>>> 1) Since rotation can be dynamic store it in the buffer flags
>> Ack. But rotation != sensor mount position.
>>
>>> 2) In the future we will most likely need an API to be able to query
>>> camera
>>>     properties
>> For sensor mount position we have them in the form of HFLIP/VFLIP readonly
>> controls. One-time check, simple to use.
>>
>> I'd like some more input on this from other people as well.
> 
> An additional comment: I'm willing to consider having both hflip/vflip
> bits and rotate bits in v4l2_buffer (although I think it is not the best
> solution), or a separate CAM_SENSOR_MOUNT control conveying the sensor
> mount information. But don't mix mount and pivot info.
> 

Agreed, and that is not what we are doing, we are only talking mount 
information here.

Lets please keep pivot out of the discussion entirely. Esp the whole handheld 
case, notice that the handheld case will have to be handled at the application 
level anyways, as the portrait versus landscape detection sensor will not be 
part of the cam, but will be handled by the kernel input subsystem.

The problems we are *currently* trying to deal with are 3 things:


1) Some (web)cams have their sensors mounted upside down and some laptops have
    their webcam module entirely mounted upside down

This is a known and solved problem. These webcams have well known usb-ID's and 
in the laptop case this get complemented by DMI info as one module type (so one 
USB-id) may be mounted correctly in one laptop model and upside down in another.

I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
and design an API to tell userspace to sensor is upside down and have code for 
all this both in the driver and in userspace makes no sense. Esp since such a 
table will probably be more easy to update in userspace too. So the conclusion 
was to just put the entire table of cams with known upside down mounted sensors 
in userspace. This is currently in libv4l and making many philips webcam users 
happy (philips has a tendency to mount the sensor upside down).

A special case here is if the driver can do flipping in hardware, in this case 
(which is rarer then we would like), the driver keeps the table itself and 
simply inverts the meaning of the v and h flip controls so userspace will never 
notice anything, this is what the m5602 driver does.


2) The new sq905X driver being worked on has the problem that all devices have 
the same USB-ID, and some model string or id is read in a sq905 specific way 
over usb and the sensor orientation differs from model.

So we need an API to relay this information to userspace, and specifically to 
libv4l, so it can correct the orientation in software.

If you look at the amount of code needed here to relay this information using a 
control at both the kernel and userspace side this is ridiculous we are talking 
about a shitload of code to transport 2 bits from kernel space to userspace. 
Adding a new ioctl just for this would be less code.

Also read-only versions of existing controls are definitively not the answer 
here. read-only already has a well defined meaning, lets not overload that and 
add all kind of vagueness to our API. API's need to be clear, well and 
precisely defined!

The discussion on solving the sq905X case was wide open until 3 came along:


3) There is a cam which can be clicked on for example the top of a notebook
screen, and then can be flipped over the screen to film someone who is not
behind the keyboard, but on the other (back) side of the notebook screen. This 
flipping happens over the X axis, causing the image to be upside down, this cam 
has a built-in sensor to detect this (and only this, its a 1 bit sensor).

Since we want to be able to correct this in software (in libv4l) on the fly, 
the idea was born to add vflip and hflip flags to the v4l buffer flags, as 
polling some control for this is too ugly for words. This also gave us a nice 
simple KISS solution for problem 2.


So all this has nothing to do with pivotting, case 3 could be seen as a very 
special case of pivotting, but it is really just a case of the becoming mounted 
upside down on the fly. and there certainly is no 90 degrees rotation (or any 
rotation other then 180 degrees) involved in this anywhere. That is a different 
  (much harder) problem, which needs to be solved at the application level as 
width and height will change.


Now can we please stop this color of the bikeshed discussion, add the 2 damn 
flags and move forward?

Regards,

Hans

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

>
>>
>>
>> Hans Verkuil wrote:
>>> On Monday 16 February 2009 05:04:40 Trent Piepho wrote:
>>>> On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
>>>>> On Sun, 15 Feb 2009 10:29:03 +0100
>>>>>
>>>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> I think we should also be able to detect 90 and 270 degree
>>>>>>> rotations. Or at the very least prepare for it. It's a safe bet to
>>>>>>> assume that webcams will arrive that can detect portrait vs
>>>>>>> landscape orientation.
>>>>>> Handling those (esp on the fly) will be rather hard as width and
>>>>>> height then get swapped. So lets worry about those when we need to.
>>>>>> We will need an additional flag for those cases anyways.
>>>>> The camera rotation is something that is already needed, at least on
>>>>> some embedded devices, like those cellular phones whose display
>>>>> changes
>>>>> when you rotate the device.
>>>>>
>>>>> By looking at the v4l2_buffer struct, we currently have 4 reserved
>>>>> bytes. It has also one flags field, with several bits not used. I can
>>>>> see 2 possibilities to extend the API:
>>>>>
>>>>> 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This
>>>>> would
>>>>> work for 90, 180 and 270 rotation;
>>>> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't
>>>> the same as flipping.
>>>>
>>>> The problem I'm seeing here is that as people are using v4l2 for
>>>> digital
>>>> cameras instead of tv capture there is more and more meta-data
>>>> available.
>>>> Things like shutter speed, aperture, focus distance, and so on.  Just
>>>> look at all the EXIF data a digital camera provides.  Four bytes and
>>>> two
>>>> flags are going to run out very quickly at this rate.
>>>>
>>>> It's a shame there are not 8 bytes left, as then they could be used
>>>> for
>>>> a
>>>> pointer to an extended meta-data structure.
>>>
>>> I think we have to distinguish between two separate types of data:
>>> fixed
>>> ('the sensor is mounted upside-down', or 'the sensor always requires a
>>> hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').
>>>
>>> The first is static data and I think we can just reuse the existing
>>> HFLIP/VFLIP controls: just make them READONLY to tell libv4l that
>>> libv4l
>>> needs to do the flipping.
>>>
>>> The second is dynamic data and should be passed through v4l2_buffer
>>> since
>>> this can change on a per-frame basis. In this case add two bits to the
>>> v4l2_buffer's flags field:
>>>
>>> V4L2_BUF_FLAG_ROTATION_MSK	0x0c00
>>> V4L2_BUF_FLAG_ROTATION_0	0x0000
>>> V4L2_BUF_FLAG_ROTATION_90	0x0400
>>> V4L2_BUF_FLAG_ROTATION_180	0x0800
>>> V4L2_BUF_FLAG_ROTATION_270	0x0c00
>>>
>>> No need to use the reserved field.
>>>
>>> This makes a lot more sense to me: static (or rarely changing) data
>>> does
>>> not
>>> belong to v4l2_buffer, that's what controls are for. And something
>>> dynamic
>>> like pivoting belongs to v4l2_buffer. This seems like a much cleaner
>>> API
>>> to
>>> me.
>>
>> I agree that we have static and dynamic camera properties, and that we
>> may
>> want
>>   to have 2 API's for them. I disagree the control API is the proper API
>> to
>> expose static properties, many existing applications will not handle
>> this
>> well.
>
> ??? And they will when exposed through v4l2_buffer? It's all new
> functionality, so that is a non-argument. The point is that libv4l has to
> be able to detect and handle oddly mounted sensors. It can do that easily
> through the already existing HFLIP/VFLIP controls. It's a one time check
> when the device is opened (does it have read-only H/VFLIP controls? If so,
> then libv4l knows it has to correct).
>
> Completely independent from that is the camera pivot: this is dynamic and
> while by default libv4l may be called upon to handle this, it should also
> be possible to disable this in libv4l by the application.
>
> You should definitely not mix pivoting information with sensor mount
> information: e.g. if you see hflip and vflip bits set, does that mean that
> the sensor is mounted upside down? Or that the camera is pivoted 180
> degrees? That's two different things.
>
>> More over in the case we are discussing now, we have one type of data
>> (sensor
>> orientation) which can be both static or dynamic depending on the camera
>> having
>> 2 API's for this is just plain silly. Thus unnecessarily complicates
>> things if
>> some camera property can be static in some cases and dynamic in others
>> then we
>> should just always treat it as dynamic. This way we will only have one
>> code
>> path to deal with instead of two (with very interesting interactions
>> also,
>> what
>> if both API's sat rotate 180 degrees, should we then not rotate at
>> all?).
>> This
>> way lies madness.
>
> I strongly disagree. Yes, if both sensor mount info and pivot info is
> handled completely inside libv4l, then indeed it doesn't have to rotate at
> all. But the application probably still wants to know that the user
> rotated the camera 180 degrees, if only to be able to report this
> situation. And this is of course even more important for the 90 and 270
> degree rotations (think handhelds).
>
>> My conclusion:
>> 1) Since rotation can be dynamic store it in the buffer flags
>
> Ack. But rotation != sensor mount position.
>
>> 2) In the future we will most likely need an API to be able to query
>> camera
>>     properties
>
> For sensor mount position we have them in the form of HFLIP/VFLIP readonly
> controls. One-time check, simple to use.
>
> I'd like some more input on this from other people as well.

An additional comment: I'm willing to consider having both hflip/vflip
bits and rotate bits in v4l2_buffer (although I think it is not the best
solution), or a separate CAM_SENSOR_MOUNT control conveying the sensor
mount information. But don't mix mount and pivot info.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

>
>
> Hans Verkuil wrote:
>> On Monday 16 February 2009 05:04:40 Trent Piepho wrote:
>>> On Sun, 15 Feb 2009, Mauro Carvalho Chehab wrote:
>>>> On Sun, 15 Feb 2009 10:29:03 +0100
>>>>
>>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> I think we should also be able to detect 90 and 270 degree
>>>>>> rotations. Or at the very least prepare for it. It's a safe bet to
>>>>>> assume that webcams will arrive that can detect portrait vs
>>>>>> landscape orientation.
>>>>> Handling those (esp on the fly) will be rather hard as width and
>>>>> height then get swapped. So lets worry about those when we need to.
>>>>> We will need an additional flag for those cases anyways.
>>>> The camera rotation is something that is already needed, at least on
>>>> some embedded devices, like those cellular phones whose display
>>>> changes
>>>> when you rotate the device.
>>>>
>>>> By looking at the v4l2_buffer struct, we currently have 4 reserved
>>>> bytes. It has also one flags field, with several bits not used. I can
>>>> see 2 possibilities to extend the API:
>>>>
>>>> 1) adding V4L2_BUF_FLAG_HFLIP and V4L2_BUF_FLAG_VFLIP flags. This
>>>> would
>>>> work for 90, 180 and 270 rotation;
>>> HFLIP and VFLIP are only good for 0 and 180 degrees.  90 and 270 isn't
>>> the same as flipping.
>>>
>>> The problem I'm seeing here is that as people are using v4l2 for
>>> digital
>>> cameras instead of tv capture there is more and more meta-data
>>> available.
>>> Things like shutter speed, aperture, focus distance, and so on.  Just
>>> look at all the EXIF data a digital camera provides.  Four bytes and
>>> two
>>> flags are going to run out very quickly at this rate.
>>>
>>> It's a shame there are not 8 bytes left, as then they could be used for
>>> a
>>> pointer to an extended meta-data structure.
>>
>> I think we have to distinguish between two separate types of data: fixed
>> ('the sensor is mounted upside-down', or 'the sensor always requires a
>> hflip/vflip') and dynamic ('the user pivoted the camera 270 degrees').
>>
>> The first is static data and I think we can just reuse the existing
>> HFLIP/VFLIP controls: just make them READONLY to tell libv4l that libv4l
>> needs to do the flipping.
>>
>> The second is dynamic data and should be passed through v4l2_buffer
>> since
>> this can change on a per-frame basis. In this case add two bits to the
>> v4l2_buffer's flags field:
>>
>> V4L2_BUF_FLAG_ROTATION_MSK	0x0c00
>> V4L2_BUF_FLAG_ROTATION_0	0x0000
>> V4L2_BUF_FLAG_ROTATION_90	0x0400
>> V4L2_BUF_FLAG_ROTATION_180	0x0800
>> V4L2_BUF_FLAG_ROTATION_270	0x0c00
>>
>> No need to use the reserved field.
>>
>> This makes a lot more sense to me: static (or rarely changing) data does
>> not
>> belong to v4l2_buffer, that's what controls are for. And something
>> dynamic
>> like pivoting belongs to v4l2_buffer. This seems like a much cleaner API
>> to
>> me.
>
> I agree that we have static and dynamic camera properties, and that we may
> want
>   to have 2 API's for them. I disagree the control API is the proper API
> to
> expose static properties, many existing applications will not handle this
> well.

??? And they will when exposed through v4l2_buffer? It's all new
functionality, so that is a non-argument. The point is that libv4l has to
be able to detect and handle oddly mounted sensors. It can do that easily
through the already existing HFLIP/VFLIP controls. It's a one time check
when the device is opened (does it have read-only H/VFLIP controls? If so,
then libv4l knows it has to correct).

Completely independent from that is the camera pivot: this is dynamic and
while by default libv4l may be called upon to handle this, it should also
be possible to disable this in libv4l by the application.

You should definitely not mix pivoting information with sensor mount
information: e.g. if you see hflip and vflip bits set, does that mean that
the sensor is mounted upside down? Or that the camera is pivoted 180
degrees? That's two different things.

> More over in the case we are discussing now, we have one type of data
> (sensor
> orientation) which can be both static or dynamic depending on the camera
> having
> 2 API's for this is just plain silly. Thus unnecessarily complicates
> things if
> some camera property can be static in some cases and dynamic in others
> then we
> should just always treat it as dynamic. This way we will only have one
> code
> path to deal with instead of two (with very interesting interactions also,
> what
> if both API's sat rotate 180 degrees, should we then not rotate at all?).
> This
> way lies madness.

I strongly disagree. Yes, if both sensor mount info and pivot info is
handled completely inside libv4l, then indeed it doesn't have to rotate at
all. But the application probably still wants to know that the user
rotated the camera 180 degrees, if only to be able to report this
situation. And this is of course even more important for the 90 and 270
degree rotations (think handhelds).

> My conclusion:
> 1) Since rotation can be dynamic store it in the buffer flags

Ack. But rotation != sensor mount position.

> 2) In the future we will most likely need an API to be able to query
> camera
>     properties

For sensor mount position we have them in the form of HFLIP/VFLIP readonly
controls. One-time check, simple to use.

I'd like some more input on this from other people as well.

Regards,

     Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: Adding a control for Sensor Orientation

[Hans Verkuil]

>
>
> kilgota@banach.math.auburn.edu wrote:
>>
>
> <huge snip>
>
>> Therefore,
>>
>> 1. Everyone seems to agree that the kernel module itself is not going to
>> do things like rotate or flip data even if a given supported device
>> always needs that done.
>>
>> However, this decision has a consequence:
>>
>> 2. Therefore, the module must send the information about what is needed
>> out of the module, to whatever place is going to deal with it.
>> Information which is known to the module but unknown anywere else must
>> be transmitted somehow.
>>
>> Now there is a further consequence:
>>
>> 3. In view of (1) and (2) there has to be a way agreed upon for the
>> module to pass the relevant information onward.
>>
>> It is precisely on item 3 that we are stuck right now. There is an
>> immediate need, not a theoretical need but an immediate need. However,
>> there is no agreed-upon method or convention for communication.
>>
>
> We are no longer stuck here, the general agreement is adding 2 new buffer
> flags, one to indicate the driver knows the data in the buffer is
> vflipped and one for hflip. Then we can handle v-flipped, h-flipped and
> 180
> degrees cameras
>
> This is agreed up on, Trent is arguing we may need more flags in the
> future,
> but that is something for the future, all we need know is these 2 flags
> and
> Hans Verkuil who AFAIK was the only one objecting to doing this with
> buffer
> flags has agreed this is the best solution.

Well, I just posted an alternative solution this morning (Hans probably
hadn't read it yet) which I want to see discussed first. I think it is a
better solution than this. Basically combining the best of two worlds
IMHO.

We are talking about a core change, so some careful thought should go into
this.

> So Adam, kilgota, please ignore the rest of this thread and move forward
> with
> the driver, just add the necessary buffer flags to videodev2.h as part of
> your
> patch (It is usually to submit new API stuff with the same patch which
> introduces the first users of this API.

Don't ignore it yet :-)

Regards,

       Hans

> I welcome libv4l patches to use these flags.
>
> Regards,
>
> Hans
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG