[libcamera-devel] [PATCH v2 4/4] android: camera_device: Initialize pixel array properties
Jacopo Mondi
jacopo at jmondi.org
Thu Dec 10 11:29:59 CET 2020
Hi Tomasz,
On Thu, Dec 10, 2020 at 01:10:19PM +0900, Tomasz Figa wrote:
> Hi Jacopo,
>
> On Thu, Dec 10, 2020 at 1:17 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hello,
> >
> > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:
> > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > Hi Hiro,
> > > >
> > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:
> > > > > Camera service on chromeos fails starting from this patch.
> > > >
> > > > I wonder why I haven't noticed. I probable have run cros_camera_test
> > > > only :/
> > > >
> > > > > Looks like our camera HAL implementation requires the entry of
> > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.
> > > > > Shall we add else-statement to the if-statement?
> > > > > That is,
> > > > >
> > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > > > > };
> > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > > > data.data(), data.size());
> > > > > + } else {
> > > > > + int32_t sensorSizes[] = {
> > > > > + 0, 0, 2560, 1920,
> > > > > + };
> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > > > + &sensorSizes, 4);
> > > > > +
> > > > > }
> > > > >
> > > > > With this change, camera on chromeos can start.
> > > > > What do you think?
> > > >
> > > > That's the million dollar question. Should the HAL try to default to
> > > > some arbitrary values properties which are not provided by the sensor
> > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically
> > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).
> > > >
> > > > If you ask me, the drivers should be fixed. It's a trivial operation
> > > > and makes sure we don't report arbitrary values to the Android
> > > > framework.
>
> Doesn't the active array size in V4L2 correspond to the resolution as
> set by S_FMT? Right now we don't have any provisions to support
Not necessarly. The sensor driver might expose modes might be smaller
than the active array size, or modes that overlaps to support
different frame resolutions (ie 4:3 = [x, y]; 16:9 = [x1, y1] and
(x > x1; y1 > x).
It's very much up to sensor driver and the active array size is a
sensor property that should not depend on the current implementation.
The discussion about how V4L2 selection targets should be used/updated
to more clearly expose these information is quest I've attempted to
start a few months ago then dropped the ball as it seems there's no
clear consensus on the direction.
Right now (I think) it's safer to rely on drivers reporting these
information through the existing selection targets with the
association:
NATIVE = physical pixel matrix size
BOUNDS = readable area of the physical pixel array size (black and
valid pixels)
DEFAULT = valid and readable area of the physical pixel matrix
> optically black pixels or other inactive pixels and it was always
> assumed that all the pixels in the mode are active.
>
> > > >
> > >
> > > +Hanlin Chen +Tomasz Figa
> > > Tomasz, can you take a look the driver implementation on soraka-libcamera?
> >
> > The patches are trivial. They're available here
> > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection
> > (based on the most recent media-master)
> >
> > I've not been able to test them (Tomasz has details) otherwise I would
> > have sent them upstream.
> >
> > What is the procedure to have them integrated on your side ?
>
> The patch needs to be submitted upstream and ideally merged to the
> maintainer tree, so that we can cherry pick directly from upstream. In
> justified cases we can also pick a work in progress patch from the
> mailing list, but we try to avoid it as much as possible.
>
> There is some more information on our kernel development process in
> https://chromium.googlesource.com/chromiumos/docs/+/master/kernel_development.md#upstream_backport_fromlist_and-you
> .
>
> >
> > Thanks
> > j
> >
> >
> > >
> > > Regards,
> > > -Hiro
> > >
> > > > Thanks
> > > > j
> > > >
> > > > >
> > > > > Best Regards,
> > > > > -Hiro
> > > > >
> > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > >
> > > > > > Initialize pixel array properties in the Android camera HAL
> > > > > > inspecting the camera properties.
> > > > > >
> > > > > > If the camera does not provide any suitable property, not static
> > > > > > metadata is registered to the Android framework.
> > > > > >
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > ---
> > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------
> > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > index 675af5705055..017a15cac284 100644
> > > > > > --- a/src/android/camera_device.cpp
> > > > > > +++ b/src/android/camera_device.cpp
> > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > > > > > }
> > > > > >
> > > > > > const ControlInfoMap &controlsInfo = camera_->controls();
> > > > > > + const ControlList &properties = camera_->properties();
> > > > > >
> > > > > > /* Color correction static metadata. */
> > > > > > {
> > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
> > > > > >
> > > > > > /* Sensor static metadata. */
> > > > > > - int32_t pixelArraySize[] = {
> > > > > > - 2592, 1944,
> > > > > > - };
> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > > > > > - &pixelArraySize, 2);
> > > > > > + if (properties.contains(properties::PixelArraySize)) {
> > > > > > + const Size &size =
> > > > > > + properties.get<Size>(properties::PixelArraySize);
> > > > > > + std::vector<int32_t> data{
> > > > > > + static_cast<int32_t>(size.width),
> > > > > > + static_cast<int32_t>(size.height),
> > > > > > + };
> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > > > > > + data.data(), data.size());
> > > > > > + }
> > > > > >
> > > > > > - int32_t sensorSizes[] = {
> > > > > > - 0, 0, 2560, 1920,
> > > > > > - };
> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > > > > - &sensorSizes, 4);
> > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {
> > > > > > + const Span<const Rectangle> &rects =
> > > > > > + properties.get<Span<const Rectangle>>(properties::PixelArrayActiveAreas);
> > > > > > + std::vector<int32_t> data{
> > > > > > + static_cast<int32_t>(rects[0].x),
> > > > > > + static_cast<int32_t>(rects[0].y),
> > > > > > + static_cast<int32_t>(rects[0].width),
> > > > > > + static_cast<int32_t>(rects[0].height),
> > > > > > + };
> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > > > > + data.data(), data.size());
> > > > > > + }
> > > > > >
> > > > > > int32_t sensitivityRange[] = {
> > > > > > 32, 2400,
> > > > > > --
> > > > > > 2.29.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > libcamera-devel mailing list
> > > > > > libcamera-devel at lists.libcamera.org
> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list