| Message ID | 20241216154124.203650-11-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Mon, Dec 16, 2024 at 04:40:50PM +0100, Stefan Klug wrote: > Query the crop bounds on the dewarper instead of the stream in case the > camera was not yet configured when updateControls() gets called. This > provides sane defaults for the controls. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v4: > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds > support > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 89946b782854..56192451eb3c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > ControlInfoMap::Map controls; > > if (dewarper_) { > - std::pair<Rectangle, Rectangle> cropLimits = > - dewarper_->inputCropBounds(&data->mainPathStream_); > + std::pair<Rectangle, Rectangle> cropLimits; > + if (dewarper_->isConfigured(&data->mainPathStream_)) > + cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); > + else > + cropLimits = dewarper_->inputCropBounds(); Now I wonder if the right way shouldn't have been /** * \fn Converter::inputCropBounds(const Stream *stream) * \brief Retrieve the crop bounds for \a stream * \param[in] stream The output stream * .... - * - * When called with an unconfigured \a stream, this function returns a pair of - * null rectangles. + * When called with an unconfigured \a stream, this function returns + * the coverter default crop rectangle + * So that you could even skip this patch completely (and probably the isConfigured() one too). Generally, I don't like API that have too many implicit behaviours and prefer the ones where the caller has to chose the right overload to call, so that the logic is clear in the calling place and is harder to mis-uses, hence I might be biased. I'll let you decide really, I have already pulled you in too many directions enough. > > controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > cropLimits.second, > -- > 2.43.0 >
Hi Jacopo, Thank you for the review. On Mon, Dec 16, 2024 at 07:14:05PM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Dec 16, 2024 at 04:40:50PM +0100, Stefan Klug wrote: > > Query the crop bounds on the dewarper instead of the stream in case the > > camera was not yet configured when updateControls() gets called. This > > provides sane defaults for the controls. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > Changes in v4: > > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds > > support > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 89946b782854..56192451eb3c 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > > ControlInfoMap::Map controls; > > > > if (dewarper_) { > > - std::pair<Rectangle, Rectangle> cropLimits = > > - dewarper_->inputCropBounds(&data->mainPathStream_); > > + std::pair<Rectangle, Rectangle> cropLimits; > > + if (dewarper_->isConfigured(&data->mainPathStream_)) > > + cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); > > + else > > + cropLimits = dewarper_->inputCropBounds(); > > Now I wonder if the right way shouldn't have been > > /** > * \fn Converter::inputCropBounds(const Stream *stream) > * \brief Retrieve the crop bounds for \a stream > * \param[in] stream The output stream > > * .... > > - * > - * When called with an unconfigured \a stream, this function returns a pair of > - * null rectangles. > + * When called with an unconfigured \a stream, this function returns > + * the coverter default crop rectangle > + * That is exactly the version from v2 - righti? :-) > > So that you could even skip this patch completely (and probably the > isConfigured() one too). > > Generally, I don't like API that have too many implicit behaviours and > prefer the ones where the caller has to chose the right overload to > call, so that the logic is clear in the calling place and is harder to > mis-uses, hence I might be biased. I'll let you decide really, I have already > pulled you in too many directions enough. I liked the version from v2 better at first. Now that the explicit one is there I actually like it because a) You get an error if you pass a non configured stream and b) it is clearer from the using code that different things can happen there. In the end both version would do the job. If no one else objects with a good argument I'd leave it like it is now. Cheers, Stefan > > > > > controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > > cropLimits.second, > > -- > > 2.43.0 > >
On Mon, Dec 16, 2024 at 04:40:50PM +0100, Stefan Klug wrote: > Query the crop bounds on the dewarper instead of the stream in case the > camera was not yet configured when updateControls() gets called. This > provides sane defaults for the controls. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v4: > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds > support > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 89946b782854..56192451eb3c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > ControlInfoMap::Map controls; > > if (dewarper_) { > - std::pair<Rectangle, Rectangle> cropLimits = > - dewarper_->inputCropBounds(&data->mainPathStream_); > + std::pair<Rectangle, Rectangle> cropLimits; > + if (dewarper_->isConfigured(&data->mainPathStream_)) > + cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); > + else > + cropLimits = dewarper_->inputCropBounds(); Ah that's how it comes together, neat. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > cropLimits.second, > -- > 2.43.0 >
Hi Stefan On Mon, Dec 16, 2024 at 09:47:16PM +0100, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the review. > > On Mon, Dec 16, 2024 at 07:14:05PM +0100, Jacopo Mondi wrote: > > Hi Stefan > > > > On Mon, Dec 16, 2024 at 04:40:50PM +0100, Stefan Klug wrote: > > > Query the crop bounds on the dewarper instead of the stream in case the > > > camera was not yet configured when updateControls() gets called. This > > > provides sane defaults for the controls. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > > --- > > > > > > Changes in v4: > > > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds > > > support > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 89946b782854..56192451eb3c 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > > > ControlInfoMap::Map controls; > > > > > > if (dewarper_) { > > > - std::pair<Rectangle, Rectangle> cropLimits = > > > - dewarper_->inputCropBounds(&data->mainPathStream_); > > > + std::pair<Rectangle, Rectangle> cropLimits; > > > + if (dewarper_->isConfigured(&data->mainPathStream_)) > > > + cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); > > > + else > > > + cropLimits = dewarper_->inputCropBounds(); > > > > Now I wonder if the right way shouldn't have been > > > > /** > > * \fn Converter::inputCropBounds(const Stream *stream) > > * \brief Retrieve the crop bounds for \a stream > > * \param[in] stream The output stream > > > > * .... > > > > - * > > - * When called with an unconfigured \a stream, this function returns a pair of > > - * null rectangles. > > + * When called with an unconfigured \a stream, this function returns > > + * the coverter default crop rectangle > > + * > > That is exactly the version from v2 - righti? :-) > > > > > So that you could even skip this patch completely (and probably the > > isConfigured() one too). > > > > Generally, I don't like API that have too many implicit behaviours and > > prefer the ones where the caller has to chose the right overload to > > call, so that the logic is clear in the calling place and is harder to > > mis-uses, hence I might be biased. I'll let you decide really, I have already > > pulled you in too many directions enough. > > I liked the version from v2 better at first. Now that the explicit one > is there I actually like it because a) You get an error if you pass a > non configured stream and b) it is clearer from the using code that > different things can happen there. In the end both version would do the > job. If no one else objects with a good argument I'd leave it like it is > now. Fine with me of course. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > Cheers, > Stefan > > > > > > > > > controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > > > cropLimits.second, > > > -- > > > 2.43.0 > > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 89946b782854..56192451eb3c 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) ControlInfoMap::Map controls; if (dewarper_) { - std::pair<Rectangle, Rectangle> cropLimits = - dewarper_->inputCropBounds(&data->mainPathStream_); + std::pair<Rectangle, Rectangle> cropLimits; + if (dewarper_->isConfigured(&data->mainPathStream_)) + cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); + else + cropLimits = dewarper_->inputCropBounds(); controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, cropLimits.second,
Query the crop bounds on the dewarper instead of the stream in case the camera was not yet configured when updateControls() gets called. This provides sane defaults for the controls. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v4: - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds support --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)