<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 5, 2023 at 11:50 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Cheng-Hao Yang (2023-01-05 15:37:59)<br>
> Hi Kieran,<br>
> <br>
> On Thu, Jan 5, 2023 at 10:38 PM Kieran Bingham <<br>
> <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> <br>
> > Quoting Cheng-Hao Yang (2023-01-05 04:41:12)<br>
> > > Hi Kieran,<br>
> > ><br>
> > > Sorry for the slow pace. In the v3 patches, I merely split the patches,<br>
> > > added MediaDeviceBase as<br>
> > > the base class of MediaDevice, and fixed some issues you mentioned. I<br>
> > still<br>
> > > need to work on<br>
> > > the buffer allocation, the virtual config, the virtual video for testing,<br>
> > > and other stuff.<br>
> > ><br>
> > > Please take a quick look and let me know if the code structure makes<br>
> > sense<br>
> > > to you. Thanks!<br>
> > ><br>
> > ><br>
> > > On Fri, Nov 25, 2022 at 2:17 PM Cheng-Hao Yang <<br>
> > <a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > wrote:<br>
> > ><br>
> > > > Thanks Kieran!<br>
> > > ><br>
> > > > I spent quite some time on other ChromeOS priorities recently, and I'm<br>
> > > > halfway through splitting the patch and starting from scratch<br>
> > > > (instead of starting from the ipu3 pipeline handler).<br>
> > > ><br>
> > > > I'll rebase on your patches and do the tests when I'm ready.<br>
> > > ><br>
> > > > On Thu, Nov 24, 2022 at 10:01 PM Kieran Bingham <<br>
> > > > <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> > > ><br>
> > > >> (offlist / direct mail)<br>
> > > >><br>
> > > >> Hi<br>
> > > >><br>
> > > >> I was doing more work on this last night and exploring - and thought<br>
> > > >> you'd be interested in seeing it early, or building on top.<br>
> > > >><br>
> > > >> I've pushed my development branch to :<br>
> > > >><br>
> > > >> <a href="https://github.com/kbingham/libcamera/tree/kbingham/fake" rel="noreferrer" target="_blank">https://github.com/kbingham/libcamera/tree/kbingham/fake</a><br>
> > > >><br>
> > > >> This shows an addition of a basic test pattern generator (only runs<br>
> > once<br>
> > > >> when the buffers are allocated, mostly to validate that the buffers<br>
> > are<br>
> > > >> usable/visible), and a udmabuf allocator.<br>
> > > >><br>
> > > >> While this is working, I expect it will/should probably already get<br>
> > > >> reworked to use a dma_heap allocator instead. Perhaps making reuse of<br>
> > > >> the dma_heap classes provided by RPi already.<br>
> > > >><br>
> > > >><br>
> > > Thanks for looking into this! So you mean that we could move the DmaHeap<br>
> > > from<br>
> > > the RPi pipeline handler to a common base class, and perhaps add helper<br>
> > > functions<br>
> > > like your `createBuffer` & `exportFrameBuffer` in the WIP commit [1],<br>
> > right?<br>
> > > I can create a separate patch to work on this :)<br>
> ><br>
> > Yes. Exactly.<br>
> ><br>
> > There might be system permissions that affect if users can or cannot<br>
> > access dma heaps or the udambufs ... but I think a common 'core'<br>
> > allocator that starts with dma_heap and fallsback to udmabuf would be<br>
> > better. Probably just start by moving the RPi dma_heap allocator to<br>
> > core so it can be used by other pipeline handlers (including this<br>
> > virtual/fake one).<br>
> ><br>
> ><br>
> I see. That makes a lot of sense. Let me prepare another patch before<br>
> this one.<br>
> <br>
> <br>
> ><br>
> > ><br>
> > > [1]:<br>
> > ><br>
> > <a href="https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c" rel="noreferrer" target="_blank">https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c</a><br>
> > ><br>
> > ><br>
> > > > --<br>
> > > >> Kieran<br>
> > > >><br>
> > > >><br>
> > > >> Quoting Cheng-Hao Yang (2022-10-31 05:50:55)<br>
> > > >> > Thanks Kieran for the further review and the help of the new<br>
> > allocator!<br>
> > > >> ><br>
> > > >> ><br>
> > > >> > On Fri, Oct 28, 2022 at 7:17 PM Kieran Bingham <<br>
> > > >> > <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> > > >> ><br>
> > > >> > > Hi Harvey,<br>
> > > >> > ><br>
> > > >> > > I do think I see benefits to a no-hardware (no-kernel) camera<br>
> > support.<br>
> > > >> > > I believe it would help developments and testing. So I'm keen to<br>
> > see<br>
> > > >> > > this progress.<br>
> > > >> > ><br>
> > > >> > > The big part here that worries me is the Fatal when trying to<br>
> > handle<br>
> > > >> > > buffer alloction.<br>
> > > >> > ><br>
> > > >> > > We need to do better than that, as it means qcam just aborts when<br>
> > I<br>
> > > >> > > apply this patch and try to test it.<br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > > Quoting Harvey Yang via libcamera-devel (2022-10-14 11:36:12)<br>
> > > >> > ><br>
> > > >> > > Missing commit message, and an SoB.<br>
> > > >> > ><br>
> > > >> > > > ---<br>
> > > >> > > > meson_options.txt | 2 +-<br>
> > > >> > > > src/libcamera/pipeline/fake/fake.cpp | 441<br>
> > > >> ++++++++++++++++++++++++<br>
> > > >> > > > src/libcamera/pipeline/fake/meson.build | 3 +<br>
> > > >> > > > test/camera/camera_reconfigure.cpp | 2 +-<br>
> > > >> > > > 4 files changed, 446 insertions(+), 2 deletions(-)<br>
> > > >> > > > create mode 100644 src/libcamera/pipeline/fake/fake.cpp<br>
> > > >> > > > create mode 100644 src/libcamera/pipeline/fake/meson.build<br>
> > > >> > > ><br>
> > > >> > > > diff --git a/meson_options.txt b/meson_options.txt<br>
> > > >> > > > index f1d67808..f08dfc5f 100644<br>
> > > >> > > > --- a/meson_options.txt<br>
> > > >> > > > +++ b/meson_options.txt<br>
> > > >> > > > @@ -37,7 +37,7 @@ option('lc-compliance',<br>
> > > >> > > ><br>
> > > >> > > > option('pipelines',<br>
> > > >> > > > type : 'array',<br>
> > > >> > > > - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple',<br>
> > > >> > > 'uvcvideo', 'vimc'],<br>
> > > >> > > > + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple',<br>
> > > >> > > 'uvcvideo', 'vimc', 'fake'],<br>
> > > >> > ><br>
> > > >> > > I know it's bikeshedding territory - but the more I've thought<br>
> > about<br>
> > > >> > > this, the more I think 'virtual' would be a better name to get<br>
> > this<br>
> > > >> in.<br>
> > > >> > ><br>
> > > >> > > 'Fake' sounds so ... negative ? fake ?<br>
> > > >> > ><br>
> > > >> > > But it's just a color of the shed.<br>
> > > >> > ><br>
> > > >><br>
> > > ><br>
> > > Thanks for the suggestion. Using 'virtual' now.<br>
> > ><br>
> ><br>
> > Please be sure to rebase to the latest master. The series you've just<br>
> > posted doesn't look like it is on top of the latest changes to the<br>
> > 'pipeline' updates.<br>
> ><br>
> ><br>
> Right. Will do.<br>
> <br>
> <br>
> > ><br>
> > > > > > > description : 'Select which pipeline handlers to<br>
> > include')<br>
> > > >> > > ><br>
> > > >> > > > option('qcam',<br>
> > > >> > > > diff --git a/src/libcamera/pipeline/fake/fake.cpp<br>
> > > >> > > b/src/libcamera/pipeline/fake/fake.cpp<br>
> > > >> > > > new file mode 100644<br>
> > > >> > > > index 00000000..1ee24e3b<br>
> > > >> > > > --- /dev/null<br>
> > > >> > > > +++ b/src/libcamera/pipeline/fake/fake.cpp<br>
> > > >> > > > @@ -0,0 +1,441 @@<br>
> > > >> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > > >> > > > +/*<br>
> > > >> > > > + * Copyright (C) 2019, Google Inc.<br>
> > > >> > ><br>
> > > >> > > 2022<br>
> > > >> > ><br>
> > > >> > > + *<br>
> > > >> > > > + * fake.cpp - Pipeline handler for fake cameras<br>
> > > >> > > > + */<br>
> > > >> > > > +<br>
> > > >> > > > +#include <algorithm><br>
> > > >> > > > +#include <iomanip><br>
> > > >> > > > +#include <memory><br>
> > > >> > > > +#include <queue><br>
> > > >> > > > +#include <set><br>
> > > >> > > > +#include <vector><br>
> > > >> > > > +<br>
> > > >> > > > +#include <libcamera/base/log.h><br>
> > > >> > > > +#include <libcamera/base/utils.h><br>
> > > >> > > > +<br>
> > > >> > > > +#include <libcamera/camera_manager.h><br>
> > > >> > > > +#include <libcamera/controls.h><br>
> > > >> > > > +#include <libcamera/control_ids.h><br>
> > > >> > > > +#include <libcamera/formats.h><br>
> > > >> > > > +#include <libcamera/property_ids.h><br>
> > > >> > > > +#include <libcamera/request.h><br>
> > > >> > > > +#include <libcamera/stream.h><br>
> > > >> > > > +<br>
> > > >> > > > +#include "libcamera/internal/camera.h"<br>
> > > >> > > > +#include "libcamera/internal/device_enumerator.h"<br>
> > > >> > > > +#include "libcamera/internal/formats.h"<br>
> > > >> > > > +#include "libcamera/internal/framebuffer.h"<br>
> > > >> > > > +#include "libcamera/internal/media_device.h"<br>
> > > >> > > > +#include "libcamera/internal/pipeline_handler.h"<br>
> > > >> > > > +<br>
> > > >> > > > +namespace libcamera {<br>
> > > >> > > > +<br>
> > > >> > > > +LOG_DEFINE_CATEGORY(Fake)<br>
> > > >> > > > +<br>
> > > >> > > > +uint64_t CurrentTimestamp()<br>
> > > >> > > > +{<br>
> > > >> > > > + struct timespec ts;<br>
> > > >> > > > + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {<br>
> > > >> > > > + LOG(Fake, 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>
> > > >> > > > +static const ControlInfoMap::Map FakeControls = {<br>
> > > >> > > > + { &controls::draft::PipelineDepth, ControlInfo(2, 3) },<br>
> > > >> > > > +};<br>
> > > >> > > > +<br>
> > > >> > > > +class FakeCameraData : public Camera::Private<br>
> > > >> > > > +{<br>
> > > >> > > > +public:<br>
> > > >> > > > + struct Resolution {<br>
> > > >> > > > + Size size;<br>
> > > >> > > > + std::vector<int> frame_rates;<br>
> > > >> > > > + std::vector<PixelFormat> formats;<br>
> > > >> > > > + };<br>
> > > >> > > > +<br>
> > > >> > > > + FakeCameraData(PipelineHandler *pipe)<br>
> > > >> > > > + : Camera::Private(pipe)<br>
> > > >> > > > + {<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + std::vector<Resolution> supportedResolutions_;<br>
> > > >> > > > +<br>
> > > >> > > > + Stream outStream_;<br>
> > > >> > > > + Stream vfStream_;<br>
> > > >> > > > + Stream rawStream_;<br>
> > > >> > > > +<br>
> > > >> > > > + bool started_ = false;<br>
> > > >> > > > +};<br>
> > > >> > > > +<br>
> > > >> > > > +class FakeCameraConfiguration : public CameraConfiguration<br>
> > > >> > > > +{<br>
> > > >> > > > +public:<br>
> > > >> > > > + static constexpr unsigned int kBufferCount = 4; // 4~6<br>
> > > >> > > > + static constexpr unsigned int kMaxStreams = 3;<br>
> > > >> > > > +<br>
> > > >> > > > + FakeCameraConfiguration(FakeCameraData *data);<br>
> > > >> > > > +<br>
> > > >> > > > + Status validate() override;<br>
> > > >> > > > +<br>
> > > >> > > > +private:<br>
> > > >> > > > + /*<br>
> > > >> > > > + * The FakeCameraData instance is guaranteed to be<br>
> > valid as<br>
> > > >> long<br>
> > > >> > > as the<br>
> > > >> > > > + * corresponding Camera instance is valid. In order to<br>
> > > >> borrow a<br>
> > > >> > > > + * reference to the camera data, store a new reference<br>
> > to<br>
> > > >> the<br>
> > > >> > > camera.<br>
> > > >> > > > + */<br>
> > > >> > > > + const FakeCameraData *data_;<br>
> > > >> > > > +};<br>
> > > >> > > > +<br>
> > > >> > > > +class PipelineHandlerFake : public PipelineHandler<br>
> > > >> > > > +{<br>
> > > >> > > > +public:<br>
> > > >> > > > + PipelineHandlerFake(CameraManager *manager);<br>
> > > >> > > > +<br>
> > > >> > > > + CameraConfiguration *generateConfiguration(Camera<br>
> > *camera,<br>
> > > >> > > > + const<br>
> > StreamRoles<br>
> > > >> > > &roles) override;<br>
> > > >> > > > + int configure(Camera *camera, CameraConfiguration<br>
> > *config)<br>
> > > >> > > override;<br>
> > > >> > > > +<br>
> > > >> > > > + int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> > > >> > > > +<br>
> > > >> std::vector<std::unique_ptr<FrameBuffer>><br>
> > > >> > > *buffers) override;<br>
> > > >> > > > +<br>
> > > >> > > > + int start(Camera *camera, const ControlList *controls)<br>
> > > >> override;<br>
> > > >> > > > + void stopDevice(Camera *camera) override;<br>
> > > >> > > > +<br>
> > > >> > > > + int queueRequestDevice(Camera *camera, Request *request)<br>
> > > >> > > override;<br>
> > > >> > > > +<br>
> > > >> > > > + bool match(DeviceEnumerator *enumerator) override;<br>
> > > >> > > > +<br>
> > > >> > > > +private:<br>
> > > >> > > > + FakeCameraData *cameraData(Camera *camera)<br>
> > > >> > > > + {<br>
> > > >> > > > + return static_cast<FakeCameraData<br>
> > *>(camera->_d());<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + int registerCameras();<br>
> > > >> > > > +<br>
> > > >> > > > + static bool registered_;<br>
> > > >> > > > +};<br>
> > > >> > > > +<br>
> > > >> > > > +bool PipelineHandlerFake::registered_ = false;<br>
> > > >> > > > +<br>
> > > >> > > > +FakeCameraConfiguration::FakeCameraConfiguration(FakeCameraData<br>
> > > >> *data)<br>
> > > >> > > > + : CameraConfiguration()<br>
> > > >> > > > +{<br>
> > > >> > > > + data_ = data;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +CameraConfiguration::Status FakeCameraConfiguration::validate()<br>
> > > >> > > > +{<br>
> > > >> > > > + Status status = Valid;<br>
> > > >> > > > +<br>
> > > >> > > > + if (config_.empty())<br>
> > > >> > > > + return Invalid;<br>
> > > >> > > > +<br>
> > > >> > > > + /* Cap the number of entries to the available streams.<br>
> > */<br>
> > > >> > > > + if (config_.size() > kMaxStreams) {<br>
> > > >> > > > + config_.resize(kMaxStreams);<br>
> > > >> > > > + status = Adjusted;<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + /*<br>
> > > >> > > > + * Validate the requested stream configuration and<br>
> > select<br>
> > > >> the<br>
> > > >> > > sensor<br>
> > > >> > > > + * format by collecting the maximum RAW stream width and<br>
> > > >> height<br>
> > > >> > > and<br>
> > > >> > > > + * picking the closest larger match.<br>
> > > >> > > > + *<br>
> > > >> > > > + * If no RAW stream is requested use the one of the<br>
> > largest<br>
> > > >> YUV<br>
> > > >> > > stream,<br>
> > > >> > > > + * plus margin pixels for the IF and BDS rectangle to<br>
> > > >> downscale.<br>
> > > >> > > > + *<br>
> > > >> > > > + * \todo Clarify the IF and BDS margins requirements.<br>
> > > >> > ><br>
> > > >> > > A fake camera doesn't have any IF/BDS ?<br>
> > > >> > > Still lots of clean up required.<br>
> > > >> > ><br>
> > > >> > > It may be cleaner to start from an empty pipeline handler (or the<br>
> > > >> vivid<br>
> > > >> > > pipeline handler [0]) and work up, rather work down from the IPU3.<br>
> > > >> > ><br>
> > > >> > > [0] <a href="https://git.libcamera.org/libcamera/vivid.git/log/" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/vivid.git/log/</a><br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > I guess you're right. Let me start from an empty one instead.<br>
> > > >> > It might take some time though.<br>
> > > >> ><br>
> > > >> ><br>
> > > >> > > > + */<br>
> > > >> > > > + unsigned int rawCount = 0;<br>
> > > >> > > > + unsigned int yuvCount = 0;<br>
> > > >> > > > + Size rawRequirement;<br>
> > > >> > > > + Size maxYuvSize;<br>
> > > >> > > > + Size rawSize;<br>
> > > >> > > > +<br>
> > > >> > > > + for (const StreamConfiguration &cfg : config_) {<br>
> > > >> > > > + const PixelFormatInfo &info =<br>
> > > >> > > PixelFormatInfo::info(cfg.pixelFormat);<br>
> > > >> > > > +<br>
> > > >> > > > + if (info.colourEncoding ==<br>
> > > >> > > PixelFormatInfo::ColourEncodingRAW) {<br>
> > > >> > > > + rawCount++;<br>
> > > >> > > > + rawSize = std::max(rawSize, cfg.size);<br>
> > > >> > > > + } else {<br>
> > > >> > > > + yuvCount++;<br>
> > > >> > > > + maxYuvSize = std::max(maxYuvSize,<br>
> > cfg.size);<br>
> > > >> > > > + rawRequirement.expandTo(cfg.size);<br>
> > > >> > > > + }<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + // TODO: Check the configuration file.<br>
> > > >> > > > + if (rawCount > 1 || yuvCount > 2) {<br>
> > > >> > > > + LOG(Fake, Debug) << "Camera configuration not<br>
> > > >> supported";<br>
> > > >> > > > + return Invalid;<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + /*<br>
> > > >> > > > + * Adjust the configurations if needed and assign<br>
> > streams<br>
> > > >> while<br>
> > > >> > > > + * iterating them.<br>
> > > >> > > > + */<br>
> > > >> > > > + bool mainOutputAvailable = true;<br>
> > > >> > > > + for (unsigned int i = 0; i < config_.size(); ++i) {<br>
> > > >> > > > + const PixelFormatInfo &info =<br>
> > > >> > > PixelFormatInfo::info(config_[i].pixelFormat);<br>
> > > >> > > > + const StreamConfiguration originalCfg =<br>
> > config_[i];<br>
> > > >> > > > + StreamConfiguration *cfg = &config_[i];<br>
> > > >> > > > +<br>
> > > >> > > > + LOG(Fake, Debug) << "Validating stream: " <<<br>
> > > >> > > config_[i].toString();<br>
> > > >> > > > +<br>
> > > >> > > > + if (info.colourEncoding ==<br>
> > > >> > > PixelFormatInfo::ColourEncodingRAW) {<br>
> > > >> > > > + /* Initialize the RAW stream with the<br>
> > CIO2<br>
> > > >> > > configuration. */<br>
> > > >> > ><br>
> > > >> > > No CIO2.<br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > > > + cfg->size = rawSize;<br>
> > > >> > > > + // TODO: check<br>
> > > >> > > > + cfg->pixelFormat = formats::SBGGR10;<br>
> > > >> > > > + cfg->bufferCount =<br>
> > > >> > > FakeCameraConfiguration::kBufferCount;<br>
> > > >> > > > + cfg->stride =<br>
> > info.stride(cfg->size.width,<br>
> > > >> 0,<br>
> > > >> > > 64);<br>
> > > >> > > > + cfg->frameSize =<br>
> > info.frameSize(cfg->size,<br>
> > > >> 64);<br>
> > > >> > ><br>
> > > >> > > Are these limits you /want/ to impose?<br>
> > > >> > ><br>
> > > >> > > > + cfg->setStream(const_cast<Stream<br>
> > > >> > > *>(&data_->rawStream_));<br>
> > > >> > > > +<br>
> > > >> > > > + LOG(Fake, Debug) << "Assigned " <<<br>
> > > >> > > cfg->toString()<br>
> > > >> > > > + << " to the raw<br>
> > stream";<br>
> > > >> > > > + } else {<br>
> > > >> > > > + /* Assign and configure the main and<br>
> > > >> viewfinder<br>
> > > >> > > outputs. */<br>
> > > >> > > > +<br>
> > > >> > > > + cfg->pixelFormat = formats::NV12;<br>
> > > >> > > > + cfg->bufferCount = kBufferCount;<br>
> > > >> > > > + cfg->stride =<br>
> > info.stride(cfg->size.width,<br>
> > > >> 0, 1);<br>
> > > >> > > > + cfg->frameSize =<br>
> > info.frameSize(cfg->size,<br>
> > > >> 1);<br>
> > > >> > > > +<br>
> > > >> > > > + /*<br>
> > > >> > > > + * Use the main output stream in case<br>
> > only<br>
> > > >> one<br>
> > > >> > > stream is<br>
> > > >> > > > + * requested or if the current<br>
> > > >> configuration is<br>
> > > >> > > the one<br>
> > > >> > > > + * with the maximum YUV output size.<br>
> > > >> > > > + */<br>
> > > >> > > > + if (mainOutputAvailable &&<br>
> > > >> > > > + (originalCfg.size == maxYuvSize ||<br>
> > > >> yuvCount<br>
> > > >> > > == 1)) {<br>
> > > >> > > > + cfg->setStream(const_cast<Stream<br>
> > > >> > > *>(&data_->outStream_));<br>
> > > >> > > > + mainOutputAvailable = false;<br>
> > > >> > > > +<br>
> > > >> > > > + LOG(Fake, Debug) << "Assigned "<br>
> > <<<br>
> > > >> > > cfg->toString()<br>
> > > >> > > > + << " to the<br>
> > main<br>
> > > >> > > output";<br>
> > > >> > > > + } else {<br>
> > > >> > > > + cfg->setStream(const_cast<Stream<br>
> > > >> > > *>(&data_->vfStream_));<br>
> > > >> > > > +<br>
> > > >> > > > + LOG(Fake, Debug) << "Assigned "<br>
> > <<<br>
> > > >> > > cfg->toString()<br>
> > > >> > > > + << " to the<br>
> > > >> viewfinder<br>
> > > >> > > output";<br>
> > > >> > > > + }<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + if (cfg->pixelFormat != originalCfg.pixelFormat<br>
> > ||<br>
> > > >> > > > + cfg->size != originalCfg.size) {<br>
> > > >> > > > + LOG(Fake, Debug)<br>
> > > >> > > > + << "Stream " << i << "<br>
> > configuration<br>
> > > >> > > adjusted to "<br>
> > > >> > > > + << cfg->toString();<br>
> > > >> > > > + status = Adjusted;<br>
> > > >> > > > + }<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + return status;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +PipelineHandlerFake::PipelineHandlerFake(CameraManager<br>
> > *manager)<br>
> > > >> > > > + : PipelineHandler(manager)<br>
> > > >> > > > +{<br>
> > > >> > > > + // TODO: read the fake hal spec file.<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +CameraConfiguration<br>
> > > >> *PipelineHandlerFake::generateConfiguration(Camera<br>
> > > >> > > *camera,<br>
> > > >> > > > +<br>
> > > >> const<br>
> > > >> > > StreamRoles &roles)<br>
> > > >> > > > +{<br>
> > > >> > > > + FakeCameraData *data = cameraData(camera);<br>
> > > >> > > > + FakeCameraConfiguration *config = new<br>
> > > >> > > FakeCameraConfiguration(data);<br>
> > > >> > > > +<br>
> > > >> > > > + if (roles.empty())<br>
> > > >> > > > + return config;<br>
> > > >> > > > +<br>
> > > >> > > > + Size minSize, sensorResolution;<br>
> > > >> > > > + for (const auto& resolution :<br>
> > data->supportedResolutions_) {<br>
> > > >> > > > + if (minSize.isNull() || minSize ><br>
> > resolution.size)<br>
> > > >> > > > + minSize = resolution.size;<br>
> > > >> > > > +<br>
> > > >> > > > + sensorResolution = std::max(sensorResolution,<br>
> > > >> > > resolution.size);<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + for (const StreamRole role : roles) {<br>
> > > >> > > > + std::map<PixelFormat, std::vector<SizeRange>><br>
> > > >> > > streamFormats;<br>
> > > >> > > > + unsigned int bufferCount;<br>
> > > >> > > > + PixelFormat pixelFormat;<br>
> > > >> > > > + Size size;<br>
> > > >> > > > +<br>
> > > >> > > > + switch (role) {<br>
> > > >> > > > + case StreamRole::StillCapture:<br>
> > > >> > > > + size = sensorResolution;<br>
> > > >> > > > + pixelFormat = formats::NV12;<br>
> > > >> > > > + bufferCount =<br>
> > > >> > > FakeCameraConfiguration::kBufferCount;<br>
> > > >> > > > + streamFormats[pixelFormat] = { {<br>
> > minSize,<br>
> > > >> size }<br>
> > > >> > > };<br>
> > > >> > > > +<br>
> > > >> > > > + break;<br>
> > > >> > > > +<br>
> > > >> > > > + case StreamRole::Raw: {<br>
> > > >> > > > + // TODO: check<br>
> > > >> > > > + pixelFormat = formats::SBGGR10;<br>
> > > >> > > > + size = sensorResolution;<br>
> > > >> > > > + bufferCount =<br>
> > > >> > > FakeCameraConfiguration::kBufferCount;<br>
> > > >> > > > + streamFormats[pixelFormat] = { {<br>
> > minSize,<br>
> > > >> size }<br>
> > > >> > > };<br>
> > > >> > > > +<br>
> > > >> > > > + break;<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + case StreamRole::Viewfinder:<br>
> > > >> > > > + case StreamRole::VideoRecording: {<br>
> > > >> > > > + /*<br>
> > > >> > > > + * Default viewfinder and<br>
> > videorecording to<br>
> > > >> > > 1280x720,<br>
> > > >> > > > + * capped to the maximum sensor<br>
> > resolution<br>
> > > >> and<br>
> > > >> > > aligned<br>
> > > >> > > > + * to the ImgU output constraints.<br>
> > > >> > > > + */<br>
> > > >> > > > + size = sensorResolution;<br>
> > > >> > > > + pixelFormat = formats::NV12;<br>
> > > >> > > > + bufferCount =<br>
> > > >> > > FakeCameraConfiguration::kBufferCount;<br>
> > > >> > > > + streamFormats[pixelFormat] = { {<br>
> > minSize,<br>
> > > >> size }<br>
> > > >> > > };<br>
> > > >> > > > +<br>
> > > >> > > > + break;<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + default:<br>
> > > >> > > > + LOG(Fake, Error)<br>
> > > >> > > > + << "Requested stream role not<br>
> > > >> supported:<br>
> > > >> > > " << role;<br>
> > > >> > > > + delete config;<br>
> > > >> > > > + return nullptr;<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + StreamFormats formats(streamFormats);<br>
> > > >> > > > + StreamConfiguration cfg(formats);<br>
> > > >> > > > + cfg.size = size;<br>
> > > >> > > > + cfg.pixelFormat = pixelFormat;<br>
> > > >> > > > + cfg.bufferCount = bufferCount;<br>
> > > >> > > > + config->addConfiguration(cfg);<br>
> > > >> > > > + }<br>
> > > >> > > > +<br>
> > > >> > > > + if (config->validate() == CameraConfiguration::Invalid)<br>
> > > >> > > > + return {};<br>
> > > >> > > > +<br>
> > > >> > > > + return config;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +int PipelineHandlerFake::configure(Camera *camera,<br>
> > > >> CameraConfiguration<br>
> > > >> > > *c)<br>
> > > >> > > > +{<br>
> > > >> > > > + if (camera || c)<br>
> > > >> > > > + return 0;<br>
> > > >> > ><br>
> > > >> > > This doesn't look right ... or helpful ?<br>
> > > >> > ><br>
> > > >> > > Oh - it's just to stop unused warnings I bet.<br>
> > > >> > ><br>
> > > >> > > Maybe we should use them?<br>
> > > >> > ><br>
> > > >> > > > + return 0;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +int PipelineHandlerFake::exportFrameBuffers(Camera *camera,<br>
> > Stream<br>
> > > >> > > *stream,<br>
> > > >> > > > +<br>
> > > >> > > std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> > > >> > > > +{<br>
> > > >> > > > + // Assume it's never called.<br>
> > > >> > > > + LOG(Fake, Fatal) << "exportFrameBuffers should never be<br>
> > > >> called";<br>
> > > >> > ><br>
> > > >> > > I /really/ don't like this though.<br>
> > > >> > ><br>
> > > >> > > And this falls to a fairly big yak-shave... we need a way to get<br>
> > dma<br>
> > > >> > > bufs easier. I think we can do this by creating an allocator with<br>
> > > >> > > /dev/udmabuf<br>
> > > >> > ><br>
> > > >> > > It's a yak though - but is it something you'd be<br>
> > > >> able/willing/interested<br>
> > > >> > > to explore? If not - I'll try to look at it.<br>
> > > >> > ><br>
> > > >> > > But I don't see benefit to merging a virtual/fake pipeline handler<br>
> > > >> that<br>
> > > >> > > can't at least be run with qcam, and currently when I test this<br>
> > patch<br>
> > > >> -<br>
> > > >> > > this is where it fails and breaks with this Fatal error.<br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > > > + if (camera || stream || buffers)<br>
> > > >> > > > + return -EINVAL;<br>
> > > >> > > > + return -EINVAL;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +int PipelineHandlerFake::start(Camera *camera, [[maybe_unused]]<br>
> > > >> const<br>
> > > >> > > ControlList *controls)<br>
> > > >> > > > +{<br>
> > > >> > > > + FakeCameraData *data = cameraData(camera);<br>
> > > >> > > > + data->started_ = true;<br>
> > > >> > > > +<br>
> > > >> > > > + return 0;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +void PipelineHandlerFake::stopDevice(Camera *camera)<br>
> > > >> > > > +{<br>
> > > >> > > > + FakeCameraData *data = cameraData(camera);<br>
> > > >> > > > +<br>
> > > >> > > > + data->started_ = false;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +int PipelineHandlerFake::queueRequestDevice(Camera *camera,<br>
> > Request<br>
> > > >> > > *request)<br>
> > > >> > > > +{<br>
> > > >> > > > + if (!camera)<br>
> > > >> > > > + return -EINVAL;<br>
> > > >> > > > +<br>
> > > >> > > > + for (auto it : request->buffers())<br>
> > > >> > > > + completeBuffer(request, it.second);<br>
> > > >> > > > +<br>
> > > >> > > > + // TODO: request.metadata()<br>
> > > >> > > > + request->metadata().set(controls::SensorTimestamp,<br>
> > > >> > > CurrentTimestamp());<br>
> > > >> > > > + completeRequest(request);<br>
> > > >> > > > +<br>
> > > >> > > > + return 0;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +bool PipelineHandlerFake::match(DeviceEnumerator *enumerator)<br>
> > > >> > > > +{<br>
> > > >> > > > + // TODO: exhaust all devices in |enumerator|.<br>
> > > >> > > > + if (!enumerator)<br>
> > > >> > > > + LOG(Fake, Info) << "Invalid enumerator";<br>
> > > >> > > > +<br>
> > > >> > ><br>
> > > >> > > I bet this might cause us issues if we don't have any devices<br>
> > found by<br>
> > > >> > > the enumerator !<br>
> > > >> > ><br>
> > > >><br>
> > > ><br>
> > > So I need your advice here: With MediaDeviceBase & MediaDeviceVirtual, we<br>
> > > could fake MediaEntity to let DeviceEnumerator match the entity. I can<br>
> > > think of<br>
> > > three ways to do that:<br>
> > > 1. Create fake `media_v2_entity` & `media_v2_interface`, while I'm not<br>
> > sure<br>
> > > how to<br>
> > > do that elegantly...<br>
> > > 2. Add a dummy MediaEntity constructor that only takes the entity name<br>
> > and<br>
> > > other<br>
> > > necessary arguments.<br>
> > > 3. Add a base class for MediaEntity that only supports basic<br>
> > > functionalities.<br>
> > ><br>
> > > I'd prefer the second approach. WDYT?<br>
> ><br>
> > I'm confused ... where did the MediaDeviceBase come from ? Ideally I<br>
> > think we should avoid subclassing the main classes. But I'll have to<br>
> > take a look in more detail at the series you've posted. It looks a bit<br>
> > invasive at first glance.<br>
> ><br>
> ><br>
> ><br>
> The reason that I need MediaDeviceBase & MediaDeviceVirtual is that<br>
> PipelineHandler::registerCamera assumes there is at least one<br>
> MediaDevice available [1]. Also, DeviceEnumerator expects MediaDevice<br>
> to be matched.<br>
> <br>
> Let me know if you have a more elegant approach, or simply loosen some<br>
> constraints. Thanks!<br>
> <br>
> [1]:<br>
> <a href="https://github.com/kbingham/libcamera/blob/master/src/libcamera/pipeline_handler.cpp#L550" rel="noreferrer" target="_blank">https://github.com/kbingham/libcamera/blob/master/src/libcamera/pipeline_handler.cpp#L550</a><br>
<br>
The Fatal at [1] looks like it's to ensure there's a media device to<br>
walk to identify the devnums.<br>
<br>
Please check into what the devnums are used for, I suspect it might just<br>
be about mapping devices to a camera to be able to match with the<br>
v4l2-adaptation layer perhaps? Or maybe it identifies which device nodes<br>
are to be 'locked' during acquire.<br>
<br>
Lets see if theres a way to do that without requiring a mediaDevices_<br>
entry, so it can do something like<br>
<br>
/*<br>
* For virtual devices with no MediaDevice, there are no devnums<br>
* to register.<br>
*/<br>
if (mediaDevice_.empty()) {<br>
manager_->addCamera(std::move(camera), {});<br>
return;<br>
}<br>
<br>
(note- 'devnums' isn't a good description up there. The comment should<br>
describe what we're actually avoiding (if it works).<br>
<br></blockquote><div><br></div><div>Yeah you're right, while I think the fatal was actually preventing pipeline</div><div>handler implementations to search and own media devices with custom</div><div>conventions, instead of using the base function |acquireMediaDevice|.</div><div>It also assumes there will be at least one media device in real cases.</div><div><br></div><div>Now that we break this assumption. Maybe we should simply drop the</div><div>fatal and everything else should still work, i.e. no MediaDeviceVirtual,</div><div>and no searches in |enumerator| in the virtual pipeline handler. WDYT?</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">
> > > > > > > + if (registered_)<br>
> > > >> > > > + return false;<br>
> > > >> > > > +<br>
> > > >> > > > + registered_ = true;<br>
> > > >> > > > + return registerCameras() == 0;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +/**<br>
> > > >> > > > + * \brief Initialise ImgU and CIO2 devices associated with<br>
> > cameras<br>
> > > >> > > > + *<br>
> > > >> > > > + * Initialise the two ImgU instances and create cameras with an<br>
> > > >> > > associated<br>
> > > >> > > > + * CIO2 device instance.<br>
> > > >> > ><br>
> > > >> > > Not an IPU3<br>
> > > >> > ><br>
> > > >> ><br>
> > > >> > > > + *<br>
> > > >> > > > + * \return 0 on success or a negative error code for error or<br>
> > if no<br>
> > > >> > > camera<br>
> > > >> > > > + * has been created<br>
> > > >> > > > + * \retval -ENODEV no camera has been created<br>
> > > >> > > > + */<br>
> > > >> > > > +int PipelineHandlerFake::registerCameras()<br>
> > > >> > > > +{<br>
> > > >> > > > + std::unique_ptr<FakeCameraData> data =<br>
> > > >> > > > + std::make_unique<FakeCameraData>(this);<br>
> > > >> > > > + std::set<Stream *> streams = {<br>
> > > >> > > > + &data->outStream_,<br>
> > > >> > > > + &data->vfStream_,<br>
> > > >> > > > + &data->rawStream_,<br>
> > > >> > > > + };<br>
> > > >> > > > +<br>
> > > >> > > > + // TODO: Read from config or from IPC.<br>
> > > >> > > > + // TODO: Check with Han-lin: Can this function be called<br>
> > > >> more<br>
> > > >> > > than once?<br>
> > > >> > > > + data->supportedResolutions_.resize(2);<br>
> > > >> > > > + data->supportedResolutions_[0].size = Size(1920, 1080);<br>
> > > >> > > > +<br>
> > data->supportedResolutions_[0].frame_rates.push_back(30);<br>
> > > >> > > > +<br>
> > > >> data->supportedResolutions_[0].formats.push_back(formats::NV12);<br>
> > > >> > > > +<br>
> > > >> data->supportedResolutions_[0].formats.push_back(formats::MJPEG);<br>
> > > >> > > > + data->supportedResolutions_[1].size = Size(1280, 720);<br>
> > > >> > > > +<br>
> > data->supportedResolutions_[1].frame_rates.push_back(30);<br>
> > > >> > > > +<br>
> > data->supportedResolutions_[1].frame_rates.push_back(60);<br>
> > > >> > > > +<br>
> > > >> data->supportedResolutions_[1].formats.push_back(formats::NV12);<br>
> > > >> > > > +<br>
> > > >> data->supportedResolutions_[1].formats.push_back(formats::MJPEG);<br>
> > > >> > ><br>
> > > >> > > I'd be weary that if we report we can send MJPEG data, and then<br>
> > send<br>
> > > >> > > buffers which do not contain mjpeg, it could cause problems or<br>
> > > >> > > essentially feed jpeg decoders with bad data.<br>
> > > >> > ><br>
> > > >> > > That 'might' be a suitable test case, but it's something to<br>
> > consider<br>
> > > >> > > explicitly.<br>
> > > >> > ><br>
> > > >><br>
> > > ><br>
> > > Right. I'll check if we want to support MJPEG format later.<br>
> > ><br>
> > ><br>
> > > > > ><br>
> > > >> > > > +<br>
> > > >> > > > + // TODO: Assign different locations for different<br>
> > cameras<br>
> > > >> based<br>
> > > >> > > on config.<br>
> > > >> > > > + data->properties_.set(properties::Location,<br>
> > > >> > > properties::CameraLocationFront);<br>
> > > >> > > > +<br>
> > data->properties_.set(properties::PixelArrayActiveAreas, {<br>
> > > >> > > Rectangle(Size(1920, 1080)) });<br>
> > > >> > > > +<br>
> > > >> > > > + // TODO: Set FrameDurationLimits based on config.<br>
> > > >> > > > + ControlInfoMap::Map controls = FakeControls;<br>
> > > >> > > > + int64_t min_frame_duration = 30, max_frame_duration =<br>
> > 60;<br>
> > > >> > > > + controls[&controls::FrameDurationLimits] =<br>
> > > >> > > ControlInfo(min_frame_duration, max_frame_duration);<br>
> > > >> > > > + data->controlInfo_ = ControlInfoMap(std::move(controls),<br>
> > > >> > > controls::controls);<br>
> > > >> > > > +<br>
> > > >> > > > + std::shared_ptr<Camera> camera =<br>
> > > >> > > > + Camera::create(std::move(data),<br>
> > > >> "\\_SB_.PCI0.I2C4.CAM1"<br>
> > > >> > > /* cameraId */, streams);<br>
> > > >> > ><br>
> > > >> > > I don't like hardcoding a path like that. I'd prefer it was<br>
> > explicitly<br>
> > > >> > > called 'Fake' or 'Virtual' ... perhaps even with an index,<br>
> > > >> > ><br>
> > > >><br>
> > > ><br>
> > > Using "Virtual0" for now, while it requires users to modify<br>
> > > `camera_hal.yaml` on the<br>
> > > chromebook dut to find the device for now. We'll update the config file<br>
> > on<br>
> > > boards like<br>
> > > soraka when we finalize the patches.<br>
> ><br>
> > I'd expect the virtual pipeline handler could certainly be worthy of a<br>
> > configuration file to support it - so the name, and what formats it<br>
> > exposes could also be specified there.<br>
> ><br>
> > But that can also be on top, and discussed in the series you've posted.<br>
> ><br>
> ><br>
> Right, that also works. We can let the android adapter read the same config<br>
> file to search for the cameras.<br>
> <br>
> <br>
> ><br>
> > > > > > > +<br>
> > > >> > > > + manager_->addCamera(std::move(camera), {});<br>
> > > >> > > > +<br>
> > > >> > > > + return 0;<br>
> > > >> > > > +}<br>
> > > >> > > > +<br>
> > > >> > > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerFake)<br>
> > > >> > > > +<br>
> > > >> > > > +} /* namespace libcamera */<br>
> > > >> > > > diff --git a/src/libcamera/pipeline/fake/meson.build<br>
> > > >> > > b/src/libcamera/pipeline/fake/meson.build<br>
> > > >> > > > new file mode 100644<br>
> > > >> > > > index 00000000..13980b4a<br>
> > > >> > > > --- /dev/null<br>
> > > >> > > > +++ b/src/libcamera/pipeline/fake/meson.build<br>
> > > >> > > > @@ -0,0 +1,3 @@<br>
> > > >> > > > +# SPDX-License-Identifier: CC0-1.0<br>
> > > >> > > > +<br>
> > > >> > > > +libcamera_sources += files([ 'fake.cpp' ])<br>
> > > >> > > > diff --git a/test/camera/camera_reconfigure.cpp<br>
> > > >> > > b/test/camera/camera_reconfigure.cpp<br>
> > > >> > > > index d12e2413..06c87730 100644<br>
> > > >> > > > --- a/test/camera/camera_reconfigure.cpp<br>
> > > >> > > > +++ b/test/camera/camera_reconfigure.cpp<br>
> > > >> > > > @@ -98,7 +98,7 @@ private:<br>
> > > >> > > > return TestFail;<br>
> > > >> > > > }<br>
> > > >> > > ><br>
> > > >> > > > - requests_.push_back(move(request));<br>
> > > >> > > > + requests_.push_back(std::move(request));<br>
> > > >> > ><br>
> > > >> > > What is this change for ? If it's required it should be broken<br>
> > out to<br>
> > > >> a<br>
> > > >> > > seperate patch.<br>
> > > >> > ><br>
> > > >> > > --<br>
> > > >> > > Kieran<br>
> > > >> > ><br>
> > > >> > ><br>
> > > >> > > > }<br>
> > > >> > > ><br>
> > > >> > > > camera_->requestCompleted.connect(this,<br>
> > > >> > > &CameraReconfigure::requestComplete);<br>
> > > >> > > > --<br>
> > > >> > > > 2.38.0.413.g74048e4d9e-goog<br>
> > > >> > > ><br>
> > > >> > ><br>
> > > >><br>
> > > ><br>
> ><br>
</blockquote></div></div>