[PATCH v7 2/4] libcamera: converter: Add interface to support cropping capability
Stefan Klug
stefan.klug at ideasonboard.com
Tue Sep 24 21:50:31 CEST 2024
Hi Umang,
Thank you for the patch.
On Fri, Sep 06, 2024 at 12:04:42PM +0530, Umang Jain wrote:
> If the converter has cropping capability on its input, the interface
> should support it by providing appropriate virtual functions. Provide
> Feature::InputCrop in Feature enumeration for the same.
>
> Provide virtual setInputCrop() and inputCropBounds() interfaces so that
> the converter can implement their own cropping functionality.
>
> The V4L2M2MConverter implements these interfaces of the Converter
> interface. Not all V4L2M2M converters will have cropping ability
> on its input, hence it needs to discovered during construction time.
> If the capability to crop to identified successfully, the cropping
> bounds are determined during configure() time.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> include/libcamera/internal/converter.h | 5 +
> .../internal/converter/converter_v4l2_m2m.h | 13 ++-
> src/libcamera/converter.cpp | 38 +++++++
> .../converter/converter_v4l2_m2m.cpp | 106 +++++++++++++++++-
> 4 files changed, 158 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 6623de4d..ffbb6f34 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -14,6 +14,7 @@
> #include <memory>
> #include <string>
> #include <tuple>
> +#include <utility>
> #include <vector>
>
> #include <libcamera/base/class.h>
> @@ -35,6 +36,7 @@ class Converter
> public:
> enum class Feature {
> None = 0,
> + InputCrop = (1 << 0),
> };
>
> using Features = Flags<Feature>;
> @@ -63,6 +65,9 @@ public:
> virtual int queueBuffers(FrameBuffer *input,
> const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>
> + virtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0;
> + virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;
> +
> Signal<FrameBuffer *> inputBufferReady;
> Signal<FrameBuffer *> outputBufferReady;
>
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index b9e59899..7d321c85 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -30,6 +30,7 @@ class Size;
> class SizeRange;
> class Stream;
> struct StreamConfiguration;
> +class Rectangle;
> class V4L2M2MDevice;
>
> class V4L2M2MConverter : public Converter
> @@ -57,11 +58,15 @@ public:
> int queueBuffers(FrameBuffer *input,
> const std::map<const Stream *, FrameBuffer *> &outputs);
>
> + int setInputCrop(const Stream *stream, Rectangle *rect);
> + std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream);
> +
> private:
> class V4L2M2MStream : protected Loggable
> {
> public:
> - V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);
> + V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> + Features features);
>
> bool isValid() const { return m2m_ != nullptr; }
>
> @@ -75,6 +80,9 @@ private:
>
> int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>
> + int setInputSelection(unsigned int target, Rectangle *rect);
> + std::pair<Rectangle, Rectangle> inputCropBounds();
> +
> protected:
> std::string logPrefix() const override;
>
> @@ -88,6 +96,9 @@ private:
>
> unsigned int inputBufferCount_;
> unsigned int outputBufferCount_;
> +
> + std::pair<Rectangle, Rectangle> inputCropBounds_;
> + Features features_;
> };
>
> std::unique_ptr<V4L2M2MDevice> m2m_;
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index d7bb7273..7cb6532a 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -11,6 +11,8 @@
>
> #include <libcamera/base/log.h>
>
> +#include <libcamera/stream.h>
> +
> #include "libcamera/internal/media_device.h"
>
> /**
> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)
> * \brief Specify the features supported by the converter
> * \var Converter::Feature::None
> * \brief No extra features supported by the converter
> + * \var Converter::Feature::InputCrop
> + * \brief Cropping capability at input is supported by the converter
> */
>
> /**
> @@ -161,6 +165,40 @@ Converter::~Converter()
> * \return 0 on success or a negative error code otherwise
> */
>
> +/**
> + * \fn Converter::setInputCrop()
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to apply and return the rectangle
> + * that is actually applied
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter has the Feature::InputCrop flag in this case.
> + *
> + * The underlying hardware can adjust the rectangle supplied by the user
> + * due to hardware constraints. Caller can inspect \a rect to determine the
> + * actual rectangle that has been applied by the converter, after this function
> + * returns.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn Converter::inputCropBounds()
> + * \brief Retrieve the crop bounds for \a stream
> + * \param[in] stream The output stream
> + *
> + * Retrieve the minimum and maximum crop bounds for \a stream. The converter
> + * should support cropping (Feature::InputCrop).
> + *
> + * The crop bounds depend on the configuration of the output stream hence, this
> + * function should be called after the \a stream has been configured using
> + * configure().
> + *
> + * \return A std::pair<Rectangle, Rectangle> containing minimum and maximum
> + * crop bound respectively.
> + */
> +
> /**
> * \var Converter::inputBufferReady
> * \brief A signal emitted when the input frame buffer completes
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 4f3e8ce4..1bf47ff9 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -34,8 +34,9 @@ LOG_DECLARE_CATEGORY(Converter)
> * V4L2M2MConverter::V4L2M2MStream
> */
>
> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)
> - : converter_(converter), stream_(stream)
> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,
> + Features features)
> + : converter_(converter), stream_(stream), features_(features)
> {
> m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());
>
> @@ -97,6 +98,33 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC
> inputBufferCount_ = inputCfg.bufferCount;
> outputBufferCount_ = outputCfg.bufferCount;
>
> + if (features_ & Feature::InputCrop) {
I don't fully understand why the stream has it's own features_ member.
Wouldn't it be sufficient to do
if(converter_->features() & Feature::InputCrop) {
or did I miss something?
> + Rectangle minCrop;
> + Rectangle maxCrop;
> +
> + /* Find crop bounds */
> + minCrop.width = 1;
> + minCrop.height = 1;
> + maxCrop.width = UINT_MAX;
> + maxCrop.height = UINT_MAX;
> +
> + ret = setInputSelection(V4L2_SEL_TGT_CROP, &minCrop);
> + if (ret) {
> + LOG(Converter, Error) << "Could not query minimum selection crop"
> + << strerror(-ret);
> + return ret;
> + }
> +
> + ret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> + if (ret) {
> + LOG(Converter, Error) << "Could not query maximum selection crop"
> + << strerror(-ret);
> + return ret;
> + }
> +
> + inputCropBounds_ = { minCrop, maxCrop };
> + }
> +
> return 0;
> }
>
> @@ -154,6 +182,20 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> return 0;
> }
>
> +int V4L2M2MConverter::V4L2M2MStream::setInputSelection(unsigned int target, Rectangle *rect)
> +{
> + int ret = m2m_->output()->setSelection(target, rect);
> + if (ret < 0)
Why the explicit check for < 0 and not if(ret)... that is used elsewhere?
> + return ret;
> +
> + return 0;
> +}
> +
> +std::pair<Rectangle, Rectangle> V4L2M2MConverter::V4L2M2MStream::inputCropBounds()
> +{
> + return inputCropBounds_;
> +}
> +
> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> {
> return stream_->configuration().toString();
> @@ -204,6 +246,34 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> m2m_.reset();
> return;
> }
> +
> + /* Discover Feature::InputCrop */
> + Rectangle maxCrop;
> + maxCrop.width = UINT_MAX;
> + maxCrop.height = UINT_MAX;
> +
> + ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> + if (!ret) {
You could early out and save an indentation with
if (ret)
return;
With or without the changes:
Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
Cheers,
Stefan
> + /*
> + * Rectangles for cropping targets are defined even if the device
> + * does not support cropping. Their sizes and positions will be
> + * fixed in such cases.
> + *
> + * Set and inspect a crop equivalent to half of the maximum crop
> + * returned earlier. Use this to determine whether the crop on
> + * input is really supported.
> + */
> + Rectangle halfCrop(maxCrop.size() / 2);
> +
> + ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);
> + if (!ret && halfCrop.width != maxCrop.width &&
> + halfCrop.height != maxCrop.height) {
> + features_ |= Feature::InputCrop;
> +
> + LOG(Converter, Info)
> + << "Converter supports cropping on its input";
> + }
> + }
> }
>
> /**
> @@ -336,7 +406,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {
> const StreamConfiguration &cfg = outputCfgs[i];
> std::unique_ptr<V4L2M2MStream> stream =
> - std::make_unique<V4L2M2MStream>(this, cfg.stream());
> + std::make_unique<V4L2M2MStream>(this, cfg.stream(), features_);
>
> if (!stream->isValid()) {
> LOG(Converter, Error)
> @@ -373,6 +443,36 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> return iter->second->exportBuffers(count, buffers);
> }
>
> +/**
> + * \copydoc libcamera::Converter::setInputCrop
> + */
> +int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)
> +{
> + if (!(features_ & Feature::InputCrop))
> + return -ENOTSUP;
> +
> + auto iter = streams_.find(stream);
> + if (iter == streams_.end()) {
> + LOG(Converter, Error) << "Invalid output stream";
> + return -EINVAL;
> + }
> +
> + return iter->second->setInputSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::inputCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::inputCropBounds(const Stream *stream)
> +{
> + auto iter = streams_.find(stream);
> + if (iter == streams_.end())
> + return {};
> +
> + return iter->second->inputCropBounds();
> +}
> +
> /**
> * \copydoc libcamera::Converter::start
> */
> --
> 2.45.0
>
More information about the libcamera-devel
mailing list