[PATCH v6 2/5] libcamera: converter: Add interface to support cropping capability
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 2 22:33:20 CEST 2024
Hi Umang,
Thank you for the patch.
On Fri, Jul 26, 2024 at 05:17:12PM +0530, Umang Jain wrote:
> If the converter has cropping capability, the interface should support it
> by providing appropriate virtual functions. Provide Feature::Crop in
> Feature enumeration for the same.
>
> Provide virtual setCrop() and getCropBounds() 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 capability,
> hence Feature::Crop should be set while construction for all those
> converters.
>
> 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 | 6 ++
> src/libcamera/converter.cpp | 52 +++++++++++
> .../converter/converter_v4l2_m2m.cpp | 92 +++++++++++++++++++
> 4 files changed, 155 insertions(+)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 652ff519..853888f4 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);
> + virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream);
> +
> 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 91701dbe..5b69b14e 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,6 +58,9 @@ 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
> {
> @@ -75,6 +79,8 @@ private:
>
> int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>
> + int setSelection(unsigned int target, Rectangle *rect);
> +
> protected:
> std::string logPrefix() const override;
>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index e2dbf5e0..7ab56b4c 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,54 @@ Converter::~Converter()
> * \return 0 on success or a negative error code otherwise
> */
>
> +/**
> + * \brief Set the crop rectangle \a rect for \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The crop rectangle to be applied
There's no explanation of the "out" side of "inout" in the text here.
> + *
> + * Set the crop rectangle \a rect for \a stream provided the converter supports
> + * cropping. The converter should have the Feature::InputCrop flag in this
"should" is not a mandatory requirement.
> + * case.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Converter::setInputCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> +{
> + if (!(features() & Feature::InputCrop)) {
> + LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> + return -ENOTSUP;
> + }
> +
> + return 0;
What's the purpose of this default implementation ?
> +}
> +
> +/**
> + * \brief Get the crop bounds \a stream
s/Get/Retrieve/
s/bounds/bounds for/
> + * \param[in] stream Pointer to output stream
s/Pointer to output stream/The output stream/
> + *
> + * Get the minimum and maximum crop bounds for \a stream. The converter
> + * should supporting cropping (Feature::InputCrop).
s/supporting/support/
> + *
> + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> + * crop bound respectively.
> + */
> +std::pair<Rectangle, Rectangle> Converter::inputCropBounds([[maybe_unused]] const Stream *stream)
> +{
> + const StreamConfiguration &config = stream->configuration();
> + Rectangle rect;
> +
> + if (!(features() & Feature::InputCrop))
> + LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> +
> + /*
> + * This is base implementation for the Converter class, so just return
> + * the stream configured size as minimum and maximum crop bounds.
> + */
Why ?
> + rect.size() = config.size;
> +
> + return { rect, rect };
> +}
> +
> /**
> * \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 4aeb7dd9..3295b988 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
> return 0;
> }
>
> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> +{
> + int ret = m2m_->output()->setSelection(target, rect);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> {
> return stream_->configuration().toString();
> @@ -375,6 +384,85 @@ 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())
> + return -EINVAL;
> +
> + return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::inputCropBounds
> + */
> +std::pair<Rectangle, Rectangle>
> +V4L2M2MConverter::inputCropBounds(const Stream *stream)
> +{
> + Rectangle minCrop;
> + Rectangle maxCrop;
> + int ret;
> +
> + if (!(features() & Feature::InputCrop)) {
> + LOG(Converter, Error) << "Input Crop functionality is not supported";
> + return {};
> + }
> +
> + minCrop.width = 1;
> + minCrop.height = 1;
> + maxCrop.width = UINT_MAX;
> + maxCrop.height = UINT_MAX;
> +
> + auto iter = streams_.find(stream);
> + if (iter == streams_.end()) {
> + /*
> + * No streams configured, return minimum and maximum crop
> + * bounds at initialization.
> + */
> + ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);
> + if (ret) {
> + LOG(Converter, Error) << "Failed to query minimum crop bound"
> + << strerror(-ret);
> + return {};
> + }
> +
> + ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> + if (ret) {
> + LOG(Converter, Error) << "Failed to query maximum crop bound"
> + << strerror(-ret);
> + return {};
> + }
> +
> + return { minCrop, maxCrop };
> + }
> +
> + /*
> + * If the streams are configured, return bounds from according to
"from according to" ?
> + * stream configuration.
> + */
The behaviour is ill-defined. The documentation of the inputCropBounds()
function makes no mention of the fact that the crop bounds will vary
depending on the stream configuration.
> + ret = setInputCrop(stream, &minCrop);
Modifying the active crop rectangle when querying the bounds isn't a
good idea. It's an unexpected side effect when the converter isn't
streaming yet, and will mess things up when it is.
> + if (ret) {
> + LOG(Converter, Error) << "Failed to query minimum crop bound"
> + << strerror(-ret);
> + return {};
> + }
> +
> + ret = setInputCrop(stream, &maxCrop);
> + if (ret) {
> + LOG(Converter, Error) << "Failed to query maximum crop bound"
> + << strerror(-ret);
> + return {};
> + }
> +
> + return { minCrop, maxCrop };
> +}
> +
> /**
> * \copydoc libcamera::Converter::start
> */
> @@ -448,6 +536,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> return 0;
> }
>
> +/*
> + * \todo: This should be extended to include Feature::Flag to denote
> + * what each converter supports feature-wise.
> + */
Does this belong to patch 1/5 ?
> static std::initializer_list<std::string> compatibles = {
> "mtk-mdp",
> "pxp",
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list