[libcamera-devel] [PATCH v3 11/13] pipeline: rkisp1: Fix stream size validation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 30 18:24:49 CET 2022
Hi Kieran,
On Fri, Oct 28, 2022 at 11:11:06PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54)
> > Unlike RkISP1Path::generateConfiguration(), the validate() function
> > doesn't take the camera sensor resolution into account but only
> > considers the absolute minimum and maximum sizes support by the ISP to
> > validate the stream size. Fix it by using the same logic as when
> > generating the configuration.
> >
> > Instead of passing the sensor resolution to the validate() function,
> > pass the CameraSensor pointer to prepare for subsequent changes that
> > will require access to more camera sensor data. While at it, update the
> > generateConfiguration() function similarly for the same reason.
>
> I have quite the surprising curveball on this patch.
>
> While the validation seems to aim to restrict sizes to the capabilities
> of the ISP and the Sensor size, it seems to miss something.
>
> I've hooked up an Arducam 16MP to test this - and the set up fails
> because the sensor size selected is larger than the maximum input size
> of the ISP.
>
> Can this somehow take the input limtiations of the ISP into account when
> selecting and filtering sensor sizes easily ?
Is this a regression ?
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++-------
> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++----
> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++--
> > 3 files changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index cca89cc13bff..dcab5286aa56 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >
> > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> > {
> > + const CameraSensor *sensor = data_->sensor_.get();
> > StreamConfiguration config;
> >
> > config = cfg;
> > - if (data_->mainPath_->validate(&config) != Valid)
> > + if (data_->mainPath_->validate(sensor, &config) != Valid)
> > return false;
> >
> > config = cfg;
> > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)
> > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)
> > return false;
> >
> > return true;
> > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > /* Try to match stream without adjusting configuration. */
> > if (mainPathAvailable) {
> > StreamConfiguration tryCfg = cfg;
> > - if (data_->mainPath_->validate(&tryCfg) == Valid) {
> > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {
> > mainPathAvailable = false;
> > cfg = tryCfg;
> > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >
> > if (selfPathAvailable) {
> > StreamConfiguration tryCfg = cfg;
> > - if (data_->selfPath_->validate(&tryCfg) == Valid) {
> > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {
> > selfPathAvailable = false;
> > cfg = tryCfg;
> > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > /* Try to match stream allowing adjusting configuration. */
> > if (mainPathAvailable) {
> > StreamConfiguration tryCfg = cfg;
> > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) {
> > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {
> > mainPathAvailable = false;
> > cfg = tryCfg;
> > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >
> > if (selfPathAvailable) {
> > StreamConfiguration tryCfg = cfg;
> > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) {
> > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {
> > selfPathAvailable = false;
> > cfg = tryCfg;
> > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > StreamConfiguration cfg;
> > if (useMainPath) {
> > cfg = data->mainPath_->generateConfiguration(
> > - data->sensor_->resolution());
> > + data->sensor_.get());
> > mainPathAvailable = false;
> > } else {
> > cfg = data->selfPath_->generateConfiguration(
> > - data->sensor_->resolution());
> > + data->sensor_.get());
> > selfPathAvailable = false;
> > }
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 8077a54494a5..cc2ac66e6939 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -12,6 +12,7 @@
> > #include <libcamera/formats.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/media_device.h"
> > #include "libcamera/internal/v4l2_subdevice.h"
> > #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats()
> > }
> > }
> >
> > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)
> > {
> > + const Size &resolution = sensor->resolution();
> > +
> > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > .boundedTo(resolution);
> > Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> > return cfg;
> > }
> >
> > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
> > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > + StreamConfiguration *cfg)
> > {
> > + const Size &resolution = sensor->resolution();
> > +
> > const StreamConfiguration reqCfg = *cfg;
> > CameraConfiguration::Status status = CameraConfiguration::Valid;
> >
> > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
> > if (!streamFormats_.count(cfg->pixelFormat))
> > cfg->pixelFormat = formats::NV12;
> >
> > - cfg->size.boundTo(maxResolution_);
> > - cfg->size.expandTo(minResolution_);
> > + /*
> > + * Adjust the size based on the sensor resolution and absolute limits
> > + * of the ISP.
> > + */
> > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > + .boundedTo(resolution);
>
> I played around here and expected that limiting this maxResolution to
> the maximum Input resolution should help - but I'm missing something and
> haven't delved deep enough to figure out what yet.
>
> > + Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> > +
> > + cfg->size.boundTo(maxResolution);
> > + cfg->size.expandTo(minResolution);
> > cfg->bufferCount = RKISP1_BUFFER_COUNT;
> >
> > V4L2DeviceFormat format;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index d88effbb6f56..bf4ad18fbbf2 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -23,6 +23,7 @@
> >
> > namespace libcamera {
> >
> > +class CameraSensor;
> > class MediaDevice;
> > class V4L2Subdevice;
> > struct StreamConfiguration;
> > @@ -39,8 +40,9 @@ public:
> > int setEnabled(bool enable) { return link_->setEnabled(enable); }
> > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> >
> > - StreamConfiguration generateConfiguration(const Size &resolution);
> > - CameraConfiguration::Status validate(StreamConfiguration *cfg);
> > + StreamConfiguration generateConfiguration(const CameraSensor *sensor);
> > + CameraConfiguration::Status validate(const CameraSensor *sensor,
> > + StreamConfiguration *cfg);
> >
> > int configure(const StreamConfiguration &config,
> > const V4L2SubdeviceFormat &inputFormat);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list