[libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1: Add format validation for self path
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 20 12:12:25 CEST 2020
Hi Niklas,
On Thu, Aug 13, 2020 at 02:52:45AM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both man and self path. The
> heuristics honors that the first stream in the configuration have the
> highest priority while still examining both streams for a best match.
>
> This change extends the formats supported by the Cameras backed by this
> pipeline to also include RGB888 and RGB656, that is only available on
> the self path.
>
> It is not possible to capture from the self path as the self stream is
> not yet exposed to applications.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 220 ++++++++++++++++++-----
> 1 file changed, 171 insertions(+), 49 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3761b7ef7a9386e3..f106b105a47bb4f7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -9,6 +9,7 @@
> #include <array>
> #include <iomanip>
> #include <memory>
> +#include <numeric>
> #include <queue>
>
> #include <linux/media-bus-format.h>
> @@ -50,6 +51,20 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> /* \todo Add support for 8-bit greyscale to DRM formats */
> };
>
> +static const Size RKISP1_RSZ_SP_SRC_MIN = { 32, 16 };
> +static const Size RKISP1_RSZ_SP_SRC_MAX = { 1920, 1920 };
> +static const std::array<PixelFormat, 9> RKISP1_RSZ_SP_FORMATS{
same comments as in previous patches.
Considering how you have to transport the template in the validatePath
function, I would use vectors here for the formats enumeration.
> + formats::YUYV,
> + formats::YVYU,
> + formats::VYUY,
> + formats::NV16,
> + formats::NV61,
> + formats::NV21,
> + formats::NV12,
> + formats::BGR888,
> + formats::RGB565,
> +};
> +
> class PipelineHandlerRkISP1;
> class RkISP1ActionQueueBuffers;
> class RkISP1CameraData;
> @@ -179,13 +194,23 @@ public:
> private:
> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>
> + bool fitAnyPath(const StreamConfiguration &cfg);
> +
> + template<std::size_t N>
> + CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> + const std::array<PixelFormat, N> &formats,
A vector would be much nicer here.
> + const Size &min, const Size &max,
> + V4L2VideoDevice *video);
> + CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> + CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> +
> /*
> * The RkISP1CameraData instance is guaranteed to be valid as long as the
> * corresponding Camera instance is valid. In order to borrow a
> * reference to the camera data, store a new reference to the camera.
> */
> std::shared_ptr<Camera> camera_;
> - const RkISP1CameraData *data_;
> + RkISP1CameraData *data_;
>
> V4L2SubdeviceFormat sensorFormat_;
> };
> @@ -495,6 +520,76 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)
> +{
> + if (std::find(RKISP1_RSZ_MP_FORMATS.begin(),
> + RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==
> + RKISP1_RSZ_MP_FORMATS.end())
> + return false;
> +
> + if (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)
> + return false;
> +
> + if (std::find(RKISP1_RSZ_SP_FORMATS.begin(),
> + RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==
> + RKISP1_RSZ_SP_FORMATS.end())
> + return false;
> +
> + if (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)
> + return false;
> +
> + return true;
> +}
> +
> +template<std::size_t N>
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> + StreamConfiguration *cfg, const std::array<PixelFormat, N> &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;
Do we need to go to the video device here ? We know the format/sizes
are supported already...
> +
> + 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_->mainPathVideo_);
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> +{
> + return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> + RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> +}
> +
> CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> {
> const CameraSensor *sensor = data_->sensor_;
> @@ -504,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> return Invalid;
>
> /* Cap the number of entries to the available streams. */
> - if (config_.size() > 1) {
> - config_.resize(1);
> + if (config_.size() > 2) {
> + config_.resize(2);
> status = Adjusted;
> }
>
> - StreamConfiguration &cfg = config_[0];
> -
> - /* Adjust the pixel format. */
> - if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> - cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> - LOG(RkISP1, Debug) << "Adjusting format to NV12";
> - cfg.pixelFormat = formats::NV12,
> - status = Adjusted;
> + /*
> + * If there are more then one stream in the configuration figure out the
> + * order to evaluate them streams. The first stream have the highest
> + * priority but if both main path and self path can satisfy it evaluate
> + * second stream first.
> + */
> + std::vector<unsigned int> order(config_.size());
> + std::iota(order.begin(), order.end(), 0);
> + if (config_.size() == 2 && fitAnyPath(config_[0]))
> + std::reverse(order.begin(), order.end());
Nice, but -why- ? What are the criteria you use to decide which
configu gets to which stream ? In example, I see the self path being
limited in sizes compared to the main, shouldn't this be taken into
account ? Same for the supported formats. How to you establish the
here reported 'order' ?
> +
> + bool mainPathAvailable = true;
> + bool selfPathAvailable = true;
> + for (unsigned int index : order) {
> + StreamConfiguration &cfg = config_[index];
> +
> + /* Try to match stream without adjusting configuration. */
> + if (mainPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateMainPath(&tryCfg) == Valid) {
> + mainPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(&data_->mainPathStream_);
> + LOG(RkISP1, Debug) << "Exact match main";
> + continue;
> + }
> + }
> +
> + if (selfPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateSelfPath(&tryCfg) == Valid) {
> + selfPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(&data_->selfPathStream_);
> + LOG(RkISP1, Debug) << "Exact match self";
> + continue;
> + }
> + }
> +
> + /* Try to match stream allowing adjusting configuration. */
> + if (mainPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateMainPath(&tryCfg) == Adjusted) {
> + mainPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(&data_->mainPathStream_);
> + status = Adjusted;
> + LOG(RkISP1, Debug) << "Adjust match main";
> + continue;
> + }
> + }
> +
> + if (selfPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateSelfPath(&tryCfg) == Adjusted) {
Can't you get the status from here instead of repeating the check for
each stream 2 times ? You would only need to return Invalid if you
detect it, otherwise assign the value of validate*Path() to status.
The only other reason I see here to keep 4 if switches is to printout
the result, but that could indeed be done depending on the returned
status.
> + selfPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(&data_->selfPathStream_);
> + status = Adjusted;
> + LOG(RkISP1, Debug) << "Adjust match self";
> + continue;
> + }
> + }
> +
> + /* All paths rejected configuraiton. */
> + LOG(RkISP1, Debug) << "Camera configuration not supported "
> + << cfg.toString();
> + return Invalid;
> }
>
> /* Select the sensor format. */
> + Size maxSize;
> + for (const StreamConfiguration &cfg : config_)
> + maxSize = std::max(maxSize, cfg.size);
> +
> sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> MEDIA_BUS_FMT_SGBRG12_1X12,
> MEDIA_BUS_FMT_SGRBG12_1X12,
> @@ -532,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> MEDIA_BUS_FMT_SGBRG8_1X8,
> MEDIA_BUS_FMT_SGRBG8_1X8,
> MEDIA_BUS_FMT_SRGGB8_1X8 },
> - cfg.size);
> + maxSize);
> if (sensorFormat_.size.isNull())
> sensorFormat_.size = sensor->resolution();
>
> - /*
> - * Provide a suitable default that matches the sensor aspect
> - * ratio and clamp the size to the hardware bounds.
> - *
> - * \todo: Check the hardware alignment constraints.
> - */
> - const Size size = cfg.size;
> -
> - if (cfg.size.isNull()) {
> - cfg.size.width = 1280;
> - cfg.size.height = 1280 * sensorFormat_.size.height
> - / sensorFormat_.size.width;
> - }
> -
> - cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> - cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> -
> - if (cfg.size != size) {
> - LOG(RkISP1, Debug)
> - << "Adjusting size from " << size.toString()
> - << " to " << cfg.size.toString();
> - status = Adjusted;
> - }
> -
> - cfg.bufferCount = RKISP1_BUFFER_COUNT;
> -
> - V4L2DeviceFormat format = {};
> - format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> - format.size = cfg.size;
> -
> - int ret = data_->mainPathVideo_->tryFormat(&format);
> - if (ret)
> - return Invalid;
> -
> - cfg.stride = format.planes[0].bpl;
> - cfg.frameSize = format.planes[0].size;
> -
> return status;
> }
>
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list