<div dir="ltr"><div dir="ltr">Thanks Jacopo,</div><div><br></div><div>The fixes are applied to v6.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 15, 2023 at 3:20 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>
mostly cosmetic comments, my concern is on 1/2 and I wuold like to<br>
hear from others, maybe it has been discussed in the past, but it<br>
worries me a bit.<br>
<br>
On Fri, Apr 07, 2023 at 06:20:50PM +0800, Harvey Yang via libcamera-devel wrote:<br>
> Add VirtualPipelineHandler for more unit tests and verfiy libcamera<br>
> infrastructure works on devices without using hardware cameras.<br>
><br>
> Updating `/etc/camera/libcamera/camera_hal.yaml` on a chromebook DUT is<br>
> required to find the virtual camera with id `Virtual0`.<br>
<br>
Why ? For the Android HAL ? If that's the case this is not related to<br>
this patch and you can drop this line<br>
<br></blockquote><div><br></div><div>Yes, it's only for chromeos that uses android adapter. Removed.</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>
> [todo Read frames from the virtual video if any]<br>
<br>
I think you can drop this as well<br>
<br></blockquote><div><br></div><div>Removed.</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>
> Test the configurations can be generated and reported with cam -I:<br>
> """<br>
> build/src/apps/cam/cam -c 1 -I<br>
> [45:19:28.901456135] [2611530] INFO IPAManager ipa_manager.cpp:143<br>
> libcamera is not installed. Adding<br>
> '/usr/local/google/home/chenghaoyang/cros2/src/third_party/libcamera/build/src<br>
> /ipa' to the IPA search path<br>
> [45:19:28.904364465] [2611530] INFO Camera camera_manager.cpp:293<br>
> libcamera v0.0.1+56-4f4554fa-dirty (2022-12-07T06:55:04+00:00)<br>
> Using camera Virtual0 as cam0<br>
> 0: 1920x1080-NV12<br>
> * Pixelformat: NV12 (1280x720)-(1920x1080)/(+1,+1)<br>
> - 1280x720<br>
> - 1280x800<br>
> - 1360x768<br>
> - 1366x768<br>
> - 1440x900<br>
> - 1280x1024<br>
> - 1536x864<br>
> - 1280x1080<br>
> - 1600x900<br>
> - 1400x1050<br>
> - 1680x1050<br>
> - 1920x1080<br>
> """<br>
><br>
> """<br>
> build/src/apps/cam/cam -l<br>
> [550:47:04.505850647] [1969734] INFO IPAManager ipa_manager.cpp:143<br>
> libcamera is not installed. Adding ... to the IPA search path<br>
> [550:47:04.509307667] [1969734] INFO Camera camera_manager.cpp:293<br>
> libcamera v0.0.1+54-55fecb4d-dirty (2022-12-01T05:47:11+00:00)<br>
> Available cameras:<br>
> 1: (Virtual0)<br>
> """<br>
><br>
> Using qcam should receive blank (all green) frames:<br>
> """<br>
> build/src/apps/qcam/qcam -c Virtual0<br>
> """<br>
<br>
All the examples are better placed in the cover letter imho, there's<br>
no need to have them in the commit history ?<br>
<br></blockquote><div><br></div><div>Removed.</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>
> 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 | 302 +++++++++++++++++++++<br>
> 4 files changed, 310 insertions(+), 1 deletion(-)<br>
> create mode 100644 src/libcamera/pipeline/virtual/meson.build<br>
> create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp<br>
><br>
> diff --git a/meson.build b/meson.build<br>
> index 0f89b45a..c2c4ba5f 100644<br>
> --- a/meson.build<br>
> +++ b/meson.build<br>
> @@ -177,6 +177,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 78a78b72..a0a75e7f 100644<br>
> --- a/meson_options.txt<br>
> +++ b/meson_options.txt<br>
> @@ -47,7 +47,8 @@ option('pipelines',<br>
> 'rkisp1',<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..ba7ff754<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 00000000..78616fab<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp<br>
> @@ -0,0 +1,302 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * fake.cpp - Pipeline handler for fake cameras<br>
> + */<br>
<br>
This is not called virtual, please update this and the copyright year<br>
maybe<br>
<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>
> +#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/heap_allocator.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>
I would use "Virtual"<br>
<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>
> +static const ControlInfoMap::Map VirtualControls = {<br>
> + { &controls::draft::PipelineDepth, ControlInfo(2, 3) },<br>
<br>
Is this just for testing ?<br>
<br></blockquote><div><br></div><div>Removed.</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>
> +uint64_t CurrentTimestamp()<br>
<br>
function names start with lowecase letter<br>
if you want a static helper please place it in an anonymous namespace<br>
<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>
> + 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>
> +class VirtualCameraData : public Camera::Private<br>
> +{<br>
> +public:<br>
> + struct Resolution {<br>
> + Size size;<br>
> + std::vector<int> frame_rates;<br>
> + std::vector<std::string> formats;<br>
> + };<br>
> + VirtualCameraData(PipelineHandler *pipe)<br>
> + : Camera::Private(pipe)<br>
> + {<br>
> + }<br>
> +<br>
> + ~VirtualCameraData() = default;<br>
> +<br>
> + std::vector<Resolution> supportedResolutions_;<br>
<br>
Do you plan to initialize this based on some configuration file ?<br>
Currently it is dynamically initialized at match() time with static<br>
data, so it could very well be a static const class member.<br>
<br></blockquote><div><br></div><div>Yes, we'll initialize this with a configuration file in the following patches.</div><div>Therefore, let's keep it as a member variable for now.</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>
> + Stream stream_;<br>
> +};<br>
> +<br>
> +class VirtualCameraConfiguration : public CameraConfiguration<br>
> +{<br>
> +public:<br>
> + static constexpr unsigned int kBufferCount = 4; // 4~6<br>
<br>
In the whole file, please do not use C++ comment style, we use C99<br>
ones. Also, this comment seems a development leftover so please drop<br>
it<br>
<br></blockquote><div><br></div><div>Dropped</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(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>
> + const StreamRoles &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>
> + HeapAllocator heapAllocator_;<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>
> + // TODO: check if we should limit |config_.size()|<br>
<br>
Please use \todo as the rest of the codebase does<br>
<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">
And yes, you only have one stream, so you should resize(1) and Adjust<br>
Otherwise, you should make VirtualCameraData::stream_ an array of<br>
multiple entries if you want to support multiple streams<br>
<br></blockquote><div><br></div><div>Updated and added a comment.</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>
> + Size maxSize;<br>
> + for (const auto &resolution : data_->supportedResolutions_)<br>
> + maxSize = std::max(maxSize, resolution.size);<br>
<br>
if supportedResolutions_ is constructed at match() time and never<br>
changed at run-time you could very well find the max and min element<br>
once at match() time<br>
<br></blockquote><div><br></div><div>Hmm, I'd prefer to keep the same source of truth. It doesn't affect the </div><div>performance that much as well. WDYT?</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>
> + for (StreamConfiguration &cfg : config_) {<br>
<br>
And you can spare this cycle if you only support one stream<br>
<br></blockquote><div><br></div><div>I'd prefer to keep this, in case we support more than one stream in the future.<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 && resolution.size.height >= cfg.size.height) {<br>
<br>
Please break this rather long line<br>
<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">
Also, is this correct ? You "found = true" when you hit a resolution<br>
>= than the requested one. Shouldn't you "Adjust" if the resolution is<br>
not exactly equal to the one you're looking for ?<br></blockquote><div><br></div><div>You're right. 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">
> + found = true;<br>
> + break;<br>
> + }<br>
> + }<br>
> +<br>
> + if (!found) {<br>
> + cfg.size = maxSize;<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.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> PipelineHandlerVirtual::generateConfiguration(Camera *camera,<br>
> + const StreamRoles &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>
Like in validate() if you only support one stream you should only<br>
consider a single role<br>
<br></blockquote><div><br></div><div>I think it's a different thing here: We currently only support only one</div><div>stream, but the StreamRole of it can be one of {StillCapture, Raw,</div><div>ViewFinder, VideoRecorder}. Should the caller get the default</div><div>configuration of each StreamRole one by one, or it can get all four</div><div>by once?</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>
> + 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>
> +<br>
> + for (const StreamRole role : roles) {<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>
> + 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>
> + return config;<br>
<br>
you can remove this line as you would return config anyway a line<br>
below<br>
<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>
> + return config;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::configure(Camera *camera, CameraConfiguration *config)<br>
> +{<br>
> + (void)camera;<br>
> + (void)config;<br>
<br>
please use [[maybe_unused]] for not used parameters<br>
<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">
> + // Nothing to be done.<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::exportFrameBuffers(Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + if (!heapAllocator_.isValid())<br>
> + return -ENOBUFS;<br>
> +<br>
> + return heapAllocator_.exportFrameBuffers(camera, stream, buffers);<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::start(Camera *camera, const ControlList *controls)<br>
> +{<br>
> + (void)camera;<br>
> + (void)controls;<br>
> + // TODO: Start reading the virtual video if any.<br>
> + return 0;<br>
> +}<br>
> +<br>
> +void PipelineHandlerVirtual::stopDevice(Camera *camera)<br>
> +{<br>
> + (void)camera;<br>
> + // TODO: Reset the virtual video if any.<br>
> +}<br>
> +<br>
> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)<br>
> +{<br>
> + (void)camera;<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(DeviceEnumerator *enumerator)<br>
> +{<br>
> + (void)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 }, .formats = { "YCbCr_420_888" } };<br>
> + data->supportedResolutions_[1] = { .size = Size(1280, 720), .frame_rates = { 30, 60 }, .formats = { "YCbCr_420_888" } };<br>
<br>
Not sure what you will use "formats" for, but those are the Android<br>
format names. Could you use the libcamera ones ?<br>
<br></blockquote><div><br>Removed `formats` in `VirtualCameraData::Resolution`.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<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 = VirtualControls;<br>
> + int64_t min_frame_duration = 30, max_frame_duration = 60;<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>
> +<br>
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVirtual)<br>
> +<br>
> +} /* namespace libcamera */<br>
> --<br>
> 2.40.0<br>
><br>
</blockquote></div></div>