[libcamera-devel] [PATCH v3 06/13] libcamera: camera_sensor: Collect pixel array properties
Jacopo Mondi
jacopo at jmondi.org
Sat Apr 25 15:47:10 CEST 2020
On Sat, Apr 25, 2020 at 03:54:02PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Apr 24, 2020 at 11:52:57PM +0200, Jacopo Mondi wrote:
> > Collect the sensor pixel array properties by retrieving the subdevice
> > native size and active pixel array size.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/camera_sensor.cpp | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 8d7abc7147a7..a54751fecf5a 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -169,6 +169,29 @@ int CameraSensor::initProperties()
> > propertyValue = rotationControl->second.def().get<int32_t>();
> > properties_.set(properties::Rotation, propertyValue);
> >
> > + /*
> > + * Sensor pixel array properties. Conditionally register them if the
> > + * sub-device provides support for the selection API.
> > + */
> > + Size size{};
> > + int ret = subdev_->getNativeSize(0, &size);
> > + if (ret && ret != -ENOTTY)
> > + return ret;
>
> This answers my previous question :-)
>
> Should failures (other than -ENOTTY) be considered fatal, or should we
> continue with other properties ?
An failure != -ENOTTY mean something went wrong, possibly on the
kernel side, so I would rather fail here. The failure is loud in the
V4L2Subdevice class already. Do you think we should continue instead ?
>
> > + if (!ret)
> > + properties_.set(properties::PixelArray, { static_cast<int>(size.width),
> > + static_cast<int>(size.height) });
> > +
> > + /*
> > + * \todo The sub-device API only support a single active area rectangle
>
> I don't think it will ever support more, I think you can drop this
> comment.
>
ack, although the property defines more rectangles, and one could
wonder why we ignore that
> > + */
> > + Rectangle rect{};
> > + ret = subdev_->getActiveArea(0, &rect);
> > + if (ret && ret != -ENOTTY)
> > + return ret;
> > + if (!ret)
> > + properties_.set(properties::ActiveAreas, { rect.x, rect.y,
> > + static_cast<int>(rect.width),
> > + static_cast<int>(rect.height) });
>
> How about adding two control types for Size and Rectangle, and using
> them for these properties ? I wrote this some time ago as a test patch:
I would be glad to do this after the series went in, the gain is
minimal imho, the main advantage I see is that it won't be possible to
get the order of fields wrong.
>
> commit 08f1cb4e8477cbf1062dcc1d6bf99a7c72347e9b
> Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Date: Sat Feb 29 03:39:46 2020 +0200
>
> [DNI] How easy is it to add a Size control type ?
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 4b2e7e9cdd6c..89d5a6a72820 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -13,6 +13,7 @@
> #include <string>
> #include <unordered_map>
>
> +#include <libcamera/geometry.h>
> #include <libcamera/span.h>
>
> namespace libcamera {
> @@ -27,6 +28,7 @@ enum ControlType {
> ControlTypeInteger64,
> ControlTypeFloat,
> ControlTypeString,
> + ControlTypeSize,
> };
>
> namespace details {
> @@ -70,6 +72,11 @@ struct control_type<std::string> {
> static constexpr ControlType value = ControlTypeString;
> };
>
> +template<>
> +struct control_type<Size> {
> + static constexpr ControlType value = ControlTypeSize;
> +};
> +
> template<typename T, std::size_t N>
> struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> };
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 83555c021b6c..97ac0c7b3942 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -58,4 +58,14 @@ controls:
> - SensorModel:
> type: string
> description: The sensor model name
> +
> + - TheSize:
> + type: Size
> + description: A Size property
> +
> + - TheSizes:
> + type: Size
> + description: A Size array property
> + size: [n]
> +
> ...
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 540cc026417a..a1ec994900a5 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -58,6 +58,7 @@ static constexpr size_t ControlValueSize[] = {
> [ControlTypeInteger64] = sizeof(int64_t),
> [ControlTypeFloat] = sizeof(float),
> [ControlTypeString] = sizeof(char),
> + [ControlTypeSize] = sizeof(Size),
> };
>
> } /* namespace */
> @@ -242,6 +243,12 @@ std::string ControlValue::toString() const
> str += std::to_string(*value);
> break;
> }
> + case ControlTypeSize: {
> + const Size *value = reinterpret_cast<const Size *>(data);
> + str += std::to_string(value->width) + "x"
> + + std::to_string(value->height);
> + break;
> + }
> case ControlTypeNone:
> case ControlTypeString:
> break;
> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
> index e1d0055d139c..4885501bdcf3 100644
> --- a/test/serialization/control_serialization.cpp
> +++ b/test/serialization/control_serialization.cpp
> @@ -47,6 +47,8 @@ protected:
> list.set(controls::Saturation, 50);
> list.set(controls::BayerGains, { 1.0f });
> list.set(controls::SensorModel, "VIMC Sensor B");
> + list.set(controls::TheSize, Size{ 640, 480 });
> + list.set(controls::TheSizes, { Size{ 640, 480 }, Size{ 1280, 720 } });
>
> /*
> * Serialize the control list, this should fail as the control
What about control serialization ?
>
> I think it would lead to cleaner code than storing rectangles and sizes
> in integer arrays.
>
> > return 0;
> > }
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list