[PATCH v7 2/4] libcamera: converter: Add interface to support cropping capability
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 25 20:19:00 CEST 2024
On Tue, Sep 24, 2024 at 09:50:31PM +0200, Stefan Klug wrote:
> 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.
s/their/its/
> >
> > 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.
s/to discovered during/to be discovered at/
> > If the capability to crop to identified successfully, the cropping
s/to identified/is identified/
> > 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
s/Pointer to/The/
(consistent with the documentation below)
> > + * \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
s/Caller/The caller/
> > + * 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
s/hence, this/and 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
s/std::pair<Rectangle, Rectangle>/pair/ (the type is already indicated
by the function prototype).
s/containing/containing the/
> > + * crop bound respectively.
s/respectively/in that order/
> > + */
> > +
> > /**
> > * \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?
I would also drop the member variable.
> > + 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"
s/crop"/crop: "/
But it should be formatted as
LOG(Converter, Error)
<< "Could not query minimum selection crop: "
<< strerror(-ret);
> > + << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + ret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);
Shouldn't this be instead implemented by getting
V4L2_SEL_TGT_CROP_BOUNDS ? I've sent a patch that adds support for
V4L2VideoDevice::getSelection() ("[PATCH] libcamera: v4l2_videodevice:
Add getSelection() function").
> > + if (ret) {
> > + LOG(Converter, Error) << "Could not query maximum selection crop"
Same here.
> > + << 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?
You could simply
return m2m_->output()->setSelection(target, rect);
> > + 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) {
How about
if (!ret && halfCrop != maxCrop) {
> > + 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
> > */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list