[{"id":27095,"web_url":"https://patchwork.libcamera.org/comment/27095/","msgid":"<20230515072019.oqfivn7dbbfpqpz2@lati>","date":"2023-05-15T07:20:19","subject":"Re: [libcamera-devel] [PATCH v5 2/2] libcamera: pipeline: Add\n\tVirtualPipelineHandler","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\n mostly cosmetic comments, my concern is on 1/2 and I wuold like to\nhear from others, maybe it has been discussed in the past, but it\nworries me a bit.\n\nOn Fri, Apr 07, 2023 at 06:20:50PM +0800, Harvey Yang via libcamera-devel wrote:\n> Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> infrastructure works on devices without using hardware cameras.\n>\n> Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is\n> required to find the virtual camera with id `Virtual0`.\n\nWhy ? For the Android HAL ? If that's the case this is not related to\nthis patch and you can drop this line\n\n>\n> [todo Read frames from the virtual video if any]\n\nI think you can drop this as well\n\n>\n> Test the configurations can be generated and reported with cam -I:\n> \"\"\"\n> build/src/apps/cam/cam -c 1 -I\n> [45:19:28.901456135] [2611530] INFO IPAManager ipa_manager.cpp:143\n> libcamera is not installed. Adding\n> '/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src\n> /ipa' to the IPA search path\n> [45:19:28.904364465] [2611530] INFO Camera camera_manager.cpp:293\n> libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)\n> Using camera Virtual0 as cam0\n> 0: 1920x1080-NV12\n> * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)\n> - 1280x720\n> - 1280x800\n> - 1360x768\n> - 1366x768\n> - 1440x900\n> - 1280x1024\n> - 1536x864\n> - 1280x1080\n> - 1600x900\n> - 1400x1050\n> - 1680x1050\n> - 1920x1080\n> \"\"\"\n>\n> \"\"\"\n> build/src/apps/cam/cam -l\n> [550:47:04.505850647] [1969734] INFO IPAManager ipa_manager.cpp:143\n> libcamera is not installed. Adding ... to the IPA search path\n> [550:47:04.509307667] [1969734] INFO Camera camera_manager.cpp:293\n> libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)\n> Available cameras:\n> 1: (Virtual0)\n> \"\"\"\n>\n> Using qcam should receive blank (all green) frames:\n> \"\"\"\n> build/src/apps/qcam/qcam -c Virtual0\n> \"\"\"\n\nAll the examples are better placed in the cover letter imho, there's\nno need to have them in the commit history ?\n\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 | 302 +++++++++++++++++++++\n> 4 files changed, 310 insertions(+), 1 deletion(-)\n> create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n>\n> diff --git a/meson.build b/meson.build\n> index 0f89b45a..c2c4ba5f 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -177,6 +177,7 @@ pipelines_support = {\n> 'simple': arch_arm,\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 78a78b72..a0a75e7f 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -47,7 +47,8 @@ option('pipelines',\n> 'rkisp1',\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..ba7ff754\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_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..78616fab\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -0,0 +1,302 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * fake.cpp - Pipeline handler for fake cameras\n> + */\n\nThis is not called virtual, please update this and the copyright year\nmaybe\n\n> +\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/formats.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(VIRTUAL)\n\nI would use \"Virtual\"\n\n> +\n> +static const ControlInfoMap::Map VirtualControls = {\n> +\t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n\nIs this just for testing ?\n\n> +};\n> +\n> +uint64_t CurrentTimestamp()\n\nfunction names start with lowecase letter\nif you want a static helper please place it in an anonymous namespace\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> +class VirtualCameraData : public Camera::Private\n> +{\n> +public:\n> +\tstruct Resolution {\n> +\t\tSize size;\n> +\t\tstd::vector frame_rates;\n> +\t\tstd::vector formats;\n> +\t};\n> +\tVirtualCameraData(PipelineHandler *pipe)\n> +\t\t: Camera::Private(pipe)\n> +\t{\n> +\t}\n> +\n> +\t~VirtualCameraData() = default;\n> +\n> +\tstd::vector supportedResolutions_;\n\nDo you plan to initialize this based on some configuration file ?\nCurrently it is dynamically initialized at match() time with static\ndata, so it could very well be a static const class member.\n\n> +\n> +\tStream stream_;\n> +};\n> +\n> +class VirtualCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\tstatic constexpr unsigned int kBufferCount = 4; // 4~6\n\nIn the whole file, please do not use C++ comment style, we use C99\nones. Also, this comment seems a development leftover so please drop\nit\n\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 const StreamRoles &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> +\tVirtualCameraData *cameraData(Camera *camera)\n> +\t{\n> +\t\treturn static_cast(camera->_d());\n> +\t}\n> +\n> +\tHeapAllocator heapAllocator_;\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// TODO: check if we should limit |config_.size()|\n\nPlease use \\todo as the rest of the codebase does\n\nAnd yes, you only have one stream, so you should resize(1) and Adjust\nOtherwise, you should make VirtualCameraData::stream_ an array of\nmultiple entries if you want to support multiple streams\n\n> +\n> +\tSize maxSize;\n> +\tfor (const auto &resolution : data_->supportedResolutions_)\n> +\t\tmaxSize = std::max(maxSize, resolution.size);\n\nif supportedResolutions_ is constructed at match() time and never\nchanged at run-time you could very well find the max and min element\nonce at match() time\n\n> +\n> +\tfor (StreamConfiguration &cfg : config_) {\n\nAnd you can spare this cycle if you only support one stream\n\n> +\t\tbool found = false;\n> +\t\tfor (const auto &resolution : data_->supportedResolutions_) {\n> +\t\t\tif (resolution.size.width >= cfg.size.width && resolution.size.height >= cfg.size.height) {\n\nPlease break this rather long line\n\nAlso, is this correct ? You \"found = true\" when you hit a resolution\n >= than the requested one. Shouldn't you \"Adjust\" if the resolution is\nnot exactly equal to the one you're looking for ?\n\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\tcfg.size = maxSize;\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.setStream(const_cast(&data_->stream_));\n> +\n> +\t\tcfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> +\t: PipelineHandler(manager)\n> +{\n> +}\n> +\n> +std::unique_ptr PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t\t\t const StreamRoles &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\nLike in validate() if you only support one stream you should only\nconsider a single role\n\n> +\n> +\tSize minSize, sensorResolution;\n> +\tfor (const auto &resolution : data->supportedResolutions_) {\n> +\t\tif (minSize.isNull() || minSize > resolution.size)\n> +\t\t\tminSize = resolution.size;\n> +\n> +\t\tsensorResolution = std::max(sensorResolution, resolution.size);\n> +\t}\n> +\n> +\tfor (const StreamRole role : roles) {\n> +\t\tstd::map> streamFormats;\n> +\t\tunsigned int bufferCount;\n> +\t\tPixelFormat pixelFormat;\n> +\n> +\t\tswitch (role) {\n> +\t\tcase StreamRole::StillCapture:\n> +\t\t\tpixelFormat = formats::NV12;\n> +\t\t\tbufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\t\t\tstreamFormats[pixelFormat] = { { minSize, sensorResolution } };\n> +\n> +\t\t\tbreak;\n> +\n> +\t\tcase StreamRole::Raw: {\n> +\t\t\t// TODO: check\n> +\t\t\tpixelFormat = formats::SBGGR10;\n> +\t\t\tbufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\t\t\tstreamFormats[pixelFormat] = { { minSize, sensorResolution } };\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase StreamRole::Viewfinder:\n> +\t\tcase StreamRole::VideoRecording: {\n> +\t\t\tpixelFormat = formats::NV12;\n> +\t\t\tbufferCount = VirtualCameraConfiguration::kBufferCount;\n> +\t\t\tstreamFormats[pixelFormat] = { { minSize, sensorResolution } };\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\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\tStreamFormats formats(streamFormats);\n> +\t\tStreamConfiguration cfg(formats);\n> +\t\tcfg.size = sensorResolution;\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.bufferCount = bufferCount;\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n> +\n> +\tif (config->validate() == CameraConfiguration::Invalid) {\n> +\t\tconfig.reset();\n> +\t\treturn config;\n\nyou can remove this line as you would return config anyway a line\nbelow\n\n> +\t}\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerVirtual::configure(Camera *camera, CameraConfiguration *config)\n> +{\n> +\t(void)camera;\n> +\t(void)config;\n\nplease use [[maybe_unused]] for not used parameters\n\n> +\t// Nothing to be done.\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerVirtual::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t std::vector> *buffers)\n> +{\n> +\tif (!heapAllocator_.isValid())\n> +\t\treturn -ENOBUFS;\n> +\n> +\treturn heapAllocator_.exportFrameBuffers(camera, stream, buffers);\n> +}\n> +\n> +int PipelineHandlerVirtual::start(Camera *camera, const ControlList *controls)\n> +{\n> +\t(void)camera;\n> +\t(void)controls;\n> +\t// TODO: Start reading the virtual video if any.\n> +\treturn 0;\n> +}\n> +\n> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n> +{\n> +\t(void)camera;\n> +\t// TODO: Reset the virtual video if any.\n> +}\n> +\n> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\t(void)camera;\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> +bool PipelineHandlerVirtual::match(DeviceEnumerator *enumerator)\n> +{\n> +\t(void)enumerator;\n> +\n> +\t// TODO: Add virtual cameras according to a config file.\n> +\n> +\tstd::unique_ptr data = std::make_unique(this);\n> +\n> +\tdata->supportedResolutions_.resize(2);\n> +\tdata->supportedResolutions_[0] = { .size = Size(1920, 1080), .frame_rates = { 30 }, .formats = { \"YCbCr_420_888\" } };\n> +\tdata->supportedResolutions_[1] = { .size = Size(1280, 720), .frame_rates = { 30, 60 }, .formats = { \"YCbCr_420_888\" } };\n\nNot sure what you will use \"formats\" for, but those are the Android\nformat names. Could you use the libcamera ones ?\n\nThanks\n j\n\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 = VirtualControls;\n> +\tint64_t min_frame_duration = 30, max_frame_duration = 60;\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{ &data->stream_ };\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 false; // Prevent infinite loops for now\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual)\n> +\n> +} /* namespace libcamera */\n> --\n> 2.40.0\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 0F349BDE6B\n\tfor ;\n\tMon, 15 May 2023 07:20:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60091627D4;\n\tMon, 15 May 2023 09:20:25 +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 C49D56039A\n\tfor ;\n\tMon, 15 May 2023 09:20:23 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E4076D6;\n\tMon, 15 May 2023 09:20:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684135225;\n\tbh=ixjg3tbpnO0btNPChAROfolh9EC19puFmCXTyaLrsdU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2X+u01KVk2PqZIH/P06TFpp4W2pFYIjHO9ONA9d/pPc0HSqgb1kjhFQI84+Lswo2M\n\tkukQzLBos0LpUmbU5BB6FN4W59Da56OF90ffxVX23pcKKs1Wj9dxYXFKKCDVtyiArr\n\tiWA3LWerhhJ2MIrYImRnSCOzfLO6vTnuceG6D4ZEYWQHqwttGzPihp/17QThtHWaKO\n\tltqIF3lWedU9+ZTwZ8fCjlFH12JYwfzqmGPRFVqWGNKewcH/T39HS58iq3nxZLt6lC\n\tva8tizpER2YElfRQUzrOGPkacjAvLugjKhnebapdIp38Sd2j5yZ/D81YEkn95PNkqM\n\tWW5xVn1AboNkA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1684135213;\n\tbh=ixjg3tbpnO0btNPChAROfolh9EC19puFmCXTyaLrsdU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AgJxlicmK+bSbwzcHAAjAaGG9brBuqJg5V0B1dcQ5Fpw4xANircP/s44Ogr7maiU9\n\tKspI6fYuXKVl0iaJJS4keBxFO0CjlZ4+47in55mqUogncOcRpJ2oqAbczpQDfI+lrt\n\tTE2uG6Xm582r+JNFynSEC4yEFmkAdQsfVj5FgjxU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AgJxlicm\"; dkim-atps=neutral","Date":"Mon, 15 May 2023 09:20:19 +0200","To":"Harvey Yang ","Message-ID":"<20230515072019.oqfivn7dbbfpqpz2@lati>","References":"<20230407102050.17537-1-harveyycyang@gmail.com>\n\t<20230407102050.17537-3-harveyycyang@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230407102050.17537-3-harveyycyang@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] libcamera: pipeline: Add\n\tVirtualPipelineHandler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Harvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30488,"web_url":"https://patchwork.libcamera.org/comment/30488/","msgid":"","date":"2024-07-26T09:58:18","subject":"Re: [libcamera-devel] [PATCH v5 2/2] libcamera: pipeline: Add\n\tVirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo,\n\nThe fixes are applied to v6.\n\nOn Mon, May 15, 2023 at 3:20 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> mostly cosmetic comments, my concern is on 1/2 and I wuold like to\n> hear from others, maybe it has been discussed in the past, but it\n> worries me a bit.\n>\n> On Fri, Apr 07, 2023 at 06:20:50PM +0800, Harvey Yang via libcamera-devel\n> wrote:\n> > Add VirtualPipelineHandler for more unit tests and verfiy libcamera\n> > infrastructure works on devices without using hardware cameras.\n> >\n> > Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is\n> > required to find the virtual camera with id `Virtual0`.\n>\n> Why ? For the Android HAL ? If that's the case this is not related to\n> this patch and you can drop this line\n>\n>\nYes, it's only for chromeos that uses android adapter. Removed.\n\n\n> >\n> > [todo Read frames from the virtual video if any]\n>\n> I think you can drop this as well\n>\n>\nRemoved.\n\n\n> >\n> > Test the configurations can be generated and reported with cam -I:\n> > \"\"\"\n> > build/src/apps/cam/cam -c 1 -I\n> > [45:19:28.901456135] [2611530] INFO IPAManager ipa_manager.cpp:143\n> > libcamera is not installed. Adding\n> >\n> '/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src\n> > /ipa' to the IPA search path\n> > [45:19:28.904364465] [2611530] INFO Camera camera_manager.cpp:293\n> > libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)\n> > Using camera Virtual0 as cam0\n> > 0: 1920x1080-NV12\n> > * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)\n> > - 1280x720\n> > - 1280x800\n> > - 1360x768\n> > - 1366x768\n> > - 1440x900\n> > - 1280x1024\n> > - 1536x864\n> > - 1280x1080\n> > - 1600x900\n> > - 1400x1050\n> > - 1680x1050\n> > - 1920x1080\n> > \"\"\"\n> >\n> > \"\"\"\n> > build/src/apps/cam/cam -l\n> > [550:47:04.505850647] [1969734] INFO IPAManager ipa_manager.cpp:143\n> > libcamera is not installed. Adding ... to the IPA search path\n> > [550:47:04.509307667] [1969734] INFO Camera camera_manager.cpp:293\n> > libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)\n> > Available cameras:\n> > 1: (Virtual0)\n> > \"\"\"\n> >\n> > Using qcam should receive blank (all green) frames:\n> > \"\"\"\n> > build/src/apps/qcam/qcam -c Virtual0\n> > \"\"\"\n>\n> All the examples are better placed in the cover letter imho, there's\n> no need to have them in the commit history ?\n>\n>\nRemoved.\n\n\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 | 302 +++++++++++++++++++++\n> > 4 files changed, 310 insertions(+), 1 deletion(-)\n> > create mode 100644 src/libcamera/pipeline/virtual/meson.build\n> > create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp\n> >\n> > diff --git a/meson.build b/meson.build\n> > index 0f89b45a..c2c4ba5f 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -177,6 +177,7 @@ pipelines_support = {\n> > 'simple': arch_arm,\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 78a78b72..a0a75e7f 100644\n> > --- a/meson_options.txt\n> > +++ b/meson_options.txt\n> > @@ -47,7 +47,8 @@ option('pipelines',\n> > 'rkisp1',\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..ba7ff754\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_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..78616fab\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -0,0 +1,302 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * fake.cpp - Pipeline handler for fake cameras\n> > + */\n>\n> This is not called virtual, please update this and the copyright year\n> maybe\n>\n>\nDone\n\n\n> > +\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/formats.h\"\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(VIRTUAL)\n>\n> I would use \"Virtual\"\n>\n>\nDone\n\n\n> > +\n> > +static const ControlInfoMap::Map VirtualControls = {\n> > + { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>\n> Is this just for testing ?\n>\n>\nRemoved.\n\n\n> > +};\n> > +\n> > +uint64_t CurrentTimestamp()\n>\n> function names start with lowecase letter\n> if you want a static helper please place it in an anonymous namespace\n>\n>\nDone\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> > +class VirtualCameraData : public Camera::Private\n> > +{\n> > +public:\n> > + struct Resolution {\n> > + Size size;\n> > + std::vector frame_rates;\n> > + std::vector formats;\n> > + };\n> > + VirtualCameraData(PipelineHandler *pipe)\n> > + : Camera::Private(pipe)\n> > + {\n> > + }\n> > +\n> > + ~VirtualCameraData() = default;\n> > +\n> > + std::vector supportedResolutions_;\n>\n> Do you plan to initialize this based on some configuration file ?\n> Currently it is dynamically initialized at match() time with static\n> data, so it could very well be a static const class member.\n>\n>\nYes, we'll initialize this with a configuration file in the following\npatches.\nTherefore, let's keep it as a member variable for now.\n\n\n> > +\n> > + Stream stream_;\n> > +};\n> > +\n> > +class VirtualCameraConfiguration : public CameraConfiguration\n> > +{\n> > +public:\n> > + static constexpr unsigned int kBufferCount = 4; // 4~6\n>\n> In the whole file, please do not use C++ comment style, we use C99\n> ones. Also, this comment seems a development leftover so please drop\n> it\n>\n>\nDropped\n\n\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> > + const\n> StreamRoles &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> > + VirtualCameraData *cameraData(Camera *camera)\n> > + {\n> > + return static_cast(camera->_d());\n> > + }\n> > +\n> > + HeapAllocator heapAllocator_;\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> > + // TODO: check if we should limit |config_.size()|\n>\n> Please use \\todo as the rest of the codebase does\n>\n>\nDone\n\n\n> And yes, you only have one stream, so you should resize(1) and Adjust\n> Otherwise, you should make VirtualCameraData::stream_ an array of\n> multiple entries if you want to support multiple streams\n>\n>\nUpdated and added a comment.\n\n\n> > +\n> > + Size maxSize;\n> > + for (const auto &resolution : data_->supportedResolutions_)\n> > + maxSize = std::max(maxSize, resolution.size);\n>\n> if supportedResolutions_ is constructed at match() time and never\n> changed at run-time you could very well find the max and min element\n> once at match() time\n>\n>\nHmm, I'd prefer to keep the same source of truth. It doesn't affect the\nperformance that much as well. WDYT?\n\n\n> > +\n> > + for (StreamConfiguration &cfg : config_) {\n>\n> And you can spare this cycle if you only support one stream\n>\n>\nI'd prefer to keep this, in case we support more than one stream in the\nfuture.\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>\n> Please break this rather long line\n>\n>\nDone\n\n\n> Also, is this correct ? You \"found = true\" when you hit a resolution\n> >= than the requested one. Shouldn't you \"Adjust\" if the resolution is\n> not exactly equal to the one you're looking for ?\n>\n\nYou're right. Updated.\n\n\n> > + found = true;\n> > + break;\n> > + }\n> > + }\n> > +\n> > + if (!found) {\n> > + cfg.size = maxSize;\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.setStream(const_cast(&data_->stream_));\n> > +\n> > + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)\n> > + : PipelineHandler(manager)\n> > +{\n> > +}\n> > +\n> > +std::unique_ptr\n> PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > +\n> const StreamRoles &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> Like in validate() if you only support one stream you should only\n> consider a single role\n>\n>\nI think it's a different thing here: We currently only support only one\nstream, but the StreamRole of it can be one of {StillCapture, Raw,\nViewFinder, VideoRecorder}. Should the caller get the default\nconfiguration of each StreamRole one by one, or it can get all four\nby once?\n\n\n> > +\n> > + Size minSize, sensorResolution;\n> > + for (const auto &resolution : data->supportedResolutions_) {\n> > + if (minSize.isNull() || minSize > resolution.size)\n> > + minSize = resolution.size;\n> > +\n> > + sensorResolution = std::max(sensorResolution,\n> resolution.size);\n> > + }\n> > +\n> > + for (const StreamRole role : roles) {\n> > + std::map>\n> streamFormats;\n> > + unsigned int bufferCount;\n> > + PixelFormat pixelFormat;\n> > +\n> > + switch (role) {\n> > + case StreamRole::StillCapture:\n> > + pixelFormat = formats::NV12;\n> > + bufferCount =\n> VirtualCameraConfiguration::kBufferCount;\n> > + streamFormats[pixelFormat] = { { minSize,\n> sensorResolution } };\n> > +\n> > + break;\n> > +\n> > + case StreamRole::Raw: {\n> > + // TODO: check\n> > + pixelFormat = formats::SBGGR10;\n> > + bufferCount =\n> VirtualCameraConfiguration::kBufferCount;\n> > + streamFormats[pixelFormat] = { { minSize,\n> sensorResolution } };\n> > +\n> > + break;\n> > + }\n> > +\n> > + case StreamRole::Viewfinder:\n> > + case StreamRole::VideoRecording: {\n> > + pixelFormat = formats::NV12;\n> > + bufferCount =\n> VirtualCameraConfiguration::kBufferCount;\n> > + streamFormats[pixelFormat] = { { minSize,\n> sensorResolution } };\n> > +\n> > + break;\n> > + }\n> > +\n> > + default:\n> > + LOG(VIRTUAL, Error)\n> > + << \"Requested stream role not supported: \"\n> << role;\n> > + config.reset();\n> > + return config;\n> > + }\n> > +\n> > + StreamFormats formats(streamFormats);\n> > + StreamConfiguration cfg(formats);\n> > + cfg.size = sensorResolution;\n> > + cfg.pixelFormat = pixelFormat;\n> > + cfg.bufferCount = bufferCount;\n> > + config->addConfiguration(cfg);\n> > + }\n> > +\n> > + if (config->validate() == CameraConfiguration::Invalid) {\n> > + config.reset();\n> > + return config;\n>\n> you can remove this line as you would return config anyway a line\n> below\n>\n>\nDone.\n\n\n> > + }\n> > +\n> > + return config;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::configure(Camera *camera,\n> CameraConfiguration *config)\n> > +{\n> > + (void)camera;\n> > + (void)config;\n>\n> please use [[maybe_unused]] for not used parameters\n>\n>\nDone\n\n\n> > + // Nothing to be done.\n> > + return 0;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::exportFrameBuffers(Camera *camera, Stream\n> *stream,\n> > +\n> std::vector> *buffers)\n> > +{\n> > + if (!heapAllocator_.isValid())\n> > + return -ENOBUFS;\n> > +\n> > + return heapAllocator_.exportFrameBuffers(camera, stream, buffers);\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::start(Camera *camera, const ControlList\n> *controls)\n> > +{\n> > + (void)camera;\n> > + (void)controls;\n> > + // TODO: Start reading the virtual video if any.\n> > + return 0;\n> > +}\n> > +\n> > +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n> > +{\n> > + (void)camera;\n> > + // TODO: Reset the virtual video if any.\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request\n> *request)\n> > +{\n> > + (void)camera;\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> > +bool PipelineHandlerVirtual::match(DeviceEnumerator *enumerator)\n> > +{\n> > + (void)enumerator;\n> > +\n> > + // TODO: Add virtual cameras according to a config file.\n> > +\n> > + std::unique_ptr data =\n> std::make_unique(this);\n> > +\n> > + data->supportedResolutions_.resize(2);\n> > + data->supportedResolutions_[0] = { .size = Size(1920, 1080),\n> .frame_rates = { 30 }, .formats = { \"YCbCr_420_888\" } };\n> > + data->supportedResolutions_[1] = { .size = Size(1280, 720),\n> .frame_rates = { 30, 60 }, .formats = { \"YCbCr_420_888\" } };\n>\n> Not sure what you will use \"formats\" for, but those are the Android\n> format names. Could you use the libcamera ones ?\n>\n>\nRemoved `formats` in `VirtualCameraData::Resolution`.\n\n\n> Thanks\n> j\n>\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 = VirtualControls;\n> > + int64_t min_frame_duration = 30, max_frame_duration = 60;\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{ &data->stream_ };\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 false; // Prevent infinite loops for now\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual)\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.40.0\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 C4929BDC71\n\tfor ;\n\tFri, 26 Jul 2024 09:58:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62C9063370;\n\tFri, 26 Jul 2024 11:58:31 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 661B263369\n\tfor ;\n\tFri, 26 Jul 2024 11:58:30 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id\n\t38308e7fff4ca-2f029e9c9cfso16783111fa.2\n\tfor ;\n\tFri, 26 Jul 2024 02:58:30 -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=\"Sj1hJn5Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1721987910; x=1722592710;\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=hzlh2y0rYlblR7PcA2HxQA/c5sr/Y8LkchBRPVEFBi4=;\n\tb=Sj1hJn5YOmcAftu5F44UdeRgX33Rsem3d49wMBBNSg906MQh+32REgotDBzD3+uI4W\n\tvRKsqffk4NeMWSNKELFB8OpYSq6TQ6LjuPYlG+KPgJByyezC1iOlE11UJUtj4hTywbp8\n\tC9Qcs05y5/QJD32RMTSn9wzuIuDfgk7gzEad8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1721987910; x=1722592710;\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=hzlh2y0rYlblR7PcA2HxQA/c5sr/Y8LkchBRPVEFBi4=;\n\tb=wVKiOpHRNBrtwOMxh5Xk2qtSvekZr+UXC0nACV8pam0WiW+lvJrsvlTE5Z2+NgdHXA\n\twTrQp10qd8/UnxqNJJhB+wrwRzowQSrzznz6Vngh9qDK0YgZiPFnk8bFlGRR7LxwKlS+\n\tUGRgK8rSAVGhixrtRQxTdui1/ZYpJY5O2GXA3+6wyhzRt3vXVhA9m5Xzm8+euGBHBRWU\n\txDMSwJWO76K+oXKUrKd02k6C2/CLgkMH+LkjCzT1+TolmU5nYfGVeRRkQXrL0RYTjyzz\n\tRchP5OJdXnAbxwYRFBVPqhR8KZ3U0vv4XGk8L0oK6qe4AnWamyTJDQcHpLwOmIN9a9KR\n\tv6eg==","X-Gm-Message-State":"AOJu0Yykd/ay3CjwWONHmboE/4LFNjn9iiq9225eN1IKUsbu7lA6DTFm\n\tKTzrSgBoNxq3Mu/N4vQwbuGqeFVrw8z+IuVWzN0KeFdt6/uZQtoQKR0F5iPMMGDy85rfgvUBIPC\n\t7BuRL2DM8NPXL500DKQ4nA763j23PoMeyKUncvf/u3g9t4KE=","X-Google-Smtp-Source":"AGHT+IGJHBfYZurQzqOzFAkaMIQ9eI1mEvemZCr+mTt7Gpi0J7youSgU0Q9MV2eqnBCd3/DX0f1cmmHWQOyObCv3rVs=","X-Received":"by 2002:a2e:9b0a:0:b0:2ef:2dfd:15db with SMTP id\n\t38308e7fff4ca-2f03db6608bmr38215131fa.19.1721987909354;\n\tFri, 26 Jul 2024 02:58:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20230407102050.17537-1-harveyycyang@gmail.com>\n\t<20230407102050.17537-3-harveyycyang@gmail.com>\n\t<20230515072019.oqfivn7dbbfpqpz2@lati>","In-Reply-To":"<20230515072019.oqfivn7dbbfpqpz2@lati>","From":"Cheng-Hao Yang ","Date":"Fri, 26 Jul 2024 17:58:18 +0800","Message-ID":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] libcamera: pipeline: Add\n\tVirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Harvey Yang ","Content-Type":"multipart/alternative; boundary=\"000000000000a523a0061e238dd9\"","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\" "}}]