| Message ID | 20200424215304.558317-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + 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. > + */ > + 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: commit 08f1cb4e8477cbf1062dcc1d6bf99a7c72347e9b Author: Laurent Pinchart <laurent.pinchart@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@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 I think it would lead to cleaner code than storing rectangles and sizes in integer arrays. > return 0; > } >
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@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@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@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
Hi Jacopo, On Sat, Apr 25, 2020 at 03:47:10PM +0200, Jacopo Mondi wrote: > On Sat, Apr 25, 2020 at 03:54:02PM +0300, Laurent Pinchart wrote: > > 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@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 ? I think you're right, it's probably best to fail to ensure drivers are correctly implemented. > > > + 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. And the fields will also be easier to access, you won't have to convert back and forth between Size/Rectangle and integer arrays. I'll post the patch below, as well as another one for Rectangle controls. > > commit 08f1cb4e8477cbf1062dcc1d6bf99a7c72347e9b > > Author: Laurent Pinchart <laurent.pinchart@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@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 ? What about it ? :-) > > I think it would lead to cleaner code than storing rectangles and sizes > > in integer arrays. > > > > > return 0; > > > }
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; + 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 + */ + 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) }); return 0; }
Collect the sensor pixel array properties by retrieving the subdevice native size and active pixel array size. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)