[{"id":27544,"web_url":"https://patchwork.libcamera.org/comment/27544/","msgid":"","date":"2023-07-12T11:00:59","subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: rpi: pipeline_base:\n\tMove findBestFormat to CameraData","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for your work!\n\nOn Wed, 12 Jul 2023 at 11:55, Naushir Patuck wrote:\n>\n> From: Jacopo Mondi \n>\n> The findBestFormat() helper operates on the list of sensor formats,\n> which is owned by the CameraData class. Move the function to that class\n> as well to:\n>\n> 1) Avoid passing the list of formats to the function\n> 2) Remove a static helper in favour of a class function\n> 3) Allow subclasses with access to CameraData to call the function\n>\n> Move to the CameraData class the scoreFormat helper as well.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 140 ++++++++++--------\n> .../pipeline/rpi/common/pipeline_base.h | 3 +\n> 2 files changed, 79 insertions(+), 64 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index fb3756a47590..725c1db0d527 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -74,63 +74,6 @@ bool isMonoSensor(std::unique_ptr &sensor)\n> return bayer.order == BayerFormat::Order::MONO;\n> }\n>\n> -double scoreFormat(double desired, double actual)\n> -{\n> - double score = desired - actual;\n> - /* Smaller desired dimensions are preferred. */\n> - if (score < 0.0)\n> - score = (-score) / 8;\n> - /* Penalise non-exact matches. */\n> - if (actual != desired)\n> - score *= 2;\n> -\n> - return score;\n> -}\n> -\n> -V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req, unsigned int bitDepth)\n> -{\n> - double bestScore = std::numeric_limits::max(), score;\n> - V4L2SubdeviceFormat bestFormat;\n> - bestFormat.colorSpace = ColorSpace::Raw;\n> -\n> - constexpr float penaltyAr = 1500.0;\n> - constexpr float penaltyBitDepth = 500.0;\n> -\n> - /* Calculate the closest/best mode from the user requested size. */\n> - for (const auto &iter : formatsMap) {\n> - const unsigned int mbusCode = iter.first;\n> - const PixelFormat format = mbusCodeToPixelFormat(mbusCode,\n> - BayerFormat::Packing::None);\n> - const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> -\n> - for (const Size &size : iter.second) {\n> - double reqAr = static_cast(req.width) / req.height;\n> - double fmtAr = static_cast(size.width) / size.height;\n> -\n> - /* Score the dimensions for closeness. */\n> - score = scoreFormat(req.width, size.width);\n> - score += scoreFormat(req.height, size.height);\n> - score += penaltyAr * scoreFormat(reqAr, fmtAr);\n> -\n> - /* Add any penalties... this is not an exact science! */\n> - score += utils::abs_diff(info.bitsPerPixel, bitDepth) * penaltyBitDepth;\n> -\n> - if (score <= bestScore) {\n> - bestScore = score;\n> - bestFormat.mbus_code = mbusCode;\n> - bestFormat.size = size;\n> - }\n> -\n> - LOG(RPI, Debug) << \"Format: \" << size\n> - << \" fmt \" << format\n> - << \" Score: \" << score\n> - << \" (best \" << bestScore << \")\";\n> - }\n> - }\n> -\n> - return bestFormat;\n> -}\n> -\n> const std::vector validColorSpaces = {\n> ColorSpace::Sycc,\n> ColorSpace::Smpte170m,\n> @@ -265,6 +208,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n>\n> /* Do any platform specific fixups. */\n> + unsigned int bitDepth = defaultRawBitDepth;\n> + if (!rawStreams.empty()) {\n> + BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> + bitDepth = bayerFormat.bitDepth;\n> + }\n> +\n> + V4L2SubdeviceFormat sensorFormat =\n> + data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> + : rawStreams[0].cfg->size,\n> + bitDepth);\n> +\n> status = data_->platformValidate(rawStreams, outStreams);\n> if (status == Invalid)\n> return Invalid;\n> @@ -275,8 +229,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> V4L2DeviceFormat rawFormat;\n>\n> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> - unsigned int bitDepth = info.isValid() ? info.bitsPerPixel : defaultRawBitDepth;\n> - V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size, bitDepth);\n> + bitDepth = info.isValid() ? info.bitsPerPixel : defaultRawBitDepth;\n> + sensorFormat = data_->findBestFormat(cfg.size, bitDepth);\n\nI think the above lines can now be removed as sensorFormat is generated above,\nand ought to be the same as if it were computed in this loop. A brief test with\nthis removed seems to confirm this.\n\n>\n> BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;\n> rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat, packing);\n> @@ -391,7 +345,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span switch (role) {\n> case StreamRole::Raw:\n> size = sensorSize;\n> - sensorFormat = findBestFormat(data->sensorFormats_, size, defaultRawBitDepth);\n> + sensorFormat = data->findBestFormat(size, defaultRawBitDepth);\n> pixelFormat = mbusCodeToPixelFormat(sensorFormat.mbus_code,\n> BayerFormat::Packing::CSI2);\n> ASSERT(pixelFormat.isValid());\n> @@ -531,10 +485,11 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> bitDepth = bayerFormat.bitDepth;\n> }\n>\n> - V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_,\n> - rawStreams.empty() ? ispStreams[0].cfg->size\n> - : rawStreams[0].cfg->size,\n> - bitDepth);\n> + V4L2SubdeviceFormat sensorFormat =\n> + data->findBestFormat(rawStreams.empty() ? ispStreams[0].cfg->size\n> + : rawStreams[0].cfg->size,\n> + bitDepth);\n> +\n> /* Apply any cached transform. */\n> const RPiCameraConfiguration *rpiConfig = static_cast(config);\n>\n> @@ -954,6 +909,63 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> return 0;\n> }\n>\n> +double CameraData::scoreFormat(double desired, double actual) const\n> +{\n> + double score = desired - actual;\n> + /* Smaller desired dimensions are preferred. */\n> + if (score < 0.0)\n> + score = (-score) / 8;\n> + /* Penalise non-exact matches. */\n> + if (actual != desired)\n> + score *= 2;\n> +\n> + return score;\n> +}\n\nThis does not need to be a member function, but I'm not too bothered either way.\n\nApart from clarification of the sensorFormat bit above,\n\nReviewed-by: Naushir Patuck \n\nRegards,\nNaush\n\n> +\n> +V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bitDepth) const\n> +{\n> + double bestScore = std::numeric_limits::max(), score;\n> + V4L2SubdeviceFormat bestFormat;\n> + bestFormat.colorSpace = ColorSpace::Raw;\n> +\n> + constexpr float penaltyAr = 1500.0;\n> + constexpr float penaltyBitDepth = 500.0;\n> +\n> + /* Calculate the closest/best mode from the user requested size. */\n> + for (const auto &iter : sensorFormats_) {\n> + const unsigned int mbusCode = iter.first;\n> + const PixelFormat format = mbusCodeToPixelFormat(mbusCode,\n> + BayerFormat::Packing::None);\n> + const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> + for (const Size &size : iter.second) {\n> + double reqAr = static_cast(req.width) / req.height;\n> + double fmtAr = static_cast(size.width) / size.height;\n> +\n> + /* Score the dimensions for closeness. */\n> + score = scoreFormat(req.width, size.width);\n> + score += scoreFormat(req.height, size.height);\n> + score += penaltyAr * scoreFormat(reqAr, fmtAr);\n> +\n> + /* Add any penalties... this is not an exact science! */\n> + score += utils::abs_diff(info.bitsPerPixel, bitDepth) * penaltyBitDepth;\n> +\n> + if (score <= bestScore) {\n> + bestScore = score;\n> + bestFormat.mbus_code = mbusCode;\n> + bestFormat.size = size;\n> + }\n> +\n> + LOG(RPI, Debug) << \"Format: \" << size\n> + << \" fmt \" << format\n> + << \" Score: \" << score\n> + << \" (best \" << bestScore << \")\";\n> + }\n> + }\n> +\n> + return bestFormat;\n> +}\n> +\n> void CameraData::freeBuffers()\n> {\n> if (ipa_) {\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> index f648e81054bb..2eda3cd89812 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> @@ -81,6 +81,9 @@ public:\n> virtual void platformStart() = 0;\n> virtual void platformStop() = 0;\n>\n> + double scoreFormat(double desired, double actual) const;\n> + V4L2SubdeviceFormat findBestFormat(const Size &req, unsigned int bitDepth) const;\n> +\n> void freeBuffers();\n> virtual void platformFreeBuffers() = 0;\n>\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 78217BDC71\n\tfor ;\n\tWed, 12 Jul 2023 11:01:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF499628BC;\n\tWed, 12 Jul 2023 13:01:17 +0200 (CEST)","from mail-oa1-x36.google.com (mail-oa1-x36.google.com\n\t[IPv6:2001:4860:4864:20::36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B166628BB\n\tfor ;\n\tWed, 12 Jul 2023 13:01:16 +0200 (CEST)","by mail-oa1-x36.google.com with SMTP id\n\t586e51a60fabf-1b0138963ffso6206918fac.0\n\tfor ;\n\tWed, 12 Jul 2023 04:01:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689159677;\n\tbh=vNNYy9hr1fh/b/A2ElpQ0Fz/RYPztCg6iZMegpAkY4Q=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XW6VH8wmT34hkGX0SVgH0PARUFXCGiHk/15kkpq1OtNQ6lYE+aC6hXsPxdY1Mz668\n\tapCuRKDXuWH24geoHW6APSfly7dOk/OIYq8AS2Nhsdf50MBzlZPhgy+LUqoIAZ06CN\n\tfmGvoab612kXINsWQOcbwUGAE+IvqIbgn0guqT+RWA+M8Zn7FLneuCEittIljS7aTZ\n\tfEFpqo44X/LE3fmObgSTAG5kWTq2fZvxcvJleDmZ/7UePRD9O7pUJ7URL4Dmb31d4e\n\tU5wLn/anrWyeg5wWWV9/oUvJururNv4U4HUosxQ4aEB3+OEkiRili0yy6VXN2GYy/G\n\tWMpL7cOi8QV1A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689159674; x=1691751674;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FXECmAkHwLjriaT18Q5L4yTMQQn65S6F3tV/WBKru4s=;\n\tb=nGUmMLfq37OCHEG4ano1lgZiKcp5+Jqpu039ybTv4CuM02R2TiHQW/r2AD8IPvIhDF\n\tAbZbe17mnXvp3uSdk3UWOyh+f36THis9YsJowW+fs/JNqdTkq79N7Z3N/dSukmp8sQ4a\n\t7Jqcz6PGsGYRJP3clkFuWJNB6v8wnNnHgOqnkoZKx7/n5ztuVpHAsPz9MKFHYzI++B35\n\tco1DAtFmXsHpowrSXfC3qDJO3vTe6hps61HkgYLbQBaQch2wQiTRfEw5/z5fQaC5gZIY\n\t3Tdut3azH1G/wz5Bz5NDlKMVAHwp6hstKIIDH/b/HiX2BqqhpoHmhBwitIvey56jHOUi\n\tw4yg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nGUmMLfq\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689159674; x=1691751674;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=FXECmAkHwLjriaT18Q5L4yTMQQn65S6F3tV/WBKru4s=;\n\tb=T3aTXbU0FrSDNgsVPkAghucrd/28504+F4pEMPeaZJO2zuwa5J2tVC9uPH06eOXlLX\n\thOgMbBiZcwHKVk3YEe7ueCpSb+roSnp6t3FNR0V9TViMgsdDIgcOSTzh69fRepRAp+dI\n\twwsCMX6hJuG904vKukRSSEvBa1sv6BgjdMf6kOekkKAX5ixywHFBDG0vQfrhQYdExdL6\n\tXJbFnfutSzVcQ2BsoFlDETbQ55VrVlAQh9jD9/BThtnrgxb044elY4SaCzNFAp0rQY/u\n\tDaDnLE3UIuJyOexHPJPSzRPjpFXDIn8bgIHws0v2t182UKTLxYzO7y9IHK/HtFsXxw++\n\t/rog==","X-Gm-Message-State":"ABy/qLY7mNc+GEf5F3/ARSd8UHck3wt+VZ97QKrvVU75Z5K3TI6n9Itt\n\t2st8B88Zn3Py1KqjKget0KQLGRp43+xgrgUTLHekMe6Af0yGnucfRSNDnw==","X-Google-Smtp-Source":"APBJJlH9naQhqi8ra1zVMHgp0dBTVJBX2Q21keqmbJjH0/K11MrsH0RQeE4SUlVvlKmHKu5FLNitFJ0CLYyYK7YUTjM=","X-Received":"by 2002:a05:6870:d7a3:b0:1b7:60a3:42c3 with SMTP id\n\tbd35-20020a056870d7a300b001b760a342c3mr4183253oab.20.1689159674406;\n\tWed, 12 Jul 2023 04:01:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20230712105510.14658-1-naush@raspberrypi.com>\n\t<20230712105510.14658-3-naush@raspberrypi.com>","In-Reply-To":"<20230712105510.14658-3-naush@raspberrypi.com>","Date":"Wed, 12 Jul 2023 12:00:59 +0100","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: rpi: pipeline_base:\n\tMove findBestFormat to CameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Jacopo Mondi ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]