| Message ID | 20241213094551.74584-3-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Fri, Dec 13, 2024 at 10:26:05AM +0100, Stefan Klug wrote: > Changes semantics of inputCropBounds to return null bounds if an > unconfigured stream is provided. Return the device crop bounds if a > nullptr is provided. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi Jacopo, > > I forgot to handle your comment in the v3. Sorry for that. This is my > proposal. It is somewhere in the middle of your suggestions, but I think > it makes the intent clearer. I like isConfigured() as it allows the pipeline to behave accordingly to the converter configuration. I like less the three-way behaviour of Converter::inputCropBounds(Stream *) 1) If called with a configured Stream it returns the input crop bounds of that stream 2) If called with a nullptr it returns the default crop bounds 3) When called with a Stream it returns an empty Rectangle To make it less confusing, instead of virtual std::pair<Rectangle, Rectangle> inputCropBounds( const Stream *stream = nullptr) = 0; I would virtual std::pair<Rectangle, Rectangle> inputCropBounds() = 0; virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0; or maybe even just virtual std::pair<Rectangle, Rectangle> cropBounds() = 0; So that pipeline handlers could std::pair<Rectangle, Rectangle> cropLimits; if (dewarper_->isConfigured(&data->mainPathStream_)) cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); else cropLimits = dewarper_->cropBounds(); Which I find clearer as an API than const Stream *stream = nullptr; if (dewarper_->isConfigured(&data->mainPathStream_)) stream = &data->mainPathStream_; std::pair<Rectangle, Rectangle> cropLimits = dewarper_->inputCropBounds(stream); Up to you > > I would like to keep the getCropBound() a static function as we > discussed before. > > The commit message will also be updated for v4. > > Is this change ok for you? > > Best regards, > Stefan > > include/libcamera/internal/converter.h | 4 +++- > .../internal/converter/converter_v4l2_m2m.h | 1 + > src/libcamera/converter.cpp | 12 +++++++++++- > .../converter/converter_v4l2_m2m.cpp | 19 ++++++++++++++++--- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++++- > 5 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h > index 9213ae5b9c33..465b0b0596d9 100644 > --- a/include/libcamera/internal/converter.h > +++ b/include/libcamera/internal/converter.h > @@ -73,6 +73,7 @@ public: > > virtual int configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0; > + virtual bool isConfigured(const Stream *stream) const = 0; > virtual int exportBuffers(const Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0; > > @@ -83,7 +84,8 @@ public: > const std::map<const Stream *, FrameBuffer *> &outputs) = 0; > > virtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0; > - virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0; > + virtual std::pair<Rectangle, Rectangle> inputCropBounds( > + const Stream *stream = nullptr) = 0; > > Signal<FrameBuffer *> inputBufferReady; > Signal<FrameBuffer *> outputBufferReady; > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h > index 89bd2878b190..175f0e1109a6 100644 > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h > @@ -54,6 +54,7 @@ public: > > int configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg); > + bool isConfigured(const Stream *stream) const override; > int exportBuffers(const Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > index c3da162b7de7..74cea46cc709 100644 > --- a/src/libcamera/converter.cpp > +++ b/src/libcamera/converter.cpp > @@ -169,6 +169,13 @@ Converter::~Converter() > * \return 0 on success or a negative error code otherwise > */ > > +/** > + * \fn Converter::isConfigured() > + * \brief Check if a given stream is configured > + * \param[in] stream The output stream > + * \return True if the \a stream is configured or false otherwise > + */ > + > /** > * \fn Converter::exportBuffers() > * \brief Export buffers from the converter device > @@ -238,9 +245,12 @@ Converter::~Converter() > * this function should be called after the \a stream has been configured using > * configure(). > * > - * When called with an invalid \a stream, the function returns the default crop > + * When called with nullptr \a stream this function returns the default crop > * bounds of the converter. > * > + * When called with an invalid \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 6857472b29f2..4df253749feb 100644 > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp > @@ -559,6 +559,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, > return 0; > } > > +/** > + * \copydoc libcamera::Converter::isConfigured > + */ > +bool V4L2M2MConverter::isConfigured(const Stream *stream) const > +{ > + return streams_.find(stream) != streams_.end(); > +} > + > /** > * \copydoc libcamera::Converter::exportBuffers > */ > @@ -595,11 +603,16 @@ int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect) > std::pair<Rectangle, Rectangle> > V4L2M2MConverter::inputCropBounds(const Stream *stream) > { > + if (stream == nullptr) > + return inputCropBounds_; > + > auto iter = streams_.find(stream); > - if (iter != streams_.end()) > - return iter->second->inputCropBounds(); > + if (iter == streams_.end()) { > + LOG(Converter, Error) << "Invalid output stream"; > + return {}; > + } > > - return inputCropBounds_; > + return iter->second->inputCropBounds(); > } > > /** > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 4dcc5a4fa6a1..ad162164df1a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1281,8 +1281,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > ControlInfoMap::Map controls; > > if (dewarper_) { > + const Stream *stream = nullptr; > + if (dewarper_->isConfigured(&data->mainPathStream_)) > + stream = &data->mainPathStream_; > + > std::pair<Rectangle, Rectangle> cropLimits = > - dewarper_->inputCropBounds(&data->mainPathStream_); > + dewarper_->inputCropBounds(stream); > > /* > * ScalerCrop is specified to be in Sensor coordinates. > -- > 2.43.0 >
diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h index 9213ae5b9c33..465b0b0596d9 100644 --- a/include/libcamera/internal/converter.h +++ b/include/libcamera/internal/converter.h @@ -73,6 +73,7 @@ public: virtual int configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0; + virtual bool isConfigured(const Stream *stream) const = 0; virtual int exportBuffers(const Stream *stream, unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0; @@ -83,7 +84,8 @@ public: const std::map<const Stream *, FrameBuffer *> &outputs) = 0; virtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0; - virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0; + virtual std::pair<Rectangle, Rectangle> inputCropBounds( + const Stream *stream = nullptr) = 0; Signal<FrameBuffer *> inputBufferReady; Signal<FrameBuffer *> outputBufferReady; diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h index 89bd2878b190..175f0e1109a6 100644 --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h @@ -54,6 +54,7 @@ public: int configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg); + bool isConfigured(const Stream *stream) const override; int exportBuffers(const Stream *stream, unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers); diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp index c3da162b7de7..74cea46cc709 100644 --- a/src/libcamera/converter.cpp +++ b/src/libcamera/converter.cpp @@ -169,6 +169,13 @@ Converter::~Converter() * \return 0 on success or a negative error code otherwise */ +/** + * \fn Converter::isConfigured() + * \brief Check if a given stream is configured + * \param[in] stream The output stream + * \return True if the \a stream is configured or false otherwise + */ + /** * \fn Converter::exportBuffers() * \brief Export buffers from the converter device @@ -238,9 +245,12 @@ Converter::~Converter() * this function should be called after the \a stream has been configured using * configure(). * - * When called with an invalid \a stream, the function returns the default crop + * When called with nullptr \a stream this function returns the default crop * bounds of the converter. * + * When called with an invalid \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 6857472b29f2..4df253749feb 100644 --- a/src/libcamera/converter/converter_v4l2_m2m.cpp +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp @@ -559,6 +559,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, return 0; } +/** + * \copydoc libcamera::Converter::isConfigured + */ +bool V4L2M2MConverter::isConfigured(const Stream *stream) const +{ + return streams_.find(stream) != streams_.end(); +} + /** * \copydoc libcamera::Converter::exportBuffers */ @@ -595,11 +603,16 @@ int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect) std::pair<Rectangle, Rectangle> V4L2M2MConverter::inputCropBounds(const Stream *stream) { + if (stream == nullptr) + return inputCropBounds_; + auto iter = streams_.find(stream); - if (iter != streams_.end()) - return iter->second->inputCropBounds(); + if (iter == streams_.end()) { + LOG(Converter, Error) << "Invalid output stream"; + return {}; + } - return inputCropBounds_; + return iter->second->inputCropBounds(); } /** diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 4dcc5a4fa6a1..ad162164df1a 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1281,8 +1281,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) ControlInfoMap::Map controls; if (dewarper_) { + const Stream *stream = nullptr; + if (dewarper_->isConfigured(&data->mainPathStream_)) + stream = &data->mainPathStream_; + std::pair<Rectangle, Rectangle> cropLimits = - dewarper_->inputCropBounds(&data->mainPathStream_); + dewarper_->inputCropBounds(stream); /* * ScalerCrop is specified to be in Sensor coordinates.
Changes semantics of inputCropBounds to return null bounds if an unconfigured stream is provided. Return the device crop bounds if a nullptr is provided. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hi Jacopo, I forgot to handle your comment in the v3. Sorry for that. This is my proposal. It is somewhere in the middle of your suggestions, but I think it makes the intent clearer. I would like to keep the getCropBound() a static function as we discussed before. The commit message will also be updated for v4. Is this change ok for you? Best regards, Stefan include/libcamera/internal/converter.h | 4 +++- .../internal/converter/converter_v4l2_m2m.h | 1 + src/libcamera/converter.cpp | 12 +++++++++++- .../converter/converter_v4l2_m2m.cpp | 19 ++++++++++++++++--- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++++- 5 files changed, 36 insertions(+), 6 deletions(-)