[libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support for CameraSensorInfo
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 28 09:31:00 CEST 2020
Hi Laurent,
On Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:
> > Add support for camera sensor information in the libcamera IPA protocol.
> >
> > Define a new 'struct ipa_sensor_info' structure in the IPA context and
> > use it to perform translation between the C and the C++ API.
> >
> > Update the IPAInterface::configure() operation to accept a new
> > CameraSensorInfo parameter and port all users of that function to
> > the new interface.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/ipa/ipa_interface.h | 26 +++++++-
> > src/ipa/libipa/ipa_interface_wrapper.cpp | 19 +++++-
> > src/ipa/libipa/ipa_interface_wrapper.h | 1 +
> > src/ipa/rkisp1/rkisp1.cpp | 13 +++-
> > src/ipa/vimc/vimc.cpp | 4 +-
> > src/libcamera/include/ipa_context_wrapper.h | 4 +-
> > src/libcamera/ipa_context_wrapper.cpp | 23 ++++++-
> > src/libcamera/ipa_interface.cpp | 73 +++++++++++++++++++++
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++-
> > src/libcamera/proxy/ipa_proxy_linux.cpp | 4 +-
> > src/libcamera/proxy/ipa_proxy_thread.cpp | 9 ++-
> > test/ipa/ipa_wrappers_test.cpp | 22 ++++++-
> > 12 files changed, 195 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index e65844bc7b34..0a6d849d8f89 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -18,6 +18,27 @@ struct ipa_context {
> > const struct ipa_context_ops *ops;
> > };
> >
> > +struct ipa_sensor_info {
> > + const char *model;
> > + uint8_t bits_per_pixel;
> > + struct {
> > + uint32_t width;
> > + uint32_t height;
> > + } active_area;
> > + struct {
> > + int32_t left;
> > + int32_t top;
> > + uint32_t width;
> > + uint32_t height;
> > + } analog_crop;
> > + struct {
> > + uint32_t width;
> > + uint32_t height;
> > + } output_size;
> > + uint32_t pixel_rate;
>
> Is 32 bits enough ? (Same question for CameraSensorInfo)
>
The V4L2 control is 64 bits, let's make this 64 too
> > + uint32_t line_length;
> > +};
> > +
> > struct ipa_stream {
> > unsigned int id;
> > unsigned int pixel_format;
> > @@ -70,6 +91,7 @@ struct ipa_context_ops {
> > const struct ipa_callback_ops *callbacks,
> > void *cb_ctx);
> > void (*configure)(struct ipa_context *ctx,
> > + const struct ipa_sensor_info *sensor_info,
> > const struct ipa_stream *streams,
> > unsigned int num_streams,
> > const struct ipa_control_info_map *maps,
> > @@ -116,6 +138,7 @@ struct IPAOperationData {
> > std::vector<ControlList> controls;
> > };
> >
> > +struct CameraSensorInfo;
>
> Could you add a blank line ?
>
> I've just realized that we'll need to move CameraSensorInfo out of
> camera_sensor.h if we want to make it accessible to IPAs without
> accessing internal APIs :-( That can be fixed on top, but could you add
> a \todo somewhere ?
>
I had the same thought, but I then realized we already include
v4l2_controls, and I considerd this to be fine.
We might want to remove both includes.
> > class IPAInterface
> > {
> > public:
> > @@ -125,7 +148,8 @@ public:
> > virtual int start() = 0;
> > virtual void stop() = 0;
> >
> > - virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + virtual void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> >
> > virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index f50f93a0185a..e65822c62315 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -15,6 +15,7 @@
> > #include <ipa/ipa_interface.h>
> >
> > #include "byte_stream_buffer.h"
> > +#include "camera_sensor.h"
> >
> > /**
> > * \file ipa_interface_wrapper.h
> > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> > }
> >
> > void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > + const struct ipa_sensor_info *sensor_info,
> > const struct ipa_stream *streams,
> > unsigned int num_streams,
> > const struct ipa_control_info_map *maps,
> > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> >
> > ctx->serializer_.reset();
> >
> > + /* Translate the IPA sensor info. */
> > + CameraSensorInfo sensorInfo{};
> > + sensorInfo.model = sensor_info->model;
> > + sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
> > + sensorInfo.activeAreaSize = { sensor_info->active_area.width,
> > + sensor_info->active_area.height };
> > + sensorInfo.analogCrop = { sensor_info->analog_crop.left,
> > + sensor_info->analog_crop.top,
> > + sensor_info->analog_crop.width,
> > + sensor_info->analog_crop.height };
> > + sensorInfo.outputSize = { sensor_info->output_size.width,
> > + sensor_info->output_size.height };
> > + sensorInfo.pixelRate = sensor_info->pixel_rate;
> > + sensorInfo.lineLength = sensor_info->line_length;
> > +
> > /* Translate the IPA stream configurations map. */
> > std::map<unsigned int, IPAStream> ipaStreams;
> >
> > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > entityControls.emplace(id, infoMaps[id]);
> > }
> >
> > - ctx->ipa_->configure(ipaStreams, entityControls);
> > + ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > }
> >
> > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> > index e4bc6dd4535d..febd6e66d0c4 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -30,6 +30,7 @@ private:
> > const struct ipa_callback_ops *callbacks,
> > void *cb_ctx);
> > static void configure(struct ipa_context *ctx,
> > + const struct ipa_sensor_info *sensor_info,
> > const struct ipa_stream *streams,
> > unsigned int num_streams,
> > const struct ipa_control_info_map *maps,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index acbbe58c7a2e..b6da672c1384 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -22,6 +22,7 @@
> > #include <libcamera/request.h>
> > #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
>
> No need to include this here, there's already at least a forward
> declaration available as the base class uses CameraSensorInfo in its
> API.
>
That's a bit fragile, as we rely on the forward declaration of
ipa_interface.h
Also, I tend to go for #include in cpp files as, compared to headers,
we probably are going to actually use the type, and this seems to be
easier to maintain, for a very little price.
> > #include "ipa_module.h"
> > #include "log.h"
> > #include "utils.h"
> >
> > @@ -36,7 +37,8 @@ public:
> > int start() override { return 0; }
> > void stop() override {}
> >
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &info,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > @@ -66,7 +68,14 @@ private:
> > uint32_t maxGain_;
> > };
> >
> > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +/**
> > + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> > + * if the connected sensor does not provide enough information to properly
> > + * assemble one. Make sure the reported sensor information are relevant
> > + * before accessing them.
> > + */
> > +void IPARkISP1::configure(const CameraSensorInfo &info,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > {
> > if (entityControls.empty())
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index d2267e11737f..b8aadd8588cb 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -19,6 +19,7 @@
> >
> > #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
>
> Same here.
>
Same commment, but for now, I'll drop.
> > #include "log.h"
> >
> > namespace libcamera {
> > @@ -36,7 +37,8 @@ public:
> > int start() override;
> > void stop() override;
> >
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> > index 0a48bfe732c8..cfb435ee1b91 100644
> > --- a/src/libcamera/include/ipa_context_wrapper.h
> > +++ b/src/libcamera/include/ipa_context_wrapper.h
> > @@ -13,6 +13,7 @@
> >
> > namespace libcamera {
> >
> > +struct CameraSensorInfo;
>
> For the same reason the forward declaration isn't needed here.
>
This seems very fragile and also a bit obscure. To me it's like not
including an header as it is already included in an header we include.
> > class IPAContextWrapper final : public IPAInterface
> > {
> > public:
> > @@ -22,7 +23,8 @@ public:
> > int init() override;
> > int start() override;
> > void stop() override;
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> >
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index ab6ce396b88a..d591c67f8791 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -12,6 +12,7 @@
> > #include <libcamera/controls.h>
> >
> > #include "byte_stream_buffer.h"
> > +#include "camera_sensor.h"
> > #include "utils.h"
> >
> > /**
> > @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()
> > ctx_->ops->stop(ctx_);
> > }
> >
> > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > {
> > if (intf_)
> > - return intf_->configure(streamConfig, entityControls);
> > + return intf_->configure(sensorInfo, streamConfig, entityControls);
> >
> > if (!ctx_)
> > return;
> >
> > serializer_.reset();
> >
> > + /* Translate the camera sensor info. */
> > + struct ipa_sensor_info sensor_info = {};
> > + sensor_info.model = sensorInfo.model.c_str();
> > + sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;
> > + sensor_info.active_area.width = sensorInfo.activeAreaSize.width;
> > + sensor_info.active_area.height = sensorInfo.activeAreaSize.height;
> > + sensor_info.analog_crop.left = sensorInfo.analogCrop.x;
> > + sensor_info.analog_crop.top = sensorInfo.analogCrop.y;
> > + sensor_info.analog_crop.width = sensorInfo.analogCrop.width;
> > + sensor_info.analog_crop.height = sensorInfo.analogCrop.height;
> > + sensor_info.output_size.width = sensorInfo.outputSize.width;
> > + sensor_info.output_size.height = sensorInfo.outputSize.height;
> > + sensor_info.pixel_rate = sensorInfo.pixelRate;
> > + sensor_info.line_length = sensorInfo.lineLength;
> > +
> > /* Translate the IPA stream configurations map. */
> > struct ipa_stream c_streams[streamConfig.size()];
> >
> > @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
> > ++i;
> > }
> >
> > - ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
> > + ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> > c_info_maps, entityControls.size());
> > }
> >
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index 890d4340e4f3..13df090f3374 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -92,6 +92,74 @@
> > * \brief The IPA context operations
> > */
> >
> > +/**
> > + * \struct ipa_sensor_info
> > + * \brief Camera sensor information for the IPA context operations
> > + * \sa libcamera::CameraSensorInfo
> > + *
> > + * \var ipa_sensor_info::model
> > + * \brief The camera sensor model name
> > + * \todo Remove this field as soon as no IPA depends on it anymore
> > + *
> > + * \var ipa_sensor_info::bits_per_pixel
> > + * \brief The camera sensor image format bit depth
> > + * \sa libcamera::CameraSensorInfo::bitsPerPixel
> > + *
> > + * \var ipa_sensor_info::active_area.width
> > + * \brief The camera sensor pixel array active area width
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::active_area.height
> > + * \brief The camera sensor pixel array active area height
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::active_area
> > + * \brief The camera sensor pixel array active size
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::analog_crop.left
> > + * \brief The left coordinate of the analog crop rectangle, relative to the
> > + * pixel array active area
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.top
> > + * \brief The top coordinate of the analog crop rectangle, relative to the pixel
> > + * array active area
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.width
> > + * \brief The horizontal size of the analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.height
> > + * \brief The vertical size of the analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop
> > + * \brief The analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::output_size.width
> > + * \brief The horizontal size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::output_size.height
> > + * \brief The vertical size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::output_size
> > + * \brief The size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::pixel_rate
> > + * \brief The number of pixel produced in a second
> > + * \sa libcamera::CameraSensorInfo::pixelClock
>
> s/pixelClock/pixelRate/
>
> No warning from Doxygen ? Could you check the compiled doc to see if
> everything is fine ?
No warning here, weird. I'll fix this.
>
> > + *
> > + * \var ipa_sensor_info::line_length
> > + * \brief The full line length, including blanking, in pixel units
> > + * \sa libcamera::CameraSensorInfo::lineLength
> > + */
> > +
> > /**
> > * \struct ipa_stream
> > * \brief Stream information for the IPA context operations
> > @@ -447,6 +515,7 @@ namespace libcamera {
> > /**
> > * \fn IPAInterface::configure()
> > * \brief Configure the IPA stream and sensor settings
> > + * \param[in] sensorInfo Camera sensor information
> > * \param[in] streamConfig Configuration of all active streams
> > * \param[in] entityControls Controls provided by the pipeline entities
> > *
> > @@ -454,6 +523,10 @@ namespace libcamera {
> > * the camera's streams and the sensor settings. The meaning of the numerical
> > * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
> > * protocol.
> > + *
> > + * The \a sensorInfo conveys information about the camera sensor settings that
> > + * the pipeline handler has selected for the configuration. The IPA may use
> > + * that information to tune its algorithms.
> > */
> >
> > /**
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f42264361785..4058b9014fb9 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >
> > activeCamera_ = camera;
> >
> > - /* Inform IPA of stream configuration and sensor controls. */
> > + /*
> > + * Inform IPA of stream configuration and sensor controls.
> > + */
>
> Any reason to switch to a multiline comment ? I don't mind much, just
> curious.
>
I probably added a line of comment and then removed, then missed the
change during the rebase. I'll restore it.
> > + CameraSensorInfo sensorInfo = {};
> > + ret = data->sensor_->sensorInfo(&sensorInfo);
> > + if (ret) {
> > + /* \todo Turn this in an hard failure. */
Ah, that's the comment :)
> > + LOG(RkISP1, Info) << "Camera sensor information not available";
>
> Let's make it a Warning already then.
>
Ack
> > + sensorInfo = {};
> > + }
> > +
> > std::map<unsigned int, IPAStream> streamConfig;
> > streamConfig[0] = {
> > .pixelFormat = data->stream_.configuration().pixelFormat,
> > @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > std::map<unsigned int, const ControlInfoMap &> entityControls;
> > entityControls.emplace(0, data->sensor_->controls());
> >
> > - data->ipa_->configure(streamConfig, entityControls);
> > + data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> >
> > return ret;
> > }
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 2aa80b946704..54b1abc4727b 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -10,6 +10,7 @@
> > #include <ipa/ipa_interface.h>
> > #include <ipa/ipa_module_info.h>
> >
> > +#include "camera_sensor.h"
>
> No need for this either.
>
> > #include "ipa_module.h"
> > #include "ipa_proxy.h"
> > #include "ipc_unixsocket.h"
> > @@ -29,7 +30,8 @@ public:
> > int init() override { return 0; }
> > int start() override { return 0; }
> > void stop() override {}
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index 368ccca1cf60..b06734371b39 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -10,6 +10,7 @@
> > #include <ipa/ipa_interface.h>
> > #include <ipa/ipa_module_info.h>
> >
> > +#include "camera_sensor.h"
>
> Same here, this file doesn't access the contents of CameraSensorInfo,
> you don't need to include the header file.
>
> > #include "ipa_context_wrapper.h"
> > #include "ipa_module.h"
> > #include "ipa_proxy.h"
> > @@ -29,7 +30,8 @@ public:
> > int start() override;
> > void stop() override;
> >
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > @@ -126,10 +128,11 @@ void IPAProxyThread::stop()
> > thread_.wait();
> > }
> >
> > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > {
> > - ipa_->configure(streamConfig, entityControls);
> > + ipa_->configure(sensorInfo, streamConfig, entityControls);
> > }
> >
> > void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index fae1d56b67c7..a6bf0836ac4e 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -15,6 +15,7 @@
> > #include <libcamera/controls.h>
> > #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
> > #include "device_enumerator.h"
> > #include "ipa_context_wrapper.h"
> > #include "media_device.h"
> > @@ -60,9 +61,17 @@ public:
> > report(Op_stop, TestPass);
> > }
> >
> > - void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > + void configure(const CameraSensorInfo &sensorInfo,
> > + const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > {
> > + /* Verify sensorInfo. */
> > + if (sensorInfo.outputSize.width != 2560 ||
> > + sensorInfo.outputSize.height != 1940) {
> > + cerr << "configure(): Invalid sensor info sizes: ("
>
> Leftover '('. I would remove the ':' too. And s/sizes/size/
>
ah ups
> > + << sensorInfo.outputSize.toString();
> > + }
> > +
> > /* Verify streamConfig. */
> > if (streamConfig.size() != 2) {
> > cerr << "configure(): Invalid number of streams "
> > @@ -287,13 +296,22 @@ protected:
> > int ret;
> >
> > /* Test configure(). */
> > + CameraSensorInfo sensorInfo{
> > + .model = "sensor",
> > + .bitsPerPixel = 8,
> > + .activeAreaSize = { 2576, 1956 },
> > + .analogCrop = { 8, 8, 2560, 1940 },
> > + .outputSize = { 2560, 1940 },
> > + .pixelRate = 96000000,
> > + .lineLength = 2918,
> > + };
> > std::map<unsigned int, IPAStream> config{
> > { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
> > { 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },
> > };
> > std::map<unsigned int, const ControlInfoMap &> controlInfo;
> > controlInfo.emplace(42, subdev_->controls());
> > - ret = INVOKE(configure, config, controlInfo);
> > + ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > if (ret == TestFail)
> > return TestFail;
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list