[libcamera-devel] [PATCH v6] libcamera: properties: Define pixel array properties

Jacopo Mondi jacopo at jmondi.org
Fri Jul 31 09:46:48 CEST 2020


Hi Laurent,
   thanks for review

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.

> > > > +        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.

>
> 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