[libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support for CameraSensorInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 28 14:15:12 CEST 2020
Hi Jacopo,
On Tue, Apr 28, 2020 at 09:31:00AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:
> > 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.
Ouch :-S Indeed. Let's do so on top.
> > > 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
There's a guaranteed forward declaration in ipa_interface.h. I tend to
rely on that when I don't need the full definition, as in here. Up to
you.
> 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.
The usage of CameraSensorInfo here is to implement IPAInterface. We
include ipa_interface.h to do so. IPAInterface uses CameraSensorInfo, so
ipa_interface.h is guaranteed to have at least a forward declaration. We
don't rely on an internal detail of ipa_interface.h, but on something it
guarantees.
> > > 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