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

[Laurent Pinchart]

On Tue, Jun 25, 2024 at 09:51:54PM +0100, Kieran Bingham wrote:
> Quoting Umang Jain (2024-06-25 07:23:25)
> > 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          | 27 +++++++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > index b9e59899..846e9f9e 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,8 @@ public:
> >         int queueBuffers(FrameBuffer *input,
> >                          const std::map<const Stream *, FrameBuffer *> &outputs);
> >  
> > +       int setSelection(const Stream *stream, unsigned int target, Rectangle *rect);

I think this feature belongs to the Converter API. The ability to crop
the input isn't restricted to V4L2-based converters. You will then also
need to extend the Converter API to report the supported features, as
not all converters will be able to crop. This also means that you
shouldn't pass a V4L2 selection target, but create functions that focus
on features, not just exposing the V4L2 API to the upper level.

> > +
> >  private:
> >         class V4L2M2MStream : protected Loggable
> >         {
> > @@ -75,6 +78,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 2e77872e..c323f677 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);
> 
> Can selections ever be expected to be set on the input()?

Thanks to V4L2's lovely naming scheme, you've confusing input and output
:-) output() here is the input of the M2M device, the other side is
capture().

> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> >  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> >  {
> >         return stream_->configuration().toString();
> > @@ -374,6 +383,24 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> >         return iter->second->exportBuffers(count, buffers);
> >  }
> >  
> > +/**
> > + * \brief Set a selection rectangle \a rect for \a target
> > + * \param[in] stream Pointer to 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(const Stream *stream, unsigned int target,
> > +                                  Rectangle *rect)
> 
> I'm not yet sure how far up the V4L2 types should be transferred in
> this, but as the class name is V4L2M2M - I think we're still good
> here.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +{
> > +       auto iter = streams_.find(stream);
> > +       if (iter == streams_.end())
> > +               return -EINVAL;
> > +
> > +       return iter->second->setSelection(target, rect);
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::start
> >   */

-- 
Regards,

Laurent Pinchart