[libcamera-devel] [PATCH v1 1/6] ipa: mojom: Move CameraSensorInfo struct exclusively to IPA IPC
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 15 18:25:45 CEST 2021
Hello,
On Fri, May 14, 2021 at 06:43:20PM +0900, paul.elder at ideasonboard.com wrote:
> On Fri, May 14, 2021 at 10:24:12AM +0100, Kieran Bingham wrote:
> > On 14/05/2021 08:58, Umang Jain wrote:
> > > CameraSensorInfo structure is designed to pass in camera sensor related
> > > information from pipeline-handler to IPA. Since the pipeline-hander
s/hander/handler/
> > > and IPA are connected via mojom IPC IPA interface, the interface
> > > itself provides a more suitable placement of CameraSensorInfo,
> > > instead of camera_sensor.h (which is a libcamera internal header
> > > ultimately, at this point).
> > >
> > > As CameraSensorInfo is already defined in core.mojom, it is just
> > > a matter of removing [skipHeader] tag to allow code-generation
> > > of CameraSensorInfo. To be consistent with naming scheme, the
> > > existing CameraSensorInfo is renamed to IPACameraSensorInfo.
> > >
> > > Since doxygen cannot directly generate documentation from the .mojom
> > > files, the core.mojom documentation is also moved to a new .cpp file.
> > >
> > > Finally, update header paths to include IPACameraSensorInfo definition
> > > from IPA interfaces instead of "libcamera/internal/camera_sensor.h".
> >
> > Hrm ... we should probably have handled the rename and move separately.
> > It adds a lot of churn to this patch.
>
> I agree, I think the part that deals with IPACameraSensorInfo and the
> part that enables the mojom documentation should be separated.
>
> Of course, the latter will cause all sorts of missing documentation
> compiler warnings, so that might need to be added before (or at the same
> time) the mojom documentation is enabled.
Documenting IPABuffer, IPASettings and IPAStream definitely belongs to a
separate patch. I would also split the rename of CameraSensorInfo to
IPACameraSensorInfo to a separate patch.
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > Documentation/Doxyfile.in | 4 +-
> > > Documentation/meson.build | 1 +
> > > include/libcamera/internal/camera_sensor.h | 19 +-
> > > include/libcamera/ipa/core.mojom | 2 +-
> > > include/libcamera/ipa/core_ipa_interface.cpp | 190 ++++++++++++++++++
> > > include/libcamera/ipa/ipa_interface.h | 2 -
> > > include/libcamera/ipa/meson.build | 4 +
> > > include/libcamera/ipa/raspberrypi.mojom | 2 +-
> > > include/libcamera/ipa/rkisp1.mojom | 2 +-
> > > src/ipa/ipu3/ipu3_agc.cpp | 2 +-
> > > src/ipa/raspberrypi/raspberrypi.cpp | 9 +-
> > > src/ipa/rkisp1/rkisp1.cpp | 6 +-
> > > src/libcamera/camera_sensor.cpp | 117 +----------
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 4 +-
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > > 16 files changed, 218 insertions(+), 152 deletions(-)
> > > create mode 100644 include/libcamera/ipa/core_ipa_interface.cpp
> > >
> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > > index af006724..213adb9b 100644
> > > --- a/Documentation/Doxyfile.in
> > > +++ b/Documentation/Doxyfile.in
> > > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \
> > > @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > > @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> > > @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > > - @TOP_BUILDDIR@/include/libcamera/ipa/ \
> >
> > Do we need to remove this?
> >
> > I think Doxygen would still parse the generated headers and include the
> > output in it's Code browsers and such to explore types?
>
> The generated headers are put in this ignored directory though :D
>
> > Not entirely sure though.
> >
> > > @TOP_BUILDDIR@/src/libcamera/proxy/
> > >
> > > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
> > > @@ -861,7 +860,8 @@ EXCLUDE_SYMLINKS = NO
> > > # Note that the wildcards are matched against the file with absolute path, so to
> > > # exclude all test directories for example use the pattern */test/*
> > >
> > > -EXCLUDE_PATTERNS =
> > > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> > > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h
> >
> > Do these need to be explicitly excluded?
>
> Disabling the ignored directory above means these guys will get picked
> up, so these need to be explicitly excluded.
>
> > Do they cause issues with doxygen if it picks them up?
>
> Lots of unwanted missing documentation compiler warnings :D
>
> > > # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names
> > > # (namespaces, classes, functions, etc.) that should be excluded from the
> > > diff --git a/Documentation/meson.build b/Documentation/meson.build
> > > index c8521574..86f1134b 100644
> > > --- a/Documentation/meson.build
> > > +++ b/Documentation/meson.build
> > > @@ -23,6 +23,7 @@ if doxygen.found() and dot.found()
> > > input : [
> > > doxyfile,
> > > libcamera_internal_headers,
> > > + libcamera_ipa_docs,
> > > libcamera_ipa_headers,
> > > libcamera_public_headers,
> > > libcamera_sources,
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 2a5c51a1..cf6c1c1e 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -14,6 +14,7 @@
> > > #include <libcamera/class.h>
> > > #include <libcamera/controls.h>
> > > #include <libcamera/geometry.h>
> > > +#include <libcamera/ipa/core_ipa_interface.h>
> > >
> > > #include "libcamera/internal/formats.h"
> > > #include "libcamera/internal/log.h"
> > > @@ -24,22 +25,6 @@ namespace libcamera {
> > > class BayerFormat;
> > > class MediaEntity;
> > >
> > > -struct CameraSensorInfo {
> > > - std::string model;
> > > -
> > > - uint32_t bitsPerPixel;
> > > -
> > > - Size activeAreaSize;
> > > - Rectangle analogCrop;
> > > - Size outputSize;
> > > -
> > > - uint64_t pixelRate;
> > > - uint32_t lineLength;
> > > -
> > > - uint32_t minFrameLength;
> > > - uint32_t maxFrameLength;
> > > -};
> > > -
> > > class CameraSensor : protected Loggable
> > > {
> > > public:
> > > @@ -66,7 +51,7 @@ public:
> > > V4L2Subdevice *device() { return subdev_.get(); }
> > >
> > > const ControlList &properties() const { return properties_; }
> > > - int sensorInfo(CameraSensorInfo *info) const;
> > > + int sensorInfo(IPACameraSensorInfo *info) const;
> > >
> > > void updateControlInfo();
> > >
> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > > index 6caaa63e..06ca87c7 100644
> > > --- a/include/libcamera/ipa/core.mojom
> > > +++ b/include/libcamera/ipa/core.mojom
> > > @@ -78,7 +78,7 @@ module libcamera;
> > > uint32 height;
> > > };
> > >
> > > -[skipHeader] struct CameraSensorInfo {
> > > +struct IPACameraSensorInfo {
> > > string model;
> > >
> > > uint32 bitsPerPixel;
> > > diff --git a/include/libcamera/ipa/core_ipa_interface.cpp b/include/libcamera/ipa/core_ipa_interface.cpp
> > > new file mode 100644
> > > index 00000000..19f769ce
> > > --- /dev/null
> > > +++ b/include/libcamera/ipa/core_ipa_interface.cpp
> >
> > As noted below - /include/ should not contain any .cpp files.
>
> I know, but it was just documentation, and it's with regard to the
> generated IPA stuff, so I put it here...
>
> > To match the header it should be in
> >
> > src/libcamera/ipa/core_ipa_interface.cpp
>
> We could make one. Would this be a better place to put IPA interface
> documentation?
Yes, I think that's a good place.
> > But we don't have a src/libcamera/ipa directory.
> >
> > Looking at how many ipa files we have under src/libcamera/ipa_ I'd be
> > tempted to create an ipa directory and move these under there to match
> > though.
There's a difference between the ipa_* files in src/libcamera/ and the
per-IPA .cpp files. It's similar in nature to the difference between
src/libcamera/pipeline_handler.cpp and the pipeline handlers in
src/libcamera/pipeline/. We're reaching bikeshedding directory, but I'm
not sure I'd move all ipa_*.cpp files to a src/libcamera/ipa/ directory.
> > > @@ -0,0 +1,190 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \struct IPACameraSensorInfo
> > > + * \brief Report the image sensor characteristics
> > > + *
> > > + * The structure reports image sensor characteristics used by IPA modules to
> > > + * tune their algorithms based on the image sensor model currently in use and
> > > + * its configuration.
> > > + *
> > > + * The reported information describes the sensor's intrinsics characteristics,
> > > + * such as its pixel array size and the sensor model name, as well as
> > > + * information relative to the currently configured mode, such as the produced
> > > + * image size and the bit depth of the requested image format.
> > > + *
> > > + * Instances of this structure are meant to be assembled by the CameraSensor
> > > + * class by inspecting the sensor static properties as well as information
> > > + * relative to the current configuration.
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::model
> > > + * \brief The image sensor model name
> > > + *
> > > + * The sensor model name is a free-formed string that uniquely identifies the
> > > + * sensor model.
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::bitsPerPixel
> > > + * \brief The number of bits per pixel of the image format produced by the
> > > + * image sensor
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::activeAreaSize
> > > + * \brief The size of the pixel array active area of the sensor
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::analogCrop
> > > + * \brief The portion of the pixel array active area which is read-out and
> > > + * processed
> > > + *
> > > + * The analog crop rectangle top-left corner is defined as the displacement
> > > + * from the top-left corner of the pixel array active area. The rectangle
> > > + * horizontal and vertical sizes define the portion of the pixel array which
> > > + * is read-out and provided to the sensor's internal processing pipeline, before
> > > + * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> > > + * take place.
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::outputSize
> > > + * \brief The size of the images produced by the camera sensor
> > > + *
> > > + * The output image size defines the horizontal and vertical sizes of the images
> > > + * produced by the image sensor. The output image size is defined as the end
> > > + * result of the sensor's internal image processing pipeline stages, applied on
> > > + * the pixel array portion defined by the analog crop rectangle. Each image
> > > + * processing stage that performs pixel sub-sampling techniques, such as pixel
> > > + * binning or skipping, or perform any additional digital scaling concur in the
> > > + * definition of the output image size.
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::pixelRate
> > > + * \brief The number of pixels produced in a second
> > > + *
> > > + * To obtain the read-out time in seconds of a full line:
> > > + *
> > > + * \verbatim
> > > + lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)
> > > + \endverbatim
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::lineLength
> > > + * \brief Total line length in pixels
> > > + *
> > > + * The total line length in pixel clock periods, including blanking.
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::minFrameLength
> > > + * \brief The minimum allowable frame length in units of lines
> > > + *
> > > + * The sensor frame length comprises of active output lines and blanking lines
> > > + * in a frame. The minimum frame length value dictates the minimum allowable
> > > + * frame duration of the sensor mode.
> > > + *
> > > + * To obtain the minimum frame duration:
> > > + *
> > > + * \verbatim
> > > + frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> > > + \endverbatim
> > > + */
> > > +
> > > +/**
> > > + * \var IPACameraSensorInfo::maxFrameLength
> > > + * \brief The maximum allowable frame length in units of lines
> > > + *
> > > + * The sensor frame length comprises of active output lines and blanking lines
> > > + * in a frame. The maximum frame length value dictates the maximum allowable
> > > + * frame duration of the sensor mode.
> > > + *
> > > + * To obtain the maximum frame duration:
> > > + *
> > > + * \verbatim
> > > + frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> > > + \endverbatim
> > > + */
> > > +
> > > +/**
> > > + * \struct IPABuffer
> > > + * \brief Buffer information for the IPA interface
> > > + *
> > > + * The IPABuffer structure associates buffer memory with a unique ID. It is
> > > + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
> > > + * buffers will be identified by their ID in the IPA interface.
> > > + */
You should remove the corresponding documentation from core.mojom.
> > > +
> > > +/**
> > > + * \var IPABuffer::id
> > > + * \brief The buffer unique ID
> > > + *
> > > + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
> > > + * are chosen by the pipeline handler to fulfil the following constraints:
> > > + *
> > > + * - IDs shall be positive integers different than zero
> > > + * - IDs shall be unique among all mapped buffers
> > > + *
> > > + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
> > > + * freed and may be reused for new buffer mappings.
> > > + */
> > > +
> > > +/**
> > > + * \var IPABuffer::planes
> > > + * \brief The buffer planes description
> > > + *
> > > + * Stores the dmabuf handle and length for each plane of the buffer.
> > > + */
> > > +
> > > +/**
> > > + * \struct IPASettings
> > > + * \brief IPA interface initialization settings
> > > + *
> > > + * The IPASettings structure stores data passed to the IPAInterface::init()
> > > + * function. The data contains settings that don't depend on a particular camera
> > > + * or pipeline configuration and are valid for the whole life time of the IPA
> > > + * interface.
> > > + */
> > > +
> > > +/**
> > > + * \var IPASettings::configurationFile
> > > + * \brief The name of the IPA configuration file
> > > + *
> > > + * This field may be an empty string if the IPA doesn't require a configuration
> > > + * file.
> > > + */
> > > +
> > > +/**
> > > + * \var IPASettings::sensorModel
> > > + * \brief The sensor model name
> > > + *
> > > + * Provides the sensor model name to the IPA.
> > > + */
> > > +
> > > +/**
> > > + * \struct IPAStream
> > > + * \brief Stream configuration for the IPA interface
> > > + *
> > > + * The IPAStream structure stores stream configuration parameters needed by the
> > > + * IPAInterface::configure() method. It mirrors the StreamConfiguration class
> > > + * that is not suitable for this purpose due to not being serializable.
> > > + */
> > > +
> > > +/**
> > > + * \var IPAStream::pixelFormat
> > > + * \brief The stream pixel format
> > > + */
> > > +
> > > +/**
> > > + * \var IPAStream::size
> > > + * \brief The stream size in pixels
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > > index dfe1b40a..4aefaa71 100644
> > > --- a/include/libcamera/ipa/ipa_interface.h
> > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > @@ -18,8 +18,6 @@
> > > #include <libcamera/geometry.h>
> > > #include <libcamera/signal.h>
> > >
> > > -#include "libcamera/internal/camera_sensor.h"
> > > -
> > > namespace libcamera {
> > >
> > > /*
> > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > > index 40c4e737..69bc855e 100644
> > > --- a/include/libcamera/ipa/meson.build
> > > +++ b/include/libcamera/ipa/meson.build
> > > @@ -6,6 +6,10 @@ libcamera_ipa_headers = files([
> > > 'ipa_module_info.h',
> > > ])
> > >
> > > +libcamera_ipa_docs = files([
> >
> > I don't think this should be called _docs and then point to .cpp files.
> >
> > The fact that it only stores documentation is just how the file is now.
> > Perhaps theoretically it could have code in here, and therefore should
> > be added to the libcamera sources accordingly. Although we're unlikely
> > to add code in here. But I think all .cpp files should be part of the
> > sources.
>
> I don't really see how putting code in these files would work...
>
> I guess the other .cpp files that are only docs are also just included
> as sources.
I agree that we'll likely never have code there.
> > > + 'core_ipa_interface.cpp',
> > > +])
> > > +
> >
> > Oh - wait - we're in include/libcamera/ipa ... we don't add .cpp files
> > in there at all.
> >
> >
> > With this file moved under src/libcamera/
> > (optionally src/libcamera/ipa/ if all ipa files are moved first)
> >
> > this gets added to the libcamera sources as normal anyway.
I'm in two minds here. On one hand I don't like the _docs suffix much,
as it's an array of source files, but on the other hand, compiling .cpp
files that we know are empty is just a waste of CPU cycles. Could we
maybe name the meson variable libcamera_ipa_interface_sources, but
refrain from adding it to libcamera_sources ?
> > Keep the filename as core_ipa_interface.cpp even if it's in
> > src/libcamera/ as I think we'll later have to move those to
> > src/libcamera/ipa anyway, and it's more important to match the header name.
> >
> > > install_headers(libcamera_ipa_headers,
> > > subdir: libcamera_include_dir / 'ipa')
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > index 42321bee..0a21f453 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -63,7 +63,7 @@ interface IPARPiInterface {
> > > * The \a ipaConfig and \a controls parameters carry data passed by the
> > > * pipeline handler to the IPA and back.
> > > */
> > > - configure(libcamera.CameraSensorInfo sensorInfo,
> > > + configure(libcamera.IPACameraSensorInfo sensorInfo,
> > > map<uint32, libcamera.IPAStream> streamConfig,
> > > map<uint32, libcamera.ControlInfoMap> entityControls,
> > > IPAConfig ipaConfig)
> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > index cca871a0..66a4607c 100644
> > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > @@ -29,7 +29,7 @@ interface IPARkISP1Interface {
> > > start() => (int32 ret);
> > > stop();
> > >
> > > - configure(libcamera.CameraSensorInfo sensorInfo,
> > > + configure(libcamera.IPACameraSensorInfo sensorInfo,
> > > map<uint32, libcamera.IPAStream> streamConfig,
> > > map<uint32, libcamera.ControlInfoMap> entityControls)
> > > => (int32 ret);
> > > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> > > index ca54d89a..3adcadf4 100644
> > > --- a/src/ipa/ipu3/ipu3_agc.cpp
> > > +++ b/src/ipa/ipu3/ipu3_agc.cpp
> > > @@ -39,7 +39,7 @@ static constexpr uint32_t kMaxGain = kMaxISO / 100;
> > > static constexpr uint32_t kMinExposure = 1;
> > > static constexpr uint32_t kMaxExposure = 1976;
> > >
> > > -/* \todo those should be get from CameraSensorInfo ! */
> > > +/* \todo those should be get from IPACameraSensorInfo ! */
While at it, s/get/got/
> > > /* line duration in microseconds */
> > > static constexpr double kLineDuration = 16.8;
> > > static constexpr double kMaxExposureTime = kMaxExposure * kLineDuration;
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 52d91db2..e5bb8159 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -25,7 +25,6 @@
> > > #include <libcamera/span.h>
> > >
> > > #include "libcamera/internal/buffer.h"
> > > -#include "libcamera/internal/camera_sensor.h"
> > > #include "libcamera/internal/log.h"
> > >
> > > #include <linux/bcm2835-isp.h>
> > > @@ -90,7 +89,7 @@ public:
> > > void start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;
> > > void stop() override {}
> > >
> > > - int configure(const CameraSensorInfo &sensorInfo,
> > > + int configure(const IPACameraSensorInfo &sensorInfo,
> > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > const ipa::RPi::IPAConfig &data,
> > > @@ -102,7 +101,7 @@ public:
> > > void signalIspPrepare(const ipa::RPi::ISPConfig &data) override;
> > >
> > > private:
> > > - void setMode(const CameraSensorInfo &sensorInfo);
> > > + void setMode(const IPACameraSensorInfo &sensorInfo);
> > > bool validateSensorControls();
> > > bool validateIspControls();
> > > void queueRequest(const ControlList &controls);
> > > @@ -279,7 +278,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> > > lastRunTimestamp_ = 0;
> > > }
> > >
> > > -void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > > +void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> > > {
> > > mode_.bitdepth = sensorInfo.bitsPerPixel;
> > > mode_.width = sensorInfo.outputSize.width;
> > > @@ -324,7 +323,7 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > > mode_.max_frame_length = sensorInfo.maxFrameLength;
> > > }
> > >
> > > -int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > > const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > const ipa::RPi::IPAConfig &ipaConfig,
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 6d45673c..b47ea324 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -37,7 +37,7 @@ public:
> > > int start() override;
> > > void stop() override {}
> > >
> > > - int configure(const CameraSensorInfo &info,
> > > + int configure(const IPACameraSensorInfo &info,
> > > const std::map<uint32_t, IPAStream> &streamConfig,
> > > const std::map<uint32_t, ControlInfoMap> &entityControls) override;
> > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > @@ -90,12 +90,12 @@ int IPARkISP1::start()
> > > }
> > >
> > > /**
> > > - * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> > > + * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > * if the connected sensor does not provide enough information to properly
> > > * assemble one. Make sure the reported sensor information are relevant
> > > * before accessing them.
> > > */
> > > -int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> > > +int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> > > const std::map<uint32_t, ControlInfoMap> &entityControls)
> > > {
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index eb84d9eb..0fb8a258 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -33,117 +33,6 @@ namespace libcamera {
> > >
> > > LOG_DEFINE_CATEGORY(CameraSensor)
> > >
> > > -/**
> > > - * \struct CameraSensorInfo
> > > - * \brief Report the image sensor characteristics
> > > - *
> > > - * The structure reports image sensor characteristics used by IPA modules to
> > > - * tune their algorithms based on the image sensor model currently in use and
> > > - * its configuration.
> > > - *
> > > - * The reported information describes the sensor's intrinsics characteristics,
> > > - * such as its pixel array size and the sensor model name, as well as
> > > - * information relative to the currently configured mode, such as the produced
> > > - * image size and the bit depth of the requested image format.
> > > - *
> > > - * Instances of this structure are meant to be assembled by the CameraSensor
> > > - * class by inspecting the sensor static properties as well as information
> > > - * relative to the current configuration.
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::model
> > > - * \brief The image sensor model name
> > > - *
> > > - * The sensor model name is a free-formed string that uniquely identifies the
> > > - * sensor model.
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::bitsPerPixel
> > > - * \brief The number of bits per pixel of the image format produced by the
> > > - * image sensor
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::activeAreaSize
> > > - * \brief The size of the pixel array active area of the sensor
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::analogCrop
> > > - * \brief The portion of the pixel array active area which is read-out and
> > > - * processed
> > > - *
> > > - * The analog crop rectangle top-left corner is defined as the displacement
> > > - * from the top-left corner of the pixel array active area. The rectangle
> > > - * horizontal and vertical sizes define the portion of the pixel array which
> > > - * is read-out and provided to the sensor's internal processing pipeline, before
> > > - * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> > > - * take place.
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::outputSize
> > > - * \brief The size of the images produced by the camera sensor
> > > - *
> > > - * The output image size defines the horizontal and vertical sizes of the images
> > > - * produced by the image sensor. The output image size is defined as the end
> > > - * result of the sensor's internal image processing pipeline stages, applied on
> > > - * the pixel array portion defined by the analog crop rectangle. Each image
> > > - * processing stage that performs pixel sub-sampling techniques, such as pixel
> > > - * binning or skipping, or perform any additional digital scaling concur in the
> > > - * definition of the output image size.
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::pixelRate
> > > - * \brief The number of pixels produced in a second
> > > - *
> > > - * To obtain the read-out time in seconds of a full line:
> > > - *
> > > - * \verbatim
> > > - lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)
> > > - \endverbatim
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::lineLength
> > > - * \brief Total line length in pixels
> > > - *
> > > - * The total line length in pixel clock periods, including blanking.
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::minFrameLength
> > > - * \brief The minimum allowable frame length in units of lines
> > > - *
> > > - * The sensor frame length comprises of active output lines and blanking lines
> > > - * in a frame. The minimum frame length value dictates the minimum allowable
> > > - * frame duration of the sensor mode.
> > > - *
> > > - * To obtain the minimum frame duration:
> > > - *
> > > - * \verbatim
> > > - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> > > - \endverbatim
> > > - */
> > > -
> > > -/**
> > > - * \var CameraSensorInfo::maxFrameLength
> > > - * \brief The maximum allowable frame length in units of lines
> > > - *
> > > - * The sensor frame length comprises of active output lines and blanking lines
> > > - * in a frame. The maximum frame length value dictates the maximum allowable
> > > - * frame duration of the sensor mode.
> > > - *
> > > - * To obtain the maximum frame duration:
> > > - *
> > > - * \verbatim
> > > - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> > > - \endverbatim
> > > - */
> > > -
> > > /**
> > > * \class CameraSensor
> > > * \brief A camera sensor based on V4L2 subdevices
> > > @@ -316,7 +205,7 @@ int CameraSensor::validateSensorDriver()
> > > *
> > > * Failures in reading any of the targets are not deemed to be fatal,
> > > * but some properties and features, like constructing a
> > > - * CameraSensorInfo for the IPA module, won't be supported.
> > > + * IPACameraSensorInfo for the IPA module, won't be supported.
> > > *
> > > * \todo Make support for selection targets mandatory as soon as all
> > > * test platforms have been updated.
> > > @@ -785,7 +674,7 @@ int CameraSensor::setControls(ControlList *ctrls)
> > > *
> > > * \return 0 on success, a negative error code otherwise
> > > */
> > > -int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > > +int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const
> > > {
> > > if (!bayerFormat_)
> > > return -EINVAL;
> > > @@ -812,7 +701,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > > }
> > >
> > > /*
> > > - * CameraSensorInfo::analogCrop::x and CameraSensorInfo::analogCrop::y
> > > + * IPACameraSensorInfo::analogCrop::x and IPACameraSensorInfo::analogCrop::y
> > > * are defined relatively to the active pixel area, while V4L2's
> > > * TGT_CROP target is defined in respect to the full pixel array.
> > > *
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index ade8ffbd..98c6160f 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -544,7 +544,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > > if (ret)
> > > return ret;
> > >
> > > - CameraSensorInfo sensorInfo;
> > > + IPACameraSensorInfo sensorInfo;
> > > cio2->sensor()->sensorInfo(&sensorInfo);
> > > data->cropRegion_ = sensorInfo.analogCrop;
> > >
> > > @@ -883,7 +883,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > > if (ret)
> > > return ret;
> > >
> > > - CameraSensorInfo sensorInfo{};
> > > + IPACameraSensorInfo sensorInfo{};
> > > ret = sensor->sensorInfo(&sensorInfo);
> > > if (ret)
> > > return ret;
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 6fbdba04..f2a94dc0 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -211,7 +211,7 @@ public:
> > > BayerFormat::Order nativeBayerOrder_;
> > >
> > > /* For handling digital zoom. */
> > > - CameraSensorInfo sensorInfo_;
> > > + IPACameraSensorInfo sensorInfo_;
> > > Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> > > Rectangle scalerCrop_; /* crop in sensor native pixels */
> > > bool updateScalerCrop_;
> > > @@ -1275,7 +1275,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > ipaConfig.lsTableHandle = lsTable_;
> > > }
> > >
> > > - /* We store the CameraSensorInfo for digital zoom calculations. */
> > > + /* We store the IPACameraSensorInfo for digital zoom calculations. */
> > > int ret = sensor_->sensorInfo(&sensorInfo_);
> > > if (ret) {
> > > LOG(RPI, Error) << "Failed to retrieve camera sensor info";
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index adebe8b5..6699839c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -648,7 +648,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > > return ret;
> > >
> > > /* Inform IPA of stream configuration and sensor controls. */
> > > - CameraSensorInfo sensorInfo = {};
> > > + IPACameraSensorInfo sensorInfo = {};
> > > ret = data->sensor_->sensorInfo(&sensorInfo);
> > > if (ret) {
> > > /* \todo Turn this into a hard failure. */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list