[PATCH v5 2/5] libcamera: converter: Add interface to support cropping capability
Umang Jain
umang.jain at ideasonboard.com
Fri Jul 19 12:01:48 CEST 2024
Hi Jacopo
On 18/07/24 3:36 pm, 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 ?
>
> 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.
cropping after scaling means the application would get a different image
output (if it tries to set crop on the output) ? Is that possible, a
runtime crop ? Image output size differs everytime a crop is applied ?
I think what you are referring here is the compose rectangle, rather
than the crop rectangle? Is that correct?
>
> 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 ?
So having read
https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html,
it seems TGT_CROP can be target for video capture as well. There is no
limitation on that front.
However, I can't wrap around my head around that kind of crop - applied
at runtime (like ScalerCrop) ? For video capture, TGT_COMPOSE makes
more sense to me, rather than exposing TGT_CROP.
Certainly the interface is lacking such a distinction in this form - and
I should clarify that the crop would be set on the input video signal.
Would that makes things acceptable?
I am also a bit wary about how should we be exposing such a interface
for converters, for output and video capture devices, have TGT_CROP and
TGT_COMPOSE targets ... its seems hairy for now, to me..
>>> 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.
>
>>>> + 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.
>
>>>> +}
>>>> +
>>>> /**
>>>> * \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