[libcamera-devel] [PATCH v3 02/13] libcamera: properties: Define pixel array properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 25 04:05:11 CEST 2020
Hi Jacopo,
Thank you for the patch.
On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:
> Add definition of pixel array related properties.
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++
> 1 file changed, 155 insertions(+)
>
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index ce627fa042ba..8f6797723a9d 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -386,4 +386,159 @@ controls:
> | |
> | |
> +--------------------+
> +
> + - PixelArraySize:
> + type: float
> + size: [2]
> + description: |
> + The physical sizes of the pixel array (width and height), in
> + millimeters.
Once we'll have categories for properties I think this should become
PhysicalSize in the sensor category. We could already rename it now to
PhysicalSize or PixelArrayPhysicalSize if you think it would be a good
idea. I don't mind much either way, but if we keep the current name, how
about adding a comment to remember this ?
# \todo Rename this property to PhysicalSize once we will have property
# categories
> +
> + - PixelArray:
I was going to say we should rename this to PixelArraySize as it's the
size of the pixel array, and then realized that the previous property
has the same name :-) Maybe we should rename both ?
> + type: int32_t
> + size: [2]
> + description: |
> + The camera sensor pixel array vertical and horizontal sizes, in pixels.
> +
> + The property describes a rectangle with its top-left corner in position
> + (0, 0) and width and height described by the first and second values
> + of this property.
> +
> + The PixelArray property defines the rectangle that includes all possible
> + rectangles defined by the ActiveAreas property, and describes the full
> + pixel array, including non-active pixels, black level calibration
> + pixels etc.
It's not entirely clear to me what pixels should be included in this
rectangle. I'm not blaming your documentation here, but rather my lack
of knowledge of what else than non-active pixels and black level
calibration pixels there are :-) Just an idea, instead of linking this
property to ActiveAreas, would it make sense to link it to
PixelArraySize ? Maybe something along the lines of
The PixelArraySize property defines the size of the full pixel
array, covered by the PixelArrayPhysicalSize area. This may
include inactive or optical black pixels.
> +
> + - ActiveAreas:
> + type: int32_t
> + size: [4 x n]
> + description: |
> + The camera sensor active pixel area rectangles, represented as
> + rectangles contained in the one described by the PixelArray property.
> +
> + This property describes an arbitrary number of overlapping rectangles,
> + representing the active pixel portions of the camera sensor pixel array.
> +
> + Each rectangle is defined by its displacement from pixel (0, 0) of
> + the rectangle described by the PixelArray property, a width and an
> + height.
s/an height/a height/
> +
> + Each rectangle described by this property represents the maximum image
> + size that the camera module can produce for a given image resolution.
How about s/for a given image resolution/for a particular aspect ratio/
?
I would also add, taken from below,
When multiple rectangles are reported, they shall be ordered
from the tallest to the shortest.
> +
> + Example 1.
> + A sensor which only produces images in the 4:3 image resolution will
> + report a single ActiveArea rectangle, from which all other image formats
> + are obtained by either cropping the field-of-view and/or applying pixel
> + sub-sampling techniques such as pixel skipping or binning.
> +
> + PixelArray(0)
> + /-----------------/
> + x1 x2
> + (0,0)-> +-o------------o-+ /
> + y1 o +------------+ | |
> + | |////////////| | |
> + | |////////////| | | PixelArray(1)
> + | |////////////| | |
> + y2 o +------------+ | |
> + +----------------+ /
> +
> + The property reports a single rectangle
> +
> + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))
If the rectangle is defined through width and height, this should be
ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)
Alternatively you could use coordinates:
ActiveArea = (x1, y1, x2, y2)
> +
> + Example 2
> + A camera sensor which can produce images in different native
s/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.
> + resolutions, will report several overlapping rectangle, one for each
s/resolutions,/resolutions/
s/rectangle/rectangles/
> + natively supported resolution, ordered from the tallest to the shortest
> + one.
> +
> + PixelArray(0)
> + /-----------------/
> + x1 x2 x3 x4
> + (0,0)-> +o---o------o---o+ /
> + y1 | +------+ | |
> + | |//////| | |
> + y2 o+---+------+---+| |
> + ||///|//////|///|| | PixelArray(1)
> + y3 o+---+------+---+| |
> + | |//////| | |
> + y4 | +------+ | |
I think you need a o next to y1 and y4.
> + +----+------+----+ /
> +
> + The property reports two rectangles
> +
> + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),
s/y4 - 1/y4 - y1/
> + (x1, y2, (x4 - x1), (y3 - y2))
And comment as above regarding the width.
> +
> + The first rectangle describes the maximum field-of-view of all image
> + formats in the 4:3 resolutions, while the second one describes the
> + maximum field of view for all image formats in the 16:9 resolutions.
> +
> + - BayerFilterArrangement:
> + type: int32_t
> + description: |
> + The pixel array color filter displacement.
I could be wrong, but is "displacement" the right term ? Maybe
arrangement ?
> +
> + This property describes the arrangement and readout sequence of the
> + three RGB color components of the sensor's Bayer Color Filter Array
> + (CFA).
As we could support sensors with non-Bayer CFAs, how about already
renaming the control to ColorFilterArrangement, and remove Bayer from
here ?
> +
> + Color filters are usually displaced in line-alternating fashion on the
s/displaced/arranged/ ?
> + sensor pixel array. In example, one line might be composed of Red-Green
s/In example/For example/
> + while the successive is composed of Blue-Green color information.
> +
> + The value of this property represents the arrangement of color filters
> + in the top-left 2x2 pixel square.
As this is only valid for Bayer, maybe
For Bayer filters, the value of this property represents the arrangement
of color filters in the top-left 2x2 pixel square.
> +
> + For example, for a sensor with the following color filter displacement
s/displacement/arrangement/
(you could also use pattern instead of arrangement here or in other
places)
> +
> + (0, 0) (max-col)
> + +---+ +--------------...---+
> + |B|G|<---|B|G|B|G|B|G|B|...B|G|
> + |G|R|<---|G|R|G|R|G|R|G|...G|R|
> + +---+ |B|G|B|G|B|G|B|...B|G|
> + ... ..
> + ... ..
> + |G|R|G|R|G|R|G|...G|R|
> + |B|G|B|G|B|G|B|...B|G| (max-lines)
> + +--------------...---+
> +
> + The filter arrangement is represented by the BGGR value, which
> + correspond to the pixel readout sequence in line interleaved mode.
> +
> + enum:
> + - name: BayerFilterRGGB
> + value: 0
> + description: |
> + Color filter array displacement is Red-Green/Green-Blue
> +
> + - name: BayerFilterGRBG
> + value: 1
> + description: |
> + Color filter array displacement is Green-Red/Blue-Green
> +
> + - name: BayerFilterGBRG
> + value: 2
> + description: |
> + Color filter array displacement is Green-Blue/Red-Green
> +
> + - name: BayerFilterBGGR
> + value: 3
> + description: |
> + Color filter array displacement is Blue-Green/Green-Red
Should these be sorted alphabetically ?
> +
> + - name: BayerFilterNonStandard
> + value: 4
> + description: |
> + The pixel array color filter does not use the standard Bayer RGB
> + color model
I would drop this value for now, as we should instead report which
filter arrangement is used. We could already add known patterns, such as
the ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,
but I think that's overkill, we can extend it later when/if needed.
> +
> + - ISOSensitivityRange:
> + type: int32_t
> + size: [2]
> + description: |
> + The range of supported ISO sensitivities, as documented by the
> + ISO 12232:2006 (or later) standard.
Is this the ISO speed or the Standard Output Sensitivity (SOS) ? I think
this property needs a bit more research, should we leave it out for now
to avoid blocking the rest ?
Overall, good work ! No major issue, so the next version should be the
last one.
> +
> ...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list