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

[Kieran Bingham]

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



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