| Message ID | 20260803095252.862470-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Aug 03, 2026 at 11:52:47AM +0200, Stefan Klug wrote: > CameraSensorLegacy and CameraSensorRaw support flipping on the sensor > side only if the driver supports both, 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> > > --- > > Changes in v2: > - Replaced duplicate query flip code with lambda > - Reworked apply flip code to call setControls only once > - Replaced a & by && > - Applied the change to CameraSensorRaw as well > - Small comment/logging changes > --- > > 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 :-) > > Version 2 adresses the comments from review. > > Best regards, > Stefan > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++-------- > src/libcamera/sensor/camera_sensor_raw.cpp | 80 +++++++++++-------- > 2 files changed, 90 insertions(+), 67 deletions(-) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..5dce0a6eacfe 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) > { > } > @@ -359,26 +360,32 @@ int CameraSensorLegacy::validateSensorDriver() > } > } > > - /* > - * Verify if sensor supports horizontal/vertical flips > - * > - * \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; > + /* Verify if sensor supports horizontal/vertical flips. */ > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support " << name << " flip"; > + return { false, false }; > + } > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > - flipsAlterBayerOrder_ = true; > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > + }; > + > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > + > + if (!hflip.first || !vflip.first || > + (hflip.second == vflip.second)) { nit: fits on one line. The idea is to only set supportHFlips_, supportVFlips_ and flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is consistent between the two flips, provided both of them are available. Albeit equivalent, isn't it clear: if (hflip.first && vflip.first && (hflip.second != vflip.second)) { LOG(CameraSensor, Error) << "Flips with differing alter bayer order properties" << " are unsupported. Disabling flipping."; } else { supportHFlips_ = hflip.first; supportVFlips_ = vflip.first; flipsAlterBayerOrder_ = hflip.second || vflip.second; } > + supportHFlips_ = hflip.first; > + supportVFlips_ = vflip.first; > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; Regardless of the logic order of the check, this can be just an || > + } else { > + LOG(CameraSensor, Error) > + << "Flips with differing alter bayer order properties" > + << " are unsupported. Disabling flipping."; > } > > - if (!supportFlips_) > - LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > - > /* > * Make sure the required selection targets are supported. > * > @@ -759,14 +766,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > - ControlList flipCtrls(subdev_->controls()); > - > + ControlList flipCtrls(subdev_->controls()); > + if (supportHFlips_) > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + if (supportVFlips_) > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > + if (!flipCtrls.empty()) { > int ret = subdev_->setControls(&flipCtrls); > if (ret) > return ret; > @@ -937,17 +946,8 @@ 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. > + * Compute the required transform to obtain 'orientation' starting from > + * the mounting rotation. > * > * As a note: > * orientation / mountingOrientation_ = transform > @@ -955,6 +955,17 @@ 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; > + } > + Minor comment apart, we're introducing the requirement that both flips, if present, shall expose the same MODIFY_LAYOUT flag layout. I presume it's reasonable, but should we also add the requirement to Documentation/sensor_driver_requirements.rst which currently only reports: The controls must be writable from userspace. In case of a RAW Bayer sensors, drivers should correctly report if vertical/horizontal flips modify the Bayer pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. Thanks j > /* > * 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_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 10eba0331fe8..6efd72727773 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -144,7 +144,8 @@ private: > Size pixelArraySize_; > Rectangle activeArea_; > BayerFormat::Order cfaPattern_; > - bool supportFlips_; > + bool supportHFlips_; > + bool supportVFlips_; > bool flipsAlterBayerOrder_; > Orientation mountingOrientation_; > > @@ -162,8 +163,9 @@ private: > */ > > CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) > - : entity_(entity), staticProps_(nullptr), supportFlips_(false), > - flipsAlterBayerOrder_(false), properties_(properties::properties) > + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), > + supportVFlips_(false), flipsAlterBayerOrder_(false), > + properties_(properties::properties) > { > } > > @@ -479,26 +481,32 @@ std::optional<int> CameraSensorRaw::init() > return { ret }; > } > > - /* > - * Verify if sensor supports horizontal/vertical flips > - * > - * \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; > + /* Verify if sensor supports horizontal/vertical flips. */ > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support " << name << " flip"; > + return { false, false }; > + } > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > - flipsAlterBayerOrder_ = true; > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > + }; > + > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > + > + if (!hflip.first || !vflip.first || > + (hflip.second == vflip.second)) { > + supportHFlips_ = hflip.first; > + supportVFlips_ = vflip.first; > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; > + } else { > + LOG(CameraSensor, Error) > + << "Flips with differing alter bayer order properties" > + << " are unsupported. Disabling flipping."; > } > > - if (!supportFlips_) > - LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > - > /* > * 5. Discover ancillary devices. > * > @@ -819,14 +827,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > - ControlList flipCtrls(subdev_->controls()); > - > + ControlList flipCtrls(subdev_->controls()); > + if (supportHFlips_) > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + if (supportVFlips_) > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > + if (!flipCtrls.empty()) { > int ret = subdev_->setControls(&flipCtrls); > if (ret) > return ret; > @@ -1054,17 +1064,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const > Transform CameraSensorRaw::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. > + * Compute the required transform to obtain 'orientation' starting from > + * the mounting rotation. > * > * As a note: > * orientation / mountingOrientation_ = transform > @@ -1072,6 +1073,17 @@ Transform CameraSensorRaw::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. > -- > 2.53.0 >
Hi Jacopo, Thank you for the review. Quoting Jacopo Mondi (2026-08-03 12:28:14) > On Mon, Aug 03, 2026 at 11:52:47AM +0200, Stefan Klug wrote: > > CameraSensorLegacy and CameraSensorRaw support flipping on the sensor > > side only if the driver supports both, 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> > > > > --- > > > > Changes in v2: > > - Replaced duplicate query flip code with lambda > > - Reworked apply flip code to call setControls only once > > - Replaced a & by && > > - Applied the change to CameraSensorRaw as well > > - Small comment/logging changes > > --- > > > > 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 :-) > > > > Version 2 adresses the comments from review. > > > > Best regards, > > Stefan > > > > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++-------- > > src/libcamera/sensor/camera_sensor_raw.cpp | 80 +++++++++++-------- > > 2 files changed, 90 insertions(+), 67 deletions(-) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 6a683821f219..5dce0a6eacfe 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) > > { > > } > > @@ -359,26 +360,32 @@ int CameraSensorLegacy::validateSensorDriver() > > } > > } > > > > - /* > > - * Verify if sensor supports horizontal/vertical flips > > - * > > - * \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; > > + /* Verify if sensor supports horizontal/vertical flips. */ > > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > > + LOG(CameraSensor, Debug) > > + << "Camera sensor does not support " << name << " flip"; > > + return { false, false }; > > + } > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > - flipsAlterBayerOrder_ = true; > > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > > + }; > > + > > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > + > > + if (!hflip.first || !vflip.first || > > + (hflip.second == vflip.second)) { > > nit: fits on one line. > > The idea is to only set supportHFlips_, supportVFlips_ and > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is > consistent between the two flips, provided both of them are available. > > Albeit equivalent, isn't it clear: > > if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > LOG(CameraSensor, Error) > << "Flips with differing alter bayer order properties" > << " are unsupported. Disabling flipping."; > } else { > supportHFlips_ = hflip.first; > supportVFlips_ = vflip.first; > flipsAlterBayerOrder_ = hflip.second || vflip.second; > } I don't really care too much on the order. I think I just took it from Laurents proposal. I had the feeling there was a tendency to put the good case into the first branch. Your order is easier to read for me as well, so if there are no other arguments, I'll flip it. > > > + supportHFlips_ = hflip.first; > > + supportVFlips_ = vflip.first; > > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; > > Regardless of the logic order of the check, this can be just an || That's a good point, thanks. > > > + } else { > > + LOG(CameraSensor, Error) > > + << "Flips with differing alter bayer order properties" > > + << " are unsupported. Disabling flipping."; > > } > > > > - if (!supportFlips_) > > - LOG(CameraSensor, Debug) > > - << "Camera sensor does not support horizontal/vertical flip"; > > - > > /* > > * Make sure the required selection targets are supported. > > * > > @@ -759,14 +766,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > { > > /* Configure flips if the sensor supports that. */ > > - if (supportFlips_) { > > - ControlList flipCtrls(subdev_->controls()); > > - > > + ControlList flipCtrls(subdev_->controls()); > > + if (supportHFlips_) > > flipCtrls.set(V4L2_CID_HFLIP, > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > + > > + if (supportVFlips_) > > flipCtrls.set(V4L2_CID_VFLIP, > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > + if (!flipCtrls.empty()) { > > int ret = subdev_->setControls(&flipCtrls); > > if (ret) > > return ret; > > @@ -937,17 +946,8 @@ 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. > > + * Compute the required transform to obtain 'orientation' starting from > > + * the mounting rotation. > > * > > * As a note: > > * orientation / mountingOrientation_ = transform > > @@ -955,6 +955,17 @@ 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; > > + } > > + > > Minor comment apart, we're introducing the requirement that both flips, if > present, shall expose the same MODIFY_LAYOUT flag layout. > > I presume it's reasonable, but should we also add the requirement to > Documentation/sensor_driver_requirements.rst which currently only > reports: > > The controls must be writable from userspace. In case of a RAW Bayer sensors, > drivers should correctly report if vertical/horizontal flips modify the Bayer > pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. I'm not sure about that. I think that restriction only applies to libcamera (I just didn't implement support for that case). On the V4L2 side I don't see a need to make that restriction. And if we ever meet a sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add support for that in libcamera. Or did I get you wrong? Best regards, Stefan > > Thanks > j > > > /* > > * 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_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > > index 10eba0331fe8..6efd72727773 100644 > > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > > @@ -144,7 +144,8 @@ private: > > Size pixelArraySize_; > > Rectangle activeArea_; > > BayerFormat::Order cfaPattern_; > > - bool supportFlips_; > > + bool supportHFlips_; > > + bool supportVFlips_; > > bool flipsAlterBayerOrder_; > > Orientation mountingOrientation_; > > > > @@ -162,8 +163,9 @@ private: > > */ > > > > CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) > > - : entity_(entity), staticProps_(nullptr), supportFlips_(false), > > - flipsAlterBayerOrder_(false), properties_(properties::properties) > > + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), > > + supportVFlips_(false), flipsAlterBayerOrder_(false), > > + properties_(properties::properties) > > { > > } > > > > @@ -479,26 +481,32 @@ std::optional<int> CameraSensorRaw::init() > > return { ret }; > > } > > > > - /* > > - * Verify if sensor supports horizontal/vertical flips > > - * > > - * \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; > > + /* Verify if sensor supports horizontal/vertical flips. */ > > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > > + LOG(CameraSensor, Debug) > > + << "Camera sensor does not support " << name << " flip"; > > + return { false, false }; > > + } > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > - flipsAlterBayerOrder_ = true; > > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > > + }; > > + > > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > + > > + if (!hflip.first || !vflip.first || > > + (hflip.second == vflip.second)) { > > + supportHFlips_ = hflip.first; > > + supportVFlips_ = vflip.first; > > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; > > + } else { > > + LOG(CameraSensor, Error) > > + << "Flips with differing alter bayer order properties" > > + << " are unsupported. Disabling flipping."; > > } > > > > - if (!supportFlips_) > > - LOG(CameraSensor, Debug) > > - << "Camera sensor does not support horizontal/vertical flip"; > > - > > /* > > * 5. Discover ancillary devices. > > * > > @@ -819,14 +827,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, > > int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > { > > /* Configure flips if the sensor supports that. */ > > - if (supportFlips_) { > > - ControlList flipCtrls(subdev_->controls()); > > - > > + ControlList flipCtrls(subdev_->controls()); > > + if (supportHFlips_) > > flipCtrls.set(V4L2_CID_HFLIP, > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > + > > + if (supportVFlips_) > > flipCtrls.set(V4L2_CID_VFLIP, > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > + if (!flipCtrls.empty()) { > > int ret = subdev_->setControls(&flipCtrls); > > if (ret) > > return ret; > > @@ -1054,17 +1064,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const > > Transform CameraSensorRaw::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. > > + * Compute the required transform to obtain 'orientation' starting from > > + * the mounting rotation. > > * > > * As a note: > > * orientation / mountingOrientation_ = transform > > @@ -1072,6 +1073,17 @@ Transform CameraSensorRaw::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. > > -- > > 2.53.0 > >
Hi Stefan On Mon, Aug 03, 2026 at 01:03:35PM +0200, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the review. > > Quoting Jacopo Mondi (2026-08-03 12:28:14) > > On Mon, Aug 03, 2026 at 11:52:47AM +0200, Stefan Klug wrote: > > > CameraSensorLegacy and CameraSensorRaw support flipping on the sensor > > > side only if the driver supports both, 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> > > > > > > --- > > > > > > Changes in v2: > > > - Replaced duplicate query flip code with lambda > > > - Reworked apply flip code to call setControls only once > > > - Replaced a & by && > > > - Applied the change to CameraSensorRaw as well > > > - Small comment/logging changes > > > --- > > > > > > 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 :-) > > > > > > Version 2 adresses the comments from review. > > > > > > Best regards, > > > Stefan > > > > > > > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++-------- > > > src/libcamera/sensor/camera_sensor_raw.cpp | 80 +++++++++++-------- > > > 2 files changed, 90 insertions(+), 67 deletions(-) > > > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > > index 6a683821f219..5dce0a6eacfe 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) > > > { > > > } > > > @@ -359,26 +360,32 @@ int CameraSensorLegacy::validateSensorDriver() > > > } > > > } > > > > > > - /* > > > - * Verify if sensor supports horizontal/vertical flips > > > - * > > > - * \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; > > > + /* Verify if sensor supports horizontal/vertical flips. */ > > > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > > > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > > > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > > > + LOG(CameraSensor, Debug) > > > + << "Camera sensor does not support " << name << " flip"; > > > + return { false, false }; > > > + } > > > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > > - flipsAlterBayerOrder_ = true; > > > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > > > + }; > > > + > > > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > > > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > > + > > > + if (!hflip.first || !vflip.first || > > > + (hflip.second == vflip.second)) { > > > > nit: fits on one line. > > > > The idea is to only set supportHFlips_, supportVFlips_ and > > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is > > consistent between the two flips, provided both of them are available. > > > > Albeit equivalent, isn't it clear: > > > > if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > > LOG(CameraSensor, Error) > > << "Flips with differing alter bayer order properties" > > << " are unsupported. Disabling flipping."; > > } else { > > supportHFlips_ = hflip.first; > > supportVFlips_ = vflip.first; > > flipsAlterBayerOrder_ = hflip.second || vflip.second; > > } > > I don't really care too much on the order. I think I just took it from > Laurents proposal. I had the feeling there was a tendency to put the > good case into the first branch. Your order is easier to read for me as > well, so if there are no other arguments, I'll flip it. > Up to you, really. It's a minor > > > > > + supportHFlips_ = hflip.first; > > > + supportVFlips_ = vflip.first; > > > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; > > > > Regardless of the logic order of the check, this can be just an || > > That's a good point, thanks. > > > > > > + } else { > > > + LOG(CameraSensor, Error) > > > + << "Flips with differing alter bayer order properties" > > > + << " are unsupported. Disabling flipping."; > > > } > > > > > > - if (!supportFlips_) > > > - LOG(CameraSensor, Debug) > > > - << "Camera sensor does not support horizontal/vertical flip"; > > > - > > > /* > > > * Make sure the required selection targets are supported. > > > * > > > @@ -759,14 +766,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > > > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > > { > > > /* Configure flips if the sensor supports that. */ > > > - if (supportFlips_) { > > > - ControlList flipCtrls(subdev_->controls()); > > > - > > > + ControlList flipCtrls(subdev_->controls()); > > > + if (supportHFlips_) > > > flipCtrls.set(V4L2_CID_HFLIP, > > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > > + > > > + if (supportVFlips_) > > > flipCtrls.set(V4L2_CID_VFLIP, > > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > > > + if (!flipCtrls.empty()) { > > > int ret = subdev_->setControls(&flipCtrls); > > > if (ret) > > > return ret; > > > @@ -937,17 +946,8 @@ 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. > > > + * Compute the required transform to obtain 'orientation' starting from > > > + * the mounting rotation. > > > * > > > * As a note: > > > * orientation / mountingOrientation_ = transform > > > @@ -955,6 +955,17 @@ 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; > > > + } > > > + > > > > Minor comment apart, we're introducing the requirement that both flips, if > > present, shall expose the same MODIFY_LAYOUT flag layout. > > > > I presume it's reasonable, but should we also add the requirement to > > Documentation/sensor_driver_requirements.rst which currently only > > reports: > > > > The controls must be writable from userspace. In case of a RAW Bayer sensors, > > drivers should correctly report if vertical/horizontal flips modify the Bayer > > pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. > > I'm not sure about that. I think that restriction only applies to > libcamera (I just didn't implement support for that case). On the V4L2 Maybe I missed your point, but Documentation/sensor_driver_requirements.rst is libcamera :) > side I don't see a need to make that restriction. And if we ever meet a > sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add > support for that in libcamera. Or did I get you wrong? Or we require that if a sensor supports flipping along both axes, both (or none) should have the MODIFY_LAYOUT flag. > > Best regards, > Stefan > > > > > Thanks > > j > > > > > /* > > > * 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_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > > > index 10eba0331fe8..6efd72727773 100644 > > > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > > > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > > > @@ -144,7 +144,8 @@ private: > > > Size pixelArraySize_; > > > Rectangle activeArea_; > > > BayerFormat::Order cfaPattern_; > > > - bool supportFlips_; > > > + bool supportHFlips_; > > > + bool supportVFlips_; > > > bool flipsAlterBayerOrder_; > > > Orientation mountingOrientation_; > > > > > > @@ -162,8 +163,9 @@ private: > > > */ > > > > > > CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) > > > - : entity_(entity), staticProps_(nullptr), supportFlips_(false), > > > - flipsAlterBayerOrder_(false), properties_(properties::properties) > > > + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), > > > + supportVFlips_(false), flipsAlterBayerOrder_(false), > > > + properties_(properties::properties) > > > { > > > } > > > > > > @@ -479,26 +481,32 @@ std::optional<int> CameraSensorRaw::init() > > > return { ret }; > > > } > > > > > > - /* > > > - * Verify if sensor supports horizontal/vertical flips > > > - * > > > - * \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; > > > + /* Verify if sensor supports horizontal/vertical flips. */ > > > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > > > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > > > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > > > + LOG(CameraSensor, Debug) > > > + << "Camera sensor does not support " << name << " flip"; > > > + return { false, false }; > > > + } > > > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > > - flipsAlterBayerOrder_ = true; > > > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > > > + }; > > > + > > > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > > > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > > + > > > + if (!hflip.first || !vflip.first || > > > + (hflip.second == vflip.second)) { > > > + supportHFlips_ = hflip.first; > > > + supportVFlips_ = vflip.first; > > > + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; > > > + } else { > > > + LOG(CameraSensor, Error) > > > + << "Flips with differing alter bayer order properties" > > > + << " are unsupported. Disabling flipping."; > > > } > > > > > > - if (!supportFlips_) > > > - LOG(CameraSensor, Debug) > > > - << "Camera sensor does not support horizontal/vertical flip"; > > > - > > > /* > > > * 5. Discover ancillary devices. > > > * > > > @@ -819,14 +827,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, > > > int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > > { > > > /* Configure flips if the sensor supports that. */ > > > - if (supportFlips_) { > > > - ControlList flipCtrls(subdev_->controls()); > > > - > > > + ControlList flipCtrls(subdev_->controls()); > > > + if (supportHFlips_) > > > flipCtrls.set(V4L2_CID_HFLIP, > > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > > + > > > + if (supportVFlips_) > > > flipCtrls.set(V4L2_CID_VFLIP, > > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > > > + if (!flipCtrls.empty()) { > > > int ret = subdev_->setControls(&flipCtrls); > > > if (ret) > > > return ret; > > > @@ -1054,17 +1064,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const > > > Transform CameraSensorRaw::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. > > > + * Compute the required transform to obtain 'orientation' starting from > > > + * the mounting rotation. > > > * > > > * As a note: > > > * orientation / mountingOrientation_ = transform > > > @@ -1072,6 +1073,17 @@ Transform CameraSensorRaw::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. > > > -- > > > 2.53.0 > > >
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 6a683821f219..5dce0a6eacfe 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) { } @@ -359,26 +360,32 @@ int CameraSensorLegacy::validateSensorDriver() } } - /* - * Verify if sensor supports horizontal/vertical flips - * - * \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; + /* Verify if sensor supports horizontal/vertical flips. */ + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { + LOG(CameraSensor, Debug) + << "Camera sensor does not support " << name << " flip"; + return { false, false }; + } - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) - flipsAlterBayerOrder_ = true; + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; + }; + + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); + + if (!hflip.first || !vflip.first || + (hflip.second == vflip.second)) { + supportHFlips_ = hflip.first; + supportVFlips_ = vflip.first; + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; + } else { + LOG(CameraSensor, Error) + << "Flips with differing alter bayer order properties" + << " are unsupported. Disabling flipping."; } - if (!supportFlips_) - LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; - /* * Make sure the required selection targets are supported. * @@ -759,14 +766,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { - ControlList flipCtrls(subdev_->controls()); - + ControlList flipCtrls(subdev_->controls()); + if (supportHFlips_) flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + if (supportVFlips_) flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); + if (!flipCtrls.empty()) { int ret = subdev_->setControls(&flipCtrls); if (ret) return ret; @@ -937,17 +946,8 @@ 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. + * Compute the required transform to obtain 'orientation' starting from + * the mounting rotation. * * As a note: * orientation / mountingOrientation_ = transform @@ -955,6 +955,17 @@ 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. diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp index 10eba0331fe8..6efd72727773 100644 --- a/src/libcamera/sensor/camera_sensor_raw.cpp +++ b/src/libcamera/sensor/camera_sensor_raw.cpp @@ -144,7 +144,8 @@ private: Size pixelArraySize_; Rectangle activeArea_; BayerFormat::Order cfaPattern_; - bool supportFlips_; + bool supportHFlips_; + bool supportVFlips_; bool flipsAlterBayerOrder_; Orientation mountingOrientation_; @@ -162,8 +163,9 @@ private: */ CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) - : entity_(entity), staticProps_(nullptr), supportFlips_(false), - flipsAlterBayerOrder_(false), properties_(properties::properties) + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), + supportVFlips_(false), flipsAlterBayerOrder_(false), + properties_(properties::properties) { } @@ -479,26 +481,32 @@ std::optional<int> CameraSensorRaw::init() return { ret }; } - /* - * Verify if sensor supports horizontal/vertical flips - * - * \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; + /* Verify if sensor supports horizontal/vertical flips. */ + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { + LOG(CameraSensor, Debug) + << "Camera sensor does not support " << name << " flip"; + return { false, false }; + } - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) - flipsAlterBayerOrder_ = true; + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; + }; + + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); + + if (!hflip.first || !vflip.first || + (hflip.second == vflip.second)) { + supportHFlips_ = hflip.first; + supportVFlips_ = vflip.first; + flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second; + } else { + LOG(CameraSensor, Error) + << "Flips with differing alter bayer order properties" + << " are unsupported. Disabling flipping."; } - if (!supportFlips_) - LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; - /* * 5. Discover ancillary devices. * @@ -819,14 +827,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { - ControlList flipCtrls(subdev_->controls()); - + ControlList flipCtrls(subdev_->controls()); + if (supportHFlips_) flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + if (supportVFlips_) flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); + if (!flipCtrls.empty()) { int ret = subdev_->setControls(&flipCtrls); if (ret) return ret; @@ -1054,17 +1064,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const Transform CameraSensorRaw::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. + * Compute the required transform to obtain 'orientation' starting from + * the mounting rotation. * * As a note: * orientation / mountingOrientation_ = transform @@ -1072,6 +1073,17 @@ Transform CameraSensorRaw::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 and CameraSensorRaw support flipping on the sensor side only if the driver supports both, 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> --- Changes in v2: - Replaced duplicate query flip code with lambda - Reworked apply flip code to call setControls only once - Replaced a & by && - Applied the change to CameraSensorRaw as well - Small comment/logging changes --- 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 :-) Version 2 adresses the comments from review. Best regards, Stefan src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++-------- src/libcamera/sensor/camera_sensor_raw.cpp | 80 +++++++++++-------- 2 files changed, 90 insertions(+), 67 deletions(-)