[{"id":30928,"web_url":"https://patchwork.libcamera.org/comment/30928/","msgid":"","date":"2024-08-27T15:42:58","subject":"Re: [PATCH v9 3/8] libcamera: pipeline: 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, Aug 20, 2024 at 04:23:34PM 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 | 251 +++++++++++++++++++++\n> src/libcamera/pipeline/virtual/virtual.h | 78 +++++++\n\nDo you expect other components to include this header in future ? If\nnot, you can move its content to the .cpp file I guess\n\n> 5 files changed, 337 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 f946eba94..3cad3249a 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -222,6 +222,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 7aa412491..c91cd241a 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 000000000..ba7ff754e\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 000000000..74eb8c7ad\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -0,0 +1,251 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * virtual.cpp - Pipeline handler for virtual cameras\n> + */\n> +\n\nYou should include the header for the standard library construct you\nuse. I see vectors, maps, unique_ptrs etc\n\n> +#include \"virtual.h\"\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera.h\"\n\nThe internal header includes the public one by definition\n\n> +#include \"libcamera/internal/formats.h\"\n\nThis doesn't as is generated. I wonder if it\nshould.\n\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> +\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\nCould https://en.cppreference.com/w/cpp/chrono/steady_clock/now save\nyou a custom function ?\n\nIn example:\n\n\tconst auto now = std::chrono::steady_clock::now();\n\tauto nsecs = std::chrono::duration_cast(now.time_since_epoch());\n\tstd::cout << nsecs.count();\n\nshould give you the time in nanoseconds since system boot (if I got it\nright)\n\n\n> +\n> +} // namespace\n\nnit: /* namespace */\nhere and in other places\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/* Currently only one stream is supported */\n> +\tif (config_.size() > 1) {\n> +\t\tconfig_.resize(1);\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tSize maxSize;\n> +\tfor (const auto &resolution : data_->supportedResolutions_)\n> +\t\tmaxSize = std::max(maxSize, resolution.size);\n> +\n> +\tfor (StreamConfiguration &cfg : config_) {\n\nyou only have config, or in the next patches will this be augmented ?\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\tcfg.size = maxSize;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n\nso it's either the exact resolution or the biggest available one ?\n\nAs you have a single config it's easy to get the closest one to the\nrequested size, if it doesn't match exactly one of the supported\nresolutions.\n\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\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> +\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\nAs you do this min/max search in a few places, why not doing it when\nyou construct data->supportedResolutions_ once ?\n\n> +\t}\n> +\n> +\tfor (const StreamRole role : roles) {\n\nIf the pipeline handler works with a single stream, you should only\nconsider the first role maybe\n\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\nbufferCount and streamFormats can be assigned outsize of the cases,\nthey're the same for all roles\n\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> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerVirtual::configure(\n> +\t[[maybe_unused]] Camera *camera,\n> +\t[[maybe_unused]] CameraConfiguration *config)\n\nint PipelineHandlerVirtual::configure([[maybe_unused]] Camera *camera,\n\t\t\t\t [[maybe_unused]] CameraConfiguration *config)\n\n> +{\n> +\t// Nothing to be done.\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerVirtual::exportFrameBuffers(\n> +\t[[maybe_unused]] Camera *camera,\n> +\tStream *stream,\n> +\tstd::vector> *buffers)\n\nint PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,\n\t\t\t\t\t Stream *stream,\n\t\t\t\t\t std::vector> *buffers)\n\nif you prefer\n\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\nah that's probably why you return count from\nDmaBufferAllocator::exportBuffers()\n\n> +}\n> +\n> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> +\t\t\t\t [[maybe_unused]] const ControlList *controls)\n> +{\n> +\t/* \\todo Start reading the virtual video if any. */\n> +\treturn 0;\n> +}\n> +\n> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> +{\n> +\t/* \\todo Reset the virtual video if any. */\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> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *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 } };\n> +\tdata->supportedResolutions_[1] = { .size = Size(1280, 720), .frame_rates = { 30, 60 } };\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 = 30, max_frame_duration = 60;\n\ndoesn't match the above frame rates and it should be expressed in\nmicroseconds. I would suggest for this patch to set both framerates to\n30 and initialize FrameDurationLimits with {33333, 333333}\n\nIt's not a big deal however, it will be replaced later in the series\n\n\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\nI presume this will also be changed in next patches...\n\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, \"virtual\")\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> new file mode 100644\n> index 000000000..6fc6b34d8\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * virtual.h - Pipeline handler for virtual cameras\n> + */\n> +\n> +#pragma once\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> +\tstruct Resolution {\n> +\t\tSize size;\n> +\t\tstd::vector frame_rates;\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> +\n> +\tStream stream_;\n> +};\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> +\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.184.g6999bdac58-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 1A992C324C\n\tfor ;\n\tTue, 27 Aug 2024 15:43:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13DBB63460;\n\tTue, 27 Aug 2024 17:43:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF5F361901\n\tfor ;\n\tTue, 27 Aug 2024 17:43:01 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 605049FF;\n\tTue, 27 Aug 2024 17:41:54 +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=\"Ql1SbB6J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724773314;\n\tbh=XQXdApOW7JEwwCd20BRXUZFHlEG/RwmhzWYB30A8tus=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ql1SbB6JK7Q/vpo8+LD/lQxxceqzCqKSG8951oJfzBv0Gge7DRQnS+i+51N8jcVZu\n\t2ouVhcGokqASgtK8YxTy9Zl1MnSqev0td1c9DULW0/NI5hBiYUn7tAkHBEYWTfUb70\n\tKU7V6IqGntTvMBBQHiqt7iz30Vj7uWMYqnoNhEX8=","Date":"Tue, 27 Aug 2024 17:42:58 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler","Message-ID":"","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-4-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240820172202.526547-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":30958,"web_url":"https://patchwork.libcamera.org/comment/30958/","msgid":"","date":"2024-08-29T19:57:58","subject":"Re: [PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks for the review, Jacopo!\n\nOn Tue, Aug 27, 2024 at 5:43 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Tue, Aug 20, 2024 at 04:23:34PM 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 | 251 +++++++++++++++++++++\n> > src/libcamera/pipeline/virtual/virtual.h | 78 +++++++\n>\n> Do you expect other components to include this header in future ? If\n> not, you can move its content to the .cpp file I guess\n>\n>\nActually `virtual/parser.h` needs to include it to return VirtualCameraData\nwhen parsing the yaml config file [1]. Does that count :p?\n\n[1]: https://patchwork.libcamera.org/patch/20971/\n\n> 5 files changed, 337 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 f946eba94..3cad3249a 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -222,6 +222,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 7aa412491..c91cd241a 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 000000000..ba7ff754e\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 000000000..74eb8c7ad\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > @@ -0,0 +1,251 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * virtual.cpp - Pipeline handler for virtual cameras\n> > + */\n> > +\n>\n> You should include the header for the standard library construct you\n> use. I see vectors, maps, unique_ptrs etc\n>\nDone, please check again.\n\n\n>\n> > +#include \"virtual.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera.h\"\n>\n> The internal header includes the public one by definition\n>\nAck. Removed the public one.\n\n\n>\n> > +#include \"libcamera/internal/formats.h\"\n>\n> This doesn't as is generated. I wonder if it\n> should.\n>\nKeeping `#include `.\n\n\n>\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> > + 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> Could https://en.cppreference.com/w/cpp/chrono/steady_clock/now save\n> you a custom function ?\n>\n> In example:\n>\n> const auto now = std::chrono::steady_clock::now();\n> auto nsecs =\n> std::chrono::duration_cast(now.time_since_epoch());\n> std::cout << nsecs.count();\n>\n> should give you the time in nanoseconds since system boot (if I got it\n> right)\n>\n>\n> > +\n> > +} // namespace\n>\n> nit: /* namespace */\n> here and in other places\n>\n> Done\n\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> > + /* Currently only one stream is supported */\n> > + if (config_.size() > 1) {\n> > + config_.resize(1);\n> > + status = Adjusted;\n> > + }\n> > +\n> > + Size maxSize;\n> > + for (const auto &resolution : data_->supportedResolutions_)\n> > + maxSize = std::max(maxSize, resolution.size);\n> > +\n> > + for (StreamConfiguration &cfg : config_) {\n>\n> you only have config, or in the next patches will this be augmented ?\n>\n> Do you mean that I should check `sensorConfig` or `orientation` as well?\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> > + cfg.size = maxSize;\n> > + status = Adjusted;\n> > + }\n>\n> so it's either the exact resolution or the biggest available one ?\n>\n> As you have a single config it's easy to get the closest one to the\n> requested size, if it doesn't match exactly one of the supported\n> resolutions.\n>\n\nHmm, I think it's a bit hard to define the \"closest\". Do we compare\nthe area size directly? Do we prefer a size that has both larger or\nthe same height and width?\n\n\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> > + 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> > + 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> As you do this min/max search in a few places, why not doing it when\n> you construct data->supportedResolutions_ once ?\n>\nAdded in VirtualCameraData.\n\n\n>\n> > + }\n> > +\n> > + for (const StreamRole role : roles) {\n>\n> If the pipeline handler works with a single stream, you should only\n> consider the first role maybe\n>\nActually I think there's no reason to only support one Stream in\nVirtual Pipeline Handler. The raw stream doesn't seem to be\nproperly supported in the following patches though. I think we\nshould drop the support of raw.\n\n\n>\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> bufferCount and streamFormats can be assigned outsize of the cases,\n> they're the same for all roles\n>\nDone\n\n\n>\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> > +\n> > + return config;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::configure(\n> > + [[maybe_unused]] Camera *camera,\n> > + [[maybe_unused]] CameraConfiguration *config)\n>\n> int PipelineHandlerVirtual::configure([[maybe_unused]] Camera *camera,\n> [[maybe_unused]] CameraConfiguration\n> *config)\n>\n> Done\n\n\n> > +{\n> > + // Nothing to be done.\n> > + return 0;\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::exportFrameBuffers(\n> > + [[maybe_unused]] Camera *camera,\n> > + Stream *stream,\n> > + std::vector> *buffers)\n>\n> int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera\n> *camera,\n> Stream *stream,\n>\n> std::vector> *buffers)\n>\n> if you prefer\n>\n> Done\n\n\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> ah that's probably why you return count from\n> DmaBufferAllocator::exportBuffers()\n>\nExactly :)\n\n\n>\n> > +}\n> > +\n> > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > + [[maybe_unused]] const ControlList\n> *controls)\n> > +{\n> > + /* \\todo Start reading the virtual video if any. */\n> > + return 0;\n> > +}\n> > +\n> > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > + /* \\todo Reset the virtual video if any. */\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> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> *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 } };\n> > + data->supportedResolutions_[1] = { .size = Size(1280, 720),\n> .frame_rates = { 30, 60 } };\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 = 30, max_frame_duration = 60;\n>\n> doesn't match the above frame rates and it should be expressed in\n> microseconds. I would suggest for this patch to set both framerates to\n> 30 and initialize FrameDurationLimits with {33333, 333333}\n>\n> It's not a big deal however, it will be replaced later in the series\n>\n>\nDone, thanks!\n\n\n>\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> I presume this will also be changed in next patches...\n>\nUpdated in this patch, with a static boolean though.\n\n\n>\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 000000000..6fc6b34d8\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * virtual.h - Pipeline handler for virtual cameras\n> > + */\n> > +\n> > +#pragma once\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> > + struct Resolution {\n> > + Size size;\n> > + std::vector frame_rates;\n> > + };\n> > + VirtualCameraData(PipelineHandler *pipe)\n> > + : Camera::Private(pipe)\n> > + {\n> > + }\n> > +\n> > + ~VirtualCameraData() = default;\n> > +\n> > + std::vector supportedResolutions_;\n> > +\n> > + Stream stream_;\n> > +};\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\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> > + 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.184.g6999bdac58-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 EDEB9C324C\n\tfor ;\n\tThu, 29 Aug 2024 19:58:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95068634C8;\n\tThu, 29 Aug 2024 21:58:12 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F4AE63458\n\tfor ;\n\tThu, 29 Aug 2024 21:58:10 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f50966c478so10104111fa.1\n\tfor ;\n\tThu, 29 Aug 2024 12:58:10 -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=\"Pakptck/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724961490; x=1725566290;\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=fKQ4recxjmdY4RtmOXTZ1thJai5+1zhvudtkE402uuM=;\n\tb=Pakptck/5LUDk9yawWxDnblpZpX3eunePdzx+0Uo50SyHISLhBh3ZxWQMT3/NwREWs\n\t0Prtxgf4pYcaytu4La0vmLWA1JvrYhkPpN/gBRhTh5omiGqK2NaJ+FVMOdWSkUFC1rKK\n\tzl+1ig53s2E8AfbhtoZJJ6GObtjumg9fJDLz0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724961490; x=1725566290;\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=fKQ4recxjmdY4RtmOXTZ1thJai5+1zhvudtkE402uuM=;\n\tb=ksjQnmIKoQZdf/pypB6lPSuqM+d/3aVDOtqeTTIRqZsWz8+d/KfXJcXoncW5vAuaNP\n\tmB6eiz+wxolecCKn/gawHtas2dO2YT3tEaqT+fpXWSpufHv5TelR6wMA6kjAHV/vekas\n\txfWlProN3R3jSz9/MJDkpmB15i3WWs8hursyAcE5Jrar3b2ZoPVn577IiKdKDO6WpCLX\n\tvNsXZMw9bRqcXvvajd24wJctQyqRJhtPjdD4JXkemQwvIzhRZnauE5BxIbeEz6OYZo9g\n\t13VQ1vcPhBzbmoo6gN953ddaJhvc8HitecTBqcPKCOhgLQDqcViBa+VS2R2a5HZPIQ6B\n\tUHYA==","X-Gm-Message-State":"AOJu0Yy+UaiDfXzjOpCqk2yJCQFB+zvJ11uOpiA/DZKzmd8ekh73mNjj\n\th0ndRKHEjqtpS9WUDY2cUnirKfWoXpqjITk7Q0YcL8Z7Yvircua503aKYMpd7O+kzYFuHL2ZS1X\n\tEeJHrjFfCYI9y3fNsQuiPMPcjdzSZNkYgixF6tP3jhFmLcvJVzwCc","X-Google-Smtp-Source":"AGHT+IFi+XBB125s3TnstHCGGH/d1jvb0wVFVMqzjpUP8cpyUVAZeVY+8VSD9kDKXLc23tn1jZq5JdC4JWA89XQ4NCo=","X-Received":"by 2002:a2e:a587:0:b0:2ef:c281:54f7 with SMTP id\n\t38308e7fff4ca-2f610514559mr26560521fa.37.1724961489403;\n\tThu, 29 Aug 2024 12:58:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-4-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 29 Aug 2024 21:57:58 +0200","Message-ID":"","Subject":"Re: [PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000d3d36b0620d7e4f8\"","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":31108,"web_url":"https://patchwork.libcamera.org/comment/31108/","msgid":"","date":"2024-09-07T14:33:35","subject":"Re: [PATCH v9 3/8] libcamera: pipeline: 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 Sat, Aug 31, 2024 at 8:49 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Sat, Aug 31, 2024 at 02:44:40PM GMT, Jacopo Mondi wrote:\n> > Hi\n> >\n> > On Thu, Aug 29, 2024 at 09:57:58PM GMT, Cheng-Hao Yang wrote:\n> > > Thanks for the review, Jacopo!\n> > >\n> > > On Tue, Aug 27, 2024 at 5:43 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > > wrote:\n> > >\n> > > > Hi Harvey\n> > > >\n> > > > On Tue, Aug 20, 2024 at 04:23:34PM 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 | 251\n> +++++++++++++++++++++\n> > > > > src/libcamera/pipeline/virtual/virtual.h | 78 +++++++\n> > > >\n> > > > Do you expect other components to include this header in future ? If\n> > > > not, you can move its content to the .cpp file I guess\n> > > >\n> > > >\n> > > Actually `virtual/parser.h` needs to include it to return\n> VirtualCameraData\n> > > when parsing the yaml config file [1]. Does that count :p?\n> >\n> > I guess it does :)\n> >\n> > >\n> > > [1]: https://patchwork.libcamera.org/patch/20971/\n> > >\n> > > > 5 files changed, 337 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 f946eba94..3cad3249a 100644\n> > > > > --- a/meson.build\n> > > > > +++ b/meson.build\n> > > > > @@ -222,6 +222,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 7aa412491..c91cd241a 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.\n> If this\n> > > > is set to \"auto\", all the pipelines applicable to the target\n> architecture\n> > > > will be built. If this is set to \"all\", all the pipelines will be\n> 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 000000000..ba7ff754e\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 000000000..74eb8c7ad\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > > @@ -0,0 +1,251 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2023, Google Inc.\n> > > > > + *\n> > > > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > > > + */\n> > > > > +\n> > > >\n> > > > You should include the header for the standard library construct you\n> > > > use. I see vectors, maps, unique_ptrs etc\n> > > >\n> > > Done, please check again.\n> > >\n> > >\n> > > >\n> > > > > +#include \"virtual.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/camera.h\"\n> > > >\n> > > > The internal header includes the public one by definition\n> > > >\n> > > Ack. Removed the public one.\n> > >\n> > >\n> > > >\n> > > > > +#include \"libcamera/internal/formats.h\"\n> > > >\n> > > > This doesn't as is generated. I wonder if it\n> > > > should.\n> > > >\n> > > Keeping `#include `.\n> > >\n> > >\n> > > >\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> > > > > + 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> > > > Could https://en.cppreference.com/w/cpp/chrono/steady_clock/now save\n> > > > you a custom function ?\n> > > >\n> > > > In example:\n> > > >\n> > > > const auto now = std::chrono::steady_clock::now();\n> > > > auto nsecs =\n> > > >\n> std::chrono::duration_cast(now.time_since_epoch());\n> > > > std::cout << nsecs.count();\n> > > >\n> > > > should give you the time in nanoseconds since system boot (if I got\n> it\n> > > > right)\n> > > >\n> > > >\n> > > > > +\n> > > > > +} // namespace\n> > > >\n> > > > nit: /* namespace */\n> > > > here and in other places\n> > > >\n> > > > Done\n> > >\n> > >\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> > > > > + /* Currently only one stream is supported */\n> > > > > + if (config_.size() > 1) {\n> > > > > + config_.resize(1);\n> > > > > + status = Adjusted;\n> > > > > + }\n> > > > > +\n> > > > > + Size maxSize;\n> > > > > + for (const auto &resolution : data_->supportedResolutions_)\n> > > > > + maxSize = std::max(maxSize, resolution.size);\n> > > > > +\n> > > > > + for (StreamConfiguration &cfg : config_) {\n> > > >\n> > > > you only have config, or in the next patches will this be augmented ?\n> > > >\n> > > > Do you mean that I should check `sensorConfig` or `orientation` as\n> well?\n> > >\n> >\n> > No I meant I was assuming the virtual pipline works with a single\n> > stream by design. But seeing your replies to the next patches makes me\n> > think my assumption was wrong.\n> >\n>\n> Let me clarify this once and for all: you currently have a single\n> stream in VirtualCameraData and your pipeline works with a single\n> stream, right ?\n>\n\nThat's correct.\n\n\n>\n> Do you plane to change this and the pipeline handler code is\n> structured in this way to prepare for that ?\n>\n\nYes, as I mentioned in the next patch version, I failed to pass\nmulti-stream-test when enabling multiple streams.\n\nI need your help :)","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 028B8C324C\n\tfor ;\n\tSat, 7 Sep 2024 14:33:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0BA9634F8;\n\tSat, 7 Sep 2024 16:33:49 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E556633CC\n\tfor ;\n\tSat, 7 Sep 2024 16:33:47 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id\n\t38308e7fff4ca-2f759b87f83so6814431fa.2\n\tfor ;\n\tSat, 07 Sep 2024 07:33:47 -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=\"R4vdW0oH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725719627; x=1726324427;\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=oZYU+xWLoRRNmHMlEpNNsDUjixAGpZmO61oAK1ryP0w=;\n\tb=R4vdW0oHXqUG6MfJ7Pe0eaknYccpQ5G/IfFuS9d/5wHTwDgjyPZWKhAZwmro7vrzYO\n\tBlLbblqe1Fg20mcnNpjTXa0EkNIHpEZRitJ6g5qG9XxQ8mHacFO1FqVl4Puxs/VuRUAW\n\tlF3MgcSF5b/ItjUa4NfqptlcvXWi05R/CUyYQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725719627; x=1726324427;\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=oZYU+xWLoRRNmHMlEpNNsDUjixAGpZmO61oAK1ryP0w=;\n\tb=Lz0UEYpqQ3gcWCCSMnHEAJoofXNNVbdV1TguULLR0RLpFCWp6XpoHej0ux/ux+wRVq\n\tja/jtFVwa7mdUspldcjxphQ2ZkVyh4Ec6mNZfR9a9uQ5Kf8R0gNAc/yjREnJaEzKaBnq\n\tUUD2+P7DC+n8+5I5zRpVVPDRafin0h4l2KHqOTgaBoDGHC+b6tsTk2jCHoioG2qca+JQ\n\t4rDLkBiIW87t7fLDf2lOVNgOJqPVcETHttIJHr9fG9tmJ1rZJBBmlEVAvH5/+dcs7P4+\n\tzzpfZhXEs5K3z7gNXoRxVm9IaWzk8fwZs7d3glg7pT5dw3d/ho+Cc5TlRzUpAytyU7Fo\n\t1ghQ==","X-Gm-Message-State":"AOJu0YyxtGzRztak2yVqj4Nt0fEUOMt3eLhgvNl3iSVOxbzXN/H3Rjae\n\t1x4VzxgKFXl95yOwhejaSsktPSw8PNCir2uYYlsHO/mlFp6s8jM7Phr5r3bnp0Zt2gCUfrYwHmO\n\tmlGYddmuk53vjD+c7JBWuS0f1wXT0yxeZnWpt","X-Google-Smtp-Source":"AGHT+IFNvz7UfX4mXCyo3KcWiUIwV1LdM+GkitqgaPPxJ2WGbGHvk1CatRK1V5Qm94w515rUgJt6WCVsNiQCeVasHUU=","X-Received":"by 2002:a05:651c:1989:b0:2ef:2bb4:45d with SMTP id\n\t38308e7fff4ca-2f751eaf2c4mr28578961fa.9.1725719626517;\n\tSat, 07 Sep 2024 07:33:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-4-chenghaoyang@google.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Sat, 7 Sep 2024 22:33:35 +0800","Message-ID":"","Subject":"Re: [PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000522b8a0621886981\"","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":31109,"web_url":"https://patchwork.libcamera.org/comment/31109/","msgid":"","date":"2024-09-07T14:34:01","subject":"Re: [PATCH v9 3/8] libcamera: pipeline: 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 Sat, Aug 31, 2024 at 8:44 PM Jacopo Mondi \nwrote:\n\n> Hi\n>\n> On Thu, Aug 29, 2024 at 09:57:58PM GMT, Cheng-Hao Yang wrote:\n> > Thanks for the review, Jacopo!\n> >\n> > On Tue, Aug 27, 2024 at 5:43 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Tue, Aug 20, 2024 at 04:23:34PM 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 | 251\n> +++++++++++++++++++++\n> > > > src/libcamera/pipeline/virtual/virtual.h | 78 +++++++\n> > >\n> > > Do you expect other components to include this header in future ? If\n> > > not, you can move its content to the .cpp file I guess\n> > >\n> > >\n> > Actually `virtual/parser.h` needs to include it to return\n> VirtualCameraData\n> > when parsing the yaml config file [1]. Does that count :p?\n>\n> I guess it does :)\n>\n> >\n> > [1]: https://patchwork.libcamera.org/patch/20971/\n> >\n> > > 5 files changed, 337 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 f946eba94..3cad3249a 100644\n> > > > --- a/meson.build\n> > > > +++ b/meson.build\n> > > > @@ -222,6 +222,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 7aa412491..c91cd241a 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\n> this\n> > > is set to \"auto\", all the pipelines applicable to the target\n> architecture\n> > > will be built. If this is set to \"all\", all the pipelines will be\n> 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 000000000..ba7ff754e\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 000000000..74eb8c7ad\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> > > > @@ -0,0 +1,251 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Google Inc.\n> > > > + *\n> > > > + * virtual.cpp - Pipeline handler for virtual cameras\n> > > > + */\n> > > > +\n> > >\n> > > You should include the header for the standard library construct you\n> > > use. I see vectors, maps, unique_ptrs etc\n> > >\n> > Done, please check again.\n> >\n> >\n> > >\n> > > > +#include \"virtual.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/camera.h\"\n> > >\n> > > The internal header includes the public one by definition\n> > >\n> > Ack. Removed the public one.\n> >\n> >\n> > >\n> > > > +#include \"libcamera/internal/formats.h\"\n> > >\n> > > This doesn't as is generated. I wonder if it\n> > > should.\n> > >\n> > Keeping `#include `.\n> >\n> >\n> > >\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> > > > + 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> > > Could https://en.cppreference.com/w/cpp/chrono/steady_clock/now save\n> > > you a custom function ?\n> > >\n> > > In example:\n> > >\n> > > const auto now = std::chrono::steady_clock::now();\n> > > auto nsecs =\n> > >\n> std::chrono::duration_cast(now.time_since_epoch());\n> > > std::cout << nsecs.count();\n> > >\n> > > should give you the time in nanoseconds since system boot (if I got it\n> > > right)\n> > >\n> > >\n> > > > +\n> > > > +} // namespace\n> > >\n> > > nit: /* namespace */\n> > > here and in other places\n> > >\n> > > Done\n> >\n> >\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> > > > + /* Currently only one stream is supported */\n> > > > + if (config_.size() > 1) {\n> > > > + config_.resize(1);\n> > > > + status = Adjusted;\n> > > > + }\n> > > > +\n> > > > + Size maxSize;\n> > > > + for (const auto &resolution : data_->supportedResolutions_)\n> > > > + maxSize = std::max(maxSize, resolution.size);\n> > > > +\n> > > > + for (StreamConfiguration &cfg : config_) {\n> > >\n> > > you only have config, or in the next patches will this be augmented ?\n> > >\n> > > Do you mean that I should check `sensorConfig` or `orientation` as\n> well?\n> >\n>\n> No I meant I was assuming the virtual pipline works with a single\n> stream by design. But seeing your replies to the next patches makes me\n> think my assumption was wrong.\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> > > > + found = true;\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + if (!found) {\n> > > > + cfg.size = maxSize;\n> > > > + status = Adjusted;\n> > > > + }\n> > >\n> > > so it's either the exact resolution or the biggest available one ?\n> > >\n> > > As you have a single config it's easy to get the closest one to the\n> > > requested size, if it doesn't match exactly one of the supported\n> > > resolutions.\n> > >\n> >\n> > Hmm, I think it's a bit hard to define the \"closest\". Do we compare\n> > the area size directly? Do we prefer a size that has both larger or\n> > the same height and width?\n> >\n>\n> Good question, it's generally a pipeline decision (which is not\n> optimal, as we should aim to a unified behaviour among pipelines).\n>\n> As this is for testing, I think it's fine to keep what you have, but\n> could you add a comment to highlight this implementation decision ?\n>\n>\nSure, will be updated in v11. Please take another look.\n\n\n> >\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 =\n> VirtualCameraConfiguration::kBufferCount;\n> > > > + }\n> > > > +\n> > > > + return status;\n> > > > +}\n> > > > +\n> > > > +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager\n> *manager)\n> > > > + : PipelineHandler(manager)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr\n> > > > +PipelineHandlerVirtual::generateConfiguration(Camera *camera,\n> > > > + Span\n> 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> > > > + 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> > > As you do this min/max search in a few places, why not doing it when\n> > > you construct data->supportedResolutions_ once ?\n> > >\n> > Added in VirtualCameraData.\n> >\n> >\n> > >\n> > > > + }\n> > > > +\n> > > > + for (const StreamRole role : roles) {\n> > >\n> > > If the pipeline handler works with a single stream, you should only\n> > > consider the first role maybe\n> > >\n> > Actually I think there's no reason to only support one Stream in\n> > Virtual Pipeline Handler. The raw stream doesn't seem to be\n> > properly supported in the following patches though. I think we\n> > should drop the support of raw.\n> >\n>\n> I assumed (maybe from a previous discussion) you were going to have a\n> single stream. I'm sorry, I was wrong.\n>\n\nNah, I prepared for multiple streams, but haven't enabled it yet :p.\n\n\n>\n> Please drop RAW, yes.\n>\n\nDone in v10.\n\n\n>\n> >\n> > >\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> > > bufferCount and streamFormats can be assigned outsize of the cases,\n> > > they're the same for all roles\n> > >\n> > Done\n> >\n> >\n> > >\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\n> 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> > > > +\n> > > > + return config;\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::configure(\n> > > > + [[maybe_unused]] Camera *camera,\n> > > > + [[maybe_unused]] CameraConfiguration *config)\n> > >\n> > > int PipelineHandlerVirtual::configure([[maybe_unused]] Camera *camera,\n> > > [[maybe_unused]]\n> CameraConfiguration\n> > > *config)\n> > >\n> > > Done\n> >\n> >\n> > > > +{\n> > > > + // Nothing to be done.\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::exportFrameBuffers(\n> > > > + [[maybe_unused]] Camera *camera,\n> > > > + Stream *stream,\n> > > > + std::vector> *buffers)\n> > >\n> > > int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera\n> > > *camera,\n> > > Stream *stream,\n> > >\n> > > std::vector> *buffers)\n> > >\n> > > if you prefer\n> > >\n> > > Done\n> >\n> >\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> > > ah that's probably why you return count from\n> > > DmaBufferAllocator::exportBuffers()\n> > >\n> > Exactly :)\n> >\n> >\n> > >\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> > > > + [[maybe_unused]] const ControlList\n> > > *controls)\n> > > > +{\n> > > > + /* \\todo Start reading the virtual video if any. */\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera\n> *camera)\n> > > > +{\n> > > > + /* \\todo Reset the virtual video if any. */\n> > > > +}\n> > > > +\n> > > > +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]]\n> 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> > > > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator\n> > > *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 } };\n> > > > + data->supportedResolutions_[1] = { .size = Size(1280, 720),\n> > > .frame_rates = { 30, 60 } };\n> > > > +\n> > > > + data->properties_.set(properties::Location,\n> > > properties::CameraLocationFront);\n> > > > + data->properties_.set(properties::Model, \"Virtual Video\n> 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 = 30, max_frame_duration = 60;\n> > >\n> > > doesn't match the above frame rates and it should be expressed in\n> > > microseconds. I would suggest for this patch to set both framerates to\n> > > 30 and initialize FrameDurationLimits with {33333, 333333}\n> > >\n> > > It's not a big deal however, it will be replaced later in the series\n> > >\n> > >\n> > Done, thanks!\n> >\n> >\n> > >\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 =\n> Camera::create(std::move(data),\n> > > id, streams);\n> > > > + registerCamera(std::move(camera));\n> > > > +\n> > > > + return false; // Prevent infinite loops for now\n> > >\n> > > I presume this will also be changed in next patches...\n> > >\n> > Updated in this patch, with a static boolean though.\n> >\n> >\n> > >\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 000000000..6fc6b34d8\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/virtual/virtual.h\n> > > > @@ -0,0 +1,78 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Google Inc.\n> > > > + *\n> > > > + * virtual.h - Pipeline handler for virtual cameras\n> > > > + */\n> > > > +\n> > > > +#pragma once\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> > > > + struct Resolution {\n> > > > + Size size;\n> > > > + std::vector frame_rates;\n> > > > + };\n> > > > + VirtualCameraData(PipelineHandler *pipe)\n> > > > + : Camera::Private(pipe)\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + ~VirtualCameraData() = default;\n> > > > +\n> > > > + std::vector supportedResolutions_;\n> > > > +\n> > > > + Stream stream_;\n> > > > +};\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\n> 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> > > > +\n> std::vector>\n> > > *buffers) override;\n> > > > +\n> > > > + int start(Camera *camera, const ControlList *controls)\n> override;\n> > > > + void stopDevice(Camera *camera) override;\n> > > > +\n> > > > + int queueRequestDevice(Camera *camera, Request *request)\n> 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> > > > + DmaBufAllocator dmaBufAllocator_;\n> > > > +};\n> > > > +\n> > > > +} // namespace libcamera\n> > > > --\n> > > > 2.46.0.184.g6999bdac58-goog\n> > > >\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 D32AAC324C\n\tfor ;\n\tSat, 7 Sep 2024 14:34:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 772DE634F4;\n\tSat, 7 Sep 2024 16:34:16 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74080633CC\n\tfor ;\n\tSat, 7 Sep 2024 16:34:14 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f75c205e4aso5193481fa.0\n\tfor ;\n\tSat, 07 Sep 2024 07:34:14 -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=\"CJQKcW1y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725719654; x=1726324454;\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=oZhcuAoZhwcwpIRW0H19jD51tfv481hfN1f17tYZRP8=;\n\tb=CJQKcW1y4NginpkpcIRf7Gq5sjKNsKChl5qmGo1NHr/RdMIEdMIz+8Y2E2UFv6ODnA\n\tZatvX03zlngvRRkWFb1fItf7/C79/HK3QBoFDSy5v9TqI0i+seePkbraBvANPPC76Uyn\n\t25CVy74KHkxckYT7qt+eGyv0KB+QlHV6l1NtI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725719654; x=1726324454;\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=oZhcuAoZhwcwpIRW0H19jD51tfv481hfN1f17tYZRP8=;\n\tb=C9KLipbCxIo7CQl7yD3Gabm9DNebKVf6FSp87RThH4Hwfv6OWEyLbWfbLzzDgmbPXC\n\tfOFCXlllpSCvacz0/Dg+/mAAhhM8MadJukSrNYPwyvIEgJp2QptZ2DeGn2eJTtKnkASv\n\tr8eyiPhykjKDPv+oQoliuJHjiHjhOrXoe0RMUaM2m0mAS/qhJJeoTVo4xkr7oVdkvOiB\n\tvOfNgjP+KujNKg//yCzor1wZdhfHmQC5Yni0wpLVyDr64D6rNAPRYq1p5cd5kTuriqT7\n\t5WjkstiZJK+dCokcUmFVjoZW38TX8a+ZE2UFrnjk0WFJpZS7uo7IoUsSQznuq30Np72B\n\tqCWA==","X-Gm-Message-State":"AOJu0Yx7itnlfBkktVJkRzFAOtihkTZhWHqqSoXRfkAsjaU+8BSw7ImS\n\ta1RLo0dOJFNqIg0zqfTdWfulYqJ6h0MOhdcdyIf0bzk+kUCD3AzB6wblkXYk02NdlwQs1jKPVOr\n\t0/4QeaiFmAJGDTDr606lJ9pTirgDT9kMQgRB0","X-Google-Smtp-Source":"AGHT+IFkHBgI2dQt5TBqe8lnXAluY5dwxA874C5y0ZW+NaIXchikvzBVYw6qgrCDyENgD7bM14tD8A8s6qUXh9rXKps=","X-Received":"by 2002:a2e:711:0:b0:2f7:4f19:abaa with SMTP id\n\t38308e7fff4ca-2f751ec8995mr34743971fa.14.1725719653180;\n\tSat, 07 Sep 2024 07:34:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820172202.526547-1-chenghaoyang@google.com>\n\t<20240820172202.526547-4-chenghaoyang@google.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Sat, 7 Sep 2024 22:34:01 +0800","Message-ID":"","Subject":"Re: [PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000e901660621886a14\"","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\" "}}]