<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 9, 2024 at 4:47 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey<br>
<br>
On Sat, Sep 07, 2024 at 02:28:28PM GMT, Harvey Yang wrote:<br>
> From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
><br>
> Add VirtualPipelineHandler for more unit tests and verfiy libcamera<br>
> infrastructure works on devices without using hardware cameras.<br>
><br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> meson.build | 1 +<br>
> meson_options.txt | 3 +-<br>
> src/libcamera/pipeline/virtual/meson.build | 5 +<br>
> src/libcamera/pipeline/virtual/virtual.cpp | 267 +++++++++++++++++++++<br>
> src/libcamera/pipeline/virtual/virtual.h | 89 +++++++<br>
> 5 files changed, 364 insertions(+), 1 deletion(-)<br>
> create mode 100644 src/libcamera/pipeline/virtual/meson.build<br>
> create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp<br>
> create mode 100644 src/libcamera/pipeline/virtual/virtual.h<br>
><br>
> diff --git a/meson.build b/meson.build<br>
> index 432ae133..ff9a70cf 100644<br>
> --- a/meson.build<br>
> +++ b/meson.build<br>
> @@ -214,6 +214,7 @@ pipelines_support = {<br>
> 'simple': ['any'],<br>
> 'uvcvideo': ['any'],<br>
> 'vimc': ['test'],<br>
> + 'virtual': ['test'],<br>
> }<br>
><br>
> if pipelines.contains('all')<br>
> diff --git a/meson_options.txt b/meson_options.txt<br>
> index 7aa41249..c91cd241 100644<br>
> --- a/meson_options.txt<br>
> +++ b/meson_options.txt<br>
> @@ -53,7 +53,8 @@ option('pipelines',<br>
> 'rpi/vc4',<br>
> 'simple',<br>
> 'uvcvideo',<br>
> - 'vimc'<br>
> + 'vimc',<br>
> + 'virtual'<br>
> ],<br>
> 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.')<br>
><br>
> diff --git a/src/libcamera/pipeline/virtual/meson.build b/src/libcamera/pipeline/virtual/meson.build<br>
> new file mode 100644<br>
> index 00000000..ada1b335<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/meson.build<br>
> @@ -0,0 +1,5 @@<br>
> +# SPDX-License-Identifier: CC0-1.0<br>
> +<br>
> +libcamera_internal_sources += files([<br>
> + 'virtual.cpp',<br>
> +])<br>
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> new file mode 100644<br>
> index 00000000..f85ec3dd<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> @@ -0,0 +1,267 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2024, Google Inc.<br>
> + *<br>
> + * virtual.cpp - Pipeline handler for virtual cameras<br>
> + */<br>
> +<br>
> +#include "virtual.h"<br>
> +<br>
> +#include <algorithm><br>
> +#include <map><br>
> +#include <memory><br>
> +#include <set><br>
<br>
#include <string><br>
<br>
> +#include <utility><br>
> +#include <vector><br>
> +<br>
> +#include <libcamera/base/log.h><br>
> +<br>
> +#include <libcamera/control_ids.h><br>
> +#include <libcamera/controls.h><br>
> +#include <libcamera/formats.h><br>
> +#include <libcamera/property_ids.h><br>
> +<br>
> +#include "libcamera/internal/camera.h"<br>
> +#include "libcamera/internal/formats.h"<br>
> +#include "libcamera/internal/pipeline_handler.h"<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +LOG_DEFINE_CATEGORY(Virtual)<br>
> +<br>
> +namespace {<br>
> +<br>
> +uint64_t currentTimestamp()<br>
<br>
As suggested on v9, this could probably be replaced with a few lines<br>
of C++ using std::chrono. Have you tested it and found it not to be<br>
working ? Not a big deal though<br></blockquote><div><br></div><div>Ah sorry... I struggled a lot with the user interface of emails and</div><div>patchwork, so from time to time I lose track of comments.</div><div><br></div><div>Yes, comparing the timestamps, those two approaches produce similar</div><div>timestamps. Updated in v12.</div><div><br></div><div>I think we can keep the function `currentTimestamp` though, just replacing</div><div>the implementation. Let me know if you think I should get rid of it as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +{<br>
> + struct timespec ts;<br>
> + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {<br>
> + LOG(Virtual, Error) << "Get clock time fails";<br>
> + return 0;<br>
> + }<br>
> +<br>
> + return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;<br>
> +}<br>
> +<br>
> +} /* namespace */<br>
> +<br>
> +VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,<br>
> + std::vector<Resolution> supportedResolutions)<br>
> + : Camera::Private(pipe), supportedResolutions_(std::move(supportedResolutions))<br>
> +{<br>
> + for (const auto &resolution : supportedResolutions_) {<br>
> + if (minResolutionSize_.isNull() || minResolutionSize_ > resolution.size)<br>
> + minResolutionSize_ = resolution.size;<br>
> +<br>
> + maxResolutionSize_ = std::max(maxResolutionSize_, resolution.size);<br>
> + }<br>
> +<br>
> + /* \todo Support multiple streams and pass multi_stream_test */<br>
> + streamConfigs_.resize(kMaxStream);<br>
> +}<br>
> +<br>
> +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)<br>
> + : CameraConfiguration(), data_(data)<br>
> +{<br>
> +}<br>
> +<br>
> +CameraConfiguration::Status VirtualCameraConfiguration::validate()<br>
> +{<br>
> + Status status = Valid;<br>
> +<br>
> + if (config_.empty()) {<br>
> + LOG(Virtual, Error) << "Empty config";<br>
> + return Invalid;<br>
> + }<br>
> +<br>
> + /* Only one stream is supported */<br>
> + if (config_.size() > VirtualCameraData::kMaxStream) {<br>
> + config_.resize(VirtualCameraData::kMaxStream);<br>
> + status = Adjusted;<br>
> + }<br>
> +<br>
> + for (StreamConfiguration &cfg : config_) {<br>
<br>
Knowing that you're going to add support for multiple streams I think<br>
it's fine having a loop here already<br>
<br></blockquote><div><br></div><div>Yes, thanks :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + bool found = false;<br>
> + for (const auto &resolution : data_->supportedResolutions_) {<br>
> + if (resolution.size.width == cfg.size.width &&<br>
> + resolution.size.height == cfg.size.height) {<br>
> + found = true;<br>
> + break;<br>
> + }<br>
> + }<br>
> +<br>
> + if (!found) {<br>
> + /*<br>
> + * \todo It's a pipeline's decision to choose a<br>
> + * resolution when the exact one is not supported.<br>
> + * Defining the default logic in PipelineHandler to<br>
> + * find the closest resolution would be nice.<br>
> + */<br>
> + cfg.size = data_->maxResolutionSize_;<br>
> + status = Adjusted;<br>
> + }<br>
> +<br>
> + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);<br>
> + cfg.stride = info.stride(cfg.size.width, 0, 1);<br>
> + cfg.frameSize = info.frameSize(cfg.size, 1);<br>
> +<br>
> + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> +<br>
> + if (cfg.pixelFormat != formats::NV12) {<br>
> + cfg.pixelFormat = formats::NV12;<br>
> + LOG(Virtual, Debug)<br>
> + << "Stream configuration adjusted to " << cfg.toString();<br>
> + status = Adjusted;<br>
> + }<br>
> + }<br>
> +<br>
> + return status;<br>
> +}<br>
> +<br>
> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)<br>
> + : PipelineHandler(manager),<br>
> + dmaBufAllocator_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |<br>
> + DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |<br>
> + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)<br>
> +{<br>
> +}<br>
> +<br>
> +std::unique_ptr<CameraConfiguration><br>
> +PipelineHandlerVirtual::generateConfiguration(Camera *camera,<br>
> + Span<const StreamRole> roles)<br>
> +{<br>
> + VirtualCameraData *data = cameraData(camera);<br>
> + auto config =<br>
> + std::make_unique<VirtualCameraConfiguration>(data);<br>
> +<br>
> + if (roles.empty())<br>
> + return config;<br>
> +<br>
> + for (const StreamRole role : roles) {<br>
<br>
I presume the loop is here to support multiple streams too<br>
<br></blockquote><div><br></div><div>Yes, exactly.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + std::map<PixelFormat, std::vector<SizeRange>> streamFormats;<br>
> + PixelFormat pixelFormat = formats::NV12;<br>
> + streamFormats[pixelFormat] = { { data->minResolutionSize_, data->maxResolutionSize_ } };<br>
> + StreamFormats formats(streamFormats);<br>
> + StreamConfiguration cfg(formats);<br>
> + cfg.pixelFormat = pixelFormat;<br>
> + cfg.size = data->maxResolutionSize_;<br>
> + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> +<br>
> + switch (role) {<br>
> + case StreamRole::StillCapture:<br>
> + case StreamRole::VideoRecording:<br>
> + case StreamRole::Viewfinder:<br>
> + break;<br>
> +<br>
> + case StreamRole::Raw:<br>
> + default:<br>
> + LOG(Virtual, Error)<br>
> + << "Requested stream role not supported: " << role;<br>
> + config.reset();<br>
> + return config;<br>
> + }<br>
<br>
I would have placed the switch before populating cfg, but it's ok, you<br>
clear config_ anyway in case of errors<br>
<br></blockquote><div><br></div><div>Right, the switch and the check on `role` can be done earlier.</div><div>Updated in v12.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + config->addConfiguration(cfg);<br>
> + }<br>
> +<br>
> + if (config->validate() == CameraConfiguration::Invalid)<br>
> + config.reset();<br>
<br>
If this happens, it's a programming error so you could even ASSERT<br>
here. No problem, the ipu3 pipeline does the same here.<br>
<br></blockquote><div><br></div><div>Ah right, updated with ASSERT in v12.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + return config;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::configure(Camera *camera,<br>
> + CameraConfiguration *config)<br>
> +{<br>
> + VirtualCameraData *data = cameraData(camera);<br>
> + for (size_t i = 0; i < config->size(); ++i)<br>
<br>
Or<br>
for (auto [i, c] : utils::enumerate(*config))<br>
c.setStream(&data->streamConfigs_[i].stream);<br>
<br>
not a big deal<br></blockquote><div><br></div><div>Thanks! Updated in v12.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + config->at(i).setStream(&data->streamConfigs_[i].stream);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,<br>
> + Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + if (!dmaBufAllocator_.isValid())<br>
> + return -ENOBUFS;<br>
> +<br>
> + const StreamConfiguration &config = stream->configuration();<br>
> +<br>
> + auto info = PixelFormatInfo::info(config.pixelFormat);<br>
> +<br>
> + std::vector<unsigned int> planeSizes;<br>
> + for (size_t i = 0; i < info.planes.size(); ++i)<br>
> + planeSizes.push_back(info.planeSize(config.size, i));<br>
> +<br>
> + return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,<br>
> + [[maybe_unused]] const ControlList *controls)<br>
> +{<br>
> + return 0;<br>
> +}<br>
> +<br>
> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)<br>
> +{<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,<br>
> + Request *request)<br>
> +{<br>
> + /* \todo Read from the virtual video if any. */<br>
> + for (auto it : request->buffers())<br>
> + completeBuffer(request, it.second);<br>
> +<br>
> + request->metadata().set(controls::SensorTimestamp, currentTimestamp());<br>
> + completeRequest(request);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +// static<br>
<br>
I'm not sure anymore how to get the message through: No C++ comments please<br></blockquote><div><br></div><div>Sorry, updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +bool PipelineHandlerVirtual::created_ = false;<br>
<br>
You can move this to the beginng of the file if you prefer. Otherwise<br>
it's fine.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> +<br>
> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)<br>
> +{<br>
> + if (created_)<br>
> + return false;<br>
> +<br>
> + created_ = true;<br>
> +<br>
> + /* \todo Add virtual cameras according to a config file. */<br>
> +<br>
> + std::vector<VirtualCameraData::Resolution> supportedResolutions;<br>
> + supportedResolutions.resize(2);<br>
> + supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };<br>
> + supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };<br>
> +<br>
> + std::unique_ptr<VirtualCameraData> data =<br>
> + std::make_unique<VirtualCameraData>(this, supportedResolutions);<br>
> +<br>
> + data->properties_.set(properties::Location, properties::CameraLocationFront);<br>
> + data->properties_.set(properties::Model, "Virtual Video Device");<br>
> + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });<br>
> +<br>
> + /* \todo Set FrameDurationLimits based on config. */<br>
> + ControlInfoMap::Map controls;<br>
> + int64_t min_frame_duration = 33333, max_frame_duration = 33333;<br>
> + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);<br>
> + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);<br>
> +<br>
> + /* Create and register the camera. */<br>
> + std::set<Stream *> streams;<br>
> + for (auto &streamConfig : data->streamConfigs_)<br>
> + streams.insert(&streamConfig.stream);<br>
> +<br>
> + const std::string id = "Virtual0";<br>
> + std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams);<br>
> + registerCamera(std::move(camera));<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual, "virtual")<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h<br>
> new file mode 100644<br>
> index 00000000..fb3dbcad<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.h<br>
> @@ -0,0 +1,89 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2024, Google Inc.<br>
> + *<br>
> + * virtual.h - Pipeline handler for virtual cameras<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <memory><br>
> +#include <vector><br>
> +<br>
> +#include <libcamera/base/file.h><br>
> +<br>
> +#include "libcamera/internal/camera.h"<br>
> +#include "libcamera/internal/dma_buf_allocator.h"<br>
> +#include "libcamera/internal/pipeline_handler.h"<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +class VirtualCameraData : public Camera::Private<br>
> +{<br>
> +public:<br>
> + const static unsigned int kMaxStream = 1;<br>
> +<br>
> + struct Resolution {<br>
> + Size size;<br>
> + std::vector<int> frameRates;<br>
> + };<br>
> + struct StreamConfig {<br>
> + Stream stream;<br>
> + };<br>
> +<br>
> + VirtualCameraData(PipelineHandler *pipe,<br>
> + std::vector<Resolution> supportedResolutions);<br>
> +<br>
> + ~VirtualCameraData() = default;<br>
> +<br>
> + const std::vector<Resolution> supportedResolutions_;<br>
> + Size maxResolutionSize_;<br>
> + Size minResolutionSize_;<br>
> +<br>
> + std::vector<StreamConfig> streamConfigs_;<br>
> +};<br>
> +<br>
> +class VirtualCameraConfiguration : public CameraConfiguration<br>
<br>
It seems to me only VirtualCameraData is used by the other files. The<br>
other classes declarations, as not used outside of virtual.cpp, could<br>
be moved there.<br>
<br>
Only nits here and there, with them addressed:<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
<br>
Thanks<br>
j<br>
<br>
> +{<br>
> +public:<br>
> + static constexpr unsigned int kBufferCount = 4;<br>
> +<br>
> + VirtualCameraConfiguration(VirtualCameraData *data);<br>
> +<br>
> + Status validate() override;<br>
> +<br>
> +private:<br>
> + const VirtualCameraData *data_;<br>
> +};<br>
> +<br>
> +class PipelineHandlerVirtual : public PipelineHandler<br>
> +{<br>
> +public:<br>
> + PipelineHandlerVirtual(CameraManager *manager);<br>
> +<br>
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,<br>
> + Span<const StreamRole> roles) override;<br>
> + int configure(Camera *camera, CameraConfiguration *config) override;<br>
> +<br>
> + int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;<br>
> +<br>
> + int start(Camera *camera, const ControlList *controls) override;<br>
> + void stopDevice(Camera *camera) override;<br>
> +<br>
> + int queueRequestDevice(Camera *camera, Request *request) override;<br>
> +<br>
> + bool match(DeviceEnumerator *enumerator) override;<br>
> +<br>
> +private:<br>
> + static bool created_;<br>
> +<br>
> + VirtualCameraData *cameraData(Camera *camera)<br>
> + {<br>
> + return static_cast<VirtualCameraData *>(camera->_d());<br>
> + }<br>
> +<br>
> + DmaBufAllocator dmaBufAllocator_;<br>
> +};<br>
> +<br>
> +} /* namespace libcamera */<br>
> --<br>
> 2.46.0.469.g59c65b2a67-goog<br>
><br>
</blockquote></div></div>