[libcamera-devel] [PATCH v3 17/22] libcamera: pipeline: rkisp1: Move path configuration generation and validation to RkISP1Path
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 29 00:39:28 CEST 2020
Hi Niklas,
Thank you for the patch.
On Fri, Sep 25, 2020 at 03:42:02AM +0200, Niklas Söderlund wrote:
> Move the path configuration generation and validation to RkISP1Path.
> This is done to increase code reuse and to encapsulate the main and self
> path differences inside the RkISP1Path class. There is no functional
> change.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 115 ++-----------------
> src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 88 +++++++++++++-
> src/libcamera/pipeline/rkisp1/rkisp1path.h | 18 ++-
> 3 files changed, 112 insertions(+), 109 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2403eec4691b5ff6..114aee3e180afb77 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -20,7 +20,6 @@
> #include <libcamera/formats.h>
> #include <libcamera/ipa/rkisp1.h>
> #include <libcamera/request.h>
> -#include <libcamera/span.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/camera_sensor.h"
> @@ -40,34 +39,6 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(RkISP1)
>
> -namespace {
> -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
> -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> -constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> - formats::YUYV,
> - formats::YVYU,
> - formats::VYUY,
> - formats::NV16,
> - formats::NV61,
> - formats::NV21,
> - formats::NV12,
> - /* \todo Add support for 8-bit greyscale to DRM formats */
> -};
> -
> -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> -constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> - formats::YUYV,
> - formats::YVYU,
> - formats::VYUY,
> - formats::NV16,
> - formats::NV61,
> - formats::NV21,
> - formats::NV12,
> - /* \todo Add support for BGR888 and RGB565 */
> -};
> -} /* namespace */
> -
> class PipelineHandlerRkISP1;
> class RkISP1ActionQueueBuffers;
> class RkISP1CameraData;
> @@ -194,14 +165,6 @@ public:
> const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
>
> private:
> - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -
> - CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> - const Span<const PixelFormat> &formats,
> - const Size &min, const Size &max,
> - V4L2VideoDevice *video);
> - CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> - CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> bool fitsAllPaths(const StreamConfiguration &cfg);
>
> /*
> @@ -514,64 +477,16 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> -CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> - StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> - const Size &min, const Size &max, V4L2VideoDevice *video)
> -{
> - const StreamConfiguration reqCfg = *cfg;
> - Status status = Valid;
> -
> - if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> - formats.end())
> - cfg->pixelFormat = formats::NV12;
> -
> - cfg->size.boundTo(max);
> - cfg->size.expandTo(min);
> - cfg->bufferCount = RKISP1_BUFFER_COUNT;
> -
> - V4L2DeviceFormat format = {};
> - format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> - format.size = cfg->size;
> -
> - int ret = video->tryFormat(&format);
> - if (ret)
> - return Invalid;
> -
> - cfg->stride = format.planes[0].bpl;
> - cfg->frameSize = format.planes[0].size;
> -
> - if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> - LOG(RkISP1, Debug)
> - << "Adjusting format from " << reqCfg.toString()
> - << " to " << cfg->toString();
> - status = Adjusted;
> - }
> -
> - return status;
> -}
> -
> -CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> -{
> - return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> - RKISP1_RSZ_MP_SRC_MAX, data_->mainPath_->video_);
> -}
> -
> -CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> -{
> - return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> - RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);
> -}
> -
> bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> {
> StreamConfiguration config;
>
> config = cfg;
> - if (validateMainPath(&config) != Valid)
> + if (data_->mainPath_->validate(&config) != Valid)
> return false;
>
> config = cfg;
> - if (validateSelfPath(&config) != Valid)
> + if (data_->selfPath_->validate(&config) != Valid)
> return false;
>
> return true;
> @@ -611,7 +526,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> /* Try to match stream without adjusting configuration. */
> if (mainPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (validateMainPath(&tryCfg) == Valid) {
> + if (data_->mainPath_->validate(&tryCfg) == Valid) {
> mainPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -622,7 +537,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> if (selfPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (validateSelfPath(&tryCfg) == Valid) {
> + if (data_->selfPath_->validate(&tryCfg) == Valid) {
> selfPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -634,7 +549,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> /* Try to match stream allowing adjusting configuration. */
> if (mainPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (validateMainPath(&tryCfg) == Adjusted) {
> + if (data_->mainPath_->validate(&tryCfg) == Adjusted) {
> mainPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -646,7 +561,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> if (selfPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (validateSelfPath(&tryCfg) == Adjusted) {
> + if (data_->selfPath_->validate(&tryCfg) == Adjusted) {
> selfPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -734,25 +649,17 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> return nullptr;
> }
>
> - std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> + StreamConfiguration cfg;
> if (useMainPath) {
> + cfg = data->mainPath_->generateConfiguration(
> + data->sensor_->resolution());
> mainPathAvailable = false;
> - for (const PixelFormat &format : RKISP1_RSZ_MP_FORMATS)
> - streamFormats[format] =
> - { { RKISP1_RSZ_MP_SRC_MIN, data->sensor_->resolution() } };
> } else {
> + cfg = data->selfPath_->generateConfiguration(
> + data->sensor_->resolution());
> selfPathAvailable = false;
> - for (const PixelFormat &format : RKISP1_RSZ_SP_FORMATS)
> - streamFormats[format] =
> - { { RKISP1_RSZ_SP_SRC_MIN, data->sensor_->resolution() } };
> }
>
> - StreamFormats formats(streamFormats);
> - StreamConfiguration cfg(formats);
> - cfg.pixelFormat = formats::NV12;
> - cfg.size = data->sensor_->resolution();
> - cfg.bufferCount = 4;
> -
> config->addConfiguration(cfg);
> }
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> index 758580934817ed6a..0be4d20bb1cd2094 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> @@ -7,6 +7,7 @@
>
> #include "rkisp1path.h"
>
> +#include <libcamera/formats.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/media_device.h"
> @@ -17,8 +18,11 @@ namespace libcamera {
>
> LOG_DECLARE_CATEGORY(RkISP1)
>
> -RkISP1Path::RkISP1Path(const char *name)
> - : video_(nullptr), name_(name), resizer_(nullptr)
> +RkISP1Path::RkISP1Path(const char *name, const std::vector<PixelFormat> &formats,
> + const Size &minResolution, const Size &maxResolution)
> + : video_(nullptr), name_(name), formats_(formats),
> + minResolution_(minResolution), maxResolution_(maxResolution),
> + resizer_(nullptr)
> {
> }
>
> @@ -44,6 +48,58 @@ bool RkISP1Path::init(MediaDevice *media)
> return true;
> }
>
> +StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> +{
> + Size maxResolution = resolution;
> + maxResolution.boundTo(maxResolution_);
> +
> + std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> + for (const PixelFormat &format : formats_)
> + streamFormats[format] = { { minResolution_, maxResolution } };
> +
> + StreamFormats formats(streamFormats);
> + StreamConfiguration cfg(formats);
> + cfg.pixelFormat = formats::NV12;
> + cfg.size = maxResolution;
> + cfg.bufferCount = RKISP1_BUFFER_COUNT;
> +
> + return cfg;
> +}
> +
> +CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
> +{
> + const StreamConfiguration reqCfg = *cfg;
> + CameraConfiguration::Status status = CameraConfiguration::Valid;
> +
> + if (std::find(formats_.begin(), formats_.end(), cfg->pixelFormat) ==
> + formats_.end())
> + cfg->pixelFormat = formats::NV12;
> +
> + cfg->size.boundTo(maxResolution_);
> + cfg->size.expandTo(minResolution_);
> + cfg->bufferCount = RKISP1_BUFFER_COUNT;
> +
> + V4L2DeviceFormat format = {};
> + format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> + format.size = cfg->size;
> +
> + int ret = video_->tryFormat(&format);
> + if (ret)
> + return CameraConfiguration::Invalid;
> +
> + cfg->stride = format.planes[0].bpl;
> + cfg->frameSize = format.planes[0].size;
> +
> + if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> + LOG(RkISP1, Debug)
> + << "Adjusting format from " << reqCfg.toString()
> + << " to " << cfg->toString();
> + status = CameraConfiguration::Adjusted;
> + }
> +
> + return status;
> +}
> +
> int RkISP1Path::configure(const StreamConfiguration &config,
> const V4L2SubdeviceFormat &inputFormat)
> {
> @@ -94,12 +150,36 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> }
>
> RkISP1MainPath::RkISP1MainPath()
> - : RkISP1Path("main")
> + : RkISP1Path("main",
> + {
> + formats::YUYV,
> + formats::YVYU,
> + formats::VYUY,
> + formats::NV16,
> + formats::NV61,
> + formats::NV21,
> + formats::NV12,
> + /* \todo Add support for 8-bit greyscale to DRM formats */
> + },
> + { 32, 16 },
> + { 4416, 3312 })
> {
> }
>
> RkISP1SelfPath::RkISP1SelfPath()
> - : RkISP1Path("self")
> + : RkISP1Path("self",
> + {
> + formats::YUYV,
> + formats::YVYU,
> + formats::VYUY,
> + formats::NV16,
> + formats::NV61,
> + formats::NV21,
> + formats::NV12,
> + /* \todo Add support for BGR888 and RGB565 */
> + },
> + { 32, 16 },
> + { 1920, 1920 })
> {
> }
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> index 6eb01529d2fddb1c..5b2917c746ee1d95 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> @@ -7,9 +7,15 @@
> #ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__
> #define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__
>
> +#include <vector>
> +
> +#include <libcamera/camera.h>
> +
> namespace libcamera {
>
> class MediaDevice;
> +class PixelFormat;
> +class Size;
You need to include the corresponding headers to avoid depending on
indirect includes, as those classes are embedded in RkISP1Path.
> class V4L2Subdevice;
> class V4L2VideoDevice;
> struct StreamConfiguration;
> @@ -18,11 +24,15 @@ struct V4L2SubdeviceFormat;
> class RkISP1Path
> {
> public:
> - RkISP1Path(const char *name);
> + RkISP1Path(const char *name, const std::vector<PixelFormat> &formats,
> + const Size &minResolution, const Size &maxResolution);
> ~RkISP1Path();
>
> bool init(MediaDevice *media);
>
> + StreamConfiguration generateConfiguration(const Size &resolution);
> + CameraConfiguration::Status validate(StreamConfiguration *cfg);
> +
> int configure(const StreamConfiguration &config,
> const V4L2SubdeviceFormat &inputFormat);
>
> @@ -30,8 +40,14 @@ public:
> V4L2VideoDevice *video_;
>
> private:
> + static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> +
> const char *name_;
>
> + const std::vector<PixelFormat> formats_;
Could we keep the formats array as constexpr globals and store a span
here (and of course move them to rkisp1path.cpp) ? I'm tempted to do the
same for the min/max sizes, as that would look less like magic numbers.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + const Size minResolution_;
> + const Size maxResolution_;
> +
> V4L2Subdevice *resizer_;
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list