[libcamera-devel] [PATCH v3 02/13] libcamera: properties: Define pixel array properties
Jacopo Mondi
jacopo at jmondi.org
Sat Apr 25 15:58:45 CEST 2020
Hi Laurent,
thanks for review
On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:
> 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 ?
I like PhysicalSize.
>
> # \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 ?
Let's 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
I would just drop rectangle maybe, as that property transports a widht
and an height.
>
> The PixelArraySize property defines the size of the full pixel
> array, covered by the PixelArrayPhysicalSize area. This may
> include inactive or optical black pixels.
>
I'll take this in, but I'm not sure if it's a good idea to link this
one to the PhysicalArraySize, as they are expressed with two different
measure units. So I would keep at least the first line of the existing
description, and s/may include/includes/ in your proposed change, as
this is not optional, the property should report inactive pixels too.
> > +
> > + - 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.
ack, I though I had it in previous versions..
>
> > +
> > + 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)
this is not correct, as y2 is the vertical distance from pixel (0,0)
while the active area vertical size is (y2 - y1 +1). Same for the
horizontal size.
>
> > +
> > + 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.
I would keep using 'camera sensor' whenever possible.
>
> > + 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.
>
Yes!
> > + +----+------+----+ /
> > +
> > + The property reports two rectangles
> > +
> > + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),
>
> s/y4 - 1/y4 - y1/
ups, yes
>
> > + (x1, y2, (x4 - x1), (y3 - y2))
>
> And comment as above regarding the width.
>
I'll add '- 1'
> > +
> > + 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 ?
>
Probably yes.
> > +
> > + 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 ?
>
Ack
> > +
> > + 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.
Ack
>
> > +
> > + 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 ?
>
We could, yes
> > +
> > + - 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.
>
Ack
> > +
> > + - 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 ?
Yes, this is barely copied without going into much detail. Let's leave
it out for now.
>
> Overall, good work ! No major issue, so the next version should be the
> last one.
Thanks and thanks for your comments.
>
> > +
> > ...
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list