[libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1: Add format validation for self path
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 15 03:32:17 CEST 2020
Hi Niklas,
Thank you for the patch.
On Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both man and self path. The
s/man/main/
s/path/paths/
> heuristics honors that the first stream in the configuration have the
s/have/has/
> highest priority while still examining both streams for a best match.
>
> This change extends the formats supported by the Cameras backed by this
s/Cameras/cameras/
> 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>
> ---
> * Changes since v1
> - Store formats in std::vector instead of std::array to avoid template
> usage for validate function.
As commented (too late) on v1, you don't have to do this, you can keep
an array, and pass a Span<PixelFormat> to validate().
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------
> 1 file changed, 171 insertions(+), 51 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bc961f8e78f2c979..851ff68f138b98dd 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>
> @@ -40,7 +41,7 @@ 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{
> + const std::vector<PixelFormat> RKISP1_RSZ_MP_FORMATS{
> formats::YUYV,
> formats::YVYU,
> formats::VYUY,
> @@ -50,7 +51,21 @@ namespace {
> 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 };
> + const std::vector<PixelFormat> RKISP1_RSZ_SP_FORMATS{
> + formats::YUYV,
> + formats::YVYU,
> + formats::VYUY,
> + formats::NV16,
> + formats::NV61,
> + formats::NV21,
> + formats::NV12,
> + formats::BGR888,
> + formats::RGB565,
> + };
> +};
>
> class PipelineHandlerRkISP1;
> class RkISP1ActionQueueBuffers;
> @@ -181,13 +196,22 @@ public:
> private:
> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>
> + bool fitAnyPath(const StreamConfiguration &cfg);
> +
> + CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> + const std::vector<PixelFormat> &formats,
> + 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_;
This isn't nice :-( I'd like to find a wait to keep it const. Can be
done in a separate patch.
>
> V4L2SubdeviceFormat sensorFormat_;
> };
> @@ -497,6 +521,75 @@ 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;
The implementation looks like a fitsAllPaths(), not any path.
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> + StreamConfiguration *cfg, const std::vector<PixelFormat> &formats,
> + const Size &min, const Size &max, V4L2VideoDevice *video)
formats, min and max are also candidates to be stored in the RkISP1Path
class.
> +{
> + 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_->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_);
> +}
And those two functions could then disappear. Are you really sure you
want to address all that on top ?
> +
> CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> {
> const CameraSensor *sensor = data_->sensor_;
> @@ -506,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
"them streams" ?
s/have/has/
> + * priority but if both main path and self path can satisfy it evaluate
> + * second stream first.
Why ?
> + */
> + 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());
A bit complicated for a vector of two elements :-)
> +
> + 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";
The message is a bit cryptic for someone reading the log without knowing
the code.
> + 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) {
> + selfPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(&data_->selfPathStream_);
> + status = Adjusted;
> + LOG(RkISP1, Debug) << "Adjust match self";
> + continue;
> + }
> + }
There's lots of code duplication, adding a RkISP1Path class would help.
> +
> + /* 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,
> @@ -534,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;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list