<div dir="ltr"><div dir="ltr">Thanks Jacopo!</div><div dir="ltr"><br></div><div>The patch v2 is uploaded based on your comments.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 12, 2022 at 11:06 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
I had the same thought as Kieran had, why not vimc ? I understan<br>
your reasons behind it and the requirement to backport patches for it<br>
to older kernel (btw, does it need anything more than CAP_IO_MC ?)<br>
<br></blockquote><div><br></div><div>Is it a great effort to backport patches to older kernel versions?</div><div>Regarding CAP_IO_MC, I don't know :p. Anyone can help with this?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I have some general questions<br>
<br>
1) What does this pipeline handler matches on ? It seems to me match()<br>
always returns true, hence if the pipeline handler is compiled in, it<br>
will always report one camera available ?<br>
<br></blockquote><div><br></div><div>I think it'll eventually depend on the configuration file provided by the tester.</div><div>We might come up with some fake media devices, without matching the real</div><div>ones. But I haven't made up my mind yet.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2) What are the requirements for this pipeline handler ?<br>
<br>
Is it enough to test the Android layer to implement stub methods,<br>
or should frames be generated somehow (I'm thinking at CTS frame<br>
rate checks in example).<br>
<br>
Should it support multiple streams ? I guess it depends on the HW<br>
level one wants to test in CTS, or should it mimik the capabilities<br>
of a known device (ie IPU3 that can produce 2 YUV streams and one<br>
RAW)<br>
<br>
I guess however this can be started as a skeleton and be populated<br>
as required to complete CTS.<br>
<br></blockquote><div>Additional features like custom configurations (of cameras) and frames from </div><div>a video file will be added.</div><div><br></div><div>In CrOS, it helps applications to test usages in different (fake) hardware setup,</div><div>while it also helps libcamera test the Android adapter.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
3) Should all mentions of IPU3 related components be removed ?<br>
<br></blockquote><div>Yes</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
4) Possible bikeshedding, but I wonder if "dummy" isn't better than<br>
"fake"<br>
<br></blockquote><div>As we need some more features, like a custom configuration file/cameras </div><div>and frames from a video file, I believe "fake" is still more appropriate.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Wed, Oct 12, 2022 at 07:59:25AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> ---<br>
> meson_options.txt | 2 +-<br>
> src/android/camera_capabilities.cpp | 1 +<br>
> src/libcamera/pipeline/fake/fake.cpp | 518 ++++++++++++++++++++++++<br>
> src/libcamera/pipeline/fake/meson.build | 3 +<br>
> src/libcamera/pipeline_handler.cpp | 2 +-<br>
> test/camera/camera_reconfigure.cpp | 2 +-<br>
> 6 files changed, 525 insertions(+), 3 deletions(-)<br>
> create mode 100644 src/libcamera/pipeline/fake/fake.cpp<br>
> create mode 100644 src/libcamera/pipeline/fake/meson.build<br>
><br>
> diff --git a/meson_options.txt b/meson_options.txt<br>
> index f1d67808..f08dfc5f 100644<br>
> --- a/meson_options.txt<br>
> +++ b/meson_options.txt<br>
> @@ -37,7 +37,7 @@ option('lc-compliance',<br>
><br>
> option('pipelines',<br>
> type : 'array',<br>
> - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],<br>
> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc', 'fake'],<br>
> description : 'Select which pipeline handlers to include')<br>
><br>
> option('qcam',<br>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp<br>
> index 64bd8dde..730ceafc 100644<br>
> --- a/src/android/camera_capabilities.cpp<br>
> +++ b/src/android/camera_capabilities.cpp<br>
> @@ -1077,6 +1077,7 @@ int CameraCapabilities::initializeStaticMetadata()<br>
> {<br>
> const Span<const Rectangle> &rects =<br>
> properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});<br>
> + // TODO: initialize at least one Rectangle as default.<br>
<br>
Unrelated, please drop<br>
<br></blockquote><div> </div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> std::vector<int32_t> data{<br>
> static_cast<int32_t>(rects[0].x),<br>
> static_cast<int32_t>(rects[0].y),<br>
> diff --git a/src/libcamera/pipeline/fake/fake.cpp b/src/libcamera/pipeline/fake/fake.cpp<br>
> new file mode 100644<br>
> index 00000000..518de6aa<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/fake/fake.cpp<br>
> @@ -0,0 +1,518 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2019, Google Inc.<br>
> + *<br>
> + * ipu3.cpp - Pipeline handler for Intel Fake<br>
> + */<br>
<br>
Please update year and remove mentions of Intel ?<br>
<br></blockquote><div><br></div><div>Done.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +#include <algorithm><br>
> +#include <iomanip><br>
> +#include <memory><br>
> +#include <queue><br>
> +#include <vector><br>
<br>
Seems like you're also using std::set<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +#include <libcamera/base/log.h><br>
> +#include <libcamera/base/utils.h><br>
> +<br>
> +#include <libcamera/camera.h><br>
<br>
all internal headers "libcamera/internal/something.h" should include<br>
<libcamera/something.h>, so if you include internal/camera.h you<br>
probably can remove this<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +#include <libcamera/control_ids.h><br>
> +#include <libcamera/formats.h><br>
> +#include <libcamera/property_ids.h><br>
> +#include <libcamera/request.h><br>
> +#include <libcamera/stream.h><br>
> +<br>
> +#include "libcamera/internal/camera.h"<br>
> +#include "libcamera/internal/camera_lens.h"<br>
> +#include "libcamera/internal/camera_sensor.h"<br>
> +#include "libcamera/internal/delayed_controls.h"<br>
<br>
The three above are not used it seems<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +#include "libcamera/internal/device_enumerator.h"<br>
> +#include "libcamera/internal/framebuffer.h"<br>
> +#include "libcamera/internal/media_device.h"<br>
> +#include "libcamera/internal/pipeline_handler.h"<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +LOG_DEFINE_CATEGORY(Fake)<br>
> +<br>
> +uint64_t CurrentTimestamp()<br>
> +{<br>
> + struct timespec ts;<br>
> + if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {<br>
> + LOG(Fake, Error) << "Get clock time fails";<br>
> + return 0;<br>
> + }<br>
> +<br>
> + return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;<br>
> +}<br>
> +<br>
> +static const ControlInfoMap::Map FakeControls = {<br>
<br>
Missing #include <libcamera/controls.h> ?<br>
<br></blockquote><div> </div><div>Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + { &controls::draft::PipelineDepth, ControlInfo(2, 3) },<br>
> +};<br>
> +<br>
> +class FakeCameraData : public Camera::Private<br>
> +{<br>
> +public:<br>
> + struct Resolution {<br>
> + Size size;<br>
> + std::vector<int> frame_rates;<br>
> + std::vector<std::string> formats;<br>
<br>
I would rather use libcamera::formats to verify the Android HAL does<br>
the translation right.<br>
<br></blockquote><div>I believe you mean libcamera::PixelFormat. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + };<br>
> +<br>
> + FakeCameraData(PipelineHandler *pipe)<br>
> + : Camera::Private(pipe), supportsFlips_(false)<br>
> + {<br>
> + }<br>
> +<br>
> + std::vector<Resolution> supported_resolutions_;<br>
<br>
supportedResolutions_<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + Stream outStream_;<br>
> + Stream vfStream_;<br>
> + Stream rawStream_;<br>
> +<br>
> + // TODO: Check if we should support.<br>
> + bool supportsFlips_;<br>
> + Transform rotationTransform_;<br>
<br>
For sake of simplicity you can remoev flip/rotation support from the<br>
first skeleton<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + // TODO: remove<br>
<br>
Please :)<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + /* Requests for which no buffer has been queued to the CIO2 device yet. */<br>
> + std::queue<Request *> pendingRequests_;<br>
> + /* Requests queued to the CIO2 device but not yet processed by the ImgU. */<br>
> + std::queue<Request *> processingRequests_;<br>
> +<br>
> + // ControlInfoMap ipaControls_;<br>
<br>
This as well<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + bool started_ = false;<br>
> +};<br>
> +<br>
> +class FakeCameraConfiguration : public CameraConfiguration<br>
> +{<br>
> +public:<br>
> + static constexpr unsigned int kBufferCount = 4; // 4~6<br>
> + static constexpr unsigned int kMaxStreams = 3;<br>
> +<br>
> + FakeCameraConfiguration(FakeCameraData *data);<br>
> +<br>
> + Status validate() override;<br>
> +<br>
> +private:<br>
> + /*<br>
> + * The FakeCameraData instance is guaranteed to be valid as long as the<br>
> + * corresponding Camera instance is valid. In order to borrow a<br>
> + * reference to the camera data, store a new reference to the camera.<br>
> + */<br>
<br>
This comes from the IPU3 pipeline handler, and I wonder if it still<br>
applies there or it should be removed.<br>
<br></blockquote><div><br></div><div>I believe it's still needed in |validate()| to set streams, right?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + const FakeCameraData *data_;<br>
> +};<br>
> +<br>
> +class PipelineHandlerFake : public PipelineHandler<br>
> +{<br>
> +public:<br>
> + static constexpr unsigned int V4L2_CID_Fake_PIPE_MODE = 0x009819c1;<br>
<br>
Unused, please drop<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + static constexpr Size kViewfinderSize{ 1280, 720 };<br>
> +<br>
> + enum FakePipeModes {<br>
> + FakePipeModeVideo = 0,<br>
> + FakePipeModeStillCapture = 1,<br>
> + };<br>
<br>
ditto<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + PipelineHandlerFake(CameraManager *manager);<br>
> +<br>
> + CameraConfiguration *generateConfiguration(Camera *camera,<br>
> + const StreamRoles &roles) override;<br>
<br>
Align to open (<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + int configure(Camera *camera, CameraConfiguration *config) override;<br>
> +<br>
> + int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;<br>
> +<br>
> + int start(Camera *camera, const ControlList *controls) override;<br>
> + void stopDevice(Camera *camera) override;<br>
> +<br>
> + int queueRequestDevice(Camera *camera, Request *request) override;<br>
> +<br>
> + bool match(DeviceEnumerator *enumerator) override;<br>
> +<br>
> +private:<br>
> + FakeCameraData *cameraData(Camera *camera)<br>
> + {<br>
> + return static_cast<FakeCameraData *>(camera->_d());<br>
> + }<br>
> +<br>
> + int registerCameras();<br>
> +<br>
> + static bool registered_;<br>
> +};<br>
> +<br>
> +bool PipelineHandlerFake::registered_ = false;<br>
<br>
What purpose does this serve ? Do you get multiple calls to match()<br>
so that you need to keep a flag ?<br>
<br></blockquote><div>Yes, in `src/libcamera/camera_manager.cpp`, it intentionally calls match() multiple times</div><div>until it returns false (which means no devices matched). Therefore, this is the hack as </div><div>the fake media devices haven't been created yet.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +FakeCameraConfiguration::FakeCameraConfiguration(FakeCameraData *data)<br>
> + : CameraConfiguration()<br>
> +{<br>
> + data_ = data;<br>
> +}<br>
> +<br>
> +CameraConfiguration::Status FakeCameraConfiguration::validate()<br>
> +{<br>
> + Status status = Valid;<br>
> +<br>
> + if (config_.empty())<br>
> + return Invalid;<br>
> +<br>
> + // Transform combined = transform * data_->rotationTransform_;<br>
> +<br>
> + // /*<br>
> + // * We combine the platform and user transform, but must "adjust away"<br>
> + // * any combined result that includes a transposition, as we can't do<br>
> + // * those. In this case, flipping only the transpose bit is helpful to<br>
> + // * applications - they either get the transform they requested, or have<br>
> + // * to do a simple transpose themselves (they don't have to worry about<br>
> + // * the other possible cases).<br>
> + // */<br>
> + // if (!!(combined & Transform::Transpose)) {<br>
> + // /*<br>
> + // * Flipping the transpose bit in "transform" flips it in the<br>
> + // * combined result too (as it's the last thing that happens),<br>
> + // * which is of course clearing it.<br>
> + // */<br>
> + // transform ^= Transform::Transpose;<br>
> + // combined &= ~Transform::Transpose;<br>
> + // status = Adjusted;<br>
> + // }<br>
> +<br>
> + // /*<br>
> + // * We also check if the sensor doesn't do h/vflips at all, in which<br>
> + // * case we clear them, and the application will have to do everything.<br>
> + // */<br>
> + // if (!data_->supportsFlips_ && !!combined) {<br>
> + // /*<br>
> + // * If the sensor can do no transforms, then combined must be<br>
> + // * changed to the identity. The only user transform that gives<br>
> + // * rise to this is the inverse of the rotation. (Recall that<br>
> + // * combined = transform * rotationTransform.)<br>
> + // */<br>
> + // transform = -data_->rotationTransform_;<br>
> + // combined = Transform::Identity;<br>
> + // status = Adjusted;<br>
> + // }<br>
> +<br>
> + /*<br>
> + * Store the final combined transform that configure() will need to<br>
> + * apply to the sensor to save us working it out again.<br>
> + */<br>
> + // combinedTransform_ = combined;<br>
<br>
Please drop commented out code :0<br>
<br>
> +<br>
> + /* Cap the number of entries to the available streams. */<br>
> + if (config_.size() > kMaxStreams) {<br>
> + config_.resize(kMaxStreams);<br>
> + status = Adjusted;<br>
> + }<br>
> +<br>
> + /*<br>
> + * Validate the requested stream configuration and select the sensor<br>
> + * format by collecting the maximum RAW stream width and height and<br>
> + * picking the closest larger match.<br>
> + *<br>
> + * If no RAW stream is requested use the one of the largest YUV stream,<br>
> + * plus margin pixels for the IF and BDS rectangle to downscale.<br>
> + *<br>
> + * \todo Clarify the IF and BDS margins requirements.<br>
<br>
Please drop IPU3 related stuff<br>
<br></blockquote><div>Will do. I'll update the number of supported streams as well. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + */<br>
> + unsigned int rawCount = 0;<br>
> + unsigned int yuvCount = 0;<br>
> + Size rawRequirement;<br>
> + Size maxYuvSize;<br>
> + Size rawSize;<br>
> +<br>
> + for (const StreamConfiguration &cfg : config_) {<br>
> + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);<br>
> +<br>
> + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {<br>
> + rawCount++;<br>
> + rawSize = std::max(rawSize, cfg.size);<br>
> + } else {<br>
> + yuvCount++;<br>
> + maxYuvSize = std::max(maxYuvSize, cfg.size);<br>
> + rawRequirement.expandTo(cfg.size);<br>
> + }<br>
> + }<br>
> +<br>
> + // TODO: Base on number of cameras?<br>
<br>
Well, should this pipeline register more than one camera ?<br>
And anyway, the number of streams is per-camera so the number of<br>
cameras should be not relevant here<br>
<br></blockquote><div> </div><div>Yes, and yes. The configuration file should specify the number of supported streams.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + if (rawCount > 1 || yuvCount > 2) {<br>
> + LOG(Fake, Debug) << "Camera configuration not supported";<br>
> + return Invalid;<br>
> + }<br>
> +<br>
> + /*<br>
> + * Generate raw configuration from CIO2.<br>
> + *<br>
> + * The output YUV streams will be limited in size to the maximum frame<br>
> + * size requested for the RAW stream, if present.<br>
> + *<br>
> + * If no raw stream is requested, generate a size from the largest YUV<br>
> + * stream, aligned to the ImgU constraints and bound<br>
> + * by the sensor's maximum resolution. See<br>
> + * <a href="https://bugs.libcamera.org/show_bug.cgi?id=32" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=32</a><br>
> + */<br>
> + // TODO<br>
> + if (rawSize.isNull())<br>
> + rawSize = rawRequirement;<br>
<br>
All of these serves on IPU3 to generate a size suitable for the CIO2<br>
and the sensor. You can remove it.<br>
<br></blockquote><div> </div><div>Right, thanks! </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + /*<br>
> + * Adjust the configurations if needed and assign streams while<br>
> + * iterating them.<br>
> + */<br>
> + bool mainOutputAvailable = true;<br>
> + for (unsigned int i = 0; i < config_.size(); ++i) {<br>
> + const PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);<br>
> + const StreamConfiguration originalCfg = config_[i];<br>
> + StreamConfiguration *cfg = &config_[i];<br>
> +<br>
> + LOG(Fake, Debug) << "Validating stream: " << config_[i].toString();<br>
> +<br>
> + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {<br>
> + /* Initialize the RAW stream with the CIO2 configuration. */<br>
> + cfg->size = rawSize;<br>
> + // TODO: check<br>
> + cfg->pixelFormat = formats::SBGGR10_IPU3;<br>
> + cfg->bufferCount = FakeCameraConfiguration::kBufferCount;<br>
> + cfg->stride = info.stride(cfg->size.width, 0, 64);<br>
> + cfg->frameSize = info.frameSize(cfg->size, 64);<br>
> + cfg->setStream(const_cast<Stream *>(&data_->rawStream_));<br>
> +<br>
> + LOG(Fake, Debug) << "Assigned " << cfg->toString()<br>
> + << " to the raw stream";<br>
> + } else {<br>
> + /* Assign and configure the main and viewfinder outputs. */<br>
> +<br>
> + cfg->pixelFormat = formats::NV12;<br>
> + cfg->bufferCount = kBufferCount;<br>
> + cfg->stride = info.stride(cfg->size.width, 0, 1);<br>
> + cfg->frameSize = info.frameSize(cfg->size, 1);<br>
> +<br>
> + /*<br>
> + * Use the main output stream in case only one stream is<br>
> + * requested or if the current configuration is the one<br>
> + * with the maximum YUV output size.<br>
> + */<br>
> + if (mainOutputAvailable &&<br>
> + (originalCfg.size == maxYuvSize || yuvCount == 1)) {<br>
> + cfg->setStream(const_cast<Stream *>(&data_->outStream_));<br>
> + mainOutputAvailable = false;<br>
> +<br>
> + LOG(Fake, Debug) << "Assigned " << cfg->toString()<br>
> + << " to the main output";<br>
> + } else {<br>
> + cfg->setStream(const_cast<Stream *>(&data_->vfStream_));<br>
> +<br>
> + LOG(Fake, Debug) << "Assigned " << cfg->toString()<br>
> + << " to the viewfinder output";<br>
> + }<br>
> + }<br>
<br>
Again I'm not sure how much of the IPU3 should this pipeline mimick,<br>
or it should establish requirements and constraints (ie max 2 YUV<br>
streams of max size width x height, and 1 RAW stream in format<br>
V4L2_PIX_FMT_..)<br>
<br></blockquote><div><br></div><div>Exactly. Will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + if (cfg->pixelFormat != originalCfg.pixelFormat ||<br>
> + cfg->size != originalCfg.size) {<br>
> + LOG(Fake, Debug)<br>
> + << "Stream " << i << " configuration adjusted to "<br>
> + << cfg->toString();<br>
> + status = Adjusted;<br>
> + }<br>
> + }<br>
> +<br>
> + return status;<br>
> +}<br>
> +<br>
> +PipelineHandlerFake::PipelineHandlerFake(CameraManager *manager)<br>
> + : PipelineHandler(manager)<br>
> +{<br>
> + // TODO: read the fake hal spec file.<br>
<br>
If there's a design document with requirements, can you share it ?<br>
<br></blockquote><div><br></div><div>Will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +}<br>
> +<br>
> +CameraConfiguration *PipelineHandlerFake::generateConfiguration(Camera *camera,<br>
> + const StreamRoles &roles)<br>
> +{<br>
> + FakeCameraData *data = cameraData(camera);<br>
> + FakeCameraConfiguration *config = new FakeCameraConfiguration(data);<br>
> +<br>
> + if (roles.empty())<br>
> + return config;<br>
> +<br>
> + Size minSize, sensorResolution;<br>
> + for (const auto& resolution : data->supported_resolutions_) {<br>
> + if (minSize.isNull() || minSize > resolution.size)<br>
> + minSize = resolution.size;<br>
> +<br>
> + sensorResolution = std::max(sensorResolution, resolution.size);<br>
> + }<br>
> +<br>
<br>
Those are hard-coded values, I think you can reuse them here.<br>
Unless the idea is to make this information come from a configuration<br>
file or something similar.<br>
<br></blockquote><div>Yes, supportedResolutions_ should eventually come from a configuration</div><div>file, provided by the tester.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + for (const StreamRole role : roles) {<br>
> + std::map<PixelFormat, std::vector<SizeRange>> streamFormats;<br>
> + unsigned int bufferCount;<br>
> + PixelFormat pixelFormat;<br>
> + Size size;<br>
> +<br>
> + switch (role) {<br>
> + case StreamRole::StillCapture:<br>
> + size = sensorResolution;<br>
> + pixelFormat = formats::NV12;<br>
> + bufferCount = FakeCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, size } };<br>
> +<br>
> + break;<br>
> +<br>
> + case StreamRole::Raw: {<br>
> + // TODO: check<br>
> + pixelFormat = formats::SBGGR10_IPU3;<br>
> + size = sensorResolution;<br>
> + bufferCount = FakeCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, size } };<br>
> +<br>
> + break;<br>
> + }<br>
> +<br>
> + case StreamRole::Viewfinder:<br>
> + case StreamRole::VideoRecording: {<br>
> + /*<br>
> + * Default viewfinder and videorecording to 1280x720,<br>
> + * capped to the maximum sensor resolution and aligned<br>
> + * to the ImgU output constraints.<br>
> + */<br>
> + size = sensorResolution;<br>
> + pixelFormat = formats::NV12;<br>
> + bufferCount = FakeCameraConfiguration::kBufferCount;<br>
> + streamFormats[pixelFormat] = { { minSize, size } };<br>
> +<br>
> + break;<br>
> + }<br>
> +<br>
> + default:<br>
> + LOG(Fake, Error)<br>
> + << "Requested stream role not supported: " << role;<br>
> + delete config;<br>
> + return nullptr;<br>
> + }<br>
> +<br>
> + StreamFormats formats(streamFormats);<br>
> + StreamConfiguration cfg(formats);<br>
> + cfg.size = size;<br>
> + cfg.pixelFormat = pixelFormat;<br>
> + cfg.bufferCount = bufferCount;<br>
> + config->addConfiguration(cfg);<br>
> + }<br>
> +<br>
> + if (config->validate() == CameraConfiguration::Invalid)<br>
> + return {};<br>
> +<br>
> + return config;<br>
> +}<br>
> +<br>
> +int PipelineHandlerFake::configure(Camera *camera, CameraConfiguration *c)<br>
> +{<br>
> + if (camera || c)<br>
> + return 0;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int PipelineHandlerFake::exportFrameBuffers(Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + // Assume it's never called.<br>
> + LOG(Fake, Fatal) << "exportFrameBuffers should never be called";<br>
> + if (camera || stream || buffers)<br>
> + return -EINVAL;<br>
> + return -EINVAL;<br>
> +}<br>
> +<br>
> +int PipelineHandlerFake::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>
> +{<br>
> + FakeCameraData *data = cameraData(camera);<br>
> + data->started_ = true;<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +void PipelineHandlerFake::stopDevice(Camera *camera)<br>
> +{<br>
> + FakeCameraData *data = cameraData(camera);<br>
> +<br>
> + data->started_ = false;<br>
> +}<br>
> +<br>
> +int PipelineHandlerFake::queueRequestDevice(Camera *camera, Request *request)<br>
> +{<br>
> + if (!camera)<br>
> + return -EINVAL;<br>
<br>
Can this happen ?<br>
<br></blockquote><div>Actually it's a hack that CrOS compiler will fail if there's any unused argument...</div><div>I'll try to clean it up later.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + for (auto it : request->buffers())<br>
> + completeBuffer(request, it.second);<br>
> +<br>
> + // TODO: request.metadata()<br>
> + request->metadata().set(controls::SensorTimestamp, CurrentTimestamp());<br>
> + completeRequest(request);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +bool PipelineHandlerFake::match(DeviceEnumerator *enumerator)<br>
> +{<br>
> + // TODO: exhaust all devices in |enumerator|.<br>
> + if (!enumerator)<br>
> + LOG(Fake, Info) << "Invalid enumerator";<br>
<br>
Can this happen ?<br>
<br></blockquote><div>ditto </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + if (registered_)<br>
> + return false;<br>
> +<br>
> + registered_ = true;<br>
> + return registerCameras() == 0;<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Initialise ImgU and CIO2 devices associated with cameras<br>
> + *<br>
> + * Initialise the two ImgU instances and create cameras with an associated<br>
> + * CIO2 device instance.<br>
> + *<br>
> + * \return 0 on success or a negative error code for error or if no camera<br>
> + * has been created<br>
> + * \retval -ENODEV no camera has been created<br>
> + */<br>
> +int PipelineHandlerFake::registerCameras()<br>
> +{<br>
> + std::unique_ptr<FakeCameraData> data =<br>
> + std::make_unique<FakeCameraData>(this);<br>
> + std::set<Stream *> streams = {<br>
> + &data->outStream_,<br>
> + &data->vfStream_,<br>
> + &data->rawStream_,<br>
> + };<br>
> +<br>
> + // TODO: Read from config or from IPC.<br>
> + // TODO: Check with Han-lin: Can this function be called more than once?<br>
> + data->supported_resolutions_.resize(2);<br>
> + data->supported_resolutions_[0].size = Size(1920, 1080);<br>
> + data->supported_resolutions_[0].frame_rates.push_back(30);<br>
> + data->supported_resolutions_[0].formats.push_back("YCbCr_420_888");<br>
> + data->supported_resolutions_[0].formats.push_back("BLOB");<br>
> + data->supported_resolutions_[1].size = Size(1280, 720);<br>
> + data->supported_resolutions_[1].frame_rates.push_back(30);<br>
> + data->supported_resolutions_[1].frame_rates.push_back(60);<br>
> + data->supported_resolutions_[1].formats.push_back("YCbCr_420_888");<br>
> + data->supported_resolutions_[1].formats.push_back("BLOB");<br>
> +<br>
> + // TODO: Assign different locations for different cameras based on config.<br>
> + data->properties_.set(properties::Location, properties::CameraLocationFront);<br>
> + data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });<br>
> +<br>
> + // TODO: Set FrameDurationLimits based on config.<br>
> + ControlInfoMap::Map controls{};<br>
> + int64_t min_frame_duration = 30, max_frame_duration = 60;<br>
> + controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);<br>
> + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);<br>
<br>
FakeControls is ignored<br>
<br></blockquote><div>Right. Use it to initialize |controls| here.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + std::shared_ptr<Camera> camera =<br>
> + Camera::create(std::move(data), "\\_SB_.PCI0.I2C4.CAM1" /* cameraId */, streams);<br>
> +<br>
> + registerCamera(std::move(camera));<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerFake)<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/fake/meson.build b/src/libcamera/pipeline/fake/meson.build<br>
> new file mode 100644<br>
> index 00000000..13980b4a<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/fake/meson.build<br>
> @@ -0,0 +1,3 @@<br>
> +# SPDX-License-Identifier: CC0-1.0<br>
> +<br>
> +libcamera_sources += files([ 'fake.cpp' ])<br>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp<br>
> index e5cb751c..4261154d 100644<br>
> --- a/src/libcamera/pipeline_handler.cpp<br>
> +++ b/src/libcamera/pipeline_handler.cpp<br>
> @@ -536,7 +536,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)<br>
> cameras_.push_back(camera);<br>
><br>
> if (mediaDevices_.empty())<br>
> - LOG(Pipeline, Fatal)<br>
> + LOG(Pipeline, Error)<br>
<br>
I don't think we want that, you should probably open code the<br>
<br>
manager_->addCamera(std::move(camera), devnums);<br>
<br>
call, with an empty list of devnums, which should be fine for this use<br>
case.<br>
<br>
The PipelineHandler::cameras_ vector will remain unpopulated, but<br>
since it is used only in case of a media device disconnection (afaict)<br>
it should be fine in your case<br>
<br></blockquote><div><br></div><div>Ah you're right. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> << "Registering camera with no media devices!";<br>
><br>
> /*<br>
> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp<br>
> index d12e2413..06c87730 100644<br>
> --- a/test/camera/camera_reconfigure.cpp<br>
> +++ b/test/camera/camera_reconfigure.cpp<br>
> @@ -98,7 +98,7 @@ private:<br>
> return TestFail;<br>
> }<br>
><br>
> - requests_.push_back(move(request));<br>
> + requests_.push_back(std::move(request));<br>
<br>
Unrelated and possibly not necessary, as the test has<br>
using namespace std;<br>
<br></blockquote><div> </div><div>Yeah... it's also a hack that it's required in the CrOS compiler... It doesn't</div><div>accept |move|...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Looking forward to seeing this skeleton getting populated, it will help<br>
testing the Android layer!<br>
<br>
Thanks<br>
j<br>
<br>
> }<br>
><br>
> camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);<br>
> --<br>
> 2.38.0.rc1.362.ged0d419d3c-goog<br>
><br>
</blockquote></div></div>