<div dir="ltr"><div dir="ltr">Thanks for the review, Jacopo!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 27, 2024 at 5:43 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 Tue, Aug 20, 2024 at 04:23:34PM 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 | 251 +++++++++++++++++++++<br>
> src/libcamera/pipeline/virtual/virtual.h | 78 +++++++<br>
<br>
Do you expect other components to include this header in future ? If<br>
not, you can move its content to the .cpp file I guess<br>
<br></blockquote><div><br></div><div>Actually `virtual/parser.h` needs to include it to return VirtualCameraData</div><div>when parsing the yaml config file [1]. Does that count :p?</div><div> </div><div>[1]: <a href="https://patchwork.libcamera.org/patch/20971/" target="_blank">https://patchwork.libcamera.org/patch/20971/</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> 5 files changed, 337 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 f946eba94..3cad3249a 100644<br>
> --- a/meson.build<br>
> +++ b/meson.build<br>
> @@ -222,6 +222,7 @@ pipelines_support = {<br>
> 'simple': arch_arm,<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 7aa412491..c91cd241a 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 000000000..ba7ff754e<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_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 000000000..74eb8c7ad<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> @@ -0,0 +1,251 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * virtual.cpp - Pipeline handler for virtual cameras<br>
> + */<br>
> +<br>
<br>
You should include the header for the standard library construct you<br>
use. I see vectors, maps, unique_ptrs etc<br></blockquote><div>Done, please check again.</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>
> +#include "virtual.h"<br>
> +<br>
> +#include <libcamera/base/log.h><br>
> +<br>
> +#include <libcamera/camera.h><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>
<br>
The internal header includes the public one by definition<br></blockquote><div>Ack. Removed the public one.</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>
> +#include "libcamera/internal/formats.h"<br>
<br>
This doesn't as <libcamera/formats.h> is generated. I wonder if it<br>
should.<br></blockquote><div>Keeping `#include <libcamera/formats.h>`.</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>
> +#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>
> + 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>
Could <a href="https://en.cppreference.com/w/cpp/chrono/steady_clock/now" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/chrono/steady_clock/now</a> save<br>
you a custom function ?<br>
<br>
In example:<br>
<br>
const auto now = std::chrono::steady_clock::now();<br>
auto nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>(now.time_since_epoch());<br>
std::cout << nsecs.count();<br>
<br>
should give you the time in nanoseconds since system boot (if I got it<br>
right)<br>
<br>
<br>
> +<br>
> +} // namespace<br>
<br>
nit: /* namespace */<br>
here and in other places<br>
<br></blockquote><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>
> +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>
> + /* Currently only one stream is supported */<br>
> + if (config_.size() > 1) {<br>
> + config_.resize(1);<br>
> + status = Adjusted;<br>
> + }<br>
> +<br>
> + Size maxSize;<br>
> + for (const auto &resolution : data_->supportedResolutions_)<br>
> + maxSize = std::max(maxSize, resolution.size);<br>
> +<br>
> + for (StreamConfiguration &cfg : config_) {<br>
<br>
you only have config, or in the next patches will this be augmented ?<br>
<br></blockquote><div>Do you mean that I should check `sensorConfig` or `orientation` as well?</div><div><br></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>
> + cfg.size = maxSize;<br>
> + status = Adjusted;<br>
> + }<br>
<br>
so it's either the exact resolution or the biggest available one ?<br>
<br>
As you have a single config it's easy to get the closest one to the<br>
requested size, if it doesn't match exactly one of the supported<br>
resolutions.<br></blockquote><div><br></div><div>Hmm, I think it's a bit hard to define the "closest". Do we compare</div><div>the area size directly? Do we prefer a size that has both larger or</div><div>the same height and width?</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>
> + 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.setStream(const_cast<Stream *>(&data_->stream_));<br>
> +<br>
> + cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> + }<br>
> +<br>
> + return status;<br>
> +}<br>
> +<br>
> +PipelineHandlerVirtual::PipelineHandlerVirtual(CameraManager *manager)<br>
> + : PipelineHandler(manager)<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>
> + Size minSize, sensorResolution;<br>
> + for (const auto &resolution : data->supportedResolutions_) {<br>
> + if (minSize.isNull() || minSize > resolution.size)<br>
> + minSize = resolution.size;<br>
> +<br>
> + sensorResolution = std::max(sensorResolution, resolution.size);<br>
<br>
As you do this min/max search in a few places, why not doing it when<br>
you construct data->supportedResolutions_ once ?<br></blockquote><div>Added in VirtualCameraData.</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>
> + for (const StreamRole role : roles) {<br>
<br>
If the pipeline handler works with a single stream, you should only<br>
consider the first role maybe<br></blockquote><div>Actually I think there's no reason to only support one Stream in<br></div><div>Virtual Pipeline Handler. The raw stream doesn't seem to be </div><div>properly supported in the following patches though. I think we</div><div>should drop the support of raw.</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>
> + std::map<PixelFormat, std::vector<SizeRange>> streamFormats;<br>
> + unsigned int bufferCount;<br>
> + PixelFormat pixelFormat;<br>
> +<br>
> + switch (role) {<br>
> + case StreamRole::StillCapture:<br>
> + pixelFormat = formats::NV12;<br>
> + bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, sensorResolution } };<br>
<br>
bufferCount and streamFormats can be assigned outsize of the cases,<br>
they're the same for all roles<br></blockquote><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>
> + break;<br>
> +<br>
> + case StreamRole::Raw: {<br>
> + /* \todo check */<br>
> + pixelFormat = formats::SBGGR10;<br>
> + bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, sensorResolution } };<br>
> +<br>
> + break;<br>
> + }<br>
> +<br>
> + case StreamRole::Viewfinder:<br>
> + case StreamRole::VideoRecording: {<br>
> + pixelFormat = formats::NV12;<br>
> + bufferCount = VirtualCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, sensorResolution } };<br>
> +<br>
> + break;<br>
> + }<br>
> +<br>
> + default:<br>
> + LOG(Virtual, Error)<br>
> + << "Requested stream role not supported: " << role;<br>
> + config.reset();<br>
> + return config;<br>
> + }<br>
> +<br>
> + StreamFormats formats(streamFormats);<br>
> + StreamConfiguration cfg(formats);<br>
> + cfg.size = sensorResolution;<br>
> + cfg.pixelFormat = pixelFormat;<br>
> + cfg.bufferCount = bufferCount;<br>
> + config->addConfiguration(cfg);<br>
> + }<br>
> +<br>
> + if (config->validate() == CameraConfiguration::Invalid)<br>
> + config.reset();<br>
> +<br>
> + return config;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::configure(<br>
> + [[maybe_unused]] Camera *camera,<br>
> + [[maybe_unused]] CameraConfiguration *config)<br>
<br>
int PipelineHandlerVirtual::configure([[maybe_unused]] Camera *camera,<br>
[[maybe_unused]] CameraConfiguration *config)<br>
<br></blockquote><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>
> + // Nothing to be done.<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::exportFrameBuffers(<br>
> + [[maybe_unused]] Camera *camera,<br>
> + Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
<br>
int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,<br>
Stream *stream,<br>
std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
<br>
if you prefer<br>
<br></blockquote><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>
> + 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<std::size_t> 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>
ah that's probably why you return count from<br>
DmaBufferAllocator::exportBuffers()<br></blockquote><div>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">
<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,<br>
> + [[maybe_unused]] const ControlList *controls)<br>
> +{<br>
> + /* \todo Start reading the virtual video if any. */<br>
> + return 0;<br>
> +}<br>
> +<br>
> +void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)<br>
> +{<br>
> + /* \todo Reset the virtual video if any. */<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>
> +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)<br>
> +{<br>
> + /* \todo Add virtual cameras according to a config file. */<br>
> +<br>
> + std::unique_ptr<VirtualCameraData> data = std::make_unique<VirtualCameraData>(this);<br>
> +<br>
> + data->supportedResolutions_.resize(2);<br>
> + data->supportedResolutions_[0] = { .size = Size(1920, 1080), .frame_rates = { 30 } };<br>
> + data->supportedResolutions_[1] = { .size = Size(1280, 720), .frame_rates = { 30, 60 } };<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 = 30, max_frame_duration = 60;<br>
<br>
doesn't match the above frame rates and it should be expressed in<br>
microseconds. I would suggest for this patch to set both framerates to<br>
30 and initialize FrameDurationLimits with {33333, 333333}<br>
<br>
It's not a big deal however, it will be replaced later in the series<br>
<br></blockquote><div><br></div><div>Done, 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">
<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{ &data->stream_ };<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 false; // Prevent infinite loops for now<br>
<br>
I presume this will also be changed in next patches...<br></blockquote><div>Updated in this patch, with a static boolean though.</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>
> +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 000000000..6fc6b34d8<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.h<br>
> @@ -0,0 +1,78 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * virtual.h - Pipeline handler for virtual cameras<br>
> + */<br>
> +<br>
> +#pragma once<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>
> + struct Resolution {<br>
> + Size size;<br>
> + std::vector<int> frame_rates;<br>
> + };<br>
> + VirtualCameraData(PipelineHandler *pipe)<br>
> + : Camera::Private(pipe)<br>
> + {<br>
> + }<br>
> +<br>
> + ~VirtualCameraData() = default;<br>
> +<br>
> + std::vector<Resolution> supportedResolutions_;<br>
> +<br>
> + Stream stream_;<br>
> +};<br>
> +<br>
> +class VirtualCameraConfiguration : public CameraConfiguration<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>
> + 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.184.g6999bdac58-goog<br>
><br>
</blockquote></div></div>