[libcamera-devel] [PATCH v2 2/5] libcamera: camera_sensor: Validate Transform
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Jan 16 13:45:32 CET 2023
Hi David
On Mon, Jan 16, 2023 at 12:24:09PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the patch!
>
> On Sat, 14 Jan 2023 at 19:47, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > The two pipeline handlers that currently support Transform (IPU3 and
> > RkISP1) implement it by operating H/V flips on the image sensor.
>
> Shouldn't that be three PHs, including Raspberry Pi... I'm feeling left out!
>
No, it's two PHs, but it should be RPi and not RkISP1 :)
> >
> > Centralize the code that validates a Transform request against the
> > sensor rotation capabilities in the CameraSensor class.
> >
> > The implementation in the IPU3 pipeline handler was copied from the
> > RaspberryPi implementation, and is now centralized in CameraSensor to
> > make it easier for other platforms.
> >
> > The CameraSensor::validateTransform() implementation comes directly from
> > the RaspberryPi pipeline handler, no functional changes intended.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > include/libcamera/internal/camera_sensor.h | 4 ++
> > src/libcamera/camera_sensor.cpp | 65 +++++++++++++++++++
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++-----------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 59 ++---------------
> > 4 files changed, 82 insertions(+), 91 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 878f3c28a3c9..bea52badaff7 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -29,6 +29,8 @@ class BayerFormat;
> > class CameraLens;
> > class MediaEntity;
> >
> > +enum class Transform;
> > +
> > struct CameraSensorProperties;
> >
> > class CameraSensor : protected Loggable
> > @@ -68,6 +70,8 @@ public:
> >
> > CameraLens *focusLens() { return focusLens_.get(); }
> >
> > + Transform validateTransform(Transform *transform) const;
> > +
> > protected:
> > std::string logPrefix() const override;
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 83ac075a4265..3518d3e3b02a 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -962,6 +962,71 @@ void CameraSensor::updateControlInfo()
> > * connected to the sensor
> > */
> >
> > +/**
> > + * \brief Validate a transform request against the sensor capabilities
> > + * \param[inout] transform The requested transformation, updated to match
> > + * the sensor capabilities
> > + *
> > + * The requested \a transform is adjusted to compensate for the sensor's
> > + * mounting rotation and validated agains the sensor capabilities.
> > + *
> > + * For example, if the requested \a transform is Transform::Identity and the
> > + * sensor rotation is 180 degrees, the resulting transform returned by the
> > + * function is Transform::Rot180 to automatically correct the image, but only if
> > + * the sensor can actually apply horizontal and vertical flips.
>
> I'm not completely sure about the explanation here... what we're doing
> is so easy to get in a muddle over that I wonder if we could be
> clearer.
>
> The critical point is that the output from this function is supposed
> to be the transform you need to apply to the sensor in order to give
> you the input transform that you requested, compensating for any
> sensor rotation. Perhaps these two paragraphs might go something like
> this:
>
> * The output transform is the transform that needs to be applied to the sensor
> * in order to produce the input \a transform, compensating for the sensor's
> * mounting rotation. It is also validated against the sensor's ability
> to perform
> * horizontal and vertical flips.
> *
> * For example, if the requested \a transform is Transform::Identity and the
> * sensor rotation is 180 degrees, the output transform will be Transform::Rot180
> * to correct the images so that they appear to have
> Transform::Identity, but only if
> * the sensor can apply horizontal and vertical flips.
>
> But please only use any of that if you think it's helpful. We should
> perhaps also say something more about the way the input transform is
> changed?
>
> * The input \a transform is the transform that the caller wants, and
> it is adjusted
> * according to the capabilities of the sensor to represent the
> "nearest" transform
> * that can actually be delivered.
>
I'm fine taking all of this in if it sounds better to you. What I want
in this series is to consolidate the current implementation that comes
from you pipeline in a single place, to discuss the general topics
about how to use the Transform on a single implementation
> > + *
> > + * \return A Transform instance that represents which transformation has been
> > + * applied to the camera sensor
> > + */
> > +Transform CameraSensor::validateTransform(Transform *transform) const
> > +{
> > + /* Adjust the requested transform to compensate the sensor rotation. */
> > + int32_t rotation = properties().get(properties::Rotation).value_or(0);
> > + bool success;
> > +
> > + Transform rotationTransform = transformFromRotation(rotation, &success);
> > + if (!success)
> > + LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> > + << " degrees - ignoring";
> > +
> > + Transform combined = *transform * rotationTransform;
>
> I think there's still an outstanding question about the direction of
> the rotation... but not an issue for this patch!
>
I would prefer to maintain the current implementation and make changes
on top, on a single location
> > +
> > + /*
> > + * We combine the platform and user transform, but must "adjust away"
> > + * any combined result that includes a transform, as we can't do those.
> > + * In this case, flipping only the transpose bit is helpful to
> > + * applications - they either get the transform they requested, or have
> > + * to do a simple transpose themselves (they don't have to worry about
> > + * the other possible cases).
> > + */
> > + if (!!(combined & Transform::Transpose)) {
> > + /*
> > + * Flipping the transpose bit in "transform" flips it in the
> > + * combined result too (as it's the last thing that happens),
> > + * which is of course clearing it.
> > + */
> > + *transform ^= Transform::Transpose;
> > + combined &= ~Transform::Transpose;
> > + }
> > +
> > + /*
> > + * We also check if the sensor doesn't do h/vflips at all, in which
> > + * case we clear them, and the application will have to do everything.
> > + */
> > + if (!supportFlips_ && !!combined) {
> > + /*
> > + * If the sensor can do no transforms, then combined must be
> > + * changed to the identity. The only user transform that gives
> > + * rise to this the inverse of the rotation. (Recall that
>
> Typo in the original code! s/this the/this is the/
Ah I thought it wasn't me getting the phrase :)
I'll fix
>
> Anyway, sorry for all the fussing over the documentation, otherwise I
> think this is a very good change.
>
No worries, the contrary actually
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
Thanks
> Thanks!
> David
>
> > + * combined = transform * rotationTransform.)
> > + */
> > + *transform = -rotationTransform;
> > + combined = Transform::Identity;
> > + }
> > +
> > + return combined;
> > +}
> > +
> > std::string CameraSensor::logPrefix() const
> > {
> > return "'" + entity_->name() + "'";
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index e4d79ea44aed..a424ac914859 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -184,48 +184,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > if (config_.empty())
> > return Invalid;
> >
> > - Transform combined = transform * data_->rotationTransform_;
> > -
> > - /*
> > - * We combine the platform and user transform, but must "adjust away"
> > - * any combined result that includes a transposition, as we can't do
> > - * those. In this case, flipping only the transpose bit is helpful to
> > - * applications - they either get the transform they requested, or have
> > - * to do a simple transpose themselves (they don't have to worry about
> > - * the other possible cases).
> > - */
> > - if (!!(combined & Transform::Transpose)) {
> > - /*
> > - * Flipping the transpose bit in "transform" flips it in the
> > - * combined result too (as it's the last thing that happens),
> > - * which is of course clearing it.
> > - */
> > - transform ^= Transform::Transpose;
> > - combined &= ~Transform::Transpose;
> > - status = Adjusted;
> > - }
> > -
> > /*
> > - * We also check if the sensor doesn't do h/vflips at all, in which
> > - * case we clear them, and the application will have to do everything.
> > + * Validate the requested transform against the sensor capabilities and
> > + * rotation and store the final combined transform that configure() will
> > + * need to apply to the sensor to save us working it out again.
> > */
> > - if (!data_->supportsFlips_ && !!combined) {
> > - /*
> > - * If the sensor can do no transforms, then combined must be
> > - * changed to the identity. The only user transform that gives
> > - * rise to this is the inverse of the rotation. (Recall that
> > - * combined = transform * rotationTransform.)
> > - */
> > - transform = -data_->rotationTransform_;
> > - combined = Transform::Identity;
> > + Transform requestedTransform = transform;
> > + combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);
> > + if (transform != requestedTransform)
> > status = Adjusted;
> > - }
> > -
> > - /*
> > - * Store the final combined transform that configure() will need to
> > - * apply to the sensor to save us working it out again.
> > - */
> > - combinedTransform_ = combined;
> >
> > /* Cap the number of entries to the available streams. */
> > if (config_.size() > kMaxStreams) {
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 8569df17976a..c086a69a913d 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -367,59 +367,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
> >
> > /*
> > - * What if the platform has a non-90 degree rotation? We can't even
> > - * "adjust" the configuration and carry on. Alternatively, raising an
> > - * error means the platform can never run. Let's just print a warning
> > - * and continue regardless; the rotation is effectively set to zero.
> > + * Validate the requested transform against the sensor capabilities and
> > + * rotation and store the final combined transform that configure() will
> > + * need to apply to the sensor to save us working it out again.
> > */
> > - int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);
> > - bool success;
> > - Transform rotationTransform = transformFromRotation(rotation, &success);
> > - if (!success)
> > - LOG(RPI, Warning) << "Invalid rotation of " << rotation
> > - << " degrees - ignoring";
> > - Transform combined = transform * rotationTransform;
> > -
> > - /*
> > - * We combine the platform and user transform, but must "adjust away"
> > - * any combined result that includes a transform, as we can't do those.
> > - * In this case, flipping only the transpose bit is helpful to
> > - * applications - they either get the transform they requested, or have
> > - * to do a simple transpose themselves (they don't have to worry about
> > - * the other possible cases).
> > - */
> > - if (!!(combined & Transform::Transpose)) {
> > - /*
> > - * Flipping the transpose bit in "transform" flips it in the
> > - * combined result too (as it's the last thing that happens),
> > - * which is of course clearing it.
> > - */
> > - transform ^= Transform::Transpose;
> > - combined &= ~Transform::Transpose;
> > - status = Adjusted;
> > - }
> > -
> > - /*
> > - * We also check if the sensor doesn't do h/vflips at all, in which
> > - * case we clear them, and the application will have to do everything.
> > - */
> > - if (!data_->supportsFlips_ && !!combined) {
> > - /*
> > - * If the sensor can do no transforms, then combined must be
> > - * changed to the identity. The only user transform that gives
> > - * rise to this the inverse of the rotation. (Recall that
> > - * combined = transform * rotationTransform.)
> > - */
> > - transform = -rotationTransform;
> > - combined = Transform::Identity;
> > + Transform requestedTransform = transform;
> > + combinedTransform_ = data_->sensor_->validateTransform(&transform);
> > + if (transform != requestedTransform)
> > status = Adjusted;
> > - }
> > -
> > - /*
> > - * Store the final combined transform that configure() will need to
> > - * apply to the sensor to save us working it out again.
> > - */
> > - combinedTransform_ = combined;
> >
> > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> > std::pair<int, Size> outSize[2];
> > @@ -454,7 +409,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > if (data_->flipsAlterBayerOrder_) {
> > BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
> > bayer.order = data_->nativeBayerOrder_;
> > - bayer = bayer.transform(combined);
> > + bayer = bayer.transform(combinedTransform_);
> > fourcc = bayer.toV4L2PixelFormat();
> > }
> >
> > --
> > 2.39.0
> >
More information about the libcamera-devel
mailing list