[PATCH v4 08/20] libcamera: converter: Add function to query crop bounds

[Jacopo Mondi]

Hi Stefan

On Mon, Dec 16, 2024 at 08:53:08PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Mon, Dec 16, 2024 at 07:08:02PM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> >
> > On Mon, Dec 16, 2024 at 04:40:48PM +0100, Stefan Klug wrote:
> > > The inputCropBounds_ member of the V4L2M2MConverter::Stream class is
> > > only initialized after a V4L2M2MConverter::configure() call, when the
> > > streams are initialized.
> > >
> > > However, the converter has crop limits that do not depend on the
> > > configured Streams, and which are currently not accessible from the
> > > class interface.
> >
> > However you seem to store the absolute limts computed at class
> > creation in inputCropBounds_ which gets updated at each configure()
> > time.
> >
> > >
> > > Add a new inputCropBounds() function to the V4L2M2MConverter class
> > > that allows to retrieve the converter crop limits before any stream
> > > is configured.
> >
> > But in this way, after a configuration, calling
> >
> >         inputCropBounds()
> >         inputCropBounds(stream)
> >
> > return the same data.
> >
> > Or have I missed something ?
>
> Maybe, let's see. The V4L2M2MConverter and the V4L2M2MStream both have a
> inputCropBounds_ member. The one on the converter is initialized at
> converter construction time. The one on the stream is written when the
> stream is configured. So the above two line should/could return
> different things. Does that explain your concern?
>
> > When we discussed it I know I suggested to split the two functions
> > apart, but my thinking was about being able to return the default crop
> > rectangle despite the currently configured format. Sorry if I wasn't
> > clear. Or do you actually prefer this version ?
>
> I think you were clear and I think it behaves the way you said :-)

Oh crabs, we have two inputCropBounds_ members indeed /0\

Ok, then this makes it easy!
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> Cheers,
> Stefan
>
> >
> > >
> > > This is particularly useful for pipelines to initialize controls and
> > > properties and to implement validation before the Camera gets
> > > configured.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Split off from libcamera: converter_v4l2_m2m: Improve crop bounds
> > >   support
> > > - Added separate inputCropBounds() function to the dewarper class
> > >
> > > Fuxup inputCrop()
> > > ---
> > >  include/libcamera/internal/converter.h         |  1 +
> > >  .../internal/converter/converter_v4l2_m2m.h    |  2 ++
> > >  src/libcamera/converter.cpp                    | 13 +++++++++++++
> > >  src/libcamera/converter/converter_v4l2_m2m.cpp | 18 ++++++++++++------
> > >  4 files changed, 28 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > index ffbb6f345cd5..04187a2a96e8 100644
> > > --- a/include/libcamera/internal/converter.h
> > > +++ b/include/libcamera/internal/converter.h
> > > @@ -66,6 +66,7 @@ public:
> > >  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
> > >
> > >  	virtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0;
> > > +	virtual std::pair<Rectangle, Rectangle> inputCropBounds() = 0;
> > >  	virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;
> > >
> > >  	Signal<FrameBuffer *> inputBufferReady;
> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > index 9b8e43ff0b91..402a803959b9 100644
> > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > @@ -60,6 +60,7 @@ public:
> > >  			 const std::map<const Stream *, FrameBuffer *> &outputs) override;
> > >
> > >  	int setInputCrop(const Stream *stream, Rectangle *rect) override;
> > > +	std::pair<Rectangle, Rectangle> inputCropBounds() override { return inputCropBounds_; }
> > >  	std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) override;
> > >
> > >  private:
> > > @@ -106,6 +107,7 @@ private:
> > >
> > >  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> > >  	std::map<FrameBuffer *, unsigned int> queue_;
> > > +	std::pair<Rectangle, Rectangle> inputCropBounds_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > index 3a3f84344c5e..73c02fdcf4bb 100644
> > > --- a/src/libcamera/converter.cpp
> > > +++ b/src/libcamera/converter.cpp
> > > @@ -185,6 +185,16 @@ Converter::~Converter()
> > >
> > >  /**
> > >   * \fn Converter::inputCropBounds()
> > > + * \brief Retrieve the crop bounds of the converter
> > > + *
> > > + * Retrieve the minimum and maximum crop bounds of the converter. This can be
> > > + * used to query the crop bounds before configuring a stream.
> > > + *
> > > + * \return A pair containing the minimum and maximum crop bound in that order
> > > + */
> > > +
> > > +/**
> > > + * \fn Converter::inputCropBounds(const Stream *stream)
> > >   * \brief Retrieve the crop bounds for \a stream
> > >   * \param[in] stream The output stream
> > >   *
> > > @@ -195,6 +205,9 @@ Converter::~Converter()
> > >   * this function should be called after the \a stream has been configured using
> > >   * configure().
> > >   *
> > > + * When called with an unconfigured \a stream, this function returns a pair of
> > > + * null rectangles.
> > > + *
> > >   * \return A pair containing the minimum and maximum crop bound in that order
> > >   */
> > >
> > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > index 8c341fe199f6..342aa32dab52 100644
> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > @@ -273,10 +273,9 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
> > >  		return;
> > >  	}
> > >
> > > -	Rectangle minCrop;
> > > -	Rectangle maxCrop;
> > > -	ret = getCropBounds(m2m_->output(), minCrop, maxCrop);
> > > -	if (!ret && minCrop != maxCrop) {
> > > +	ret = getCropBounds(m2m_->output(), inputCropBounds_.first,
> > > +			    inputCropBounds_.second);
> > > +	if (!ret && inputCropBounds_.first != inputCropBounds_.second) {
> > >  		features_ |= Feature::InputCrop;
> > >
> > >  		LOG(Converter, Info)
> > > @@ -469,14 +468,21 @@ int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)
> > >  }
> > >
> > >  /**
> > > - * \copydoc libcamera::Converter::inputCropBounds
> > > + * \fn libcamera::V4L2M2MConverter::inputCropBounds()
> > > + * \copydoc libcamera::Converter::inputCropBounds()
> > > + */
> > > +
> > > +/**
> > > + * \copydoc libcamera::Converter::inputCropBounds(const Stream *stream)
> > >   */
> > >  std::pair<Rectangle, Rectangle>
> > >  V4L2M2MConverter::inputCropBounds(const Stream *stream)
> > >  {
> > >  	auto iter = streams_.find(stream);
> > > -	if (iter == streams_.end())
> > > +	if (iter == streams_.end()) {
> > > +		LOG(Converter, Error) << "Invalid output stream";
> > >  		return {};
> > > +	}
> > >
> > >  	return iter->second->inputCropBounds();
> > >  }
> > > --
> > > 2.43.0
> > >