[v2,2/4] libcamera: converter_v4l2_m2m: Support crop selection
diff mbox series

Message ID 20240519115622.32170-3-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Plumb i.MX8MP DW100 dewarper
Related show

Commit Message

Umang Jain May 19, 2024, 11:56 a.m. UTC 
Add a helper to set selection rectangle on the video node
to support cropping and scaling capabilites.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../internal/converter/converter_v4l2_m2m.h   |  5 ++++
 .../converter/converter_v4l2_m2m.cpp          | 26 +++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Kieran Bingham May 20, 2024, 10:52 a.m. UTC | #1 
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@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).



>  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?

Maybe that will be clearer on the next patches/users. But I'd also
probably call this 'stream', rather than 'output' I think.

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?

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...



> + * \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
>
Umang Jain May 20, 2024, 12:43 p.m. UTC | #2 
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@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
>>

Patch
diff mbox series 

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);
+
 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);
+	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
+ * \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
  */