[libcamera-devel] [PATCH v2 4/4] android: camera_device: Initialize pixel array properties

[Laurent Pinchart]

Hi Jacopo,

On Thu, Dec 10, 2020 at 05:39:28PM +0100, Jacopo Mondi wrote:
> On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote:
> > On Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:
> > > 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:
> > > > > 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.
> > > > >
> > > >
> > > > +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.
> >
> > To avoid blocking this series, how about merging it with the above
> > workaround (with a \todo comment), which we'll remove as soon as the
> > sensor patches can get integrated ?
> 
> The series is merged. It's libcamera master that's now broken on
> soraka. Should we temporary address this by defaulting
> ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE (and ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE
> which the framework requires as well) as Hiro proposed ?
> 
> I have a patch I'm carrying in my local tree, I can easily send it
> out.

Oops, sorry, I should have looked at the latest code. Yes, I think a
temporary fix would be good until the patches get merged in mainline.
Backporting to the Soraka kernel should then hopefully be fast.

> > > What is the procedure to have them integrated on your side ?
> > >
> > > > > > 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,

-- 
Regards,

Laurent Pinchart