[{"id":31118,"web_url":"https://patchwork.libcamera.org/comment/31118/","msgid":"","date":"2024-09-09T08:47:29","subject":"Re: [PATCH v11 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Sat, Sep 07, 2024 at 02:28:28PM GMT, Harvey Yang wrote:\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> ---\n> meson.build | 1 +\n> meson_options.txt | 3 +-\n> src/libcamera/pipeline/virtual/meson.build | 5 +\n> src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++\n> src/libcamera/pipeline/virtual/virtual.h | 89 +++++++\n> 5 files changed, 364 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 432ae133..ff9a70cf 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..f85ec3dd\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -0,0 +1,267 @@\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\n#include \n\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera.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\nAs suggested on v9, this could probably be replaced with a few lines\nof C++ using std::chrono. Have you tested it and found it not to be\nworking ? Not a big deal though\n\n> +{\n> +\tstruct timespec ts;\n> +\tif (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {\n> +\t\tLOG(Virtual, Error) << \"Get clock time fails\";\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;\n> +}\n> +\n> +} /* namespace */\n> +\n> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> +\t\t\t\t std::vector supportedResolutions)\n> +\t: Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))\n> +{\n> +\tfor (const auto &resolution : supportedResolutions_) {\n> +\t\tif (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)\n> +\t\t\tminResolutionSize_ = resolution.size;\n> +\n> +\t\tmaxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);\n> +\t}\n> +\n> +\t/* \\todo Support multiple streams and pass multi_stream_test */\n> +\tstreamConfigs_.resize(kMaxStream);\n> +}\n> +\n> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> +\t: CameraConfiguration(), data_(data)\n> +{\n> +}\n> +\n> +CameraConfiguration::Status VirtualCameraConfiguration::validate()\n> +{\n> +\tStatus status = Valid;\n> +\n> +\tif (config_.empty()) {\n> +\t\tLOG(Virtual, Error) << \"Empty config\";\n> +\t\treturn Invalid;\n> +\t}\n> +\n> +\t/* Only one stream is supported */\n> +\tif (config_.size() > VirtualCameraData::kMaxStream) {\n> +\t\tconfig_.resize(VirtualCameraData::kMaxStream);\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tfor (StreamConfiguration &cfg : config_) {\n\nKnowing that you're going to add support for multiple streams I think\nit's fine having a loop here already\n\n> +\t\tbool found = false;\n> +\t\tfor (const auto &resolution : data_->supportedResolutions_) {\n> +\t\t\tif (resolution.size.width == cfg.size.width &&\n> +\t\t\t resolution.size.height == cfg.size.height) {\n> +\t\t\t\tfound = true;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!found) {\n> +\t\t\t/*\n> +\t\t\t * \\todo It's a pipeline's decision to choose a\n> +\t\t\t * resolution when the exact one is not supported.\n> +\t\t\t * Defining the default logic in PipelineHandler to\n> +\t\t\t * find the closest resolution would be nice.\n> +\t\t\t */\n> +\t\t\tcfg.size = data_->maxResolutionSize_;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> +\t\tcfg.stride = info.stride(cfg.size.width, 0, 1);\n> +\t\tcfg.frameSize = info.frameSize(cfg.size, 1);\n> +\n> +\t\tcfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\n> +\t\tif (cfg.pixelFormat != formats::NV12) {\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tLOG(Virtual, Debug)\n> +\t\t\t\t<< \"Stream configuration adjusted to \" << cfg.toString();\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> +\t: PipelineHandler(manager),\n> +\t dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> +\t\t\t DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> +\t\t\t DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> +{\n> +}\n> +\n> +std::unique_ptr\n> +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t Span roles)\n> +{\n> +\tVirtualCameraData *data = cameraData(camera);\n> +\tauto config =\n> +\t\tstd::make_unique(data);\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tfor (const StreamRole role : roles) {\n\nI presume the loop is here to support multiple streams too\n\n> +\t\tstd::map> streamFormats;\n> +\t\tPixelFormat pixelFormat = formats::NV12;\n> +\t\tstreamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n> +\t\tStreamFormats formats(streamFormats);\n> +\t\tStreamConfiguration cfg(formats);\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.size = data->maxResolutionSize_;\n> +\t\tcfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\n> +\t\tswitch (role) {\n> +\t\tcase StreamRole::StillCapture:\n> +\t\tcase StreamRole::VideoRecording:\n> +\t\tcase StreamRole::Viewfinder:\n> +\t\t\tbreak;\n> +\n> +\t\tcase StreamRole::Raw:\n> +\t\tdefault:\n> +\t\t\tLOG(Virtual, Error)\n> +\t\t\t\t<< \"Requested stream role not supported: \" << role;\n> +\t\t\tconfig.reset();\n> +\t\t\treturn config;\n> +\t\t}\n\nI would have placed the switch before populating cfg, but it's ok, you\nclear config_ anyway in case of errors\n\n> +\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n> +\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\tconfig.reset();\n\nIf this happens, it's a programming error so you could even ASSERT\nhere. No problem, the ipu3 pipeline does the same here.\n\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerVirtual::configure(Camera *camera,\n> +\t\t\t\t CameraConfiguration *config)\n> +{\n> +\tVirtualCameraData *data = cameraData(camera);\n> +\tfor (size_t i = 0; i < config->size(); ++i)\n\nOr\n\tfor (auto [i, c] : utils::enumerate(*config))\n\t\tc.setStream(&data->streamConfigs_[i].stream);\n\nnot a big deal\n\n> +\t\tconfig->at(i).setStream(&data->streamConfigs_[i].stream);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> +\t\t\t\t\t Stream *stream,\n> +\t\t\t\t\t std::vector> *buffers)\n> +{\n> +\tif (!dmaBufAllocator_.isValid())\n> +\t\treturn -ENOBUFS;\n> +\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\n> +\tauto info = PixelFormatInfo::info(config.pixelFormat);\n> +\n> +\tstd::vector planeSizes;\n> +\tfor (size_t i = 0; i < info.planes.size(); ++i)\n> +\t\tplaneSizes.push_back(info.planeSize(config.size, i));\n> +\n> +\treturn dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);\n> +}\n> +\n> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> +\t\t\t\t [[maybe_unused]] const ControlList *controls)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> +{\n> +}\n> +\n> +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> +\t\t\t\t\t Request *request)\n> +{\n> +\t/* \\todo Read from the virtual video if any. */\n> +\tfor (auto it : request->buffers())\n> +\t\tcompleteBuffer(request, it.second);\n> +\n> +\trequest->metadata().set(controls::SensorTimestamp, currentTimestamp());\n> +\tcompleteRequest(request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +// static\n\nI'm not sure anymore how to get the message through: No C++ comments please\n\n> +bool PipelineHandlerVirtual::created_ = false;\n\nYou can move this to the beginng of the file if you prefer. Otherwise\nit's fine.\n\n\n> +\n> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)\n> +{\n> +\tif (created_)\n> +\t\treturn false;\n> +\n> +\tcreated_ = true;\n> +\n> +\t/* \\todo Add virtual cameras according to a config file. */\n> +\n> +\tstd::vector supportedResolutions;\n> +\tsupportedResolutions.resize(2);\n> +\tsupportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };\n> +\tsupportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };\n> +\n> +\tstd::unique_ptr data =\n> +\t\tstd::make_unique(this, supportedResolutions);\n> +\n> +\tdata->properties_.set(properties::Location, properties::CameraLocationFront);\n> +\tdata->properties_.set(properties::Model, \"Virtual Video Device\");\n> +\tdata->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });\n> +\n> +\t/* \\todo Set FrameDurationLimits based on config. */\n> +\tControlInfoMap::Map controls;\n> +\tint64_t min_frame_duration = 33333, max_frame_duration = 33333;\n> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> +\n> +\t/* Create and register the camera. */\n> +\tstd::set streams;\n> +\tfor (auto &streamConfig : data->streamConfigs_)\n> +\t\tstreams.insert(&streamConfig.stream);\n> +\n> +\tconst std::string id = \"Virtual0\";\n> +\tstd::shared_ptr camera = Camera::create(std::move(data), id, streams);\n> +\tregisterCamera(std::move(camera));\n> +\n> +\treturn true;\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..fb3dbcad\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -0,0 +1,89 @@\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> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +class VirtualCameraData : public Camera::Private\n> +{\n> +public:\n> +\tconst static unsigned int kMaxStream = 1;\n> +\n> +\tstruct Resolution {\n> +\t\tSize size;\n> +\t\tstd::vector frameRates;\n> +\t};\n> +\tstruct StreamConfig {\n> +\t\tStream stream;\n> +\t};\n> +\n> +\tVirtualCameraData(PipelineHandler *pipe,\n> +\t\t\t std::vector supportedResolutions);\n> +\n> +\t~VirtualCameraData() = default;\n> +\n> +\tconst std::vector supportedResolutions_;\n> +\tSize maxResolutionSize_;\n> +\tSize minResolutionSize_;\n> +\n> +\tstd::vector streamConfigs_;\n> +};\n> +\n> +class VirtualCameraConfiguration : public CameraConfiguration\n\nIt seems to me only VirtualCameraData is used by the other files. The\nother classes declarations, as not used outside of virtual.cpp, could\nbe moved there.\n\nOnly nits here and there, with them addressed:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +{\n> +public:\n> +\tstatic constexpr unsigned int kBufferCount = 4;\n> +\n> +\tVirtualCameraConfiguration(VirtualCameraData *data);\n> +\n> +\tStatus validate() override;\n> +\n> +private:\n> +\tconst VirtualCameraData *data_;\n> +};\n> +\n> +class PipelineHandlerVirtual : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerVirtual(CameraManager *manager);\n> +\n> +\tstd::unique_ptr generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t Span roles) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t std::vector> *buffers) override;\n> +\n> +\tint start(Camera *camera, const ControlList *controls) override;\n> +\tvoid stopDevice(Camera *camera) override;\n> +\n> +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> +\tbool match(DeviceEnumerator *enumerator) override;\n> +\n> +private:\n> +\tstatic bool created_;\n> +\n> +\tVirtualCameraData *cameraData(Camera *camera)\n> +\t{\n> +\t\treturn static_cast(camera->_d());\n> +\t}\n> +\n> +\tDmaBufAllocator dmaBufAllocator_;\n> +};\n> +\n> +} /* namespace libcamera */\n> --\n> 2.46.0.469.g59c65b2a67-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 1C39DC324C\n\tfor ;\n\tMon, 9 Sep 2024 08:47:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80EB5634F5;\n\tMon, 9 Sep 2024 10:47:37 +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 7345A634E4\n\tfor ;\n\tMon, 9 Sep 2024 10:47:35 +0200 (CEST)","from ideasonboard.com (unknown [213.208.157.109])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CCD095A4;\n\tMon, 9 Sep 2024 10:46:18 +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=\"uYJMg+Wv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725871579;\n\tbh=4v4pA8L4zChHaaDOpvTDJwYSfd7j6+ExXvRSVly9Gj0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uYJMg+WvjS85cjyJAngthprX8hdb94J0eYf+BKbK7/aG65u41cepIGzgWKYhO5nak\n\t6SLeSO/RP0+UV0dFhkW9VA4xYcAXXP5cIh8aqiTmpqdq5eb8wA3iEd8ij82tzs9Z/y\n\ta/GcqkHH7rCRCyYII9j2/5iuImTGyZKW5B6S1dJ0=","Date":"Mon, 9 Sep 2024 10:47:29 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v11 3/7] libcamera: virtual: Add VirtualPipelineHandler","Message-ID":"","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-4-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240907143110.2210711-4-chenghaoyang@google.com>","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":31148,"web_url":"https://patchwork.libcamera.org/comment/31148/","msgid":"","date":"2024-09-10T04:49:43","subject":"Re: [PATCH v11 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 Jacopo,\n\nOn Mon, Sep 9, 2024 at 4:47 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Sat, Sep 07, 2024 at 02:28:28PM GMT, Harvey Yang wrote:\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> > ---\n> > meson.build | 1 +\n> > meson_options.txt | 3 +-\n> > src/libcamera/pipeline/virtual/meson.build | 5 +\n> > src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++\n> > src/libcamera/pipeline/virtual/virtual.h | 89 +++++++\n> > 5 files changed, 364 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 432ae133..ff9a70cf 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\n> is set to \"auto\", all the pipelines applicable to the target architecture\n> will be built. If this is set to \"all\", all the pipelines will be built. If\n> both are selected then \"all\" will take precedence.')\n> >\n> > diff --git a/src/libcamera/pipeline/virtual/meson.build\n> 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\n> b/src/libcamera/pipeline/virtual/virtual.cpp\n> > new file mode 100644\n> > index 00000000..f85ec3dd\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -0,0 +1,267 @@\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>\n> #include \n>\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera.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> As suggested on v9, this could probably be replaced with a few lines\n> of C++ using std::chrono. Have you tested it and found it not to be\n> working ? Not a big deal though\n>\n\nAh sorry... I struggled a lot with the user interface of emails and\npatchwork, so from time to time I lose track of comments.\n\nYes, comparing the timestamps, those two approaches produce similar\ntimestamps. Updated in v12.\n\nI think we can keep the function `currentTimestamp` though, just replacing\nthe implementation. Let me know if you think I should get rid of it as well.\n\n\n>\n> > +{\n> > + struct timespec ts;\n> > + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {\n> > + LOG(Virtual, Error) << \"Get clock time fails\";\n> > + return 0;\n> > + }\n> > +\n> > + return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > + std::vector\n> supportedResolutions)\n> > + : Camera::Private(pipe),\n> supportedResolutions_(std::move(supportedResolutions))\n> > +{\n> > + for (const auto &resolution : supportedResolutions_) {\n> > + if (minResolutionSize_.isNull() || minResolutionSize_ >\n> resolution.size)\n> > + minResolutionSize_ = resolution.size;\n> > +\n> > + maxResolutionSize_ = std::max(maxResolutionSize_,\n> resolution.size);\n> > + }\n> > +\n> > + /* \\todo Support multiple streams and pass multi_stream_test */\n> > + streamConfigs_.resize(kMaxStream);\n> > +}\n> > +\n> >\n> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData\n> *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>\n> Knowing that you're going to add support for multiple streams I think\n> it's fine having a loop here already\n>\n>\nYes, thanks :)\n\n\n> > + bool found = false;\n> > + for (const auto &resolution :\n> 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> > + const PixelFormatInfo &info =\n> 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> > + if (cfg.pixelFormat != formats::NV12) {\n> > + cfg.pixelFormat = formats::NV12;\n> > + LOG(Virtual, Debug)\n> > + << \"Stream configuration adjusted to \" <<\n> cfg.toString();\n> > + status = Adjusted;\n> > + }\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> > + : PipelineHandler(manager),\n> > + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > +\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 =\n> > + std::make_unique(data);\n> > +\n> > + if (roles.empty())\n> > + return config;\n> > +\n> > + for (const StreamRole role : roles) {\n>\n> I presume the loop is here to support multiple streams too\n>\n>\nYes, exactly.\n\n\n> > + std::map>\n> 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> > + 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: \"\n> << role;\n> > + config.reset();\n> > + return config;\n> > + }\n>\n> I would have placed the switch before populating cfg, but it's ok, you\n> clear config_ anyway in case of errors\n>\n>\nRight, the switch and the check on `role` can be done earlier.\nUpdated in v12.\n\n\n> > +\n> > + config->addConfiguration(cfg);\n> > + }\n> > +\n> > + if (config->validate() == CameraConfiguration::Invalid)\n> > + config.reset();\n>\n> If this happens, it's a programming error so you could even ASSERT\n> here. No problem, the ipu3 pipeline does the same here.\n>\n>\nAh right, updated with ASSERT in v12.\n\n\n> > +\n> > + return config;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::configure(Camera *camera,\n> > + CameraConfiguration *config)\n> > +{\n> > + VirtualCameraData *data = cameraData(camera);\n> > + for (size_t i = 0; i < config->size(); ++i)\n>\n> Or\n> for (auto [i, c] : utils::enumerate(*config))\n> c.setStream(&data->streamConfigs_[i].stream);\n>\n> not a big deal\n>\n\nThanks! Updated in v12.\n\n\n>\n> > + config->at(i).setStream(&data->streamConfigs_[i].stream);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera\n> *camera,\n> > + Stream *stream,\n> > +\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,\n> planeSizes, buffers);\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > + [[maybe_unused]] const ControlList\n> *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\n> *camera,\n> > + Request *request)\n> > +{\n> > + /* \\todo Read from the virtual video if any. */\n> > + for (auto it : request->buffers())\n> > + completeBuffer(request, it.second);\n> > +\n> > + request->metadata().set(controls::SensorTimestamp,\n> currentTimestamp());\n> > + completeRequest(request);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +// static\n>\n> I'm not sure anymore how to get the message through: No C++ comments please\n>\n\nSorry, updated.\n\n\n>\n> > +bool PipelineHandlerVirtual::created_ = false;\n>\n> You can move this to the beginng of the file if you prefer. Otherwise\n> it's fine.\n>\n\nDone.\n\n\n>\n>\n> > +\n> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> *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\n> = { 30 } };\n> > + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates =\n> { 30 } };\n> > +\n> > + std::unique_ptr data =\n> > + std::make_unique(this,\n> supportedResolutions);\n> > +\n> > + data->properties_.set(properties::Location,\n> properties::CameraLocationFront);\n> > + data->properties_.set(properties::Model, \"Virtual Video Device\");\n> > + data->properties_.set(properties::PixelArrayActiveAreas, {\n> 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] =\n> ControlInfo(min_frame_duration, max_frame_duration);\n> > + data->controlInfo_ = ControlInfoMap(std::move(controls),\n> 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),\n> id, streams);\n> > + registerCamera(std::move(camera));\n> > +\n> > + return true;\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/virtual/virtual.h\n> b/src/libcamera/pipeline/virtual/virtual.h\n> > new file mode 100644\n> > index 00000000..fb3dbcad\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -0,0 +1,89 @@\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> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/dma_buf_allocator.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> > + 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> > +class VirtualCameraConfiguration : public CameraConfiguration\n>\n> It seems to me only VirtualCameraData is used by the other files. The\n> other classes declarations, as not used outside of virtual.cpp, could\n> be moved there.\n>\n> Only nits here and there, with them addressed:\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\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\n> *camera,\n> > +\n> Span roles) override;\n> > + int configure(Camera *camera, CameraConfiguration *config)\n> override;\n> > +\n> > + int exportFrameBuffers(Camera *camera, Stream *stream,\n> > + std::vector>\n> *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> > + VirtualCameraData *cameraData(Camera *camera)\n> > + {\n> > + return static_cast(camera->_d());\n> > + }\n> > +\n> > + DmaBufAllocator dmaBufAllocator_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.46.0.469.g59c65b2a67-goog\n> >\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 DBEABC324C\n\tfor ;\n\tTue, 10 Sep 2024 04:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A418634FB;\n\tTue, 10 Sep 2024 06:49:58 +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 21800634FB\n\tfor ;\n\tTue, 10 Sep 2024 06:49:56 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2f74e468baeso61173741fa.2\n\tfor ;\n\tMon, 09 Sep 2024 21:49:56 -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=\"UhOyrjvP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725943795; x=1726548595;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=N+GrUAciVCXHSWsgX44FuXXQpEPXv8Vh8jy5eiGAGT4=;\n\tb=UhOyrjvPb7a+ViRR5mAa+fB54WUx+jriqG5SHwSh9Pt3bkzu01SYypoOpvWuWEYSVP\n\tQHlKSHDObu+IW7sFD4qC2Fshh4YVoKPx6XArHiO5iQGsulCzf78CYsgdIgHh3IWR3hkC\n\tGPR1dY1blov7orIKxDi7oAAlQhZw2YHoXzs6s=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725943795; x=1726548595;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=N+GrUAciVCXHSWsgX44FuXXQpEPXv8Vh8jy5eiGAGT4=;\n\tb=MHRxAfPuL67/SsoO2tcQiwR+gP5zPwwRZqGTaB1pkel8WK+h3IrroCp2Iu1DhozlTC\n\twd2Nv5XDujksskpU56pp05gaF96E6wJWBMXASzojeccfk26yZHTD+hPhfh8W3f27R4/f\n\tl7fo6nRFsrwOb81X+KOQYUgfNO6RXv/M8RHWwqlaRk65kKaZ/q9XNi2/y8U3+TyHMD8S\n\tRflx6SACyK+Yu3Jynvq/UDlq0P7G6PNm8wH2whd249Ha5eGM9w9IP6Uxz2TSfeAHmmRU\n\t+OT0ag9o6vFrPWfGIa62gGEsRJirzvmouGaSbGAzjXuuPgZCv/VG+eoAG5lH7pEoFS9m\n\t3xnA==","X-Gm-Message-State":"AOJu0YxPzJEC1Wmd9PXGS4ZSnlcKrPn8DebCe16k15M5XT2D4lXjf6zP\n\twVqrBDNJfivbM5JRiZbRH+axaCR5FcVV0A074jmYNoORyZDva5A1z17rKW4xWJrxI6Ac4UJGSat\n\tdiTNjjBlThTv3gcrA/2p++06CZxoO4NI3EfrSMvPnYjzA1qs=","X-Google-Smtp-Source":"AGHT+IFjVVWyhZxuBLTdNiJ9ZHsBgPdtWjhceHFSVTFKL8jHS0KRP9ccreks6Q0zhKROibGje4srQaGaETqJOV8HId0=","X-Received":"by 2002:a05:651c:502:b0:2f5:487:e87a with SMTP id\n\t38308e7fff4ca-2f75b892283mr48994711fa.18.1725943794982;\n\tMon, 09 Sep 2024 21:49:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20240907143110.2210711-1-chenghaoyang@google.com>\n\t<20240907143110.2210711-4-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 10 Sep 2024 12:49:43 +0800","Message-ID":"","Subject":"Re: [PATCH v11 3/7] libcamera: virtual: Add VirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000cd77c30621bc9a9e\"","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\" "}}]