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