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

[Stefan Klug]

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

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