| Message ID | 20260731143741.689779-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > CameraSensorLegacy supports flipping on the sensor side only if the > driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a > driver only supports a single flip mode, flipping support gets disabled > completely. This is unexpected and annoying when validating sensor > drivers. What about `CameraSensorRaw`? > > Fix that by handling vertical and horizontal flips separately. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > Hi all, > > I had this patch somewhere in my tree for a long time. Now I stumbled > over this issue again and had to manually rebase the changes. So maybe > it is time to upstream :-) > > Best regards, > Stefan > > src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > 1 file changed, 52 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..0f8e3c8cf251 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -133,7 +133,8 @@ private: > Size pixelArraySize_; > Rectangle activeArea_; > const BayerFormat *bayerFormat_; > - bool supportFlips_; > + bool supportHFlips_; > + bool supportVFlips_; > bool flipsAlterBayerOrder_; > Orientation mountingOrientation_; > > @@ -153,7 +154,7 @@ private: > > CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > - bayerFormat_(nullptr), supportFlips_(false), > + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > flipsAlterBayerOrder_(false), properties_(properties::properties) > { > } > @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > * \todo Handle horizontal and vertical flips independently. > */ > const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > - supportFlips_ = true; > + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + supportHFlips_ = true; > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > flipsAlterBayerOrder_ = true; > + } else { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support horizontal flip"; > } > > - if (!supportFlips_) > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + supportVFlips_ = true; > + > + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > + flipsAlterBayerOrder_ = true; > + } else { > LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > + << "Camera sensor does not support vertical flip"; > + } > + > + if (supportHFlips_ && supportVFlips_ && > + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > + LOG(CameraSensor, Error) > + << "Flips have different alter bayer order properties." > + << " Disable flipping altogether"; This is theoretically possible, right? Should it be a warning with an "unsupported" somewhere in the message? > + supportHFlips_ = false; > + supportVFlips_ = false; > + flipsAlterBayerOrder_ = false; > + } > > /* > * Make sure the required selection targets are supported. > @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > + if (supportHFlips_) { > ControlList flipCtrls(subdev_->controls()); > > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + int ret = subdev_->setControls(&flipCtrls); > + if (ret) > + return ret; > + } > + > + if (supportVFlips_) { > + ControlList flipCtrls(subdev_->controls()); > + > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > > Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > { > - /* > - * If we cannot do any flips we cannot change the native camera mounting > - * orientation. > - */ > - if (!supportFlips_) { > - *orientation = mountingOrientation_; > - return Transform::Identity; > - } > - > /* > * Now compute the required transform to obtain 'orientation' starting > * from the mounting rotation. > @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > */ > Transform transform = *orientation / mountingOrientation_; > > + /* > + * If we cannot do the required flips, fall back to identity. > + */ > + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { I would write if (!supportHFlips_ && (transform & Transform::HFlip)) > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { Similarly here. > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > /* > * If transform contains any Transpose we cannot do it, so adjust > * 'orientation' to report the image native orientation and return Identity.
On Fri, Jul 31, 2026 at 05:07:27PM +0200, Barnabás Pőcze wrote: > 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > > CameraSensorLegacy supports flipping on the sensor side only if the > > driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a > > driver only supports a single flip mode, flipping support gets disabled > > completely. This is unexpected and annoying when validating sensor > > drivers. > > What about `CameraSensorRaw`? > > > Fix that by handling vertical and horizontal flips separately. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Hi all, > > > > I had this patch somewhere in my tree for a long time. Now I stumbled > > over this issue again and had to manually rebase the changes. So maybe > > it is time to upstream :-) > > > > Best regards, > > Stefan > > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > > 1 file changed, 52 insertions(+), 20 deletions(-) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 6a683821f219..0f8e3c8cf251 100644 > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > @@ -133,7 +133,8 @@ private: > > Size pixelArraySize_; > > Rectangle activeArea_; > > const BayerFormat *bayerFormat_; > > - bool supportFlips_; > > + bool supportHFlips_; > > + bool supportVFlips_; > > bool flipsAlterBayerOrder_; > > Orientation mountingOrientation_; > > > > @@ -153,7 +154,7 @@ private: > > > > CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > > : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > > - bayerFormat_(nullptr), supportFlips_(false), > > + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > > flipsAlterBayerOrder_(false), properties_(properties::properties) > > { > > } > > @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > > * \todo Handle horizontal and vertical flips independently. This needs an update. > > */ > > const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > > - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > > - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > > - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > - supportFlips_ = true; > > + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportHFlips_ = true; > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > flipsAlterBayerOrder_ = true; > > + } else { > > + LOG(CameraSensor, Debug) > > + << "Camera sensor does not support horizontal flip"; > > } > > > > - if (!supportFlips_) > > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > > + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportVFlips_ = true; > > + > > + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + flipsAlterBayerOrder_ = true; > > + } else { > > LOG(CameraSensor, Debug) > > - << "Camera sensor does not support horizontal/vertical flip"; > > + << "Camera sensor does not support vertical flip"; > > + } > > + This essentially repeats the same code twice. A small helper function could help std::tuple<bool, bool> CameraSensorLegacy::queryFlip(uint32_t id, const char *name) { const struct v4l2_query_ext_ctrl *info = subdev_->controlInfo(id); if (!info || info->flags & V4L2_CTRL_FLAG_READ_ONLY)) { LOG(CameraSensor, Debug) << "Camera sensor does not support " << name << " flip"; return { false, false }; } return { true, info->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; } which can be used with something like auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); auto vflip = queryFlip(V4L2_CID_VFLIP, "vertical"); if (hflip.get<1> == vflip.get<1>) { supportHFlips_= hflip.get<0>; supportVFlips_= vflip.get<0>; flipsAlterBayerOrder_ = hflip.get<1>; } else { LOG(CameraSensor, Error) << "Flips have different alter bayer order properties." << " Disable flipping altogether"; } A small structure to avoid the get<0> and get<1> may be better. > > + if (supportHFlips_ && supportVFlips_ && > > + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > > + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > > + LOG(CameraSensor, Error) > > + << "Flips have different alter bayer order properties." > > + << " Disable flipping altogether"; > > This is theoretically possible, right? Should it be a warning with > an "unsupported" somewhere in the message? > > > + supportHFlips_ = false; > > + supportVFlips_ = false; > > + flipsAlterBayerOrder_ = false; > > + } > > > > /* > > * Make sure the required selection targets are supported. > > @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > { > > /* Configure flips if the sensor supports that. */ > > - if (supportFlips_) { > > + if (supportHFlips_) { > > ControlList flipCtrls(subdev_->controls()); > > > > flipCtrls.set(V4L2_CID_HFLIP, > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > + > > + int ret = subdev_->setControls(&flipCtrls); > > + if (ret) > > + return ret; > > + } > > + > > + if (supportVFlips_) { > > + ControlList flipCtrls(subdev_->controls()); > > + > > flipCtrls.set(V4L2_CID_VFLIP, > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > > > > Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > > { > > - /* > > - * If we cannot do any flips we cannot change the native camera mounting > > - * orientation. > > - */ > > - if (!supportFlips_) { > > - *orientation = mountingOrientation_; > > - return Transform::Identity; > > - } > > - > > /* > > * Now compute the required transform to obtain 'orientation' starting s/Now compute/Compute/ > > * from the mounting rotation. > > @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > > */ > > Transform transform = *orientation / mountingOrientation_; > > > > + /* > > + * If we cannot do the required flips, fall back to identity. > > + */ > > + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { > > I would write > > if (!supportHFlips_ && (transform & Transform::HFlip)) > > > + *orientation = mountingOrientation_; > > + return Transform::Identity; > > + } > > + > > + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { > > Similarly here. > > > + *orientation = mountingOrientation_; > > + return Transform::Identity; > > + } > > + > > /* > > * If transform contains any Transpose we cannot do it, so adjust > > * 'orientation' to report the image native orientation and return Identity.
Hi Barnabás, Quoting Barnabás Pőcze (2026-07-31 17:07:27) > Hi > > 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > > CameraSensorLegacy supports flipping on the sensor side only if the > > driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a > > driver only supports a single flip mode, flipping support gets disabled > > completely. This is unexpected and annoying when validating sensor > > drivers. > > What about `CameraSensorRaw`? I didn't implement it because I didn't need it and have no easy way to test it. But given that the code is the same, I'll try t add it. > > > > > > Fix that by handling vertical and horizontal flips separately. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Hi all, > > > > I had this patch somewhere in my tree for a long time. Now I stumbled > > over this issue again and had to manually rebase the changes. So maybe > > it is time to upstream :-) > > > > Best regards, > > Stefan > > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > > 1 file changed, 52 insertions(+), 20 deletions(-) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 6a683821f219..0f8e3c8cf251 100644 > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > @@ -133,7 +133,8 @@ private: > > Size pixelArraySize_; > > Rectangle activeArea_; > > const BayerFormat *bayerFormat_; > > - bool supportFlips_; > > + bool supportHFlips_; > > + bool supportVFlips_; > > bool flipsAlterBayerOrder_; > > Orientation mountingOrientation_; > > > > @@ -153,7 +154,7 @@ private: > > > > CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > > : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > > - bayerFormat_(nullptr), supportFlips_(false), > > + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > > flipsAlterBayerOrder_(false), properties_(properties::properties) > > { > > } > > @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > > * \todo Handle horizontal and vertical flips independently. > > */ > > const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > > - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > > - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > > - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > - supportFlips_ = true; > > + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportHFlips_ = true; > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > flipsAlterBayerOrder_ = true; > > + } else { > > + LOG(CameraSensor, Debug) > > + << "Camera sensor does not support horizontal flip"; > > } > > > > - if (!supportFlips_) > > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > > + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportVFlips_ = true; > > + > > + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + flipsAlterBayerOrder_ = true; > > + } else { > > LOG(CameraSensor, Debug) > > - << "Camera sensor does not support horizontal/vertical flip"; > > + << "Camera sensor does not support vertical flip"; > > + } > > + > > + if (supportHFlips_ && supportVFlips_ && > > + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > > + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > > + LOG(CameraSensor, Error) > > + << "Flips have different alter bayer order properties." > > + << " Disable flipping altogether"; > > This is theoretically possible, right? Should it be a warning with > an "unsupported" somewhere in the message? I can change that, but the message is quite verbose already. Would you prefer: Flips with differing alter bayer oder properties are unsupported. Disable flipping. > > > > + supportHFlips_ = false; > > + supportVFlips_ = false; > > + flipsAlterBayerOrder_ = false; > > + } > > > > /* > > * Make sure the required selection targets are supported. > > @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > { > > /* Configure flips if the sensor supports that. */ > > - if (supportFlips_) { > > + if (supportHFlips_) { > > ControlList flipCtrls(subdev_->controls()); > > > > flipCtrls.set(V4L2_CID_HFLIP, > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > + > > + int ret = subdev_->setControls(&flipCtrls); > > + if (ret) > > + return ret; > > + } > > + > > + if (supportVFlips_) { > > + ControlList flipCtrls(subdev_->controls()); > > + > > flipCtrls.set(V4L2_CID_VFLIP, > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > > > > Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > > { > > - /* > > - * If we cannot do any flips we cannot change the native camera mounting > > - * orientation. > > - */ > > - if (!supportFlips_) { > > - *orientation = mountingOrientation_; > > - return Transform::Identity; > > - } > > - > > /* > > * Now compute the required transform to obtain 'orientation' starting > > * from the mounting rotation. > > @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > > */ > > Transform transform = *orientation / mountingOrientation_; > > > > + /* > > + * If we cannot do the required flips, fall back to identity. > > + */ > > + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { > > I would write > > if (!supportHFlips_ && (transform & Transform::HFlip)) Yes that is intuitively what one would write. But the & operator on transform returns a Transform, which has no cast operator to bool. As Transform is an enum class we even can't add the bool() operator. The !! pattern is already used a few times in the file. I could write if (!supportHFlips_ && !!(transform & Transform::HFlip)) if you prefer. Best regards, Stefan > > > > + *orientation = mountingOrientation_; > > + return Transform::Identity; > > + } > > + > > + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { > > Similarly here. > > > > + *orientation = mountingOrientation_; > > + return Transform::Identity; > > + } > > + > > /* > > * If transform contains any Transpose we cannot do it, so adjust > > * 'orientation' to report the image native orientation and return Identity. >
2026. 08. 03. 11:03 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-07-31 17:07:27) >> Hi >> >> 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: >>> CameraSensorLegacy supports flipping on the sensor side only if the >>> driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a >>> driver only supports a single flip mode, flipping support gets disabled >>> completely. This is unexpected and annoying when validating sensor >>> drivers. >> >> What about `CameraSensorRaw`? > > I didn't implement it because I didn't need it and have no easy way to > test it. But given that the code is the same, I'll try t add it. I just wanted to know the reasoning, no need to add it, although if it's essentially the same change in both files, maybe there is no harm. It could make sense to add some private functions to `CameraSensor` to avoid the duplication. But I have to assume there is a reason why this wasn't done when the two classes were created. > >> >> >>> >>> Fix that by handling vertical and horizontal flips separately. >>> >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >>> --- >>> Hi all, >>> >>> I had this patch somewhere in my tree for a long time. Now I stumbled >>> over this issue again and had to manually rebase the changes. So maybe >>> it is time to upstream :-) >>> >>> Best regards, >>> Stefan >>> >>> src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ >>> 1 file changed, 52 insertions(+), 20 deletions(-) >>> >>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp >>> index 6a683821f219..0f8e3c8cf251 100644 >>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >>> @@ -133,7 +133,8 @@ private: >>> Size pixelArraySize_; >>> Rectangle activeArea_; >>> const BayerFormat *bayerFormat_; >>> - bool supportFlips_; >>> + bool supportHFlips_; >>> + bool supportVFlips_; >>> bool flipsAlterBayerOrder_; >>> Orientation mountingOrientation_; >>> >>> @@ -153,7 +154,7 @@ private: >>> >>> CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) >>> : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), >>> - bayerFormat_(nullptr), supportFlips_(false), >>> + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), >>> flipsAlterBayerOrder_(false), properties_(properties::properties) >>> { >>> } >>> @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() >>> * \todo Handle horizontal and vertical flips independently. >>> */ >>> const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); >>> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); >>> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && >>> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >>> - supportFlips_ = true; >>> + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >>> + supportHFlips_ = true; >>> >>> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || >>> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) >>> + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) >>> flipsAlterBayerOrder_ = true; >>> + } else { >>> + LOG(CameraSensor, Debug) >>> + << "Camera sensor does not support horizontal flip"; >>> } >>> >>> - if (!supportFlips_) >>> + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); >>> + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >>> + supportVFlips_ = true; >>> + >>> + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) >>> + flipsAlterBayerOrder_ = true; >>> + } else { >>> LOG(CameraSensor, Debug) >>> - << "Camera sensor does not support horizontal/vertical flip"; >>> + << "Camera sensor does not support vertical flip"; >>> + } >>> + >>> + if (supportHFlips_ && supportVFlips_ && >>> + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != >>> + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { >>> + LOG(CameraSensor, Error) >>> + << "Flips have different alter bayer order properties." >>> + << " Disable flipping altogether"; >> >> This is theoretically possible, right? Should it be a warning with >> an "unsupported" somewhere in the message? > > I can change that, but the message is quite verbose already. > > Would you prefer: > Flips with differing alter bayer oder properties are unsupported. > Disable flipping. I'd say "Disabling" so as not to imply that it is a command for the reader, but otherwise looks ok to me. > >> >> >>> + supportHFlips_ = false; >>> + supportVFlips_ = false; >>> + flipsAlterBayerOrder_ = false; >>> + } >>> >>> /* >>> * Make sure the required selection targets are supported. >>> @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, >>> int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) >>> { >>> /* Configure flips if the sensor supports that. */ >>> - if (supportFlips_) { >>> + if (supportHFlips_) { >>> ControlList flipCtrls(subdev_->controls()); >>> >>> flipCtrls.set(V4L2_CID_HFLIP, >>> static_cast<int32_t>(!!(transform & Transform::HFlip))); >>> + >>> + int ret = subdev_->setControls(&flipCtrls); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (supportVFlips_) { >>> + ControlList flipCtrls(subdev_->controls()); >>> + >>> flipCtrls.set(V4L2_CID_VFLIP, >>> static_cast<int32_t>(!!(transform & Transform::VFlip))); This has just occurred to me, but couldn't these be done with a single `setControls()` call? Something like this: if (supportHFlips_ || supportVFlips_) { ControlList ...; if (supportHFlips_) ... ... } >>> >>> @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const >>> >>> Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const >>> { >>> - /* >>> - * If we cannot do any flips we cannot change the native camera mounting >>> - * orientation. >>> - */ >>> - if (!supportFlips_) { >>> - *orientation = mountingOrientation_; >>> - return Transform::Identity; >>> - } >>> - >>> /* >>> * Now compute the required transform to obtain 'orientation' starting >>> * from the mounting rotation. >>> @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const >>> */ >>> Transform transform = *orientation / mountingOrientation_; >>> >>> + /* >>> + * If we cannot do the required flips, fall back to identity. >>> + */ >>> + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { >> >> I would write >> >> if (!supportHFlips_ && (transform & Transform::HFlip)) > > Yes that is intuitively what one would write. But the & operator on > transform returns a Transform, which has no cast operator to bool. Ahh of course, I should've checked... > > As Transform is an enum class we even can't add the bool() operator. > > The !! pattern is already used a few times in the file. > > I could write > if (!supportHFlips_ && !!(transform & Transform::HFlip)) > > if you prefer. I would prefer `&&` over `&`, yes. > > Best regards, > Stefan > >> >> >>> + *orientation = mountingOrientation_; >>> + return Transform::Identity; >>> + } >>> + >>> + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { >> >> Similarly here. >> >> >>> + *orientation = mountingOrientation_; >>> + return Transform::Identity; >>> + } >>> + >>> /* >>> * If transform contains any Transpose we cannot do it, so adjust >>> * 'orientation' to report the image native orientation and return Identity. >>
Quoting Barnabás Pőcze (2026-08-03 11:15:41) > 2026. 08. 03. 11:03 keltezéssel, Stefan Klug írta: > > Hi Barnabás, > > > > Quoting Barnabás Pőcze (2026-07-31 17:07:27) > >> Hi > >> > >> 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > >>> CameraSensorLegacy supports flipping on the sensor side only if the > >>> driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a > >>> driver only supports a single flip mode, flipping support gets disabled > >>> completely. This is unexpected and annoying when validating sensor > >>> drivers. > >> > >> What about `CameraSensorRaw`? > > > > I didn't implement it because I didn't need it and have no easy way to > > test it. But given that the code is the same, I'll try t add it. > > I just wanted to know the reasoning, no need to add it, although if > it's essentially the same change in both files, maybe there is no harm. > > It could make sense to add some private functions to `CameraSensor` to > avoid the duplication. But I have to assume there is a reason why this > wasn't done when the two classes were created. > > > > > >> > >> > >>> > >>> Fix that by handling vertical and horizontal flips separately. > >>> > >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > >>> --- > >>> Hi all, > >>> > >>> I had this patch somewhere in my tree for a long time. Now I stumbled > >>> over this issue again and had to manually rebase the changes. So maybe > >>> it is time to upstream :-) > >>> > >>> Best regards, > >>> Stefan > >>> > >>> src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > >>> 1 file changed, 52 insertions(+), 20 deletions(-) > >>> > >>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > >>> index 6a683821f219..0f8e3c8cf251 100644 > >>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > >>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > >>> @@ -133,7 +133,8 @@ private: > >>> Size pixelArraySize_; > >>> Rectangle activeArea_; > >>> const BayerFormat *bayerFormat_; > >>> - bool supportFlips_; > >>> + bool supportHFlips_; > >>> + bool supportVFlips_; > >>> bool flipsAlterBayerOrder_; > >>> Orientation mountingOrientation_; > >>> > >>> @@ -153,7 +154,7 @@ private: > >>> > >>> CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > >>> : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > >>> - bayerFormat_(nullptr), supportFlips_(false), > >>> + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > >>> flipsAlterBayerOrder_(false), properties_(properties::properties) > >>> { > >>> } > >>> @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > >>> * \todo Handle horizontal and vertical flips independently. > >>> */ > >>> const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > >>> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > >>> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > >>> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > >>> - supportFlips_ = true; > >>> + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > >>> + supportHFlips_ = true; > >>> > >>> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > >>> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > >>> + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > >>> flipsAlterBayerOrder_ = true; > >>> + } else { > >>> + LOG(CameraSensor, Debug) > >>> + << "Camera sensor does not support horizontal flip"; > >>> } > >>> > >>> - if (!supportFlips_) > >>> + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > >>> + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > >>> + supportVFlips_ = true; > >>> + > >>> + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > >>> + flipsAlterBayerOrder_ = true; > >>> + } else { > >>> LOG(CameraSensor, Debug) > >>> - << "Camera sensor does not support horizontal/vertical flip"; > >>> + << "Camera sensor does not support vertical flip"; > >>> + } > >>> + > >>> + if (supportHFlips_ && supportVFlips_ && > >>> + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > >>> + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > >>> + LOG(CameraSensor, Error) > >>> + << "Flips have different alter bayer order properties." > >>> + << " Disable flipping altogether"; > >> > >> This is theoretically possible, right? Should it be a warning with > >> an "unsupported" somewhere in the message? > > > > I can change that, but the message is quite verbose already. > > > > Would you prefer: > > Flips with differing alter bayer oder properties are unsupported. > > Disable flipping. > > I'd say "Disabling" so as not to imply that it is a command for the reader, > but otherwise looks ok to me. > Ack > > > > >> > >> > >>> + supportHFlips_ = false; > >>> + supportVFlips_ = false; > >>> + flipsAlterBayerOrder_ = false; > >>> + } > >>> > >>> /* > >>> * Make sure the required selection targets are supported. > >>> @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > >>> int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > >>> { > >>> /* Configure flips if the sensor supports that. */ > >>> - if (supportFlips_) { > >>> + if (supportHFlips_) { > >>> ControlList flipCtrls(subdev_->controls()); > >>> > >>> flipCtrls.set(V4L2_CID_HFLIP, > >>> static_cast<int32_t>(!!(transform & Transform::HFlip))); > >>> + > >>> + int ret = subdev_->setControls(&flipCtrls); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + > >>> + if (supportVFlips_) { > >>> + ControlList flipCtrls(subdev_->controls()); > >>> + > >>> flipCtrls.set(V4L2_CID_VFLIP, > >>> static_cast<int32_t>(!!(transform & Transform::VFlip))); > > This has just occurred to me, but couldn't these be done with a single `setControls()` call? > Something like this: > > if (supportHFlips_ || supportVFlips_) { > ControlList ...; > > if (supportHFlips_) > ... > > ... > } > While writing v2 I stumbled over that aswell... fixed in v2. Regards, Stefan > > >>> > >>> @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > >>> > >>> Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > >>> { > >>> - /* > >>> - * If we cannot do any flips we cannot change the native camera mounting > >>> - * orientation. > >>> - */ > >>> - if (!supportFlips_) { > >>> - *orientation = mountingOrientation_; > >>> - return Transform::Identity; > >>> - } > >>> - > >>> /* > >>> * Now compute the required transform to obtain 'orientation' starting > >>> * from the mounting rotation. > >>> @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > >>> */ > >>> Transform transform = *orientation / mountingOrientation_; > >>> > >>> + /* > >>> + * If we cannot do the required flips, fall back to identity. > >>> + */ > >>> + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { > >> > >> I would write > >> > >> if (!supportHFlips_ && (transform & Transform::HFlip)) > > > > Yes that is intuitively what one would write. But the & operator on > > transform returns a Transform, which has no cast operator to bool. > > Ahh of course, I should've checked... > > > > > > As Transform is an enum class we even can't add the bool() operator. > > > > The !! pattern is already used a few times in the file. > > > > I could write > > if (!supportHFlips_ && !!(transform & Transform::HFlip)) > > > > if you prefer. > > I would prefer `&&` over `&`, yes. > > > > > > Best regards, > > Stefan > > > >> > >> > >>> + *orientation = mountingOrientation_; > >>> + return Transform::Identity; > >>> + } > >>> + > >>> + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { > >> > >> Similarly here. > >> > >> > >>> + *orientation = mountingOrientation_; > >>> + return Transform::Identity; > >>> + } > >>> + > >>> /* > >>> * If transform contains any Transpose we cannot do it, so adjust > >>> * 'orientation' to report the image native orientation and return Identity. > >> >
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 6a683821f219..0f8e3c8cf251 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -133,7 +133,8 @@ private: Size pixelArraySize_; Rectangle activeArea_; const BayerFormat *bayerFormat_; - bool supportFlips_; + bool supportHFlips_; + bool supportVFlips_; bool flipsAlterBayerOrder_; Orientation mountingOrientation_; @@ -153,7 +154,7 @@ private: CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), - bayerFormat_(nullptr), supportFlips_(false), + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), flipsAlterBayerOrder_(false), properties_(properties::properties) { } @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() * \todo Handle horizontal and vertical flips independently. */ const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { - supportFlips_ = true; + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + supportHFlips_ = true; - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) flipsAlterBayerOrder_ = true; + } else { + LOG(CameraSensor, Debug) + << "Camera sensor does not support horizontal flip"; } - if (!supportFlips_) + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + supportVFlips_ = true; + + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) + flipsAlterBayerOrder_ = true; + } else { LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; + << "Camera sensor does not support vertical flip"; + } + + if (supportHFlips_ && supportVFlips_ && + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { + LOG(CameraSensor, Error) + << "Flips have different alter bayer order properties." + << " Disable flipping altogether"; + supportHFlips_ = false; + supportVFlips_ = false; + flipsAlterBayerOrder_ = false; + } /* * Make sure the required selection targets are supported. @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { + if (supportHFlips_) { ControlList flipCtrls(subdev_->controls()); flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + int ret = subdev_->setControls(&flipCtrls); + if (ret) + return ret; + } + + if (supportVFlips_) { + ControlList flipCtrls(subdev_->controls()); + flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); @@ -936,15 +964,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const { - /* - * If we cannot do any flips we cannot change the native camera mounting - * orientation. - */ - if (!supportFlips_) { - *orientation = mountingOrientation_; - return Transform::Identity; - } - /* * Now compute the required transform to obtain 'orientation' starting * from the mounting rotation. @@ -955,6 +974,19 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const */ Transform transform = *orientation / mountingOrientation_; + /* + * If we cannot do the required flips, fall back to identity. + */ + if (!supportHFlips_ & !!(transform & Transform::HFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + /* * If transform contains any Transpose we cannot do it, so adjust * 'orientation' to report the image native orientation and return Identity.
CameraSensorLegacy supports flipping on the sensor side only if the driver supports V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a driver only supports a single flip mode, flipping support gets disabled completely. This is unexpected and annoying when validating sensor drivers. Fix that by handling vertical and horizontal flips separately. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hi all, I had this patch somewhere in my tree for a long time. Now I stumbled over this issue again and had to manually rebase the changes. So maybe it is time to upstream :-) Best regards, Stefan src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ 1 file changed, 52 insertions(+), 20 deletions(-)