[libcamera-devel] [PATCH v3 11/13] pipeline: rkisp1: Fix stream size validation
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 26 17:40:31 CEST 2022
Hi Laurent
On Mon, Oct 24, 2022 at 03:03:54AM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> ---
> 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);
> + 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