[PATCH v4 2/5] libcamera: converter: Add interface to support cropping capability
Paul Elder
paul.elder at ideasonboard.com
Wed Jul 3 09:39:50 CEST 2024
On Wed, Jul 03, 2024 at 08:58:53AM +0530, Umang Jain wrote:
> Hi Paul
>
> On 02/07/24 6:06 pm, Paul Elder wrote:
> > On Thu, Jun 27, 2024 at 07:16:53PM +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.
> > >
> > > For implementing the getCropBounds(), V4L2VideoDevice needs
> > > to be extended to support VIDIOC_G_SELECTION via
> > > V4L2VideoDevice::getSelection().
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > include/libcamera/internal/converter.h | 5 ++
> > > .../internal/converter/converter_v4l2_m2m.h | 7 ++
> > > include/libcamera/internal/v4l2_videodevice.h | 1 +
> > > src/libcamera/converter.cpp | 35 ++++++++
> > > .../converter/converter_v4l2_m2m.cpp | 85 +++++++++++++++++++
> > > src/libcamera/v4l2_videodevice.cpp | 32 +++++++
> > > 6 files changed, 165 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > index 15bc7dca..deba75e3 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 = (1 << 0),
> > > + Crop = (1 << 1),
> > > };
> > > 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);
> > > + virtual std::pair<Rectangle, Rectangle> getCropBounds(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..e97fbdd2 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,9 @@ private:
> > > int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > + int getSelection(unsigned int target, Rectangle *rect);
> > > + int setSelection(unsigned int target, Rectangle *rect);
> > > +
> > > protected:
> > > std::string logPrefix() const override;
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 9057be08..506d6a65 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -209,6 +209,7 @@ public:
> > > Formats formats(uint32_t code = 0);
> > > int setSelection(unsigned int target, Rectangle *rect);
> > > + int getSelection(unsigned int target, Rectangle *rect);
> > > int allocateBuffers(unsigned int count,
> > > std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > index 2dc7d984..19634e1c 100644
> > > --- a/src/libcamera/converter.cpp
> > > +++ b/src/libcamera/converter.cpp
> > > @@ -39,6 +39,9 @@ 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 +164,38 @@ 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)
> > > +{
> > > + return 0;
> > > +}
Actually, it just occurred to me that this should probably return
-ENOTSUP?
> > > +
> > > +/**
> > > + * \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)
> > > +{
> > > + Rectangle rect;
> > > +
> > > + return { rect, rect };
> > > +}
And this should return the size of the configured format?
Those sound more correct to me imo.
Paul
> > These look strange to me... they seem to not to expected to ever be
> > called... is there no way to keep them virtual and require them to be
> > overridden by inheritors? Otherwise the implementations clearly imply
>
> Do you mean I should make the getCropBounds() and setCrop() 'pure' virtual
> functions (like the others in the converter interface) ? In that case, all
> inherited class will need to provide a crop implementation regardless they
> support the capability or not.
>
> Currently, they are just virtual functions, and inherited ones are not
> mandated to provide implementation. But the converter interface need to give
> a its implementation in this case.
>
> > "crop is not supported", and any application that does actually call
> > these would end up with unexpected nops.
> >
> > > +
> > > /**
> > > * \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..4bb625e9 100644
> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > @@ -155,6 +155,24 @@ 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;
> > > +}
> > > +
> > > +int V4L2M2MConverter::V4L2M2MStream::getSelection(unsigned int target, Rectangle *rect)
> > > +{
> > > + int ret = m2m_->output()->getSelection(target, rect);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> > > {
> > > return stream_->configuration().toString();
> > > @@ -375,6 +393,69 @@ 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;
> > > +
> > > + auto iter = streams_.find(stream);
> > > + if (iter == streams_.end())
> > > + return {};
> > > +
> > > + if (!(getFeatures() & Feature::Crop)) {
> > > + LOG(Converter, Error) << "Crop functionality is not supported";
> > > + return {};
> > > + }
> > > +
> > > + /* Get the maximum crop bound */
> > > + ret = iter->second->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);
> > Do you not need to set an astronomically large crop bound like the
> > minimum one? Isn't it possible to simply use the size of the configured
> > format?
>
> Possibly. That would reduce a lot of code here as well (esp. the
> introduction of G_SELECTION)
>
> >
> > Paul
> >
> > > + if (ret) {
> > > + LOG(Converter, Error) << "Failed to query maximum crop bound";
> > > + return {};
> > > + }
> > > +
> > > + /*
> > > + * Get the minimum crop bound by setting 1x1 crop rectangle. The returned
> > > + * rectangle should be them minimum crop supported by the driver.
> > > + */
> > > + minCrop.width = 1;
> > > + minCrop.height = 1;
> > > + ret = setCrop(stream, &minCrop);
> > > + if (ret) {
> > > + LOG(Converter, Error) << "Failed to query minimum crop bound";
> > > + return {};
> > > + }
> > > +
> > > + /* Reset the crop rectangle to maximum */
> > > + ret = setCrop(stream, &maxCrop);
> > > + if (ret) {
> > > + LOG(Converter, Error) << "Failed to reset the crop to " << maxCrop.toString();
> > > + return {};
> > > + }
> > > +
> > > + return { minCrop, maxCrop };
> > > +}
> > > +
> > > /**
> > > * \copydoc libcamera::Converter::start
> > > */
> > > @@ -448,6 +529,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.
> > > + */
> > > static std::initializer_list<std::string> compatibles = {
> > > "mtk-mdp",
> > > "pxp",
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 4947aa3d..dfbfae2c 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1226,6 +1226,38 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)
> > > return 0;
> > > }
> > > +/**
> > > + * \brief Get the selection rectangle \a rect for \a target
> > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > + * \param[inout] rect The selection rectangle that has been queried
> > > + *
> > > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
> > > +{
> > > + struct v4l2_selection sel = {};
> > > +
> > > + sel.type = bufferType_;
> > > + sel.target = target;
> > > + sel.flags = 0;
> > > +
> > > + int ret = ioctl(VIDIOC_G_SELECTION, &sel);
> > > + if (ret < 0) {
> > > + LOG(V4L2, Error) << "Unable to get rectangle " << target
> > > + << ": " << strerror(-ret);
> > > + return ret;
> > > + }
> > > +
> > > + rect->x = sel.r.left;
> > > + rect->y = sel.r.top;
> > > + rect->width = sel.r.width;
> > > + rect->height = sel.r.height;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > int V4L2VideoDevice::requestBuffers(unsigned int count,
> > > enum v4l2_memory memoryType)
> > > {
> > > --
> > > 2.44.0
> > >
>
More information about the libcamera-devel
mailing list