[{"id":31547,"web_url":"https://patchwork.libcamera.org/comment/31547/","msgid":"<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","date":"2024-10-03T08:38:26","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-09-30 07:29:44)\n> From: Harvey Yang \n> \n> Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> infrastructure works on devices without using hardware cameras.\n> \n> Signed-off-by: Harvey Yang \n> Reviewed-by: Jacopo Mondi \n> ---\n> meson.build | 1 +\n> meson_options.txt | 3 +-\n> src/libcamera/pipeline/virtual/meson.build | 5 +\n> src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> 5 files changed, 370 insertions(+), 1 deletion(-)\n> create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> \n> diff --git a/meson.build b/meson.build\n> index 63e45465..5e533b0c 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -214,6 +214,7 @@ pipelines_support = {\n> 'simple': ['any'],\n> 'uvcvideo': ['any'],\n> 'vimc': ['test'],\n> + 'virtual': ['test'],\n> }\n> \n> if pipelines.contains('all')\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 7aa41249..c91cd241 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -53,7 +53,8 @@ option('pipelines',\n> 'rpi/vc4',\n> 'simple',\n> 'uvcvideo',\n> - 'vimc'\n> + 'vimc',\n> + 'virtual'\n> ],\n> description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> \n> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> new file mode 100644\n> index 00000000..ada1b335\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_internal_sources += files([\n> + 'virtual.cpp',\n> +])\n> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> new file mode 100644\n> index 00000000..d1584f59\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -0,0 +1,317 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * virtual.cpp - Pipeline handler for virtual cameras\n> + */\n> +\n> +#include \"virtual.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Virtual)\n> +\n> +namespace {\n> +\n> +uint64_t currentTimestamp()\n> +{\n> + const auto now = std::chrono::steady_clock::now();\n> + auto nsecs = std::chrono::duration_cast(\n> + now.time_since_epoch());\n> +\n> + return nsecs.count();\n> +}\n> +\n> +} /* namespace */\n> +\n> +class VirtualCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> + static constexpr unsigned int kBufferCount = 4;\n> +\n> + VirtualCameraConfiguration(VirtualCameraData *data);\n> +\n> + Status validate() override;\n> +\n> +private:\n> + const VirtualCameraData *data_;\n> +};\n> +\n> +class PipelineHandlerVirtual : public PipelineHandler\n> +{\n> +public:\n> + PipelineHandlerVirtual(CameraManager *manager);\n> +\n> + std::unique_ptr generateConfiguration(Camera *camera,\n> + Span roles) override;\n> + int configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> + int exportFrameBuffers(Camera *camera, Stream *stream,\n> + std::vector> *buffers) override;\n> +\n> + int start(Camera *camera, const ControlList *controls) override;\n> + void stopDevice(Camera *camera) override;\n> +\n> + int queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> + bool match(DeviceEnumerator *enumerator) override;\n> +\n> +private:\n> + static bool created_;\n\nCould this be \n\tstatic bool created_ = false;\n\nsaving the need for the outer initialisation below?\n\n> +\n> + VirtualCameraData *cameraData(Camera *camera)\n> + {\n> + return static_cast(camera->_d());\n> + }\n> +\n> + DmaBufAllocator dmaBufAllocator_;\n> +};\n> +\n> +/* static */\n> +bool PipelineHandlerVirtual::created_ = false;\n\nThis stands out as a curious way to do this initialisation, but I\nwouldn't block on this here ...\n\nI'm curious why we 'need' a static created_ and how that will be used\nthough...\n\n\n> +\n> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> + const std::vector &supportedResolutions)\n> + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> +{\n> + for (const auto &resolution : supportedResolutions_) {\n> + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> + minResolutionSize_ = resolution.size;\n> +\n> + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> + }\n> +\n> + /* \\todo Support multiple streams and pass multi_stream_test */\n> + streamConfigs_.resize(kMaxStream);\n> +}\n> +\n> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> + : CameraConfiguration(), data_(data)\n> +{\n> +}\n> +\n> +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> +{\n> + Status status = Valid;\n> +\n> + if (config_.empty()) {\n> + LOG(Virtual, Error) << \"Empty config\";\n> + return Invalid;\n> + }\n> +\n> + /* Only one stream is supported */\n> + if (config_.size() > VirtualCameraData::kMaxStream) {\n> + config_.resize(VirtualCameraData::kMaxStream);\n> + status = Adjusted;\n> + }\n> +\n> + for (StreamConfiguration &cfg : config_) {\n> + bool found = false;\n> + for (const auto &resolution : data_->supportedResolutions_) {\n> + if (resolution.size.width == cfg.size.width &&\n> + resolution.size.height == cfg.size.height) {\n> + found = true;\n> + break;\n> + }\n> + }\n> +\n> + if (!found) {\n> + /*\n> + * \\todo It's a pipeline's decision to choose a\n> + * resolution when the exact one is not supported.\n> + * Defining the default logic in PipelineHandler to\n> + * find the closest resolution would be nice.\n> + */\n> + cfg.size = data_->maxResolutionSize_;\n> + status = Adjusted;\n> + }\n> +\n> + if (cfg.pixelFormat != formats::NV12) {\n> + cfg.pixelFormat = formats::NV12;\n> + LOG(Virtual, Debug)\n> + << \"Stream configuration adjusted to \" << cfg.toString();\n\nWouldn't it be worth reporting this if any adjustment is made?\n\nI'd probably have put this at the end of this scope as \n\t\t\tif (status == Adjusted)\n LOG(Virtual, Info)\n << \"Stream configuration adjusted to \" << cfg.toString();\n\n\nAnd I'd think 'info' would be suitable here over debug - as this is a\ntest / virtual pipeline handler so the additional log would be\nbeneficial IMO.\n\n\n\n> + status = Adjusted;\n> + }\n> +\n> + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> + cfg.stride = info.stride(cfg.size.width, 0, 1);\n> + cfg.frameSize = info.frameSize(cfg.size, 1);\n> +\n> + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> + }\n> +\n> + return status;\n> +}\n> +\n> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> + : PipelineHandler(manager),\n> + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> +{\n> +}\n> +\n> +std::unique_ptr\n> +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> + Span roles)\n> +{\n> + VirtualCameraData *data = cameraData(camera);\n> + auto config = std::make_unique(data);\n> +\n> + if (roles.empty())\n> + return config;\n> +\n> + for (const StreamRole role : roles) {\n> + switch (role) {\n> + case StreamRole::StillCapture:\n> + case StreamRole::VideoRecording:\n> + case StreamRole::Viewfinder:\n> + break;\n> +\n> + case StreamRole::Raw:\n> + default:\n> + LOG(Virtual, Error)\n> + << \"Requested stream role not supported: \" << role;\n> + config.reset();\n> + return config;\n> + }\n> +\n> + std::map> streamFormats;\n> + PixelFormat pixelFormat = formats::NV12;\n> + streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> + data->maxResolutionSize_ } };\n> + StreamFormats formats(streamFormats);\n> + StreamConfiguration cfg(formats);\n> + cfg.pixelFormat = pixelFormat;\n> + cfg.size = data->maxResolutionSize_;\n> + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\n> + config->addConfiguration(cfg);\n> + }\n> +\n> + ASSERT(config->validate() != CameraConfiguration::Invalid);\n> +\n> + return config;\n> +}\n> +\n> +int PipelineHandlerVirtual::configure(Camera *camera,\n> + CameraConfiguration *config)\n> +{\n> + VirtualCameraData *data = cameraData(camera);\n> + for (auto [i, c] : utils::enumerate(*config))\n> + c.setStream(&data->streamConfigs_[i].stream);\n> +\n> + return 0;\n> +}\n> +\n> +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> + Stream *stream,\n> + std::vector> *buffers)\n> +{\n> + if (!dmaBufAllocator_.isValid())\n> + return -ENOBUFS;\n> +\n> + const StreamConfiguration &config = stream->configuration();\n> +\n> + auto info = PixelFormatInfo::info(config.pixelFormat);\n> +\n> + std::vector planeSizes;\n> + for (size_t i = 0; i < info.planes.size(); ++i)\n> + planeSizes.push_back(info.planeSize(config.size, i));\n> +\n> + return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> +}\n> +\n> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> + [[maybe_unused]] const ControlList *controls)\n> +{\n> + return 0;\n> +}\n> +\n> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> +{\n> +}\n> +\n> +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> + Request *request)\n> +{\n> + for (auto it : request->buffers())\n> + completeBuffer(request, it.second);\n> +\n> + request->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> + completeRequest(request);\n> +\n> + return 0;\n> +}\n> +\n> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> +{\n> + if (created_)\n> + return false;\n> +\n> + created_ = true;\n> +\n> + /* \\todo Add virtual cameras according to a config file. */\n> +\n> + std::vector supportedResolutions;\n> + supportedResolutions.resize(2);\n> + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> +\n> + std::unique_ptr data =\n> + std::make_unique(this, supportedResolutions);\n> +\n> + data->properties_.set(properties::Location, properties::CameraLocationFront);\n> + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> +\n> + /* \\todo Set FrameDurationLimits based on config. */\n> + ControlInfoMap::Map controls;\n> + int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> +\n> + /* Create and register the camera. */\n> + std::set streams;\n> + for (auto &streamConfig : data->streamConfigs_)\n> + streams.insert(&streamConfig.stream);\n> +\n> + const std::string id = \"Virtual0\";\n> + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> + registerCamera(std::move(camera));\n> +\n> + return true;\n\nI think if you return false here, you don't need any of the created_; \n\n\t/*\n\t * Ensure we only ever register and create a single virtual\n\t * pipeline handler.\n\t */\n\treturn false;\n\nThis is core libcamera not your patch, - I still dislike this return\nusage of match(). It's either not clear or not fitting well to the needs\nof most pipeline handlers.\n\nIt's should be clearer at core that returning true here simply means\n'please try to create more pipeline handlers'\n\n\n\n\nAnyway, comments above could be applied on top if you wish - or handled\nhow you prefer.\n\nThe created_ just seems like an awkward workaround, but it's a\nworkaround for trying to return the right thing at the end of match. But\nI think the 'right thing' to return there is poorly defined maybe...\n\nAnyway, with any of the above handled or not - or handled on top - none\nof that blocks this patch for me.\n\n\nReviewed-by: Kieran Bingham \n\n\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> new file mode 100644\n> index 00000000..4df70a13\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * virtual.h - Pipeline handler for virtual cameras\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +class VirtualCameraData : public Camera::Private\n> +{\n> +public:\n> + const static unsigned int kMaxStream = 1;\n> +\n> + struct Resolution {\n> + Size size;\n> + std::vector frameRates;\n> + };\n> + struct StreamConfig {\n> + Stream stream;\n> + };\n> +\n> + VirtualCameraData(PipelineHandler *pipe,\n> + const std::vector &supportedResolutions);\n> +\n> + ~VirtualCameraData() = default;\n> +\n> + const std::vector supportedResolutions_;\n> + Size maxResolutionSize_;\n> + Size minResolutionSize_;\n> +\n> + std::vector streamConfigs_;\n> +};\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.46.1.824.gd892dcdcdd-goog\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 6D500BD80A\n\tfor ;\n\tThu, 3 Oct 2024 08:38:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E36B63525;\n\tThu, 3 Oct 2024 10:38:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 781D362C93\n\tfor ;\n\tThu, 3 Oct 2024 10:38:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 218604C7;\n\tThu, 3 Oct 2024 10:36:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i1DPKilX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727944616;\n\tbh=F8LHjSQpZKEXRfW3qfV+J3eaxu/zSySmMCBbVMWcUbs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=i1DPKilX0SiVbv8XVsblL3okvON88L0cNjCpzT/K3G7PmBtN/QX0E7fwBgm8G45yt\n\tgQMv2iLIN6Nn2JEfcjcfegsLyPtYcdsWWlZjNls218s5rueY4PU9x++lBhl+w0591l\n\t72jvvebaAaynfpNT7QQcR7FN6qkYWxPu5zaBUF0o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240930063342.3014837-4-chenghaoyang@google.com>","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tJacopo Mondi ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Oct 2024 09:38:26 +0100","Message-ID":"<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31548,"web_url":"https://patchwork.libcamera.org/comment/31548/","msgid":"<172794568160.1619946.1463380125933844730@ping.linuxembedded.co.uk>","date":"2024-10-03T08:54:41","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2024-10-03 09:38:26)\n> Quoting Harvey Yang (2024-09-30 07:29:44)\n> > From: Harvey Yang \n> > \n> > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > infrastructure works on devices without using hardware cameras.\n> > \n> > Signed-off-by: Harvey Yang \n> > Reviewed-by: Jacopo Mondi \n> > ---\n... snip ...\n\n> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> > +{\n> > + if (created_)\n> > + return false;\n> > +\n> > + created_ = true;\n> > +\n> > + /* \\todo Add virtual cameras according to a config file. */\n> > +\n> > + std::vector supportedResolutions;\n> > + supportedResolutions.resize(2);\n> > + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > +\n> > + std::unique_ptr data =\n> > + std::make_unique(this, supportedResolutions);\n> > +\n> > + data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > +\n> > + /* \\todo Set FrameDurationLimits based on config. */\n> > + ControlInfoMap::Map controls;\n> > + int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > +\n> > + /* Create and register the camera. */\n> > + std::set streams;\n> > + for (auto &streamConfig : data->streamConfigs_)\n> > + streams.insert(&streamConfig.stream);\n> > +\n> > + const std::string id = \"Virtual0\";\n> > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > + registerCamera(std::move(camera));\n> > +\n> > + return true;\n> \n> I think if you return false here, you don't need any of the created_; \n> \n> /*\n> * Ensure we only ever register and create a single virtual\n> * pipeline handler.\n> */\n> return false;\n> \n> This is core libcamera not your patch, - I still dislike this return\n> usage of match(). It's either not clear or not fitting well to the needs\n> of most pipeline handlers.\n> \n> It's should be clearer at core that returning true here simply means\n> 'please try to create more pipeline handlers'\n\nHrm - Jacopo has highlighted that if the 'successful' creation returns\nfalse - then there would be no output that the pipeline handler had\nmatched successfully ...\n\nSo given that - what you have is correct...\n\nI'd like it to be different but that requires core pipeline handler\niteration rework ;-) so lets not dwell on that right now.\n\n\n> Anyway, comments above could be applied on top if you wish - or handled\n> how you prefer.\n> \n> The created_ just seems like an awkward workaround, but it's a\n> workaround for trying to return the right thing at the end of match. But\n> I think the 'right thing' to return there is poorly defined maybe...\n> \n> Anyway, with any of the above handled or not - or handled on top - none\n> of that blocks this patch for me.\n> \n> \n> Reviewed-by: Kieran Bingham ","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 A8315C3257\n\tfor ;\n\tThu, 3 Oct 2024 08:54:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 721C463525;\n\tThu, 3 Oct 2024 10:54:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6305C62C93\n\tfor ;\n\tThu, 3 Oct 2024 10:54:44 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 274934C7;\n\tThu, 3 Oct 2024 10:53:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pAbvMdVM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727945591;\n\tbh=aegH3ZlsOn16E4t47j5jcfJ82yTcHC0weXavNNnNo9Y=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=pAbvMdVM9XmTieD2rMM+VFc3teVVTivx+bUBz7Sz9Qm2tWZJjwF0RGlyvjGV+uLyo\n\tm9Yf/v8K2zPPIHxN1Yuu2etDHYjb1hDaYRv2mUEbSCwQu2PnCLCKrBGP1+qZaFcJBt\n\txt5RBOFuVvM+2Ulc0sblsyogGRGKMxseGvDVtdQc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tJacopo Mondi ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Oct 2024 09:54:41 +0100","Message-ID":"<172794568160.1619946.1463380125933844730@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31549,"web_url":"https://patchwork.libcamera.org/comment/31549/","msgid":"","date":"2024-10-03T09:16:15","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham\n wrote:\n>\n> Quoting Harvey Yang (2024-09-30 07:29:44)\n> > From: Harvey Yang \n> >\n> > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > infrastructure works on devices without using hardware cameras.\n> >\n> > Signed-off-by: Harvey Yang \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > meson.build | 1 +\n> > meson_options.txt | 3 +-\n> > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> > src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> > 5 files changed, 370 insertions(+), 1 deletion(-)\n> > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> > create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> >\n> > diff --git a/meson.build b/meson.build\n> > index 63e45465..5e533b0c 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -214,6 +214,7 @@ pipelines_support = {\n> > 'simple': ['any'],\n> > 'uvcvideo': ['any'],\n> > 'vimc': ['test'],\n> > + 'virtual': ['test'],\n> > }\n> >\n> > if pipelines.contains('all')\n> > diff --git a/meson_options.txt b/meson_options.txt\n> > index 7aa41249..c91cd241 100644\n> > --- a/meson_options.txt\n> > +++ b/meson_options.txt\n> > @@ -53,7 +53,8 @@ option('pipelines',\n> > 'rpi/vc4',\n> > 'simple',\n> > 'uvcvideo',\n> > - 'vimc'\n> > + 'vimc',\n> > + 'virtual'\n> > ],\n> > description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > new file mode 100644\n> > index 00000000..ada1b335\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > @@ -0,0 +1,5 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +libcamera_internal_sources += files([\n> > + 'virtual.cpp',\n> > +])\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > new file mode 100644\n> > index 00000000..d1584f59\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -0,0 +1,317 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * virtual.cpp - Pipeline handler for virtual cameras\n> > + */\n> > +\n> > +#include \"virtual.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > +#include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Virtual)\n> > +\n> > +namespace {\n> > +\n> > +uint64_t currentTimestamp()\n> > +{\n> > + const auto now = std::chrono::steady_clock::now();\n> > + auto nsecs = std::chrono::duration_cast(\n> > + now.time_since_epoch());\n> > +\n> > + return nsecs.count();\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +class VirtualCameraConfiguration : public CameraConfiguration\n> > +{\n> > +public:\n> > + static constexpr unsigned int kBufferCount = 4;\n> > +\n> > + VirtualCameraConfiguration(VirtualCameraData *data);\n> > +\n> > + Status validate() override;\n> > +\n> > +private:\n> > + const VirtualCameraData *data_;\n> > +};\n> > +\n> > +class PipelineHandlerVirtual : public PipelineHandler\n> > +{\n> > +public:\n> > + PipelineHandlerVirtual(CameraManager *manager);\n> > +\n> > + std::unique_ptr generateConfiguration(Camera *camera,\n> > + Span roles) override;\n> > + int configure(Camera *camera, CameraConfiguration *config) override;\n> > +\n> > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > + std::vector> *buffers) override;\n> > +\n> > + int start(Camera *camera, const ControlList *controls) override;\n> > + void stopDevice(Camera *camera) override;\n> > +\n> > + int queueRequestDevice(Camera *camera, Request *request) override;\n> > +\n> > + bool match(DeviceEnumerator *enumerator) override;\n> > +\n> > +private:\n> > + static bool created_;\n>\n> Could this be\n> static bool created_ = false;\n>\n> saving the need for the outer initialisation below?\n\nUnfortunately, there will be a compile error:\n`ISO C++ forbids in-class initialization of non-const static member`\n, so let's keep it this way.\n\n>\n> > +\n> > + VirtualCameraData *cameraData(Camera *camera)\n> > + {\n> > + return static_cast(camera->_d());\n> > + }\n> > +\n> > + DmaBufAllocator dmaBufAllocator_;\n> > +};\n> > +\n> > +/* static */\n> > +bool PipelineHandlerVirtual::created_ = false;\n>\n> This stands out as a curious way to do this initialisation, but I\n> wouldn't block on this here ...\n>\n> I'm curious why we 'need' a static created_ and how that will be used\n> though...\n>\n>\n> > +\n> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > + const std::vector &supportedResolutions)\n> > + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> > +{\n> > + for (const auto &resolution : supportedResolutions_) {\n> > + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > + minResolutionSize_ = resolution.size;\n> > +\n> > + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > + }\n> > +\n> > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > + streamConfigs_.resize(kMaxStream);\n> > +}\n> > +\n> > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> > + : CameraConfiguration(), data_(data)\n> > +{\n> > +}\n> > +\n> > +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > +{\n> > + Status status = Valid;\n> > +\n> > + if (config_.empty()) {\n> > + LOG(Virtual, Error) << \"Empty config\";\n> > + return Invalid;\n> > + }\n> > +\n> > + /* Only one stream is supported */\n> > + if (config_.size() > VirtualCameraData::kMaxStream) {\n> > + config_.resize(VirtualCameraData::kMaxStream);\n> > + status = Adjusted;\n> > + }\n> > +\n> > + for (StreamConfiguration &cfg : config_) {\n> > + bool found = false;\n> > + for (const auto &resolution : data_->supportedResolutions_) {\n> > + if (resolution.size.width == cfg.size.width &&\n> > + resolution.size.height == cfg.size.height) {\n> > + found = true;\n> > + break;\n> > + }\n> > + }\n> > +\n> > + if (!found) {\n> > + /*\n> > + * \\todo It's a pipeline's decision to choose a\n> > + * resolution when the exact one is not supported.\n> > + * Defining the default logic in PipelineHandler to\n> > + * find the closest resolution would be nice.\n> > + */\n> > + cfg.size = data_->maxResolutionSize_;\n> > + status = Adjusted;\n> > + }\n> > +\n> > + if (cfg.pixelFormat != formats::NV12) {\n> > + cfg.pixelFormat = formats::NV12;\n> > + LOG(Virtual, Debug)\n> > + << \"Stream configuration adjusted to \" << cfg.toString();\n>\n> Wouldn't it be worth reporting this if any adjustment is made?\n>\n> I'd probably have put this at the end of this scope as\n> if (status == Adjusted)\n> LOG(Virtual, Info)\n> << \"Stream configuration adjusted to \" << cfg.toString();\n\nAs we share the same `status` among all `StreamConfiguration`s,\nI think it might be better to add an info log for each place we might\nadjust a StreamConfiguration?\n\nLet me know if you think we should have a local variable of status\nfor each StreamConfiguration.\n\n\n>\n>\n> And I'd think 'info' would be suitable here over debug - as this is a\n> test / virtual pipeline handler so the additional log would be\n> beneficial IMO.\n>\n>\n>\n> > + status = Adjusted;\n> > + }\n> > +\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > + cfg.stride = info.stride(cfg.size.width, 0, 1);\n> > + cfg.frameSize = info.frameSize(cfg.size, 1);\n> > +\n> > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> > + : PipelineHandler(manager),\n> > + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> > +{\n> > +}\n> > +\n> > +std::unique_ptr\n> > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > + Span roles)\n> > +{\n> > + VirtualCameraData *data = cameraData(camera);\n> > + auto config = std::make_unique(data);\n> > +\n> > + if (roles.empty())\n> > + return config;\n> > +\n> > + for (const StreamRole role : roles) {\n> > + switch (role) {\n> > + case StreamRole::StillCapture:\n> > + case StreamRole::VideoRecording:\n> > + case StreamRole::Viewfinder:\n> > + break;\n> > +\n> > + case StreamRole::Raw:\n> > + default:\n> > + LOG(Virtual, Error)\n> > + << \"Requested stream role not supported: \" << role;\n> > + config.reset();\n> > + return config;\n> > + }\n> > +\n> > + std::map> streamFormats;\n> > + PixelFormat pixelFormat = formats::NV12;\n> > + streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> > + data->maxResolutionSize_ } };\n> > + StreamFormats formats(streamFormats);\n> > + StreamConfiguration cfg(formats);\n> > + cfg.pixelFormat = pixelFormat;\n> > + cfg.size = data->maxResolutionSize_;\n> > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > +\n> > + config->addConfiguration(cfg);\n> > + }\n> > +\n> > + ASSERT(config->validate() != CameraConfiguration::Invalid);\n> > +\n> > + return config;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::configure(Camera *camera,\n> > + CameraConfiguration *config)\n> > +{\n> > + VirtualCameraData *data = cameraData(camera);\n> > + for (auto [i, c] : utils::enumerate(*config))\n> > + c.setStream(&data->streamConfigs_[i].stream);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> > + Stream *stream,\n> > + std::vector> *buffers)\n> > +{\n> > + if (!dmaBufAllocator_.isValid())\n> > + return -ENOBUFS;\n> > +\n> > + const StreamConfiguration &config = stream->configuration();\n> > +\n> > + auto info = PixelFormatInfo::info(config.pixelFormat);\n> > +\n> > + std::vector planeSizes;\n> > + for (size_t i = 0; i < info.planes.size(); ++i)\n> > + planeSizes.push_back(info.planeSize(config.size, i));\n> > +\n> > + return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > + [[maybe_unused]] const ControlList *controls)\n> > +{\n> > + return 0;\n> > +}\n> > +\n> > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> > + Request *request)\n> > +{\n> > + for (auto it : request->buffers())\n> > + completeBuffer(request, it.second);\n> > +\n> > + request->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> > + completeRequest(request);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> > +{\n> > + if (created_)\n> > + return false;\n> > +\n> > + created_ = true;\n> > +\n> > + /* \\todo Add virtual cameras according to a config file. */\n> > +\n> > + std::vector supportedResolutions;\n> > + supportedResolutions.resize(2);\n> > + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > +\n> > + std::unique_ptr data =\n> > + std::make_unique(this, supportedResolutions);\n> > +\n> > + data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > +\n> > + /* \\todo Set FrameDurationLimits based on config. */\n> > + ControlInfoMap::Map controls;\n> > + int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > +\n> > + /* Create and register the camera. */\n> > + std::set streams;\n> > + for (auto &streamConfig : data->streamConfigs_)\n> > + streams.insert(&streamConfig.stream);\n> > +\n> > + const std::string id = \"Virtual0\";\n> > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > + registerCamera(std::move(camera));\n> > +\n> > + return true;\n>\n> I think if you return false here, you don't need any of the created_;\n>\n> /*\n> * Ensure we only ever register and create a single virtual\n> * pipeline handler.\n> */\n> return false;\n>\n> This is core libcamera not your patch, - I still dislike this return\n> usage of match(). It's either not clear or not fitting well to the needs\n> of most pipeline handlers.\n>\n> It's should be clearer at core that returning true here simply means\n> 'please try to create more pipeline handlers'\n>\n>\n>\n>\n> Anyway, comments above could be applied on top if you wish - or handled\n> how you prefer.\n>\n> The created_ just seems like an awkward workaround, but it's a\n> workaround for trying to return the right thing at the end of match. But\n> I think the 'right thing' to return there is poorly defined maybe...\n>\n> Anyway, with any of the above handled or not - or handled on top - none\n> of that blocks this patch for me.\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> > new file mode 100644\n> > index 00000000..4df70a13\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -0,0 +1,45 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * virtual.h - Pipeline handler for virtual cameras\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class VirtualCameraData : public Camera::Private\n> > +{\n> > +public:\n> > + const static unsigned int kMaxStream = 1;\n> > +\n> > + struct Resolution {\n> > + Size size;\n> > + std::vector frameRates;\n> > + };\n> > + struct StreamConfig {\n> > + Stream stream;\n> > + };\n> > +\n> > + VirtualCameraData(PipelineHandler *pipe,\n> > + const std::vector &supportedResolutions);\n> > +\n> > + ~VirtualCameraData() = default;\n> > +\n> > + const std::vector supportedResolutions_;\n> > + Size maxResolutionSize_;\n> > + Size minResolutionSize_;\n> > +\n> > + std::vector streamConfigs_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.46.1.824.gd892dcdcdd-goog\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 62460BD80A\n\tfor ;\n\tThu, 3 Oct 2024 09:16:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D85E463525;\n\tThu, 3 Oct 2024 11:16:28 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8308062C93\n\tfor ;\n\tThu, 3 Oct 2024 11:16:27 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id\n\t38308e7fff4ca-2fad0f66d49so11562461fa.3\n\tfor ;\n\tThu, 03 Oct 2024 02:16:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"DFtxTQSf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1727946986; x=1728551786;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=a+qB0TQw7GIUx5bMXp1jPJGHT+cIpLSp4DmHqzCu00w=;\n\tb=DFtxTQSfS1+/efM6MLcKsc69hIaXjsnvWahu8pT7HP00C1RyP63xJH+1iMSiaVJhzU\n\tWipvXqgZwMXiIab52PI2wZv1UVZiuYAuVzPon2aUD4OEHm6XJIEx8SPO11ipQLPs+NWQ\n\ti8bJT2vOPOgQNkfB9HlTcirdriKkWjNSiwhiY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727946986; x=1728551786;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=a+qB0TQw7GIUx5bMXp1jPJGHT+cIpLSp4DmHqzCu00w=;\n\tb=qxZhtJUJfDoTVSbjC6ZiC6Z0IPElxkCI9mq6cL/gg9B5UmRYkp/O4q7QxUD2PxvG9O\n\tXgHqZ0I6Tf1YXmgbfzC5WTLMpJNTiaI74eyO0sEpSM2zkIx9hdTD8VpDILtVaJOKNNiV\n\tSpAwaETk+o5vHyUTENrqRcC/1v3ToUlDczVeYatkz1Fcn1Vwiz3NdaNjb4i5VVJKgNRe\n\tA0Zt3vUN44UjVHSx1U7tYtp9+BPpxtYAt9n1mSdujWBNKX6LuCZYiDQ8BsRibfx/nap5\n\tIQ3oucb6/toPItMPLpiqT8MVs45wc66hp8aAbDon762aNsYviXciy91UyuSUcLBhh90y\n\td7xw==","X-Gm-Message-State":"AOJu0YxOaYRdD8dkZ8f1yzV8EkVTgXHSH8hhgWK1YJ9SSbGJ9XlYGNfK\n\ttxY9nvl0FxZGzDiYGkjlAZE6Y3Nbl9WmXuvkq5DWPK3NHUObed8Bjb8ckRzXwTcR5RNjS7ntEdO\n\tE5KbZnD3mKhliaHKuC5HlNOcYfUQx+nZUv7FV","X-Google-Smtp-Source":"AGHT+IE0SXCzDRiPgwxXJ+f0E+z53S9d5o4cCJO4wbr+KGo3l0ddDc5UAFIdIUKB/5YpAKQ13xJZfO03HC+4yHvvcLQ=","X-Received":"by 2002:a2e:81a:0:b0:2fa:dfd6:4451 with SMTP id\n\t38308e7fff4ca-2fae1033a26mr29305541fa.26.1727946986339;\n\tThu, 03 Oct 2024 02:16:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","In-Reply-To":"<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Thu, 3 Oct 2024 17:16:15 +0800","Message-ID":"","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31551,"web_url":"https://patchwork.libcamera.org/comment/31551/","msgid":"<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>","date":"2024-10-03T09:50:40","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Cheng-Hao Yang (2024-10-03 10:16:15)\n> Hi Kieran,\n> \n> On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham\n> wrote:\n> >\n> > Quoting Harvey Yang (2024-09-30 07:29:44)\n> > > From: Harvey Yang \n> > >\n> > > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > > infrastructure works on devices without using hardware cameras.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > meson.build | 1 +\n> > > meson_options.txt | 3 +-\n> > > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > > src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> > > src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> > > 5 files changed, 370 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> > > create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> > >\n> > > diff --git a/meson.build b/meson.build\n> > > index 63e45465..5e533b0c 100644\n> > > --- a/meson.build\n> > > +++ b/meson.build\n> > > @@ -214,6 +214,7 @@ pipelines_support = {\n> > > 'simple': ['any'],\n> > > 'uvcvideo': ['any'],\n> > > 'vimc': ['test'],\n> > > + 'virtual': ['test'],\n> > > }\n> > >\n> > > if pipelines.contains('all')\n> > > diff --git a/meson_options.txt b/meson_options.txt\n> > > index 7aa41249..c91cd241 100644\n> > > --- a/meson_options.txt\n> > > +++ b/meson_options.txt\n> > > @@ -53,7 +53,8 @@ option('pipelines',\n> > > 'rpi/vc4',\n> > > 'simple',\n> > > 'uvcvideo',\n> > > - 'vimc'\n> > > + 'vimc',\n> > > + 'virtual'\n> > > ],\n> > > description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> > >\n> > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > > new file mode 100644\n> > > index 00000000..ada1b335\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > @@ -0,0 +1,5 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +libcamera_internal_sources += files([\n> > > + 'virtual.cpp',\n> > > +])\n> > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > new file mode 100644\n> > > index 00000000..d1584f59\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > @@ -0,0 +1,317 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > + */\n> > > +\n> > > +#include \"virtual.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/camera.h\"\n> > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > > +#include \"libcamera/internal/formats.h\"\n> > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Virtual)\n> > > +\n> > > +namespace {\n> > > +\n> > > +uint64_t currentTimestamp()\n> > > +{\n> > > + const auto now = std::chrono::steady_clock::now();\n> > > + auto nsecs = std::chrono::duration_cast(\n> > > + now.time_since_epoch());\n> > > +\n> > > + return nsecs.count();\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +class VirtualCameraConfiguration : public CameraConfiguration\n> > > +{\n> > > +public:\n> > > + static constexpr unsigned int kBufferCount = 4;\n> > > +\n> > > + VirtualCameraConfiguration(VirtualCameraData *data);\n> > > +\n> > > + Status validate() override;\n> > > +\n> > > +private:\n> > > + const VirtualCameraData *data_;\n> > > +};\n> > > +\n> > > +class PipelineHandlerVirtual : public PipelineHandler\n> > > +{\n> > > +public:\n> > > + PipelineHandlerVirtual(CameraManager *manager);\n> > > +\n> > > + std::unique_ptr generateConfiguration(Camera *camera,\n> > > + Span roles) override;\n> > > + int configure(Camera *camera, CameraConfiguration *config) override;\n> > > +\n> > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > + std::vector> *buffers) override;\n> > > +\n> > > + int start(Camera *camera, const ControlList *controls) override;\n> > > + void stopDevice(Camera *camera) override;\n> > > +\n> > > + int queueRequestDevice(Camera *camera, Request *request) override;\n> > > +\n> > > + bool match(DeviceEnumerator *enumerator) override;\n> > > +\n> > > +private:\n> > > + static bool created_;\n> >\n> > Could this be\n> > static bool created_ = false;\n> >\n> > saving the need for the outer initialisation below?\n> \n> Unfortunately, there will be a compile error:\n> `ISO C++ forbids in-class initialization of non-const static member`\n> , so let's keep it this way.\n> \n\nAck.\n\n> >\n> > > +\n> > > + VirtualCameraData *cameraData(Camera *camera)\n> > > + {\n> > > + return static_cast(camera->_d());\n> > > + }\n> > > +\n> > > + DmaBufAllocator dmaBufAllocator_;\n> > > +};\n> > > +\n> > > +/* static */\n> > > +bool PipelineHandlerVirtual::created_ = false;\n> >\n> > This stands out as a curious way to do this initialisation, but I\n> > wouldn't block on this here ...\n> >\n> > I'm curious why we 'need' a static created_ and how that will be used\n> > though...\n> >\n> >\n> > > +\n> > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > + const std::vector &supportedResolutions)\n> > > + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> > > +{\n> > > + for (const auto &resolution : supportedResolutions_) {\n> > > + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > > + minResolutionSize_ = resolution.size;\n> > > +\n> > > + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > > + }\n> > > +\n> > > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > > + streamConfigs_.resize(kMaxStream);\n> > > +}\n> > > +\n> > > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> > > + : CameraConfiguration(), data_(data)\n> > > +{\n> > > +}\n> > > +\n> > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > > +{\n> > > + Status status = Valid;\n> > > +\n> > > + if (config_.empty()) {\n> > > + LOG(Virtual, Error) << \"Empty config\";\n> > > + return Invalid;\n> > > + }\n> > > +\n> > > + /* Only one stream is supported */\n> > > + if (config_.size() > VirtualCameraData::kMaxStream) {\n> > > + config_.resize(VirtualCameraData::kMaxStream);\n> > > + status = Adjusted;\n> > > + }\n> > > +\n> > > + for (StreamConfiguration &cfg : config_) {\n> > > + bool found = false;\n> > > + for (const auto &resolution : data_->supportedResolutions_) {\n> > > + if (resolution.size.width == cfg.size.width &&\n> > > + resolution.size.height == cfg.size.height) {\n> > > + found = true;\n> > > + break;\n> > > + }\n> > > + }\n> > > +\n> > > + if (!found) {\n> > > + /*\n> > > + * \\todo It's a pipeline's decision to choose a\n> > > + * resolution when the exact one is not supported.\n> > > + * Defining the default logic in PipelineHandler to\n> > > + * find the closest resolution would be nice.\n> > > + */\n> > > + cfg.size = data_->maxResolutionSize_;\n> > > + status = Adjusted;\n> > > + }\n> > > +\n> > > + if (cfg.pixelFormat != formats::NV12) {\n> > > + cfg.pixelFormat = formats::NV12;\n> > > + LOG(Virtual, Debug)\n> > > + << \"Stream configuration adjusted to \" << cfg.toString();\n> >\n> > Wouldn't it be worth reporting this if any adjustment is made?\n> >\n> > I'd probably have put this at the end of this scope as\n> > if (status == Adjusted)\n> > LOG(Virtual, Info)\n> > << \"Stream configuration adjusted to \" << cfg.toString();\n> \n> As we share the same `status` among all `StreamConfiguration`s,\n> I think it might be better to add an info log for each place we might\n> adjust a StreamConfiguration?\n> \n> Let me know if you think we should have a local variable of status\n> for each StreamConfiguration.\n> \n\nThe output of \"Stream configuration adjusted to \" << cfg.toString();\nshould say the full new output configuration right ? I.e. already\nincluding the size if the resolution wasn't found and defaulted to\ndata_->maxResolutionSize_;\n\nBut I think that only needs to be printed once.\n\n> > And I'd think 'info' would be suitable here over debug - as this is a\n> > test / virtual pipeline handler so the additional log would be\n> > beneficial IMO.\n> >\n> >\n> >\n> > > + status = Adjusted;\n> > > + }\n> > > +\n> > > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > > + cfg.stride = info.stride(cfg.size.width, 0, 1);\n> > > + cfg.frameSize = info.frameSize(cfg.size, 1);\n> > > +\n> > > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > > + }\n> > > +\n> > > + return status;\n> > > +}\n> > > +\n> > > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> > > + : PipelineHandler(manager),\n> > > + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > > + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> > > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> > > +{\n> > > +}\n> > > +\n> > > +std::unique_ptr\n> > > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > > + Span roles)\n> > > +{\n> > > + VirtualCameraData *data = cameraData(camera);\n> > > + auto config = std::make_unique(data);\n> > > +\n> > > + if (roles.empty())\n> > > + return config;\n> > > +\n> > > + for (const StreamRole role : roles) {\n> > > + switch (role) {\n> > > + case StreamRole::StillCapture:\n> > > + case StreamRole::VideoRecording:\n> > > + case StreamRole::Viewfinder:\n> > > + break;\n> > > +\n> > > + case StreamRole::Raw:\n> > > + default:\n> > > + LOG(Virtual, Error)\n> > > + << \"Requested stream role not supported: \" << role;\n> > > + config.reset();\n> > > + return config;\n> > > + }\n> > > +\n> > > + std::map> streamFormats;\n> > > + PixelFormat pixelFormat = formats::NV12;\n> > > + streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> > > + data->maxResolutionSize_ } };\n> > > + StreamFormats formats(streamFormats);\n> > > + StreamConfiguration cfg(formats);\n> > > + cfg.pixelFormat = pixelFormat;\n> > > + cfg.size = data->maxResolutionSize_;\n> > > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > > +\n> > > + config->addConfiguration(cfg);\n> > > + }\n> > > +\n> > > + ASSERT(config->validate() != CameraConfiguration::Invalid);\n> > > +\n> > > + return config;\n> > > +}\n> > > +\n> > > +int PipelineHandlerVirtual::configure(Camera *camera,\n> > > + CameraConfiguration *config)\n> > > +{\n> > > + VirtualCameraData *data = cameraData(camera);\n> > > + for (auto [i, c] : utils::enumerate(*config))\n> > > + c.setStream(&data->streamConfigs_[i].stream);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> > > + Stream *stream,\n> > > + std::vector> *buffers)\n> > > +{\n> > > + if (!dmaBufAllocator_.isValid())\n> > > + return -ENOBUFS;\n> > > +\n> > > + const StreamConfiguration &config = stream->configuration();\n> > > +\n> > > + auto info = PixelFormatInfo::info(config.pixelFormat);\n> > > +\n> > > + std::vector planeSizes;\n> > > + for (size_t i = 0; i < info.planes.size(); ++i)\n> > > + planeSizes.push_back(info.planeSize(config.size, i));\n> > > +\n> > > + return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> > > +}\n> > > +\n> > > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > > + [[maybe_unused]] const ControlList *controls)\n> > > +{\n> > > + return 0;\n> > > +}\n> > > +\n> > > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > > +{\n> > > +}\n> > > +\n> > > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> > > + Request *request)\n> > > +{\n> > > + for (auto it : request->buffers())\n> > > + completeBuffer(request, it.second);\n> > > +\n> > > + request->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> > > + completeRequest(request);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> > > +{\n> > > + if (created_)\n> > > + return false;\n> > > +\n> > > + created_ = true;\n> > > +\n> > > + /* \\todo Add virtual cameras according to a config file. */\n> > > +\n> > > + std::vector supportedResolutions;\n> > > + supportedResolutions.resize(2);\n> > > + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > > + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > > +\n> > > + std::unique_ptr data =\n> > > + std::make_unique(this, supportedResolutions);\n> > > +\n> > > + data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > > + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > > + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > > +\n> > > + /* \\todo Set FrameDurationLimits based on config. */\n> > > + ControlInfoMap::Map controls;\n> > > + int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > > + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > > +\n> > > + /* Create and register the camera. */\n> > > + std::set streams;\n> > > + for (auto &streamConfig : data->streamConfigs_)\n> > > + streams.insert(&streamConfig.stream);\n> > > +\n> > > + const std::string id = \"Virtual0\";\n> > > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > > + registerCamera(std::move(camera));\n> > > +\n> > > + return true;\n> >\n> > I think if you return false here, you don't need any of the created_;\n> >\n> > /*\n> > * Ensure we only ever register and create a single virtual\n> > * pipeline handler.\n> > */\n> > return false;\n> >\n> > This is core libcamera not your patch, - I still dislike this return\n> > usage of match(). It's either not clear or not fitting well to the needs\n> > of most pipeline handlers.\n> >\n> > It's should be clearer at core that returning true here simply means\n> > 'please try to create more pipeline handlers'\n> >\n> >\n> >\n> >\n> > Anyway, comments above could be applied on top if you wish - or handled\n> > how you prefer.\n> >\n> > The created_ just seems like an awkward workaround, but it's a\n> > workaround for trying to return the right thing at the end of match. But\n> > I think the 'right thing' to return there is poorly defined maybe...\n> >\n> > Anyway, with any of the above handled or not - or handled on top - none\n> > of that blocks this patch for me.\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > +}\n> > > +\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> > > new file mode 100644\n> > > index 00000000..4df70a13\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > @@ -0,0 +1,45 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * virtual.h - Pipeline handler for virtual cameras\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/camera.h\"\n> > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class VirtualCameraData : public Camera::Private\n> > > +{\n> > > +public:\n> > > + const static unsigned int kMaxStream = 1;\n> > > +\n> > > + struct Resolution {\n> > > + Size size;\n> > > + std::vector frameRates;\n> > > + };\n> > > + struct StreamConfig {\n> > > + Stream stream;\n> > > + };\n> > > +\n> > > + VirtualCameraData(PipelineHandler *pipe,\n> > > + const std::vector &supportedResolutions);\n> > > +\n> > > + ~VirtualCameraData() = default;\n> > > +\n> > > + const std::vector supportedResolutions_;\n> > > + Size maxResolutionSize_;\n> > > + Size minResolutionSize_;\n> > > +\n> > > + std::vector streamConfigs_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > --\n> > > 2.46.1.824.gd892dcdcdd-goog\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 B9E49BD80A\n\tfor ;\n\tThu, 3 Oct 2024 09:50:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 818EA63525;\n\tThu, 3 Oct 2024 11:50:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E3F962C93\n\tfor ;\n\tThu, 3 Oct 2024 11:50:43 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB3CD4C7;\n\tThu, 3 Oct 2024 11:49:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fgSSPjiA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727948950;\n\tbh=2PQyeNuVFKgXllOD+TSXIuXNICaT+CIW7dCOCPC0l6s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=fgSSPjiAWor3KK2Bg7hTfjj5jWXPFkymDUCgpNFLG/f/KKFB0L56tNXUlwApREzlt\n\tKD0uu/VtB7qDqiYzC8RxZM7lNlxo4d1HOYjUJmG64ZrxnV4Evisb+Bz1W0CVgdalrY\n\tqhTjjr0ffg2dXFfZSI/7RtpvXult4Wz8P47QZWG0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","To":"Cheng-Hao Yang ","Date":"Thu, 03 Oct 2024 10:50:40 +0100","Message-ID":"<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31552,"web_url":"https://patchwork.libcamera.org/comment/31552/","msgid":"","date":"2024-10-03T09:58:45","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham\n wrote:\n>\n> Quoting Cheng-Hao Yang (2024-10-03 10:16:15)\n> > Hi Kieran,\n> >\n> > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Harvey Yang (2024-09-30 07:29:44)\n> > > > From: Harvey Yang \n> > > >\n> > > > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > > > infrastructure works on devices without using hardware cameras.\n> > > >\n> > > > Signed-off-by: Harvey Yang \n> > > > Reviewed-by: Jacopo Mondi \n> > > > ---\n> > > > meson.build | 1 +\n> > > > meson_options.txt | 3 +-\n> > > > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > > > src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> > > > src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> > > > 5 files changed, 370 insertions(+), 1 deletion(-)\n> > > > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> > > >\n> > > > diff --git a/meson.build b/meson.build\n> > > > index 63e45465..5e533b0c 100644\n> > > > --- a/meson.build\n> > > > +++ b/meson.build\n> > > > @@ -214,6 +214,7 @@ pipelines_support = {\n> > > > 'simple': ['any'],\n> > > > 'uvcvideo': ['any'],\n> > > > 'vimc': ['test'],\n> > > > + 'virtual': ['test'],\n> > > > }\n> > > >\n> > > > if pipelines.contains('all')\n> > > > diff --git a/meson_options.txt b/meson_options.txt\n> > > > index 7aa41249..c91cd241 100644\n> > > > --- a/meson_options.txt\n> > > > +++ b/meson_options.txt\n> > > > @@ -53,7 +53,8 @@ option('pipelines',\n> > > > 'rpi/vc4',\n> > > > 'simple',\n> > > > 'uvcvideo',\n> > > > - 'vimc'\n> > > > + 'vimc',\n> > > > + 'virtual'\n> > > > ],\n> > > > description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > > > new file mode 100644\n> > > > index 00000000..ada1b335\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > > @@ -0,0 +1,5 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +libcamera_internal_sources += files([\n> > > > + 'virtual.cpp',\n> > > > +])\n> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > new file mode 100644\n> > > > index 00000000..d1584f59\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > @@ -0,0 +1,317 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Google Inc.\n> > > > + *\n> > > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > > + */\n> > > > +\n> > > > +#include \"virtual.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/camera.h\"\n> > > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(Virtual)\n> > > > +\n> > > > +namespace {\n> > > > +\n> > > > +uint64_t currentTimestamp()\n> > > > +{\n> > > > + const auto now = std::chrono::steady_clock::now();\n> > > > + auto nsecs = std::chrono::duration_cast(\n> > > > + now.time_since_epoch());\n> > > > +\n> > > > + return nsecs.count();\n> > > > +}\n> > > > +\n> > > > +} /* namespace */\n> > > > +\n> > > > +class VirtualCameraConfiguration : public CameraConfiguration\n> > > > +{\n> > > > +public:\n> > > > + static constexpr unsigned int kBufferCount = 4;\n> > > > +\n> > > > + VirtualCameraConfiguration(VirtualCameraData *data);\n> > > > +\n> > > > + Status validate() override;\n> > > > +\n> > > > +private:\n> > > > + const VirtualCameraData *data_;\n> > > > +};\n> > > > +\n> > > > +class PipelineHandlerVirtual : public PipelineHandler\n> > > > +{\n> > > > +public:\n> > > > + PipelineHandlerVirtual(CameraManager *manager);\n> > > > +\n> > > > + std::unique_ptr generateConfiguration(Camera *camera,\n> > > > + Span roles) override;\n> > > > + int configure(Camera *camera, CameraConfiguration *config) override;\n> > > > +\n> > > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > + std::vector> *buffers) override;\n> > > > +\n> > > > + int start(Camera *camera, const ControlList *controls) override;\n> > > > + void stopDevice(Camera *camera) override;\n> > > > +\n> > > > + int queueRequestDevice(Camera *camera, Request *request) override;\n> > > > +\n> > > > + bool match(DeviceEnumerator *enumerator) override;\n> > > > +\n> > > > +private:\n> > > > + static bool created_;\n> > >\n> > > Could this be\n> > > static bool created_ = false;\n> > >\n> > > saving the need for the outer initialisation below?\n> >\n> > Unfortunately, there will be a compile error:\n> > `ISO C++ forbids in-class initialization of non-const static member`\n> > , so let's keep it this way.\n> >\n>\n> Ack.\n>\n> > >\n> > > > +\n> > > > + VirtualCameraData *cameraData(Camera *camera)\n> > > > + {\n> > > > + return static_cast(camera->_d());\n> > > > + }\n> > > > +\n> > > > + DmaBufAllocator dmaBufAllocator_;\n> > > > +};\n> > > > +\n> > > > +/* static */\n> > > > +bool PipelineHandlerVirtual::created_ = false;\n> > >\n> > > This stands out as a curious way to do this initialisation, but I\n> > > wouldn't block on this here ...\n> > >\n> > > I'm curious why we 'need' a static created_ and how that will be used\n> > > though...\n> > >\n> > >\n> > > > +\n> > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > > + const std::vector &supportedResolutions)\n> > > > + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> > > > +{\n> > > > + for (const auto &resolution : supportedResolutions_) {\n> > > > + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > > > + minResolutionSize_ = resolution.size;\n> > > > +\n> > > > + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > > > + }\n> > > > +\n> > > > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > > > + streamConfigs_.resize(kMaxStream);\n> > > > +}\n> > > > +\n> > > > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> > > > + : CameraConfiguration(), data_(data)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > > > +{\n> > > > + Status status = Valid;\n> > > > +\n> > > > + if (config_.empty()) {\n> > > > + LOG(Virtual, Error) << \"Empty config\";\n> > > > + return Invalid;\n> > > > + }\n> > > > +\n> > > > + /* Only one stream is supported */\n> > > > + if (config_.size() > VirtualCameraData::kMaxStream) {\n> > > > + config_.resize(VirtualCameraData::kMaxStream);\n> > > > + status = Adjusted;\n> > > > + }\n> > > > +\n> > > > + for (StreamConfiguration &cfg : config_) {\n> > > > + bool found = false;\n> > > > + for (const auto &resolution : data_->supportedResolutions_) {\n> > > > + if (resolution.size.width == cfg.size.width &&\n> > > > + resolution.size.height == cfg.size.height) {\n> > > > + found = true;\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + if (!found) {\n> > > > + /*\n> > > > + * \\todo It's a pipeline's decision to choose a\n> > > > + * resolution when the exact one is not supported.\n> > > > + * Defining the default logic in PipelineHandler to\n> > > > + * find the closest resolution would be nice.\n> > > > + */\n> > > > + cfg.size = data_->maxResolutionSize_;\n> > > > + status = Adjusted;\n> > > > + }\n> > > > +\n> > > > + if (cfg.pixelFormat != formats::NV12) {\n> > > > + cfg.pixelFormat = formats::NV12;\n> > > > + LOG(Virtual, Debug)\n> > > > + << \"Stream configuration adjusted to \" << cfg.toString();\n> > >\n> > > Wouldn't it be worth reporting this if any adjustment is made?\n> > >\n> > > I'd probably have put this at the end of this scope as\n> > > if (status == Adjusted)\n> > > LOG(Virtual, Info)\n> > > << \"Stream configuration adjusted to \" << cfg.toString();\n> >\n> > As we share the same `status` among all `StreamConfiguration`s,\n> > I think it might be better to add an info log for each place we might\n> > adjust a StreamConfiguration?\n> >\n> > Let me know if you think we should have a local variable of status\n> > for each StreamConfiguration.\n> >\n>\n> The output of \"Stream configuration adjusted to \" << cfg.toString();\n> should say the full new output configuration right ? I.e. already\n> including the size if the resolution wasn't found and defaulted to\n> data_->maxResolutionSize_;\n>\n> But I think that only needs to be printed once.\n\nYeah that's right, `cfg.toString()` contains the full output.\nSo, do you mean that you prefer to keep an extra variable in the\nfor loop to record if the current StreamConfiguration is changed?\nIt's only needed for multi-stream support though, but I want to\nmake sure the log still makes sense when we enable multiple\nstreams in the future.\n\nBR,\nHarvey\n\n>\n> > > And I'd think 'info' would be suitable here over debug - as this is a\n> > > test / virtual pipeline handler so the additional log would be\n> > > beneficial IMO.\n> > >\n> > >\n> > >\n> > > > + status = Adjusted;\n> > > > + }\n> > > > +\n> > > > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > > > + cfg.stride = info.stride(cfg.size.width, 0, 1);\n> > > > + cfg.frameSize = info.frameSize(cfg.size, 1);\n> > > > +\n> > > > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > > > + }\n> > > > +\n> > > > + return status;\n> > > > +}\n> > > > +\n> > > > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> > > > + : PipelineHandler(manager),\n> > > > + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > > > + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> > > > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr\n> > > > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > > > + Span roles)\n> > > > +{\n> > > > + VirtualCameraData *data = cameraData(camera);\n> > > > + auto config = std::make_unique(data);\n> > > > +\n> > > > + if (roles.empty())\n> > > > + return config;\n> > > > +\n> > > > + for (const StreamRole role : roles) {\n> > > > + switch (role) {\n> > > > + case StreamRole::StillCapture:\n> > > > + case StreamRole::VideoRecording:\n> > > > + case StreamRole::Viewfinder:\n> > > > + break;\n> > > > +\n> > > > + case StreamRole::Raw:\n> > > > + default:\n> > > > + LOG(Virtual, Error)\n> > > > + << \"Requested stream role not supported: \" << role;\n> > > > + config.reset();\n> > > > + return config;\n> > > > + }\n> > > > +\n> > > > + std::map> streamFormats;\n> > > > + PixelFormat pixelFormat = formats::NV12;\n> > > > + streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> > > > + data->maxResolutionSize_ } };\n> > > > + StreamFormats formats(streamFormats);\n> > > > + StreamConfiguration cfg(formats);\n> > > > + cfg.pixelFormat = pixelFormat;\n> > > > + cfg.size = data->maxResolutionSize_;\n> > > > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > > > +\n> > > > + config->addConfiguration(cfg);\n> > > > + }\n> > > > +\n> > > > + ASSERT(config->validate() != CameraConfiguration::Invalid);\n> > > > +\n> > > > + return config;\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::configure(Camera *camera,\n> > > > + CameraConfiguration *config)\n> > > > +{\n> > > > + VirtualCameraData *data = cameraData(camera);\n> > > > + for (auto [i, c] : utils::enumerate(*config))\n> > > > + c.setStream(&data->streamConfigs_[i].stream);\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> > > > + Stream *stream,\n> > > > + std::vector> *buffers)\n> > > > +{\n> > > > + if (!dmaBufAllocator_.isValid())\n> > > > + return -ENOBUFS;\n> > > > +\n> > > > + const StreamConfiguration &config = stream->configuration();\n> > > > +\n> > > > + auto info = PixelFormatInfo::info(config.pixelFormat);\n> > > > +\n> > > > + std::vector planeSizes;\n> > > > + for (size_t i = 0; i < info.planes.size(); ++i)\n> > > > + planeSizes.push_back(info.planeSize(config.size, i));\n> > > > +\n> > > > + return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > > > + [[maybe_unused]] const ControlList *controls)\n> > > > +{\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> > > > + Request *request)\n> > > > +{\n> > > > + for (auto it : request->buffers())\n> > > > + completeBuffer(request, it.second);\n> > > > +\n> > > > + request->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> > > > + completeRequest(request);\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> > > > +{\n> > > > + if (created_)\n> > > > + return false;\n> > > > +\n> > > > + created_ = true;\n> > > > +\n> > > > + /* \\todo Add virtual cameras according to a config file. */\n> > > > +\n> > > > + std::vector supportedResolutions;\n> > > > + supportedResolutions.resize(2);\n> > > > + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> > > > + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> > > > +\n> > > > + std::unique_ptr data =\n> > > > + std::make_unique(this, supportedResolutions);\n> > > > +\n> > > > + data->properties_.set(properties::Location, properties::CameraLocationFront);\n> > > > + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > > > + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> > > > +\n> > > > + /* \\todo Set FrameDurationLimits based on config. */\n> > > > + ControlInfoMap::Map controls;\n> > > > + int64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> > > > + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> > > > + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > > > +\n> > > > + /* Create and register the camera. */\n> > > > + std::set streams;\n> > > > + for (auto &streamConfig : data->streamConfigs_)\n> > > > + streams.insert(&streamConfig.stream);\n> > > > +\n> > > > + const std::string id = \"Virtual0\";\n> > > > + std::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> > > > + registerCamera(std::move(camera));\n> > > > +\n> > > > + return true;\n> > >\n> > > I think if you return false here, you don't need any of the created_;\n> > >\n> > > /*\n> > > * Ensure we only ever register and create a single virtual\n> > > * pipeline handler.\n> > > */\n> > > return false;\n> > >\n> > > This is core libcamera not your patch, - I still dislike this return\n> > > usage of match(). It's either not clear or not fitting well to the needs\n> > > of most pipeline handlers.\n> > >\n> > > It's should be clearer at core that returning true here simply means\n> > > 'please try to create more pipeline handlers'\n> > >\n> > >\n> > >\n> > >\n> > > Anyway, comments above could be applied on top if you wish - or handled\n> > > how you prefer.\n> > >\n> > > The created_ just seems like an awkward workaround, but it's a\n> > > workaround for trying to return the right thing at the end of match. But\n> > > I think the 'right thing' to return there is poorly defined maybe...\n> > >\n> > > Anyway, with any of the above handled or not - or handled on top - none\n> > > of that blocks this patch for me.\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >\n> > > > +}\n> > > > +\n> > > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> > > > new file mode 100644\n> > > > index 00000000..4df70a13\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > > @@ -0,0 +1,45 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Google Inc.\n> > > > + *\n> > > > + * virtual.h - Pipeline handler for virtual cameras\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/camera.h\"\n> > > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class VirtualCameraData : public Camera::Private\n> > > > +{\n> > > > +public:\n> > > > + const static unsigned int kMaxStream = 1;\n> > > > +\n> > > > + struct Resolution {\n> > > > + Size size;\n> > > > + std::vector frameRates;\n> > > > + };\n> > > > + struct StreamConfig {\n> > > > + Stream stream;\n> > > > + };\n> > > > +\n> > > > + VirtualCameraData(PipelineHandler *pipe,\n> > > > + const std::vector &supportedResolutions);\n> > > > +\n> > > > + ~VirtualCameraData() = default;\n> > > > +\n> > > > + const std::vector supportedResolutions_;\n> > > > + Size maxResolutionSize_;\n> > > > + Size minResolutionSize_;\n> > > > +\n> > > > + std::vector streamConfigs_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > --\n> > > > 2.46.1.824.gd892dcdcdd-goog\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 B1E2DC3257\n\tfor ;\n\tThu, 3 Oct 2024 09:59:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81B2963522;\n\tThu, 3 Oct 2024 11:59:00 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE4A762C93\n\tfor ;\n\tThu, 3 Oct 2024 11:58:57 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2fac47f0b1aso10040721fa.1\n\tfor ;\n\tThu, 03 Oct 2024 02:58:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BRet2RWI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1727949537; x=1728554337;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=cCHK/HxRFFWtAG3S0ybcZcAra2TJU7ThxsS955JnFdI=;\n\tb=BRet2RWIRzDe9xiDfRxlYa9aY7davHJV5wHkn9XM1UMYFNVjUZ9g8TKuCh5MSWOqQl\n\tzk1fjq4jx/z6wEyzERNSFsZcnNld3JJp40aXwB40G49vHWFhBz8yNLc99728SDLNYcHz\n\ttmIwG8oi4E0CikNut2qB7YNn8Iaq1ZmlC1mhs=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727949537; x=1728554337;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=cCHK/HxRFFWtAG3S0ybcZcAra2TJU7ThxsS955JnFdI=;\n\tb=xBNCSzUL+gm0/BeDWO0cUug8wrESKCDWIHwLBySDrJNC5VPSp7f2LqJ7DHKxlf0iqs\n\tyOmM/K+hd0Z/WCdmrqi4mV1zOd5Nuf9ry/w3XVI0Jgmz7x/ZxqMwtPD8wuiiPhWuiysr\n\tUZ2hNz0G9V6JGCeSjZvxVgmPGx8VyA2/Rm/yttK3A6RVj94RKeFwAukVjxQWU4Lp3QZh\n\tPDp2gaZUJGvZsiGsw2o/UcMCNGioMdFO3QYWFsWSbFyCoj3W+SgmyC4d/hEC6EETaqsN\n\tDtX/FmEZY+frS+zd3jPLE+/M1MFDTO0h60Af1W2Ma3B6x3cxGDqHRFqXqXPdi4Scs+Y3\n\t9XDw==","X-Gm-Message-State":"AOJu0YyijrYt3o6zhe4KK6QtsbgTRLWeTa2o5zJ8V5BtFwJIqxg36cGQ\n\tYoI6BAARfNWxUU5sYR4dg+0PRXdI4CH+vhtfKS+KI0gNp5/Vi0Mz01awj2q1b3rrvt1XVCwr/Jm\n\t47IwxQdzylZIYkOeGyleIRQdFabvVyOQAEr60IgRNY97AhfI=","X-Google-Smtp-Source":"AGHT+IGxTnGHyxr6kmQiI0IVuE8fErKp5/amrVL2q7ypb/3Me8S1kQtSFT8kme8la5HqrdlOlKNvYVSuGsR/Lb8Lgd4=","X-Received":"by 2002:a2e:a543:0:b0:2fa:cff1:c6b2 with SMTP id\n\t38308e7fff4ca-2fae10232d9mr39249141fa.1.1727949536694;\n\tThu, 03 Oct 2024 02:58:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>\n\t\n\t<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>","In-Reply-To":"<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Thu, 3 Oct 2024 17:58:45 +0800","Message-ID":"","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31556,"web_url":"https://patchwork.libcamera.org/comment/31556/","msgid":"<172795009536.1619946.4117705877488668304@ping.linuxembedded.co.uk>","date":"2024-10-03T10:08:15","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Cheng-Hao Yang (2024-10-03 10:58:45)\n> Hi Kieran,\n> \n> On Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham\n> wrote:\n> >\n> > Quoting Cheng-Hao Yang (2024-10-03 10:16:15)\n> > > Hi Kieran,\n> > >\n> > > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Quoting Harvey Yang (2024-09-30 07:29:44)\n> > > > > From: Harvey Yang \n> > > > >\n> > > > > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > > > > infrastructure works on devices without using hardware cameras.\n> > > > >\n> > > > > Signed-off-by: Harvey Yang \n> > > > > Reviewed-by: Jacopo Mondi \n> > > > > ---\n> > > > > meson.build | 1 +\n> > > > > meson_options.txt | 3 +-\n> > > > > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > > > > src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> > > > > src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> > > > > 5 files changed, 370 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> > > > >\n> > > > > diff --git a/meson.build b/meson.build\n> > > > > index 63e45465..5e533b0c 100644\n> > > > > --- a/meson.build\n> > > > > +++ b/meson.build\n> > > > > @@ -214,6 +214,7 @@ pipelines_support = {\n> > > > > 'simple': ['any'],\n> > > > > 'uvcvideo': ['any'],\n> > > > > 'vimc': ['test'],\n> > > > > + 'virtual': ['test'],\n> > > > > }\n> > > > >\n> > > > > if pipelines.contains('all')\n> > > > > diff --git a/meson_options.txt b/meson_options.txt\n> > > > > index 7aa41249..c91cd241 100644\n> > > > > --- a/meson_options.txt\n> > > > > +++ b/meson_options.txt\n> > > > > @@ -53,7 +53,8 @@ option('pipelines',\n> > > > > 'rpi/vc4',\n> > > > > 'simple',\n> > > > > 'uvcvideo',\n> > > > > - 'vimc'\n> > > > > + 'vimc',\n> > > > > + 'virtual'\n> > > > > ],\n> > > > > description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > > > > new file mode 100644\n> > > > > index 00000000..ada1b335\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > > > @@ -0,0 +1,5 @@\n> > > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > > +\n> > > > > +libcamera_internal_sources += files([\n> > > > > + 'virtual.cpp',\n> > > > > +])\n> > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..d1584f59\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > @@ -0,0 +1,317 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Google Inc.\n> > > > > + *\n> > > > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > > > + */\n> > > > > +\n> > > > > +#include \"virtual.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/camera.h\"\n> > > > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(Virtual)\n> > > > > +\n> > > > > +namespace {\n> > > > > +\n> > > > > +uint64_t currentTimestamp()\n> > > > > +{\n> > > > > + const auto now = std::chrono::steady_clock::now();\n> > > > > + auto nsecs = std::chrono::duration_cast(\n> > > > > + now.time_since_epoch());\n> > > > > +\n> > > > > + return nsecs.count();\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > +class VirtualCameraConfiguration : public CameraConfiguration\n> > > > > +{\n> > > > > +public:\n> > > > > + static constexpr unsigned int kBufferCount = 4;\n> > > > > +\n> > > > > + VirtualCameraConfiguration(VirtualCameraData *data);\n> > > > > +\n> > > > > + Status validate() override;\n> > > > > +\n> > > > > +private:\n> > > > > + const VirtualCameraData *data_;\n> > > > > +};\n> > > > > +\n> > > > > +class PipelineHandlerVirtual : public PipelineHandler\n> > > > > +{\n> > > > > +public:\n> > > > > + PipelineHandlerVirtual(CameraManager *manager);\n> > > > > +\n> > > > > + std::unique_ptr generateConfiguration(Camera *camera,\n> > > > > + Span roles) override;\n> > > > > + int configure(Camera *camera, CameraConfiguration *config) override;\n> > > > > +\n> > > > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > > + std::vector> *buffers) override;\n> > > > > +\n> > > > > + int start(Camera *camera, const ControlList *controls) override;\n> > > > > + void stopDevice(Camera *camera) override;\n> > > > > +\n> > > > > + int queueRequestDevice(Camera *camera, Request *request) override;\n> > > > > +\n> > > > > + bool match(DeviceEnumerator *enumerator) override;\n> > > > > +\n> > > > > +private:\n> > > > > + static bool created_;\n> > > >\n> > > > Could this be\n> > > > static bool created_ = false;\n> > > >\n> > > > saving the need for the outer initialisation below?\n> > >\n> > > Unfortunately, there will be a compile error:\n> > > `ISO C++ forbids in-class initialization of non-const static member`\n> > > , so let's keep it this way.\n> > >\n> >\n> > Ack.\n> >\n> > > >\n> > > > > +\n> > > > > + VirtualCameraData *cameraData(Camera *camera)\n> > > > > + {\n> > > > > + return static_cast(camera->_d());\n> > > > > + }\n> > > > > +\n> > > > > + DmaBufAllocator dmaBufAllocator_;\n> > > > > +};\n> > > > > +\n> > > > > +/* static */\n> > > > > +bool PipelineHandlerVirtual::created_ = false;\n> > > >\n> > > > This stands out as a curious way to do this initialisation, but I\n> > > > wouldn't block on this here ...\n> > > >\n> > > > I'm curious why we 'need' a static created_ and how that will be used\n> > > > though...\n> > > >\n> > > >\n> > > > > +\n> > > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > > > + const std::vector &supportedResolutions)\n> > > > > + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> > > > > +{\n> > > > > + for (const auto &resolution : supportedResolutions_) {\n> > > > > + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > > > > + minResolutionSize_ = resolution.size;\n> > > > > +\n> > > > > + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > > > > + }\n> > > > > +\n> > > > > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > > > > + streamConfigs_.resize(kMaxStream);\n> > > > > +}\n> > > > > +\n> > > > > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> > > > > + : CameraConfiguration(), data_(data)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > > > > +{\n> > > > > + Status status = Valid;\n> > > > > +\n> > > > > + if (config_.empty()) {\n> > > > > + LOG(Virtual, Error) << \"Empty config\";\n> > > > > + return Invalid;\n> > > > > + }\n> > > > > +\n> > > > > + /* Only one stream is supported */\n> > > > > + if (config_.size() > VirtualCameraData::kMaxStream) {\n> > > > > + config_.resize(VirtualCameraData::kMaxStream);\n> > > > > + status = Adjusted;\n> > > > > + }\n> > > > > +\n> > > > > + for (StreamConfiguration &cfg : config_) {\n> > > > > + bool found = false;\n> > > > > + for (const auto &resolution : data_->supportedResolutions_) {\n> > > > > + if (resolution.size.width == cfg.size.width &&\n> > > > > + resolution.size.height == cfg.size.height) {\n> > > > > + found = true;\n> > > > > + break;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + if (!found) {\n> > > > > + /*\n> > > > > + * \\todo It's a pipeline's decision to choose a\n> > > > > + * resolution when the exact one is not supported.\n> > > > > + * Defining the default logic in PipelineHandler to\n> > > > > + * find the closest resolution would be nice.\n> > > > > + */\n> > > > > + cfg.size = data_->maxResolutionSize_;\n> > > > > + status = Adjusted;\n> > > > > + }\n> > > > > +\n> > > > > + if (cfg.pixelFormat != formats::NV12) {\n> > > > > + cfg.pixelFormat = formats::NV12;\n> > > > > + LOG(Virtual, Debug)\n> > > > > + << \"Stream configuration adjusted to \" << cfg.toString();\n> > > >\n> > > > Wouldn't it be worth reporting this if any adjustment is made?\n> > > >\n> > > > I'd probably have put this at the end of this scope as\n> > > > if (status == Adjusted)\n> > > > LOG(Virtual, Info)\n> > > > << \"Stream configuration adjusted to \" << cfg.toString();\n> > >\n> > > As we share the same `status` among all `StreamConfiguration`s,\n> > > I think it might be better to add an info log for each place we might\n> > > adjust a StreamConfiguration?\n> > >\n> > > Let me know if you think we should have a local variable of status\n> > > for each StreamConfiguration.\n> > >\n> >\n> > The output of \"Stream configuration adjusted to \" << cfg.toString();\n> > should say the full new output configuration right ? I.e. already\n> > including the size if the resolution wasn't found and defaulted to\n> > data_->maxResolutionSize_;\n> >\n> > But I think that only needs to be printed once.\n> \n> Yeah that's right, `cfg.toString()` contains the full output.\n> So, do you mean that you prefer to keep an extra variable in the\n> for loop to record if the current StreamConfiguration is changed?\n> It's only needed for multi-stream support though, but I want to\n> make sure the log still makes sense when we enable multiple\n> streams in the future.\n\nWhen you enable multiple streams, then yes - I think if you are going to\nreturn Adjusted here, it would be worth reporting the entirety of\n\n`StreamConfiguration &cfg`\n\nin the log.\n\nBut right now - this only affects a single stream.\n\n--\nKieran","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 202FFBD80A\n\tfor ;\n\tThu, 3 Oct 2024 10:08:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFA5263528;\n\tThu, 3 Oct 2024 12:08:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F7E163522\n\tfor ;\n\tThu, 3 Oct 2024 12:08:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 378D84C7;\n\tThu, 3 Oct 2024 12:06:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lqJX5Tr+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727950005;\n\tbh=O7AYDowcoMVHGP/MehkrURbVwpMr/wT9y+uozV/Anhk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=lqJX5Tr+lhcInYI/IPnOc2r9R4zujGGTy0eycr+xYGaT6huxUyNocgnTdvrwcLGGB\n\tH16ZmH4q6BjvQsPHE4vL1x9FNDZi56H+eHBMnDwltwYxaB+aMTE3FlvcTbcpX+RvVJ\n\tfvfh8irU7TttORPQZp4L01RNZKR7crY3RXRBLDPo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>\n\t\n\t<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","To":"Cheng-Hao Yang ","Date":"Thu, 03 Oct 2024 11:08:15 +0100","Message-ID":"<172795009536.1619946.4117705877488668304@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31588,"web_url":"https://patchwork.libcamera.org/comment/31588/","msgid":"","date":"2024-10-04T10:00:26","subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 3, 2024 at 6:08 PM Kieran Bingham\n wrote:\n>\n> Quoting Cheng-Hao Yang (2024-10-03 10:58:45)\n> > Hi Kieran,\n> >\n> > On Thu, Oct 3, 2024 at 5:50 PM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Cheng-Hao Yang (2024-10-03 10:16:15)\n> > > > Hi Kieran,\n> > > >\n> > > > On Thu, Oct 3, 2024 at 4:38 PM Kieran Bingham\n> > > > wrote:\n> > > > >\n> > > > > Quoting Harvey Yang (2024-09-30 07:29:44)\n> > > > > > From: Harvey Yang \n> > > > > >\n> > > > > > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > > > > > infrastructure works on devices without using hardware cameras.\n> > > > > >\n> > > > > > Signed-off-by: Harvey Yang \n> > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > meson.build | 1 +\n> > > > > > meson_options.txt | 3 +-\n> > > > > > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > > > > > src/libcamera/pipeline/virtual/virtual.cpp | 317 +++++++++++++++++++++\n> > > > > > src/libcamera/pipeline/virtual/virtual.h | 45 +++\n> > > > > > 5 files changed, 370 insertions(+), 1 deletion(-)\n> > > > > > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > > > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > > create mode 100644 src/libcamera/pipeline/virtual/virtual.h\n> > > > > >\n> > > > > > diff --git a/meson.build b/meson.build\n> > > > > > index 63e45465..5e533b0c 100644\n> > > > > > --- a/meson.build\n> > > > > > +++ b/meson.build\n> > > > > > @@ -214,6 +214,7 @@ pipelines_support = {\n> > > > > > 'simple': ['any'],\n> > > > > > 'uvcvideo': ['any'],\n> > > > > > 'vimc': ['test'],\n> > > > > > + 'virtual': ['test'],\n> > > > > > }\n> > > > > >\n> > > > > > if pipelines.contains('all')\n> > > > > > diff --git a/meson_options.txt b/meson_options.txt\n> > > > > > index 7aa41249..c91cd241 100644\n> > > > > > --- a/meson_options.txt\n> > > > > > +++ b/meson_options.txt\n> > > > > > @@ -53,7 +53,8 @@ option('pipelines',\n> > > > > > 'rpi/vc4',\n> > > > > > 'simple',\n> > > > > > 'uvcvideo',\n> > > > > > - 'vimc'\n> > > > > > + 'vimc',\n> > > > > > + 'virtual'\n> > > > > > ],\n> > > > > > description : 'Select which pipeline handlers to build. If this is set to \"auto\", all the pipelines applicable to the target architecture will be built. If this is set to \"all\", all the pipelines will be built. If both are selected then \"all\" will take precedence.')\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build\n> > > > > > new file mode 100644\n> > > > > > index 00000000..ada1b335\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/libcamera/pipeline/virtual/meson.build\n> > > > > > @@ -0,0 +1,5 @@\n> > > > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > > > +\n> > > > > > +libcamera_internal_sources += files([\n> > > > > > + 'virtual.cpp',\n> > > > > > +])\n> > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..d1584f59\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > > @@ -0,0 +1,317 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2024, Google Inc.\n> > > > > > + *\n> > > > > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \"virtual.h\"\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libcamera/internal/camera.h\"\n> > > > > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +LOG_DEFINE_CATEGORY(Virtual)\n> > > > > > +\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +uint64_t currentTimestamp()\n> > > > > > +{\n> > > > > > + const auto now = std::chrono::steady_clock::now();\n> > > > > > + auto nsecs = std::chrono::duration_cast(\n> > > > > > + now.time_since_epoch());\n> > > > > > +\n> > > > > > + return nsecs.count();\n> > > > > > +}\n> > > > > > +\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > > > +class VirtualCameraConfiguration : public CameraConfiguration\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + static constexpr unsigned int kBufferCount = 4;\n> > > > > > +\n> > > > > > + VirtualCameraConfiguration(VirtualCameraData *data);\n> > > > > > +\n> > > > > > + Status validate() override;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + const VirtualCameraData *data_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +class PipelineHandlerVirtual : public PipelineHandler\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + PipelineHandlerVirtual(CameraManager *manager);\n> > > > > > +\n> > > > > > + std::unique_ptr generateConfiguration(Camera *camera,\n> > > > > > + Span roles) override;\n> > > > > > + int configure(Camera *camera, CameraConfiguration *config) override;\n> > > > > > +\n> > > > > > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > > > + std::vector> *buffers) override;\n> > > > > > +\n> > > > > > + int start(Camera *camera, const ControlList *controls) override;\n> > > > > > + void stopDevice(Camera *camera) override;\n> > > > > > +\n> > > > > > + int queueRequestDevice(Camera *camera, Request *request) override;\n> > > > > > +\n> > > > > > + bool match(DeviceEnumerator *enumerator) override;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + static bool created_;\n> > > > >\n> > > > > Could this be\n> > > > > static bool created_ = false;\n> > > > >\n> > > > > saving the need for the outer initialisation below?\n> > > >\n> > > > Unfortunately, there will be a compile error:\n> > > > `ISO C++ forbids in-class initialization of non-const static member`\n> > > > , so let's keep it this way.\n> > > >\n> > >\n> > > Ack.\n> > >\n> > > > >\n> > > > > > +\n> > > > > > + VirtualCameraData *cameraData(Camera *camera)\n> > > > > > + {\n> > > > > > + return static_cast(camera->_d());\n> > > > > > + }\n> > > > > > +\n> > > > > > + DmaBufAllocator dmaBufAllocator_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +/* static */\n> > > > > > +bool PipelineHandlerVirtual::created_ = false;\n> > > > >\n> > > > > This stands out as a curious way to do this initialisation, but I\n> > > > > wouldn't block on this here ...\n> > > > >\n> > > > > I'm curious why we 'need' a static created_ and how that will be used\n> > > > > though...\n> > > > >\n> > > > >\n> > > > > > +\n> > > > > > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > > > > > + const std::vector &supportedResolutions)\n> > > > > > + : Camera::Private(pipe), supportedResolutions_(supportedResolutions)\n> > > > > > +{\n> > > > > > + for (const auto &resolution : supportedResolutions_) {\n> > > > > > + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> > > > > > + minResolutionSize_ = resolution.size;\n> > > > > > +\n> > > > > > + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > > > > > + streamConfigs_.resize(kMaxStream);\n> > > > > > +}\n> > > > > > +\n> > > > > > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> > > > > > + : CameraConfiguration(), data_(data)\n> > > > > > +{\n> > > > > > +}\n> > > > > > +\n> > > > > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> > > > > > +{\n> > > > > > + Status status = Valid;\n> > > > > > +\n> > > > > > + if (config_.empty()) {\n> > > > > > + LOG(Virtual, Error) << \"Empty config\";\n> > > > > > + return Invalid;\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* Only one stream is supported */\n> > > > > > + if (config_.size() > VirtualCameraData::kMaxStream) {\n> > > > > > + config_.resize(VirtualCameraData::kMaxStream);\n> > > > > > + status = Adjusted;\n> > > > > > + }\n> > > > > > +\n> > > > > > + for (StreamConfiguration &cfg : config_) {\n> > > > > > + bool found = false;\n> > > > > > + for (const auto &resolution : data_->supportedResolutions_) {\n> > > > > > + if (resolution.size.width == cfg.size.width &&\n> > > > > > + resolution.size.height == cfg.size.height) {\n> > > > > > + found = true;\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + if (!found) {\n> > > > > > + /*\n> > > > > > + * \\todo It's a pipeline's decision to choose a\n> > > > > > + * resolution when the exact one is not supported.\n> > > > > > + * Defining the default logic in PipelineHandler to\n> > > > > > + * find the closest resolution would be nice.\n> > > > > > + */\n> > > > > > + cfg.size = data_->maxResolutionSize_;\n> > > > > > + status = Adjusted;\n> > > > > > + }\n> > > > > > +\n> > > > > > + if (cfg.pixelFormat != formats::NV12) {\n> > > > > > + cfg.pixelFormat = formats::NV12;\n> > > > > > + LOG(Virtual, Debug)\n> > > > > > + << \"Stream configuration adjusted to \" << cfg.toString();\n> > > > >\n> > > > > Wouldn't it be worth reporting this if any adjustment is made?\n> > > > >\n> > > > > I'd probably have put this at the end of this scope as\n> > > > > if (status == Adjusted)\n> > > > > LOG(Virtual, Info)\n> > > > > << \"Stream configuration adjusted to \" << cfg.toString();\n> > > >\n> > > > As we share the same `status` among all `StreamConfiguration`s,\n> > > > I think it might be better to add an info log for each place we might\n> > > > adjust a StreamConfiguration?\n> > > >\n> > > > Let me know if you think we should have a local variable of status\n> > > > for each StreamConfiguration.\n> > > >\n> > >\n> > > The output of \"Stream configuration adjusted to \" << cfg.toString();\n> > > should say the full new output configuration right ? I.e. already\n> > > including the size if the resolution wasn't found and defaulted to\n> > > data_->maxResolutionSize_;\n> > >\n> > > But I think that only needs to be printed once.\n> >\n> > Yeah that's right, `cfg.toString()` contains the full output.\n> > So, do you mean that you prefer to keep an extra variable in the\n> > for loop to record if the current StreamConfiguration is changed?\n> > It's only needed for multi-stream support though, but I want to\n> > make sure the log still makes sense when we enable multiple\n> > streams in the future.\n>\n> When you enable multiple streams, then yes - I think if you are going to\n> return Adjusted here, it would be worth reporting the entirety of\n>\n> `StreamConfiguration &cfg`\n>\n> in the log.\n>\n> But right now - this only affects a single stream.\n\nEnabled multi-streams in the next version. Therefore, I added a\nlocal variable that records if the current StreamConfiguration is\nadjusted or not.\n\nBR,\nHarvey\n\n\n>\n> --\n> Kieran","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 BB1DDC3257\n\tfor ;\n\tFri, 4 Oct 2024 10:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 675EF63530;\n\tFri, 4 Oct 2024 12:00:40 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A0F963523\n\tfor ;\n\tFri, 4 Oct 2024 12:00:39 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2fac275471dso19695841fa.0\n\tfor ;\n\tFri, 04 Oct 2024 03:00:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BICSXIv+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728036039; x=1728640839;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=CsKmQHBUaDCBL567LeoWFG0Qk8trN3K3CObLmZW9QnI=;\n\tb=BICSXIv+yBQKtzC0JSYScD886gj6tRbdA4Si2ZV7HCe3xKhyQRuc+Du9WTHIoaIwm3\n\trg9lEQ2JyUbq4hmHGpHL6uEdhigDeGP2wOxJNczMgIVeM/2dE4JY4C26tuK1xgNdpAEX\n\tToH+UoUohY/4gahg2oeT55zmSR4zBolfWTISw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728036039; x=1728640839;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=CsKmQHBUaDCBL567LeoWFG0Qk8trN3K3CObLmZW9QnI=;\n\tb=gufzsN4kpq8PpFAHJrbuR/qlZ18wB9UHGSN/qGHqkr1UpPIVxTlurYK9eSu6ZoQMz9\n\t5volvyUDUQfeofsXcGVJcTnwPVMAJFnhXb7BZaRvfhoXXfgItXfgZRUUcnK87MMlECdK\n\tEIuB2bLHabQWhiry1daOEpFcp6+a4oJingU+Zg6Xtrl/zZtzdqreyFueHXmbpKQ2F3+u\n\tw02GfcNKKGmZgtT/Yd19UZlJIyiudfAVVqRftpFx1sU5oiIi8dcpoNt3WBhX0sejapV5\n\t50QgH5ItmA/iU740DrDmj6Fjp/usrWrdaLVG9GaajyVUBJri2rU/PKVcp2H04Rx83dCN\n\tA1fw==","X-Gm-Message-State":"AOJu0YxxOdJDmHpzyeT5ztB6UKRhPnNmbrmdFFMHAnfVKO5P4NtwyG+/\n\tjr3ZA9rFpHSoSAfPSAeH1Ktt4XJZHYomM3TUiQayx5tJTDXveWorR1b1QEGvA6vxMVcOSosfs5B\n\tn0obKDOtumur1urMQ513Oads7Us1PXi1LVxq8","X-Google-Smtp-Source":"AGHT+IFQaNLzTN3rcaAayNS7DToLByIIHbICt+L40V6OMpz2gmuJIWvcGAn4Bbo0blG3uWNLTIIkig9fLsVimkKR7gU=","X-Received":"by 2002:a05:651c:4ca:b0:2f1:922d:cc49 with SMTP id\n\t38308e7fff4ca-2faea266c29mr17086761fa.19.1728036038633;\n\tFri, 04 Oct 2024 03:00:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930063342.3014837-1-chenghaoyang@google.com>\n\t<20240930063342.3014837-4-chenghaoyang@google.com>\n\t<172794470635.1619946.11514123572767586813@ping.linuxembedded.co.uk>\n\t\n\t<172794904016.4015294.15433706423263467208@ping.linuxembedded.co.uk>\n\t\n\t<172795009536.1619946.4117705877488668304@ping.linuxembedded.co.uk>","In-Reply-To":"<172795009536.1619946.4117705877488668304@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Fri, 4 Oct 2024 18:00:26 +0800","Message-ID":"","Subject":"Re: [PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]