[libcamera-devel] [PATCH v3 02/13] libcamera: properties: Define pixel array properties

[Jacopo Mondi]

Hi again,

On Tue, May 19, 2020 at 05:50:07PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>    I just re-started looking into this
>
> On Sat, Apr 25, 2020 at 07:50:11PM +0300, Laurent Pinchart wrote:
> > 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 ?
> >
>
> I had a look at a few manuals of recent-ish sensors and the their
> (active or full pixel array size) range from ~2 to ~6 mm, with a
> precision of usually 3 more digits.
>
> Considering the single pixel size will shrink, even if resolution
> would likely go up, we might go below 1mm, and anyway, it's easier to
> express 1234 um than 1,234 mm
>
> > > >
> > > >       # \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, ... ?
>
> A few manuals and products briefs from OV report the chip size (I
> assume, at least, as it varies depending on the package). But as you
> noticed below, I presume the size is mostly used for optical
> caluculations, so the chip area size is less relevant indeed.
>
> >
> > 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.
> >
>
> I have checked the same for the ov13858, and it reports
> the physical size of the 'active area'.
>
> ov5670 instead reports as phyisical size an area larger than the
> active one.
>
> I fear there is no standard but I agree with linking the physical area
> size to the active pixel area size, unfortunaly defining the 'active
> area' itself might not be trivial
>

Now that I re-read that, I wonder: do we need to expose the already
calculated physical size ? V4L2 already offers a control to retrieve
the unit cell area, we expose the rectangle(s) in unit cells for each
area, application can do the calculation by themselves, if they want
to, right ?

In this way, my proposal now would be:
1) Expose the unit cell size (there's a V4L2 control already)
2) Expose three rectangles (in pixel units)
  - Total number of pixels
    Includes optically blank pixels, exposed but not valid, and valid
    pixels
  - Effective pixels
    Total number of exposed but not valid pixels, includes all the
    possible 'valid' rectangles
  - Valid(/active) pixels
    The rectangle(s) of exposed and valid pixels, contained in the
    effective pixels rectangles, which represents the maximum image
    size for an image resolution.

Application can calculate the phyisical area of interestes with these
properties.

The Android HAL will do the calculations to report what android needs.

The only drawback I see is that we require one more information from
the sensor driver, the UNIT_CELL_SIZE control.

Ideally, if we could match the above three rectangles with the V4L2
selection targets it would be lovely. I fear that part is
under-specified, and for the moment we would like to keep an array of
per-sensor static information to report those values.

Ack to explore this direction ?

Thanks
  j