[{"id":31399,"web_url":"https://patchwork.libcamera.org/comment/31399/","msgid":"<4w4vtsp7nqt7yu5tv4kp3upj5hmgrticuhtwvnwgnultnjdi3m@dvggaqwpywpp>","date":"2024-09-26T11:03:24","subject":"Re: [PATCH v12 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 Tue, Sep 10, 2024 at 04:40:16AM 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> 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 | 309 +++++++++++++++++++++\n> src/libcamera/pipeline/virtual/virtual.h | 44 +++\n> 5 files changed, 361 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..56e05528\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -0,0 +1,309 @@\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\nI get this output from iwyu\n\n#include // for ENOBUFS\n#include // for int64_t, uint64_t\n#include // for timespec, clock_get...\n#include // for array\n#include // for basic_ostream, oper...\n#include // for char_traits, operat...\n#include \"libcamera/base/flags.h\" // for operator|, Flags\n#include \"libcamera/pixel_format.h\" // for PixelFormat\n#include \"libcamera/request.h\" // for Request\n\n> +#include \"virtual.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \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/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> +\tconst auto now = std::chrono::steady_clock::now();\n> +\tauto nsecs = std::chrono::duration_cast(\n> +\t\tnow.time_since_epoch());\n> +\n> +\treturn nsecs.count();\n> +}\n\nI won't push on this, but as commented on previous versions this could\nbe replaced by a single line of C++. Not a big deal, you can keep what\nyou have here\n\n> +\n> +} /* namespace */\n> +\n> +class VirtualCameraConfiguration : public CameraConfiguration\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> +/* static */\n> +bool PipelineHandlerVirtual::created_ = false;\n> +\n> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> +\t\t\t\t std::vector supportedResolutions)\n\nsupportedResolution should be passed by reference\n\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> +\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\nYou should calculate stride and frameSize after having adjusted the\nformat to NV12\n\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\nFits on one line\n\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tfor (const StreamRole role : roles) {\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> +\n> +\t\tstd::map> streamFormats;\n> +\t\tPixelFormat pixelFormat = formats::NV12;\n> +\t\tstreamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };\n\nCan easily be shortened to\n\n\t\tstreamFormats[pixelFormat] = { { data->minResolutionSize_,\n\t\t\t\t\t\t data->maxResolutionSize_ } };\n\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\tconfig->addConfiguration(cfg);\n> +\t}\n> +\n> +\tASSERT(config->validate() != CameraConfiguration::Invalid);\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 (auto [i, c] : utils::enumerate(*config))\n> +\t\tc.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\nI thought this had to be dropped\n\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> +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..77823c41\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -0,0 +1,44 @@\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\nI get this output from iwyu\n\n../src/libcamera/pipeline/virtual/virtual.h should add these lines:\n#include \"libcamera/base/span.h\" // for Span\n#include \"libcamera/camera.h\" // for Camera, CameraConf...\n#include \"libcamera/geometry.h\" // for Size\n#include \"libcamera/stream.h\" // for Stream, StreamRole...\n\n../src/libcamera/pipeline/virtual/virtual.h should remove these lines:\n- #include // lines 13-13\n\n\n> +#pragma once\n> +\n> +#include \n> +\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> +\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\nWith the above fixed you can keep my tag\n\nThanks\n j\n\n> +\n> +} /* namespace libcamera */\n> --\n> 2.46.0.598.g6f2099f65c-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 DB315C3257\n\tfor ;\n\tThu, 26 Sep 2024 11:03:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E09E06350F;\n\tThu, 26 Sep 2024 13:03: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 8BFB1634F9\n\tfor ;\n\tThu, 26 Sep 2024 13:03:28 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-229.net.vodafone.it\n\t[5.90.51.229])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1321C8D4;\n\tThu, 26 Sep 2024 13:02:00 +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=\"V3RmtB2O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727348520;\n\tbh=l0+rKMEfPhdWQvrMSz3s0kaYr050xaUmZd4qzfKQwxY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V3RmtB2OP0vwam9T8ji5J/Jtc8J6vupccpUAvT9xEFK2JkLqvbgGtYqykQU9+WwTt\n\tjyLarseukMuB4ntVeDHaGFwazmlpXK7P0rM/LKS0FCvfzedXRdzblGkBsscXrYcwXd\n\teFmLxj1oMWFGlqiujDHp1nodmIHRfgGC22qKbuFU=","Date":"Thu, 26 Sep 2024 13:03:24 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Subject":"Re: [PATCH v12 3/7] libcamera: virtual: Add VirtualPipelineHandler","Message-ID":"<4w4vtsp7nqt7yu5tv4kp3upj5hmgrticuhtwvnwgnultnjdi3m@dvggaqwpywpp>","References":"<20240910044834.2477701-1-chenghaoyang@google.com>\n\t<20240910044834.2477701-4-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240910044834.2477701-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":31414,"web_url":"https://patchwork.libcamera.org/comment/31414/","msgid":"","date":"2024-09-26T16:32:15","subject":"Re: [PATCH v12 3/7] libcamera: virtual: Add VirtualPipelineHandler","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 26, 2024 at 7:03 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Tue, Sep 10, 2024 at 04:40:16AM 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> > 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 | 309 +++++++++++++++++++++\n> > src/libcamera/pipeline/virtual/virtual.h | 44 +++\n> > 5 files changed, 361 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..56e05528\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -0,0 +1,309 @@\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>\n> I get this output from iwyu\n>\n> #include // for ENOBUFS\n> #include // for int64_t, uint64_t\n> #include // for timespec, clock_get...\n> #include // for array\n> #include // for basic_ostream, oper...\n> #include // for char_traits, operat...\n> #include \"libcamera/base/flags.h\" // for operator|, Flags\n> #include \"libcamera/pixel_format.h\" // for PixelFormat\n> #include \"libcamera/request.h\" // for Request\n\nAdded.\n\n>\n> > +#include \"virtual.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \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/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> I won't push on this, but as commented on previous versions this could\n> be replaced by a single line of C++. Not a big deal, you can keep what\n> you have here\n\nHmm, I still think defining it as a function is cleaner. Let's keep it as is :)\n\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> > + 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> > +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> > + std::vector supportedResolutions)\n>\n> supportedResolution should be passed by reference\n\nUsed const reference, and removed `std::move` below.\n\n>\n> > + : Camera::Private(pipe), supportedResolutions_(std::move(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> > + 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> You should calculate stride and frameSize after having adjusted the\n> format to NV12\n\nRight, moved this block after checking pixelFormat.\n\n>\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 \" << 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> > + 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> Fits on one line\n\nDone\n\n>\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_, data->maxResolutionSize_ } };\n>\n> Can easily be shortened to\n>\n> streamFormats[pixelFormat] = { { data->minResolutionSize_,\n> data->maxResolutionSize_ } };\n\nDone\n\n>\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> > + /* \\todo Read from the virtual video if any. */\n>\n> I thought this had to be dropped\n\nDone\n\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> > +\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..77823c41\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -0,0 +1,44 @@\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>\n> I get this output from iwyu\n>\n> ../src/libcamera/pipeline/virtual/virtual.h should add these lines:\n> #include \"libcamera/base/span.h\" // for Span\n\nDidn't use this in this header. Only in the source file.\n\n> #include \"libcamera/camera.h\" // for Camera, CameraConf...\n\nOnly used Camera::Private, and I already included\n`libcamera/internal/camera.h`\n\n> #include \"libcamera/geometry.h\" // for Size\n> #include \"libcamera/stream.h\" // for Stream, StreamRole...\n>\n> ../src/libcamera/pipeline/virtual/virtual.h should remove these lines:\n> - #include // lines 13-13\n\nDone\n\n\n>\n>\n> > +#pragma once\n> > +\n> > +#include \n> > +\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> > + 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> With the above fixed you can keep my tag\n>\n> Thanks\n> j\n>\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.46.0.598.g6f2099f65c-goog\n> >\n\n\n\n--\nBR,\nHarvey Yang","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 D443FC0F1B\n\tfor ;\n\tThu, 26 Sep 2024 16:32:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 914756350F;\n\tThu, 26 Sep 2024 18:32:54 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64521634F9\n\tfor ;\n\tThu, 26 Sep 2024 18:32:52 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id\n\t2adb3069b0e04-53690eb134bso117e87.0\n\tfor ;\n\tThu, 26 Sep 2024 09:32:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"jkkJTbV3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20230601; t=1727368372; x=1727973172;\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=6wYYfW74U97WhQUXJuyes2dkaqz00Krr/th5QLBt4Vc=;\n\tb=jkkJTbV3jGuLIOCUdJUsRokrGNijRn9r8qbFlT5t/TwhUPHf78654LBAmGHBHgVSwd\n\tetcjs/sqek3zfa2ROCFFLfv7ZQO6FQQw5VfsqRv7GFN8uJ/PDClG6ZGszw1Rpte8Hpvm\n\t09dQoRWJ9fDrxDC0QiFu6srV6IKcX2/4cNnBx21moy5upta/kw33RQbUkG8kN5R9AJcJ\n\t3ypezXOna/QoYEIoLWH67R4XO6mOafQLvcxbcro9YmJD9e3Flx1KV8n7vH1JISWW+f4u\n\t1Y4eVPRys3n1DhDGmWVZZRou+HJaES6BTSkIs2vDk3R9s5xQtf+ozTb1TNFFIVHWE10K\n\tZMPg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727368372; x=1727973172;\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=6wYYfW74U97WhQUXJuyes2dkaqz00Krr/th5QLBt4Vc=;\n\tb=q9PE58FjPiPahM3QZFUKh9vm7KV71WYUK6UXxGlIJvz5cJQ+XVA+sSk0EfO1TUXzn8\n\tRF1q2qyuTX/L3BN8MzVV4RLTgeWhBUh/BmGPmjE5tZp5V4KpU3r8F4xd0up6/1X+i09/\n\tmSm0s7MlwRb7W7rIOFTxpfM/0eVUEpvOsMX2IG5zkMlY2y8MvZ2y3nAquJidqu96OKDm\n\t35L5RSQqxn4jtl2BnNfxVqlQlOaIvlyDARItcFZrdbv3/AM8CB8CdnxlsW3ZlXk3TM3V\n\ttZtbZOxv5yqhHylp2hXKBvp4LmRrK6kuxfRhTBlSQ14aW5gnl+xUNtWqR+AkBVQo9MKG\n\trTjg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXerAJRkH/lID7cQBeC9mwKzrABcUbxLZ4gBbRqLMkdYkRUvN1nT4rpT4FYuwS8EeqGm9WlNxEx63EEQK5Iqn0=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy22s7bpHULZD79z88b/QdkEeM/1Oi9kXPlZmL0mBXtkTUoCe0Y\n\t0Jfz1wZYNpmk4ODo7RNevKFaUfU62/z/NLtE2Py02uVG3+fvP1jiJDzPzJDbWXagUDqptzvCmXd\n\tMnogc0H6jOknLcIKbuk7azasb/fUj8VmgvcvA","X-Google-Smtp-Source":"AGHT+IEJId+p+qTEZtEGh3juM6hUNrpCSpIrh0h9UY3BnoF1NhPa5DAz2CmeUOSUeHZ+zmAgEm0tbjRSOOvkHrLABWw=","X-Received":"by 2002:a05:6512:ba2:b0:52e:8475:7c23 with SMTP id\n\t2adb3069b0e04-5389863bf49mr279745e87.7.1727368371251; Thu, 26 Sep 2024\n\t09:32:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20240910044834.2477701-1-chenghaoyang@google.com>\n\t<20240910044834.2477701-4-chenghaoyang@google.com>\n\t<4w4vtsp7nqt7yu5tv4kp3upj5hmgrticuhtwvnwgnultnjdi3m@dvggaqwpywpp>","In-Reply-To":"<4w4vtsp7nqt7yu5tv4kp3upj5hmgrticuhtwvnwgnultnjdi3m@dvggaqwpywpp>","From":"Cheng-Hao Yang ","Date":"Fri, 27 Sep 2024 00:32:15 +0800","Message-ID":"","Subject":"Re: [PATCH v12 3/7] libcamera: virtual: Add VirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","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\" "}}]