[libcamera-devel] [PATCH 04/23] libcamera: properties: Add rotation property
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Feb 13 22:46:22 CET 2020
On 13/02/2020 21:01, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Feb 05, 2020 at 11:51:05AM +0100, jacopo mondi wrote:
>> On Tue, Feb 04, 2020 at 01:42:49AM +0200, Laurent Pinchart wrote:
>>> Hi Jacopo,
>>>
>>> Thank you for the patch.
>>>
>>> This looks really good. The description is quite long, but that's
>>> unavoidable if we want to be both precise and generic enough to cover
>>> all use cases.
>>>
>>> Please see below for a few minor comments.
>>>
>>> On Mon, Jan 13, 2020 at 05:42:26PM +0100, Jacopo Mondi wrote:
>>>> The rotation property describes the rotation of the camera sensor.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>>> ---
>>>> src/libcamera/property_ids.yaml | 309 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 309 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
>>>> index aaadcbd3e52b..243af7bd0a03 100644
>>>> --- a/src/libcamera/property_ids.yaml
>>>> +++ b/src/libcamera/property_ids.yaml
>>>> @@ -25,4 +25,313 @@ controls:
>>>> description: |
>>>> The camera is attached to the device in a way that allows it to
>>>> be moved freely
>>>> +
>>>> + - Rotation:
>>>> + type: int32_t
>>>> + description: |
>>>> + The camera rotation is expressed as the angular difference in degrees
>>>> + between two reference systems, one implicitly defined by the camera
>>>
>>> I wouldn't say "implicitly defined" here, as the reference system is
>>> defined below, quite explicitly :-) How about "one relative to the
>>> camera module" ?
>>
>> Would you keep "camera module intrinsics characteristics" or just
>> "camera module" ?
>>
>> I would prefer the former, as the coordintate system is defined by the
>> sensor's pixel array displacement.
>
> I would prefer the latter actually, I'm not sure what "relative to the
> camera module intrinsic characteristics" actually mean.
>
>>>> + module intrinsics characteristics, and one artificially defined on the
>>>> + external world scene to be captured when projected on the image sensor
>>>> + pixel array.
>>>> +
>>>> + A camera sensor has an implicitly defined 2-dimensional reference
>>>> + system 'Rc' defined by its pixel array scan-out order, with its origin
>>>
>>> "scan-out" is mostly used for displays, for camera sensors the usual
>>> term is "read-out".
>>
>> Ack
>>
>>>> + posed at pixel address (0,0), the x-axis progressing from there towards
>>>
>>> Maybe "placed" instead of "posed" ? The latter doesn't match a
>>> definition I can find, but I may be wrong.
>>>
>>> Pixels don't have addresses, they have coordinates.
I'd use 'positioned at coordinate 0,0' or such then ...
The origin hasn't been 'placed'.
You could also use "located at ..."
>> I've seen address used in many sensor chip manuals, I think I took it
>> from there, but I can indeed change it.
>>
>>> Unless I'm mistaken axes are usually named with capital letters.
>>>
>>>> + the last scanned out column of the pixel array and the y-axis
>>>> + progressing towards the last scanned out line.
>>>
>>> To match the above,
>>>
>>> "A camera sensor has a 2-dimensional reference system 'Rc' defined by
>>> its pixel array read-out order. The origin is set to the first pixel
>>> being read out, the X-axis points along the column read-out direction
>>> towards the last columns, and the Y-axis along the row read-out
>>> direction towards the last row."
>>
>> I'm not sure I'm super happy with the expression
>>
>> "The X-axis points along the column read-out direction towards the
>> last column."
>>
>> if compared to:
>>
>> "The X-axis progressing from the here [the first read-out pixel]
>> towards the last read-out column."
>
> Did you mean "there" instead of "the here" ? I find the second a bit
> strange (otherwise I wouldn't have commented on it :-)), but it may be a
> matter of personal taste.
>
> Kieran, what can your native English skills teach us ?
We'd never really use "the here" ... but in the sentences above "there"
doesn't make sense either...
I think I'm missing the original context statement...
>
>>>> +
>>>> + A typical example for a sensor with a (2592x1944) pixel array matrix
>>>
>>> You can drop the parentheses here, they're used when expressing
>>> coordinate tuples, but not for sizes.
>>
>> Ack
>>
>>>> + observed from the front is
>>>> +
>>>> + (2592) x-axis 0
>>>
>>> s/x/X/ (and below, and same for y)
>>
>> Ack
>>
>>>> + <------------------------+ 0
>>>> + .......... ... ..........!
>>>> + .......... ... ..........! y-axis
>>>> + ... !
>>>> + .......... ... ..........!
>>>> + .......... ... ..........! (1944)
>>>> + V
>>>
>>> 2592 and 1944 should be 2591 and 1943 respectively as you count from 0.
>>> The parentheses are also not needed.
>>
>> Ack
>>
>>> Typically sensors also have dark and active boundary rows and columns,
>>> but that may be too much details to include here.
>>
>> Not a detail I would include here. And, by the way, I the number I
>> have used here have been taken from the full pixel array size of a
>> sensor producing 2560x1920 images. They include black and calibration
>> pixels.
>>
>>>> + The external world scene reference system scene 'Rs' is defined as a
>>>
>>> s/system scene/system/ ?
>>
>> Ack
>>
>>>> + 2-dimensional reference system on the parallel plane posed in front
>>>> + of the camera module's focal plane, with its origin placed on the
>>>
>>> This is a bit complicated, how about simply saying that the reference
>>> system is defined on the focal plane of the sensor ?
>>
>> Ack
>>
>>>> + visible top-left corner, the x-axis progressing towards the right from
>>>> + there and the y-axis progressing towards the bottom of the visible
>>>> + scene.
>>>
>>> Shouldn't we point out that the terms top, left, right and bottom are
>>> intentionally not defined ?
>>>
>>> With the above comments applied here,
>>>
>>> "The external world scene reference system 'Rs' is a 2-dimensional
>>> reference system on the focal plane of the camera module. The origin is
>>> placed on the top-left corner of the visible scene, the X-axis points
>>> towards the right, and the Y-axis points towards the bottom of the
>>> scene. The top, bottom, left and right directions are intentionally not
>>> defined and depend on the environment in which the camera is used."
>>
>> Ack
>>
>>>> +
>>>> + A typical example of a (very common) picture of a shark swimming from
>>>> + left to right is
>>>
>>> "from left to right, as seen from the camera, is"
>>
>> Ack
>>
>>>> +
>>>> + x-axis
>>>> + (0,0)---------------------->
>>>> + !
>>>> + !
>>>> + ! |\____)\___
>>>> + ! ) _____ __`<
>>>> + ! |/ )/
>>>> + !
>>>> + V
>>>> + y-axis
>>>> +
>>>> + With the reference plane posed in front of the camera module and
>>>> + parallel to its focal plane
>>>> +
>>>> + !
>>>> + / !
>>>> + / !
>>>> + / !
>>>> + _ / !
>>>> + +-/ \-+ / !
>>>> + | (o) | ! 'Rs' reference plane
>>>> + +-----+ \ !
>>>> + \ !
>>>> + \ !
>>>> + \ !
>>>> + \ !
>>>> + !
>>>> +
>>>> + When projected on the sensor's pixel array, the image and the associated
>>>> + reference system 'Rs' are typically inverted, due to the camera module's
>>>
>>> Maybe "typically (but not always)" ?
>>>
>>>> + lens optical inversion effect.
>>>> +
>>>> + Assuming the above represented scene of the swimming shark, the lens
>>>> + inversion projects on the sensor pixel array the reference plane 'Rp'
>>>
>>> I got confused for a second, wondering why only the Y axis was inverted,
>>> until I realized the this drawing shows the scene projected onto the
>>> camera module as seen when looking at the camera module. I think we
>>> should mention this explicitly:
>>
>> Indeed, thanks for pointing it out. I inverted the position of the
>> observer without even noticing.
>>
>>> "Assuming the above represented scene of the swimming shark, the lens
>>> inversion projects the scene and its reference system onto the sensor
>>> pixel array as follows, seen from the front of the camera sensor."
>>
>> Ack
>>
>>>> +
>>>> + y-axis
>>>> + ^
>>>> + !
>>>> + ! |\_____)\__
>>>> + ! ) ____ ___.<
>>>> + ! |/ )/
>>>> + !
>>>> + !
>>>> + (0,0)---------------------->
>>>> + x-axis
>>>
>>> "Note the shark being upside-down.
>>>
>>> The resulting projected reference system is named 'Rp'."
>>>
>>>> +
>>>> + The camera rotation property is then defined as the angular difference
>>>> + in counterclockwise direction between the origin of the camera reference
s/counterclockwise/counter-clockwise/
>>>
>>> Nitpicking again, an angular difference can't be defined between
>>> origins, as origins are points. It should be defined between reference
>>> systems.
>>>
>>> We should specify that the rotation is expressed in degrees as a number
>>> in the [0, 360[ range.
>>
>> true
>>
>>>> + system 'Rc', defined by the camera sensor scan-out direction and its
>>>> + mounting position, and the origin of the projected scene reference
>>>> + system 'Rp', result of the optical projection of the scene reference
>>>> + system 'Rs' on the sensor pixel array.
>>>
>>> I don't think we need to repeat the definitions of the two reference
>>> systems.
>>
>> I was not sure it was needed, but it seemed to me it was not too heavy
>> to read, but it can be dropped indeed
>>
>>> "The camera rotation property is then defined as the angular difference
>>> in the counterclockwise direction between the camera reference system
>>> 'Rc' and the projected scene reference system 'Rp'. It is expressed in
>>> degrees as a number in the range [0, 360[."
>>
>> Thanks, works well
>>
>>>> +
>>>> + Examples
>>>> +
>>>> + 0 degrees camera rotation
>>>> +
>>>> + y-Rp
>>>> + y-Rc ^
>>>> + ^ !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! (0,0)---------------------->
>>>> + ! x-Rp
>>>> + 0 +------------------------------------->
>>>> + 0 x-Rc
>>>
>>> The X-axis 0 should be aligned with the +, or the Y-axis 0 moved one
>>> line up. You may want to use the same notation for the Rc and Rp
>>> reference systems ('(0,0)' or split coordinates in both cases).
>>
>> I am not sure why I used two different notations for the camera and
>> the projected scene reference systems. I will use a single one
>>
>>> Should the x-Rc axis have the same length as the x-Rp axis ?
>>
>> They could, let's see how the drawing looks like.
>>
>>>> +
>>>> +
>>>> + x-Rc 0
>>>> + <------------------------+ 0
>>>> + x-Rp !
>>>> + <-----------------------(0,0) !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! V
>>>> + ! y-Rc
>>>> + V
>>>> + y-Rp
>>>> +
>>>> + 90 degrees camera rotation
>>>> +
>>>> + 0 y-Rc
>>>> + 0 +----------------------->
>>>> + !
>>>> + ! y-Rp
>>>> + ! ^
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! (0,0)---------------------->
>>>> + ! x-Rp
>>>> + !
>>>> + V
>>>> + x-Rc
>>>> +
>>>> + 180 degrees camera rotation
>>>> +
>>>> + x-Cr 0
>>>
>>> s/Cr/Rc/
>>>
>>>> + <------------------------+ 0
>>>> + y-Rp !
>>>> + ^ !
>>>> + ! ! y-Cr
>>>> + ! !
>>>> + ! !
>>>> + ! V
>>>> + !
>>>> + !
>>>> + (0,0)--------------------->
>>>> + x-Rp
>>>> +
>>>> + 270 degrees camera rotation
>>>> +
>>>> + 0 y-Rc
>>>> + 0 +----------------------->
>>>> + ! x-Rp
>>>> + ! <-----------------------(0,0
>>>
>>> Missing ) at the end of the line.
>>>
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! !
>>>> + ! V
>>>> + ! y-Rp
>>>> + !
>>>> + !
>>>> + V
>>>> + x-Rc
>>>> +
>>>> +
>>>> +
>>>> + Example one - Webcam
>>>> +
>>>> + A camera module installed on the user facing part of a laptop screen
>>>> + casing used for video calls. The captured images are meant to be
>>>> + displayed in landscape mode (width > height) on the laptop screen.
>>>> +
>>>> + The camera is typically mounted 180 degrees rotated to compensate the
>>>> + lens optical inversion effect.
>>>
>>> I understand what you mean here, but can be confusing for the reader.
>>> The direction of the Rc and Rp reference systems depicted below
>>> correspond to the 0° rotation above, and here the text states 180°. How
>>> about writing "The camera sensor is typically mounted upside-down to
>>> compensate ..." ?
>>
>> Ack
>>
>>>> +
>>>> + y-Rp
>>>> + y-Rc ^
>>>> + ^ !
>>>> + ! ! |\_____)\__
>>>> + ! ! ) ____ ___.<
>>>> + ! ! |/ )/
>>>> + ! !
>>>> + ! !
>>>> + ! (0,0)---------------------->
>>>> + ! x-Rp
>>>> + 0 +------------------------------------->
>>>> + 0 x-Rc
>>>> +
>>>> + The two reference systems are aligned, the resulting camera rotation is
>>>> + 0 degrees, no rotation correction should be applied to the resulting
>>>
>>> s/should/needs to/ ?
>>
>> ack
>>
>>>> + image once captured to memory buffers to correctly display it to users.
>>>> +
>>>> + +--------------------------+
>>>> + ! |\____)\___ !
>>>> + ! ) _____ __`< !
>>>> + ! |/ )/ !
>>>> + +--------------------------+
>>>> +
>>>> + If the camera module is not mounted 180 degrees rotated to compensate
>>>
>>> And here, for the reason explained above, "is not mounted upside-down to
>>> compensate ..." ?
>>>
>>>> + the lens optical inversion, the two reference system will result not
>>>
>>> s/system/systems/
>>>
>>>> + aligned, with 'Rp' plane 180 degrees rotated in respect to the 'Rc'
>>>
>>> s/will result not aligned/will not be aligned/ ?
>>
>> ack on all the above comments
>>
>>>> + plane.
>>>
>>> As above, we should probably not use plane to refer to Rc and Rp, as
>>> they're defined as reference systems. How about the following ?
>>>
>>> "If the camera sensor is not mounted upside-down to compensate for the
>>> lens optical inversion, the two reference systems will not be aligned,
>>> with 'Rp' being rotated 180 degrees relatively to 'Rc'."
>>
>> It's more clear, thanks
>>
>>>> + x-Rc 0
>>>> + <------------------------+ 0
>>>> + y-Rp !
>>>> + ^ !
>>>> + ! ! y-Rc
>>>> + ! |\_____)\__ !
>>>> + ! ) ____ ___.< !
>>>> + ! |/ )/ V
>>>> + !
>>>> + !
>>>> + (0,0)--------------------->
>>>> + x-Rp
>>>> +
>>>> + The image once captured to memory will then be 180 degrees rotated
>>>
>>> "will then be rotated by 180 degrees" ?
>>
>> matter of tastes I guess, but ok
>>
>>>> +
>>>> + +--------------------------+
>>>> + ! __/(_____/| !
>>>> + ! >.___ ____ ( !
>>>> + ! \( \| !
>>>> + +--------------------------+
>>>
>>> Should we add two lines above and below the shark to keep the aspect
>>> ration ?
>>>
>>>> +
>>>> + A software rotation correction of 180 degrees should be applied to
>>>> + correctly display the image.
>>>> +
>>>> + +--------------------------+
>>>> + ! |\____)\___ !
>>>> + ! ) _____ __`< !
>>>> + ! |/ )/ !
>>>> + +--------------------------+
>>>
>>> Here too.
>>>
>>>> +
>>>> + Example two - Phone camera
>>>> +
>>>> + A camera installed on the back-side of a mobile device facing away from
>>>
>>> s/back-side/back side/
>>>
>>>> + the user. The captured images are meant to be displayed in portrait mode
>>>> + (height > width) to match the device screen orientation and the device
>>>> + usage orientation used when taking the picture.
>>>> +
>>>> + The camera is typically mounted with its pixel array longer side aligned
>>>
>>> s/camera/camera sensor/
>>>
>>>> + to the device longer side, 180 degrees rotated to compensate the lens
>>>
>>> s/compensate/compensate for/ ?
>>>
>>>> + optical inversion effect.
>>>> +
>>>> + 0 y-Rc
>>>> + 0 +----------------------->
>>>> + !
>>>> + ! y-Rp
>>>> + ! ^
>>>> + ! !
>>>> + ! ! |\_____)\__
>>>> + ! ! ) ____ ___.<
>>>> + ! ! |/ )/
>>>> + ! !
>>>> + ! !
>>>> + ! (0,0)---------------------->
>>>> + ! x-Rp
>>>> + !
>>>> + !
>>>> + V
>>>> + x-Rc
>>>> +
>>>> + The two reference systems are not aligned and the 'Rp' reference
>>>> + system is 90 degrees rotated in counterclockwise direction in respect
>>>
>>> "... is rotated by 90 degrees in the counterclockwise direction
>>> relatively to the ..."
s/counterclockwise/counter-clockwise/
>>>
>>>> + to the 'Rc' reference system.
>>>> +
>>>> + The image, when captured to memory buffer will be rotated.
>>>
>>> To match the previous example,
>>>
>>> "The image once captured to memory will be rotated."
>>>
>>>> +
>>>> + +---------------------------------------+
>>>> + | _ _ |
>>>> + | \ / |
>>>> + | | | |
>>>> + | | | |
>>>> + | | > |
>>>> + | < | |
>>>> + | | | |
>>>> + | . |
>>>> + | V |
>>>> + +---------------------------------------+
>>>> +
>>>> + A correction of 90 degrees in counterclockwise direction has to be
And again
>>>> + applied to correctly display the image in portrait mode on the device
>>>> + screen.
>>>> +
>>>> + +-----------------+
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |\____)\___ |
>>>> + | ) _____ __`< |
>>>> + | |/ )/ |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + | |
>>>> + +-----------------+
>>>
>>> I had to think a bit to check if the sharks are in the right
>>> orientation, and they are :-) I would however reduce the frame a little
>>> bit to match the size of the references systems above.
>>
>> I received the opposed comments from Andrey and I agree with him that
>> we should try to keep the image proportions similar to the ones above
>> to avoid the idea of cropping. In my view, the coordinate system does
>> not represent the size of the captured image, but I can work to make
>> them look like it.
>>
>>>> ...
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list