| Message ID | 20201106154947.261132-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-11-06 16:49:47 +0100, Jacopo Mondi 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. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 4690346e05cb..8a71d15be8ca 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. */ > { > @@ -724,17 +725,32 @@ 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); > - > - int32_t sensorSizes[] = { > - 0, 0, 2560, 1920, > - }; > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, > - &sensorSizes, 4); > + { Why this extra code block level? > + 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()); > + } > + } > + { > + 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 Niklas, On Mon, Nov 09, 2020 at 01:44:54PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-11-06 16:49:47 +0100, Jacopo Mondi 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. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 4690346e05cb..8a71d15be8ca 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. */ > > { > > @@ -724,17 +725,32 @@ 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); > > - > > - int32_t sensorSizes[] = { > > - 0, 0, 2560, 1920, > > - }; > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, > > - &sensorSizes, 4); > > + { > > Why this extra code block level? > I wrapped handling of each control in a separate block for two reasons (one of which is totally personal taste) 1) I re-use the same names for throwaway variables like 'size', 'data' and 'infoMap' that are only required for the conversion between libcamera controls and android metadata format. I really don't want to have 'dataPixelArraySize' or 'infoMapLensShading' for variables with such limited usage 2) I like separating each control translation in its own block, but that's merely personal taste. Thanks j > > + 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()); > > + } > > + } > > + { > > + 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 > > -- > Regards, > Niklas Söderlund
Hi Jacopo, Thank you for the patch. On Mon, Nov 09, 2020 at 02:04:08PM +0100, Jacopo Mondi wrote: > On Mon, Nov 09, 2020 at 01:44:54PM +0100, Niklas Söderlund wrote: > > On 2020-11-06 16:49:47 +0100, Jacopo Mondi 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. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 4690346e05cb..8a71d15be8ca 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. */ > > > { > > > @@ -724,17 +725,32 @@ 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); > > > - > > > - int32_t sensorSizes[] = { > > > - 0, 0, 2560, 1920, > > > - }; > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, > > > - &sensorSizes, 4); > > > + { > > > > Why this extra code block level? > > > > I wrapped handling of each control in a separate block for two > reasons (one of which is totally personal taste) > 1) I re-use the same names for throwaway variables like 'size', 'data' and > 'infoMap' that are only required for the conversion between libcamera > controls and android metadata format. I really don't want to have > 'dataPixelArraySize' or 'infoMapLensShading' for variables with > such limited usage But the variables are defined within the if () { ... } scope, so there should be no issue. As there's already a specific scope, I'd tend to drop the extra level. In the other locations where no specific scope exist, it makes sense to create one. Nice to see two hardcoded metadata going away :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 2) I like separating each control translation in its own block, but > that's merely personal taste. > > > > + 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()); > > > + } > > > + } > > > + { > > > + 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 Tue, Dec 01, 2020 at 09:04:48PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Nov 09, 2020 at 02:04:08PM +0100, Jacopo Mondi wrote: > > On Mon, Nov 09, 2020 at 01:44:54PM +0100, Niklas Söderlund wrote: > > > On 2020-11-06 16:49:47 +0100, Jacopo Mondi 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. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 4690346e05cb..8a71d15be8ca 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. */ > > > > { > > > > @@ -724,17 +725,32 @@ 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); > > > > - > > > > - int32_t sensorSizes[] = { > > > > - 0, 0, 2560, 1920, > > > > - }; > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, > > > > - &sensorSizes, 4); > > > > + { > > > > > > Why this extra code block level? > > > > > > > I wrapped handling of each control in a separate block for two > > reasons (one of which is totally personal taste) > > 1) I re-use the same names for throwaway variables like 'size', 'data' and > > 'infoMap' that are only required for the conversion between libcamera > > controls and android metadata format. I really don't want to have > > 'dataPixelArraySize' or 'infoMapLensShading' for variables with > > such limited usage > > But the variables are defined within the if () { ... } scope, so there > should be no issue. As there's already a specific scope, I'd tend to > drop the extra level. In the other locations where no specific scope > exist, it makes sense to create one. Ack > > Nice to see two hardcoded metadata going away :-) > If a pipeline does not report these information, Android will not receive it and will probably fail at all validation/testing. I think this is expected, or should we try to default to something ? > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thanks j > > 2) I like separating each control translation in its own block, but > > that's merely personal taste. > > > > > > + 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()); > > > > + } > > > > + } > > > > + { > > > > + 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
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 4690346e05cb..8a71d15be8ca 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. */ { @@ -724,17 +725,32 @@ 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); - - int32_t sensorSizes[] = { - 0, 0, 2560, 1920, - }; - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, - &sensorSizes, 4); + { + 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()); + } + } + { + 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,
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 11 deletions(-)