[PATCH v5 2/5] libcamera: converter: Add interface to support cropping capability
Paul Elder
paul.elder at ideasonboard.com
Fri Jul 19 09:03:03 CEST 2024
On Thu, Jul 18, 2024 at 12:06:12PM +0200, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:
> > Hi Jacopo
> >
> > On 17/07/24 3:32 pm, Jacopo Mondi wrote:
> > > Hi Umang
> > >
> > > On Wed, Jul 10, 2024 at 01:51:48AM GMT, 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>
> > > > ---
> > > > include/libcamera/internal/converter.h | 5 ++
> > > > .../internal/converter/converter_v4l2_m2m.h | 6 ++
> > > > src/libcamera/converter.cpp | 51 +++++++++++
> > > > .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++
> > > > 4 files changed, 150 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > > index 7e478356..9b1223fd 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,
> > > > + Crop = (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 setCrop(const Stream *stream, Rectangle *rect);
> > > "crop" is loosely defined as a concept.
> > > A converter has an input and an output and I presume it scales.
> > >
> > > Where is the crop rectangle applied ?
> >
> > A converter is a processing block - outside of ISP. Hence there is a input
> > and output, and the crop is applied at/w.r.t input. The resultant should be
> > obtained/visible on the output.
> >
>
> And where is the assumption that crop is applied to the input
> documented ? Aren't there converter that allow to crop on the output
> maybe -after- scaling ?
I suppose this should be added to the documentation of the cropping
feature flag.
>
> Cropping before or after scaling (or, if you want to put it different,
> cropping on the input or on the output) are both possible options, and
> this interface doesn't allow you to specify that.
>
> You have wired the V4L2 converter to apply selection on the TGT_CROP
> target on the input queue (aka on the 'output' video device, oh funny
> names)
>
> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> {
>
> return iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);
> }
>
>
> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)
> {
> int ret = m2m_->output()->setSelection(target, rect);
> if (ret < 0)
> return ret;
>
> return 0;
> }
>
> Can't a converter expose a TGT_CROP target on the capture device to allow
> userspace to apply a crop on the "final" image before returning it to
> applications ?
>
> > >
> > > Does this need to be part of the base class interface ? Or should only
> > > derived classes that support it implement it ? (it's reasonable for a
> > > V4L2 M2M device to support this, once better specified the interface
> > > where the crop rectangle has to be applied).
> >
> > I think so. Atleast that's what Laurent initally asked for. Converter
> > interface should dictate the features supported, the implementation left to
> > the derived converter classes. No implementation specific details should be
> > handled by the interface itself, only the "feature" part
> >
>
> Yes, but concepts like 'crop' and 'selection' might only apply to
> certain types of converter, like the v4l2 m2m ones. I wonder if it is
> better to provide a stub implementation in the whole hierarcy or only
> expose such methods in the classes derived from, in example,
> V4L2M2MConverter.
Maybe we can have another feature flag for output crop when the need
arises?
>
> > > > + virtual std::pair<Rectangle, Rectangle> getCropBounds(const Stream *stream);
> > > not "get" in getters
> >
> > ah yes
> > >
> > > > +
> > > > 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..2697eed9 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 setCrop(const Stream *stream, Rectangle *rect);
> > > > + std::pair<Rectangle, Rectangle> getCropBounds(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 2c3da6d4..d51e77a0 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::Crop
> > > > + * \brief Cropping capability is supported by the converter
> > > > */
> > > >
> > > > /**
> > > > @@ -161,6 +165,53 @@ 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
> > > > + *
> > > > + * Set the crop rectangle \a rect for \a stream provided the converter supports
> > > > + * cropping. The converter should have the Feature::Crop flag in this case.
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)
> > > > +{
> > > > + if (!(getFeatures() & Feature::Crop)) {
> > > > + LOG(Converter, Error) << "Converter doesn't support cropping capabilities";
> > > > + return -ENOTSUP;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Get the crop bounds \a stream
> > > > + * \param[in] stream Pointer to output stream
> > > > + *
> > > > + * Get the minimum and maximum crop bounds for \a stream. The converter
> > > > + * should supporting cropping (Feature::Crop).
> > > > + *
> > > > + * \return A std::pair<Rectangle, Rectangle> containining minimum and maximum
> > > > + * crop bound respectively.
> > > > + */
> > > > +std::pair<Rectangle, Rectangle> Converter::getCropBounds([[maybe_unused]] const Stream *stream)
> > > > +{
> > > > + const StreamConfiguration &config = stream->configuration();
> > > > + Rectangle rect;
> > > > +
> > > > + if (!(getFeatures() & Feature::Crop))
> > > > + 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.
> > > > + */
> > > > + rect.size() = config.size;
> > > > +
> > > > + return { rect, rect };
> > > I would rather not make it part of the base class interface
> >
> > no? If the crop is a feature, getting the supported bounds would be
> > something callers would like to know. Do you think this should be left only
> > to derived converter classes ?
>
> Only to the hierarchy of derived classes that supports it, like the
> classes derived from V4L2M2MConverter (and the class itself). For some
> other "converters" cropping and scaling might not make sense at all.
>
I thought the features flag tells you which features the converter
supports. And then for C++ purposes for converters that don't support
the feature, we have default nops for the setter , and a reasonable
default that makes sense for getters.
Paul
> > > > +}
> > > > +
> > > > /**
> > > > * \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..eaae3528 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,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> > > > return iter->second->exportBuffers(count, buffers);
> > > > }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::Converter::setCrop
> > > > + */
> > > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)
> > > > +{
> > > > + if (!(getFeatures() & Feature::Crop))
> > > > + 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::getCropBounds
> > > > + */
> > > > +std::pair<Rectangle, Rectangle>
> > > > +V4L2M2MConverter::getCropBounds(const Stream *stream)
> > > > +{
> > > > + Rectangle minCrop;
> > > > + Rectangle maxCrop;
> > > > + int ret;
> > > > +
> > > > + minCrop.width = 1;
> > > > + minCrop.height = 1;
> > > > + maxCrop.width = UINT_MAX;
> > > > + maxCrop.height = UINT_MAX;
> > > You can move all of this after the check for the feature support
> > >
> > > > +
> > > > + if (!(getFeatures() & Feature::Crop)) {
> > > > + LOG(Converter, Error) << "Crop functionality is not supported";
> > > > + return {};
> > > > + }
> > > > +
> > > > + 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";
> > > > + return {};
> > > > + }
> > > > +
> > > > + ret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);
> > > > + if (ret) {
> > > > + LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > + return {};
> > > > + }
> > > > +
> > > > + return { minCrop, maxCrop };
> > > > + }
> > > > +
> > > > + /*
> > > > + * If the streams are configured, return bounds from according to
> > > > + * stream configuration.
> > > > + */
> > > > + ret = setCrop(stream, &minCrop);
> > > > + if (ret) {
> > > > + LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > > + return {};
> > > > + }
> > > > +
> > > > + ret = setCrop(stream, &maxCrop);
> > > > + if (ret) {
> > > > + LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > > + return {};
> > > > + }
> > > > +
> > > > + return { minCrop, maxCrop };
> > > > +}
> > > > +
> > > > /**
> > > > * \copydoc libcamera::Converter::start
> > > > */
> > > > @@ -448,6 +532,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.
> > > > + */
> > > That's another option.
> > >
> > > I'll send to the list the patches I have on top of your series that
> > > makes it possible to specify features at registration time for a
> > > comparison.
> >
> > Right , this bit is something I have to develop once I get a testing setup.
> >
> > The compatibles below are currently used for simple pipeline handler only.
> > >
> > > > static std::initializer_list<std::string> compatibles = {
> > > > "mtk-mdp",
> > > > "pxp",
> > > > --
> > > > 2.45.2
> > > >
> >
More information about the libcamera-devel
mailing list