[libcamera-devel] [PATCH 04/23] libcamera: properties: Add rotation property
jacopo mondi
jacopo at jmondi.org
Wed Feb 5 11:51:05 CET 2020
Hi Laurent,
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.
> > + 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'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."
> > +
> > + 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
>
> 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 ..."
>
> > + 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
> > + 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.
Thanks
j
> > ...
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list