[libcamera-devel] [PATCH v6] libcamera: properties: Define pixel array properties
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 31 13:45:34 CEST 2020
On Fri, Jul 31, 2020 at 01:54:46PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jul 31, 2020 at 09:46:48AM +0200, Jacopo Mondi wrote:
> > On Fri, Jul 31, 2020 at 05:21:25AM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch (and sorry for such a late reply)
> > >
> > > First of all, I really like this, I think it's really good work.
> > > Describing the pixel array size may seem like an easy task, but I know
> > > how much variation we can see in camera sensors, and how much research
> > > you've but into this. Coming up with the properties below that are both
> > > flexible and clearly defined was not a simple task.
> > >
> > > I have quite a few comments (you know me...), but nothing that requires
> > > significant changes. Since you've posted this series David has submitted
> > > a proposal for transformation (including rotation) support, which needs
> > > to be taken into account as Sakari pointed out. I've proposed in the
> > > review below a way to do so (don't worry, it's not a complete rewrite
> > > :-)), please feel free to tell me if you disagree with some (or all) of
> > > the proposals.
> > >
> > > On Mon, Jun 08, 2020 at 12:55:09PM +0200, Jacopo Mondi wrote:
> > >> On Mon, Jun 08, 2020 at 01:26:04PM +0300, Sakari Ailus wrote:
> > >>> On Thu, Jun 04, 2020 at 05:31:22PM +0200, Jacopo Mondi wrote:
> > >>>> Add definition of pixel array related properties.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >>>> ---
> > >>>> src/libcamera/property_ids.yaml | 263 ++++++++++++++++++++++++++++++++
> > >>>> 1 file changed, 263 insertions(+)
> > >>>>
> > >>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > >>>> index ce627fa042ba..762d60881568 100644
> > >>>> --- a/src/libcamera/property_ids.yaml
> > >>>> +++ b/src/libcamera/property_ids.yaml
> > >>>> @@ -386,4 +386,267 @@ controls:
> > >>>> | |
> > >>>> | |
> > >>>> +--------------------+
> > >>>> +
> > >>>> + - UnitCellSize:
> > >>>> + type: Size
> > >>>> + description: |
> > >>>> + The pixel unit cell physical size, in nanometers.
> > >>>> +
> > >>>> + The UnitCellSize properties defines the horizontal and vertical sizes
> > >>>> + of a single pixel unit, including its active and non-active parts.
> > >
> > > Would it be useful to add "In other words, it expresses the horizontal
> > > and vertical distance between the top-left corners of adjacent pixels."
> > > ?
> >
> > It won't hurt, I can add it...
> >
> > >>>> +
> > >>>> + The property can be used to calculate the physical size of the sensor's
> > >>>> + pixel array area and for calibration purposes.
> > >>>
> > >>> Do we need this? Could it not be calculated from PixelArrayPhysicalSize and
> > >>> PixelArraySize?
> > >>
> > >> Not really, as PixelArrayPhysicalSize reports the physical dimension
> > >> of the full pixel array (readable and non readable pixels) while
> > >> PixelArraySize reports the size in pixel of the largest readable
> > >> image. To sum it up: PixelArrayPhysicalSize reports the chip area,
> > >> which covers more space that the readable PixelArraySize.
> > >>
> > >>>> +
> > >>>> + - PixelArrayPhysicalSize:
> > >>>> + type: Size
> > >>>> + description: |
> > >>>> + The camera sensor full pixel array size, in nanometers.
> > >>>> +
> > >>>> + The PixelArrayPhysicalSize property reports the physical dimensions
> > >>>> + (width x height) of the full pixel array matrix, including readable
> > >>>> + and non-readable pixels.
> > >>>> +
> > >>>> + \todo Rename this property to PhysicalSize once we will have property
> > >>>> + categories (i.e. Properties::PixelArray::PhysicalSize)
> > >
> > > I understand Sakari's confusion regarding the UnitCellSize calculation
> > > from PixelArrayPhysicalSize and PixelArraySize. I also agree with your
> > > answer, but I think we could actually drop the PixelArrayPhysicalSize
> > > property. I don't see what it could be used for, it describes the
> > > physical size of something that can't be accessed at all from the host,
> > > and should thus not matter at all.
> > >
> > > If we later find a need for this, we can always at it. At that point, I
> > > also think it would be best to express the property in pixel units
> > > (although depending on the use case that drives addition of this
> > > property, I could very well be wrong).
> >
> > Ack, I'll drop
> >
> > >>>> +
> > >>>> + - PixelArraySize:
> > >>>> + type: Size
> > >>>> + description: |
> > >>>> + The camera sensor pixel array readable area vertical and horizontal
> > >>>> + sizes, in pixels.
> > >>>> +
> > >>>> + The PixelArraySize property defines the size in pixel units of the
> > >>>> + readable part of full pixel array matrix, including optically black
> > >
> > > s/optically/optical/
> > >
> > > I've also come across the term "optical dark pixels" which I think is a
> > > bit more accurate, as the pixels are covered and thus dark, but not
> > > fully black due to leakages currents (otherwise there would be no need
> > > to black level correction). "optical black" is used much more often
> > > though, so I think we should keep that term.
> > >
> > >>>> + pixels used for calibration, pixels which are not considered valid for
> > >>>> + capture and active pixels valid for image capture.
> > >
> > > Maybe "and active pixels containing valid image data" ?
> >
> > I can change this
> >
> > >>>> +
> > >>>> + The property describes a rectangle whose top-left corner is placed
> > >>>> + in position (0, 0) and whose vertical and horizontal sizes are defined
> > >>>> + by the Size element transported by the property.
> > >
> > > I would move this paragraph below to [1].
> > >
> > >>>> +
> > >>>> + The property describes the maximum size of the raw data produced by
> > >>>> + the sensor, which might not correspond to the physical size of the
> > >
> > > Maybe s/produced by the sensor/captured by the camera/, as the CSI-2
> > > receiver could drop data too ? For instance, the invalid lines between
> > > the optical black and valid regions could be sent by the sensor, with a
> > > specific data type, and dropped on the receiving side by a decision of
> > > the pipeline handler that chooses to configure the CSI-2 receiver to
> > > drop the corresponding data type.
> >
> > Ok, we're really picking details here, but yes, I can change this. However I
> > wonder if that should not actually describe what the sensor puts on
> > the bus. How the receiver filters it, it's a separate issue.
>
> I've been pondering about that, and concluded that, from an application
> point of view (as properties are designed to be consumed by
> applications), what matters is what is being captured. Even if we
> disregard the CSI-2 receiver configuration, it's conceivable that the
> sensor would output invalid lines with a data type that the CSI-2
> receiver isn't able to capture due to the hardware being hardcoded to
> skip some data types.
>
> I agree it won't make much difference in practice, application
> developers will likely not think about all this.
>
> > >>>> + sensor pixel array matrix, as some portions of the physical pixel
> > >>>> + array matrix are not accessible and cannot be transmitted out.
> > >>>> +
> > >>>> + For example, a pixel array matrix assembled as follow
> > >
> > > s/a pixel array/let's consider a pixel array/
> > > s/follow/follows/
> > >
> > >>>> +
> > >>>> + +--------------------------------------------------+
> > >>>> + |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
> > >>>> + |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + ... ... ... ... ...
> > >>>> +
> > >>>> + ... ... ... ... ...
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDxx|
> > >>>> + |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
> > >>>> + |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
> > >>>> + +--------------------------------------------------+
> > >>>> +
> > >>>> + composed by two lines of non-readable pixels (x) followed by N lines of
> > >
> > > s/composed by/starting with/
> > > s/(x)/(x),/
> > >
> > >>>> + readable data (D) surrounded by two columns of non-readable pixels on
> > >>>> + each side, only the readable portion is transmitted to the receiving
> > >
> > > s/each side,/each side, and ending with two more lines of non-readable pixels./
> > > s/only/Only/
> > >
> > >>>> + side, defining the sizes of the largest possible buffer of raw data
> > >
> > > s/sizes/size/
> > >
> > >>>> + that can be presented to applications.
> > >>>> +
> > >>>> + PixelArraySize[0]
> > >
> > > should this be PixelArraySize.width ? Same for height instead of [1],
> > > and in other locations below.
> >
> > ack
> >
> > >>>> + /----------------------------------------------/
> > >>>> + +----------------------------------------------+ /
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| | PixelArraySize[1]
> > >>>> + ... ... ... ... ...
> > >>>> + ... ... ... ... ...
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + |DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD| |
> > >>>> + +----------------------------------------------+ /
> > >>>> +
> > >
> > > [1], with a small modification:
> > >
> > > "This defines a rectangle ... sizes are defined by this property."
> > >
> > > and merge it with the following paragraph in a single paragraph.
> > >
> > >>>> + All other rectangles that describes portions of the pixel array, such as
> > >
> > > s/describes/describe/
> > >
> > >>>> + the optical black pixels rectangles and active pixel areas are defined
> > >
> > > s/areas/areas,/
> > >
> > >>>> + relatively to the rectangle described by this property.
> > >>>> +
> > >>>> + \todo Rename this property to Size once we will have property
> > >>>> + categories (i.e. Properties::PixelArray::Size)
> > >>>> +
> > >>>> + - PixelArrayOpticalBlackRectangles:
> > >>>> + type: Rectangle
> > >>>> + size: [1 x n]
> > >
> > > This can just be [n], not all sizes need to be bi-dimensional.
> > >
> > >>>> + description: |
> > >>>> + The raw data buffer regions which contains optical black pixels
> > >
> > > s/raw data buffer/pixel array/ for the reason explained below in [2]
> > > s/regions/region(s)/ ?
> > > s/contains/contain/
> > >
> > >>>> + considered valid for calibration purposes.
> > >>>> +
> > >>>
> > >>> How does this interact with the rotation property?
> > >>>
> > >>> If the sensor is rotated 180°, V4L2 sub-device sensor drivers generally
> > >>> invert the flipping controls. I presume the same would apply to e.g. dark
> > >>> pixels in case they are read out, but that should be something for a driver
> > >>> to handle.
> > >>
> > >> Right. I think this also depends how black pixels are read out. I here
> > >> assumed the sensor is capable of reading out the whole PixelArraySize area,
> > >> and the receiver is able to capture its whole content in a single
> > >> buffer, where application can then go and pick the areas of interest
> > >> using the information conveyed by this property. If this model holds,
> > >> then if sensor flipping is enabled, indeed the here reported
> > >> information are mis-leading, unless the chip architecture is
> > >> perfectly symmetric in vertical and horizontal dimensions (which seems
> > >> unlikely).
> > >>
> > >>> But if the frame layout isn't conveyed to the user space by the driver,
> > >>> then we need another way to convey the sensor is actually rotated. Oh well.
> > >>
> > >> Not sure how to interpret this part. Do you mean "convey that a
> > >> rotation is applied by, ie, setting the canonical V\HFLIP controls,
> > >> which cause the sensor pixel array to be transmitted out with its
> > >> vertical/horizontal read-out directions inverted ?"
> > >>
> > >> We currently have a read-only property that reports the mounting
> > >> rotation (like the dt-property you have just reviewed :) I assume we
> > >> will have a rotation control that reports instead if a V/HFLIP is
> > >> applied, so that application know how to compensate that.
> > >>
> > >>> Not every sensor that is mounted upside down has flipping controls so the
> > >>> user space will still somehow need to manage the rotation in that case.
> > >>
> > >> I think it should, and I think we'll provide all information to be
> > >> able to do so.
> > >
> > > I think all the properties here should be expressed relative to the
> > > default sensor readout direction. Any transformation (such as h/v flip)
> > > will need to be taken into account by the application if the use cases
> > > require it. That would be the easiest course of action, otherwise these
> > > properties couldn't be static, as they would depend on the selected
> > > transformation.
> > >
> > > David has submitted a patch series with a Transform control (actually
> > > specified in CameraConfiguration, not as a control). We will need to
> > > document how the properties in this patch, the Rotation property, and
> > > the Transform control, interact.
> > >
> > > How about adding in the PixelArraySize documentation a paragraph that
> > > states
> > >
> > > "All the coordinates are expressed relative to the default sensor
> > > readout direction, without any transformation (such as horizontal and
> > > vertical flipping) applied. When mapping them to the raw pixel buffer,
> > > applications shall take any configured transformation into account."
> > >
> > > ?
> >
> > That's the easieast way forward, sounds good to me
> >
> > >
> > >>>> + The PixelArrayOpticalBlackRectangles describes (possibly multiple)
> > >
> > > s/describes/property describes/ (it feels weird to have a plural name
> > > with a singular verb).
> > >
> > >>>> + rectangular areas of the raw data buffer, where optical black pixels are
> > >
> > > s/buffer,/buffer/
> > >
> > >>>> + located and could be accessed for calibration and black level
> > >>>> + correction.
> > >
> > > Maybe just "... of the raw data buffers that contain optical black
> > > pixels." ? That duplicates the previous paragraph a bit, maybe we could
> > > just drop it.
> > >
> > >>>> +
> > >>>> + This property describes the position and size of optically black pixel
> > >
> > > s/optically/optical/
> > >
> > >>>> + rectangles relatively to their position in the raw data buffer as stored
> > >
> > > s/rectangles/regions/
> > > s/relatively to their position in the raw data buffer/in the raw data buffer/
> > >
> > >>>> + in memory, which might differ from their actual physical location in the
> > >>>> + pixel array matrix.
> > >>>> +
> > >>>> + It is important to note, in facts, that camera sensor might
> > >
> > > s/facts/fact/
> > > s/sensors/sensor/
> > >
> > >>>> + automatically re-order, shuffle or skip portions of their pixels array
> > >
> > > s/re-order/reorder/
> > >
> > > I'd drop shuffle, I think "reorder or skip" is enough, hopefully the
> > > sensors won't shuffle pixels randomly :-)
> > >
> > >>>> + matrix when transmitting data to the receiver.
> > >
> > > Should we add an example ?
> > >
> > > "For instance, a sensor may merge the top and bottom optical black
> > > rectangles into a single rectangle, transmitted at the beginning of the
> > > frame."
> > >
> > >>>> +
> > >>>> + The pixel array contains several areas with different purposes,
> > >>>> + interleaved by lines and columns which are said not to be valid for
> > >>>> + capturing purposes. Invalid lines and columns are defined as invalid as
> > >>>> + they could be positioned too close to the chip margins or to the optical
> > >>>> + blank shielding placed on top of optical black pixels.
> > >
> > > s/blank/black/
> > >
> > >>>> +
> > >>>> + PixelArraySize[0]
> > >>>> + /----------------------------------------------/
> > >>>> + x1 x2
> > >>>> + +--o---------------------------------------o---+ /
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + y1 oIIOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOII| |
> > >>>> + |IIOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOII| |
> > >>>> + |IIOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOII| |
> > >>>> + y2 oIIOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOII| |
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + y3 |IIOOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOOII| |
> > >>>> + |IIOOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOOII| | PixelArraySize[1]
> > >>>> + |IIOOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOOII| |
> > >>>> + ... ... ... ... ...
> > >>>> + ... ... ... ... ...
> > >>>> + y4 |IIOOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOOII| |
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> > >>>> + +----------------------------------------------+ /
> > >>>> +
> > >>>> + The readable pixel array matrix is composed by
> > >>>> + 2 invalid lines (I)
> > >>>> + 4 lines of valid optically black pixels (O)
> > >
> > > s/optically/optical/
> > >
> > >>>> + 2 invalid lines (I)
> > >>>> + n lines of valid pixel data (P)
> > >>>> + 2 invalid lines (I)
> > >>>> +
> > >>>> + And the position of the optical black pixel rectangles is defined by
> > >>>> +
> > >>>> + PixelArrayOpticalBlackRectangles = {
> > >>>> + { x1, y1, x2 - x1 + 1, y2 - y1 + },
> > >>>
> > >>> s/\+ \K}/1 }/
> > >
> > > :perldo s/\+ \K}/1 }/
> > >
> > > ;-)
> > >
> > >>>> + { x1, y3, 2, y4 - y3 + 1 },
> > >>>> + { x2, y3, 2, y4 - y3 + 1 },
> > >>>> + };
> > >>>> +
> > >>>> + If the sensor, when required to output the full pixel array matrix,
> > >>>> + automatically skip the invalid lines and columns, producing the
> > >
> > > s/skip/skips/
> > >
> > >>>> + following data buffer, when captured to memory
> > >
> > > For the same reason as above, this could become
> > >
> > > If the camera, when capturing the full pixel array matrix, automatically skips the invalid lines and columns, producing the following data buffer, when captured to memory
> > >
> > >>>> +
> > >>>> + PixelArraySize[0]
> > >>>> + /----------------------------------------------/
> > >>>> + x1
> > >>>> + +--------------------------------------------o-+ /
> > >>>> + |OOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO| |
> > >>>> + |OOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO| |
> > >>>> + |OOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO| |
> > >>>> + |OOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO| |
> > >>>> + y1 oOOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOO| |
> > >>>> + |OOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOO| |
> > >>>> + |OOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOO| | PixelArraySize[1]
> > >>>> + ... ... ... ... ... |
> > >>>> + ... ... ... ... ... |
> > >>>> + |OOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOO| |
> > >>>> + |OOPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPOO| |
> > >>>> + +----------------------------------------------+ /
> > >>>> +
> > >>>> + The invalid lines and columns should not be reported as part of the
> > >
> > > s/The/then the/
> > >
> > >>>> + PixelArraySize property in first place.
> > >>>> +
> > >>>> + In this case, the position of the black pixel rectangles will be
> > >>>> +
> > >>>> + PixelArrayOpticalBlackRectangles = {
> > >>>> + { 0, 0, y1 + 1, PixelArraySize[0] },
> > >>>> + { 0, y1, 2, PixelArraySize[1] - y1 + 1 },
> > >>>> + { x1, y1, 2, PixelArraySize[1] - y1 + 1 },
> > >>>> + };
> > >
> > > I think part of this should be moved to the PixelArraySize property, as
> > > it defines what the PixelArraySize property should report. Let's merge
> > > this patch first though, and improvements can go on top (unless you
> > > would prefer addressing this first :-)).
> > >
> > >>>> +
> > >>>> + \todo Rename this property to Size once we will have property
> > >>>> + categories (i.e. Properties::PixelArray::OpticalBlackRectangles)
> > >>>> +
> > >>>> + - PixelArrayActiveAreas:
> > >>>> + type: Rectangle
> > >>>> + size: [1 x n]
> > >
> > > [n] here too.
> > >
> > >>>> + description: |
> > >>>> + The PixelArrayActiveAreas property defines the (possibly multiple and
> > >>>> + overlapping) portions of the camera sensor readable pixel matrix
> > >>>> + which are considered valid for image acquisition purposes.
> > >>>> +
> > >>>> + Each rectangle is defined relatively to the PixelArraySize rectangle,
> > >>>> + with its top-left corner defined by its horizontal and vertical
> > >>>> + distances from the PixelArraySize rectangle top-left corner, placed in
> > >>>> + position (0, 0).
> > >
> > > I think you can drop the last three lines, they're a bit confusing.
> > >
> > >>>> +
> > >>>> + This property describes an arbitrary number of overlapping rectangles,
> > >>>> + with each rectangle representing the maximum image size that the camera
> > >>>> + sensor can produce for a particular aspect ratio.
> > >
> > > Maybe even moving the previous paragraph here as "They are defined
> > > relatively to the PixelArraySize rectangle." ?
> > >
> >
> > Ack, I'll merge the two
> >
> > >>>> +
> > >>>> + When multiple rectangles are reported, they shall be ordered from the
> > >>>> + tallest to the shortest.
> > >>>> +
> > >>>> + Example 1
> > >>>> + A camera sensor which only produces images in the 4:3 image resolution
> > >>>> + will report a single PixelArrayActiveAreas 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.
> > >>>> +
> > >>>> + PixelArraySize[0]
> > >>>> + /----------------/
> > >>>> + x1 x2
> > >>>> + (0,0)-> +-o------------o-+ /
> > >>>> + y1 o +------------+ | |
> > >>>> + | |////////////| | |
> > >>>> + | |////////////| | | PixelArraySize[1]
> > >>>> + | |////////////| | |
> > >>>> + y2 o +------------+ | |
> > >>>> + +----------------+ /
> > >>>> +
> > >>>> + The property reports a single rectangle
> > >>>> +
> > >>>> + PixelArrayActiveAreas = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)
> > >>>> +
> > >>>> + Example 2
> > >>>> + A camera sensor which can produce images in different native
> > >>>> + resolutions will report several overlapping rectangles, one for each
> > >>>> + natively supported resolution.
> > >>>> +
> > >>>> + PixelArraySize[0]
> > >>>> + /------------------/
> > >>>> + x1 x2 x3 x4
> > >>>> + (0,0)-> +o---o------o---o+ /
> > >>>> + y1 o +------+ | |
> > >>>> + | |//////| | |
> > >>>> + y2 o+---+------+---+| |
> > >>>> + ||///|//////|///|| | PixelArraySize[1]
> > >>>> + y3 o+---+------+---+| |
> > >>>> + | |//////| | |
> > >>>> + y4 o +------+ | |
> > >>>> + +----+------+----+ /
> > >>>> +
> > >>>> + The property reports two rectangles
> > >>>> +
> > >>>> + PixelArrayActiveAreas = ((x2, y1, x3 - x2 + 1, y4 - y1 + 1),
> > >>>> + (x1, y2, x4 - x1 + 1, y3 - y2 + 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.
> > >
> > > Should we add one paragraph here as a clarification:
> > >
> > > Multiple rectangles shall only be reported when the sensor can't capture
> > > the pixels in the corner regions. If all the pixels in the (x1,y1) -
> > > (x4,y4) area can be captured, the PixelArrayActiveAreas property shall
> > > contains the single rectangle (x1,y1) - (x4,y4).
> >
> > Isn't this the example above ? Re-stating the same here seems
> > confusing.
>
> What I meant to say is that a camera should not implement example 2 just
> because the pipeline handler developer wants to expose different aspect
> ratios, when the hardware actually implements example 1. For instance,
> considering a sensor with a native resolution of 3840x2880 (4/3) that
> can be captured fully, a 16/9 picture can be captured by cropping to
> 3840*2160. In that case PixelArrayActiveAreas should only report
> (0,0)/3840x2880, not both (0,0)/3840x2880 and (0,360)/3840x2880.
>
> But maybe I'm just worrying too much and nobody will think about doing
> that :-)
I ended up taking this part in in v7, as after reading it again I got
the same concern.
Thanks
j
>
> > > Or do you think it's overkill ?
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Thanks, I'll take comments in and resend a v7
> >
> > >>>> +
> > >>>> + \todo Rename this property to ActiveAreas once we will have property
> > >>>> + categories (i.e. Properties::PixelArray::ActiveAreas)
> > >>>> +
> > >>>> ...
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list