[PATCH v2 2/4] libcamera: converter_v4l2_m2m: Support crop selection

[Umang Jain]

Hi Kirean

On 20/05/24 4:22 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-05-19 12:56:20)
>> Add a helper to set selection rectangle on the video node
>> to support cropping and scaling capabilites.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   .../internal/converter/converter_v4l2_m2m.h   |  5 ++++
>>   .../converter/converter_v4l2_m2m.cpp          | 26 +++++++++++++++++++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> index 1126050c..acc6424c 100644
>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> @@ -29,6 +29,7 @@ class MediaDevice;
>>   class Size;
>>   class SizeRange;
>>   struct StreamConfiguration;
>> +class Rectangle;
>>   class V4L2M2MDevice;
>>   
>>   class V4L2M2MConverter : public Converter
>> @@ -56,6 +57,8 @@ public:
>>          int queueBuffers(FrameBuffer *input,
>>                           const std::map<unsigned int, FrameBuffer *> &outputs);
>>   
>> +       int setSelection(unsigned int output, unsigned int target, Rectangle *rect);
>> +
> Would there be any value in also adding the corresponding 'getSelection'
> call here too? Or would it not be used? (Not using it /yet/ is also a
> valid reason I suspect).

I haven't found a use yet so...
>
>
>>   private:
>>          class Stream : protected Loggable
>>          {
>> @@ -74,6 +77,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/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> index d8929fc5..2d3ee257 100644
>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *outp
>>          return 0;
>>   }
>>   
>> +int V4L2M2MConverter::Stream::setSelection(unsigned int target, Rectangle *rect)
>> +{
>> +       int ret = m2m_->output()->setSelection(target, rect);
>
> Aha, ok - so we really only apply on outputs..
>
>
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>>   std::string V4L2M2MConverter::Stream::logPrefix() const
>>   {
>>          return "stream" + std::to_string(index_);
>> @@ -370,6 +379,23 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
>>          return streams_[output].exportBuffers(count, buffers);
>>   }
>>   
>> +/**
>> + * \brief Set a selection rectangle \a rect for \a target
>> + * \param[in] output Index of the output stream
> Eek, this bit looks a bit awkward, as we're utilising the internal
> stream index, while I suspect the APIs here would instead reference a
> Stream object?

It should reference a Stream object ideally. The converter interface 
should be fixed for doing that actually,

For .e.g even queuing frame buffers to the converter, works on indexes 
of the stream

```
  * \fn Converter::queueBuffers()
  * \brief Queue buffers to converter device
  * \param[in] input The frame buffer to apply the conversion
  * \param[out] outputs The container holding the output stream indexes and
  * their respective frame buffer outputs.
...
```

Probably a separate but orthogonal task to this.
>
> Maybe that will be clearer on the next patches/users. But I'd also
> probably call this 'stream', rather than 'output' I think.

I can rename to streamIdx for now
> Can selection rectangles only meaningfully be set on the outputs? or
> will we find a reason to apply (or get) a rectangle from the Input?

It errors out if you try to set on the capture

>
> I suspect as this is an M2M device there's a bit more nuance here than I
> imagine, or simply restrictions depending on the type of device used...

It was naunce, and I didn't actually realise for a day that 
V4L2M2MConverter actually has m2m instance per-stream + a separate 
instance just at constructor-level or to check isValid() calls.

I was setting crop rectangle at the latter, and wasn't getting expected 
results until I noticed, the m2m instance is per-stream and I have set 
rectangles on that m2m instance which corresponds to the stream.

>
>
>> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
>> + * \param[inout] rect The selection rectangle to be applied
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int V4L2M2MConverter::setSelection(unsigned int output, unsigned int target,
>> +                                  Rectangle *rect)
>> +{
>> +       if (output >= streams_.size())
>> +               return -EINVAL;
>> +
>> +       return streams_[output].setSelection(target, rect);
>> +}
>> +
>>   /**
>>    * \copydoc libcamera::Converter::start
>>    */
>> -- 
>> 2.44.0
>>