| Message ID | 20210514075808.282479-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, 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 > 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. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@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? 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? Do they cause issues with doxygen if it picks them up? > > # 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. To match the header it should be in src/libcamera/ipa/core_ipa_interface.cpp 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. > @@ -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. > + */ > + > +/** > + * \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. > + '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. 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 ! */ > /* 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. */ >
Hi Kieran and Umang, On Fri, May 14, 2021 at 10:24:12AM +0100, Kieran Bingham wrote: > Hi Umang, > > 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 > > 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. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Signed-off-by: Umang Jain <umang.jain@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? > > 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. > > > > > @@ -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. > > + */ > > + > > +/** > > + * \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. Paul > > > + '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. > > > 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 ! */ > > /* 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. */ > >
Hello, On Fri, May 14, 2021 at 06:43:20PM +0900, paul.elder@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@ideasonboard.com> > > > Signed-off-by: Umang Jain <umang.jain@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. */
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/ \ @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 # 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 @@ -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. + */ + +/** + * \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([ + 'core_ipa_interface.cpp', +]) + 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 ! */ /* 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. */