[libcamera-devel] [PATCH v3 02/13] libcamera: properties: Define pixel array properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 25 18:50:11 CEST 2020
Hi Jacopo,
On Sat, Apr 25, 2020 at 03:58:45PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:
> > 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.
BTW, is mm precise enough, or should we use µm ?
> >
> > # \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.
I think linking the two are important, but we need to figure out what
the link is. Otherwise, does the physical size represent the size of the
chip (quite unlikelu), the size of the full pixel array, the size of the
optical window, ... ?
I'm looking at the IMX219 datasheet, and it reports
- Image size: diagonal 4.60mm
- Total number of pixels: 3296x2512
- Number of effective pixels: 3296x2480
- Number of active pixels: 3280x2464
- Unit cell size: 1.12µm x 1.12µm
Calculating the diagonal leads to 4641µm, 4620µm and 4595µm for the
three rectangles respectively. The last one match 4.60mm.
With the above definition of the physical size, the diagonal would be
4641µm (for a size of 3692µm x 2813µm). I wonder if we should report the
active image size instead (3674µm x 2760µm for a diagonal of 4595µm).
It's not difficult to convert between the two (assuming all the pixels
have the same size), but I'd like to pick the one that makes the most
sense. We can of course reconsider that decision later if we find out we
made a mistake.
My reasoning for picking the size of the active area is that the
physical size will most likely be used to make optics calculation, and
the lens should be selected and positioned according to the active area.
However, there could be systems with a different lens positioning, and
I'm not sure yet what that would imply.
Android seems to report the physical size of the full pixel array,
including inactive pixels. I'm not sure what the rationale is, and maybe
the difference between the two is lost in the measurement error noise in
the end, so I wouldn't rule out that they may not have been picked the
best option (but I also don't imply I know better :-)).
As a conclusion, let's pick one option (physical size covering the
active area or the full array), and document it.
> 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.
I think it would be better to refer to the pixel array size property in
the definition of ActiveAreas below, not the other way around, otherwise
you'll have a cycle :-)
The sensors I've seen typically have the following structure, from top
to bottom:
- A few dummy lines, for unknown purpose
- Optical black lines, split in three areas (invalid, valid, invalid)
- Exposed lines, split in three areas (invalid, valid, invalid)
Exposed lines are sometimes called effective, and "active" pixels can
refer to either all the exposed lines or to the valid exposed lines,
depending on datasheets.
The invalid lines are considered not to be usable, usually because
they're too close to the edges and thus can contain artifacts. Some of
them may be used for internal purpose. The dummy lines, optical black
lines (both invalid and valid) and invalid exposed lines may or may not
be readable, depending on the sensor.
The question now is what else than the valid exposed pixels need to be
reported in this property. I think we need to look at it from the point
of view of what the sensor can readout (assuming we map the physical
size to the valid active pixels), otherwise I don't really see the point
in giving information about something that would have absolutely no
influence on the receiver side.
> >> +
> >> + - 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.
My point was that we could define the rectangle through the coordinates
of the top-left and bottom-right pixels (x1, y1, x2, y2), or through the
coordinates of the top-left pixel and its size (x1, y1, x2 - x1 + 1, y2
- y1 + 1). I don't have a preference.
> >> +
> >> + 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
> >
>
> 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