[libcamera-devel,v2,1/1] Add fake pipeline handler

17595 diff mbox series

---
 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

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.

>          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/

> +        */
> +       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 !

> +       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.


> +
> +       // 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, 

> +
> +       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
>

Quoting Kieran Bingham (2022-10-28 11:17:40)
> 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.
> 

<snip>

> > +
> > +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.

As a heads up, to prevent duplication of work - I've started this - and
now I have an allocator that seems to allocate dmabufs from
/dev/udmabuf.

More work required, but I think it could be useful in more places.

As soon as it's tested, I'll get it on the list, which could then act as
an allocator here.

> 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.
> 
>

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@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.
>
> >          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 !
>
> > +       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.
>
>
> > +
> > +       // 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,
>
> > +
> > +       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
> >
>

Hi Kieran,

On Thu, Jan 5, 2023 at 11:50 PM Kieran Bingham <
kieran.bingham@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@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@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@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@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
> > > > >> > > >
> > > > >> > >
> > > > >>
> > > > >
> > >
>