| Message ID | 20201202135354.264212-5-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Camera service on chromeos fails starting from this patch.
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?
Best Regards,
-Hiro
On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
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. Thanks j > > Best Regards, > -Hiro > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi <jacopo@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. > +Hanlin Chen +Tomasz Figa Tomasz, can you take a look the driver implementation on soraka-libcamera? Regards, -Hiro > Thanks > j > > > > > Best Regards, > > -Hiro > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > > > > +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 ? Thanks j > > Regards, > -Hiro > > > Thanks > > j > > > > > > > > Best Regards, > > > -Hiro > > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, Dec 10, 2020 at 1:17 AM Jacopo Mondi <jacopo@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@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 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@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@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@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@ideasonboard.com> > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Dec 10, 2020 at 7:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > 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@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@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). Good point. > > 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 Makes sense. We should try to get this definition merged upstream. > > > 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@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@ideasonboard.com> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, 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@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 ? > What is the procedure to have them integrated on your side ? > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@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,
Hi Laurent, On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > 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@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. > > > What is the procedure to have them integrated on your side ? > > > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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
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@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@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@ideasonboard.com> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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,
Hello, On Thu, Dec 10, 2020 at 05:39:28PM +0100, Jacopo Mondi wrote: > Hi Laurent, > > On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > 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@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. Finally tested on Soraka, the patches have been sent to linux-media https://patchwork.linuxtv.org/project/linux-media/list/?series=4127 Should I merge the temporary workaround to give time to upstream to review and cros to integrate them in their downstream ? Thanks j > > > > > > What is the procedure to have them integrated on your side ? > > > > > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Fri, Dec 11, 2020 at 03:22:25PM +0100, Jacopo Mondi wrote: > 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@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. > > Finally tested on Soraka, the patches have been sent to linux-media > https://patchwork.linuxtv.org/project/linux-media/list/?series=4127 Thanks a million for your hard work there, I know what was involved in getting a recent enough kernel up and running on the device :-) > Should I merge the temporary workaround to give time to upstream to > review and cros to integrate them in their downstream ? Please do. > > > > What is the procedure to have them integrated on your side ? > > > > > > > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@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,
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,