| Message ID | 20210511115611.57642-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 11/05/2021 12:56, 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. Also, update header paths to include > CameraSensorInfo definition from IPA interfaces instead of > "libcamera/internal/camera_sensor.h". > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/internal/camera_sensor.h | 17 +--- > include/libcamera/ipa/core.mojom | 112 ++++++++++++++++++++- > include/libcamera/ipa/ipa_interface.h | 2 - > src/ipa/raspberrypi/raspberrypi.cpp | 1 - > src/libcamera/camera_sensor.cpp | 111 -------------------- > 5 files changed, 112 insertions(+), 131 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index 2a5c51a1..0905ebfa 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> Hrm, I feel like that deserves it's own 'group' (i.e. a new line above it) because it's the IPA as a sub component almost ... but equally, it's in the right place here by alphabetical sorting so ... I suspect it's fine. > > #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: > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > index 6caaa63e..2a09b233 100644 > --- a/include/libcamera/ipa/core.mojom > +++ b/include/libcamera/ipa/core.mojom > @@ -78,7 +78,117 @@ module libcamera; > uint32 height; > }; > > -[skipHeader] struct CameraSensorInfo { > +/** > + * \struct CameraSensorInfo > + * \brief Report the image sensor characteristics I haven't checked yet, but do we need to do anything to make sure doxygen parses this? Hopefully doxygen will pick it up from the generated header file - but can you check that it builds please? > + * > + * 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 > + */ > +struct CameraSensorInfo { > string model; > > uint32 bitsPerPixel; > 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" > - Certainly glad to see this go now ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > namespace libcamera { > > /* > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 52d91db2..87774500 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> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index eb84d9eb..170de827 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 >
Hello, On Tue, May 11, 2021 at 05:31:38PM +0100, Kieran Bingham wrote: > On 11/05/2021 12:56, 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. Also, update header paths to include > > CameraSensorInfo definition from IPA interfaces instead of > > "libcamera/internal/camera_sensor.h". > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > include/libcamera/internal/camera_sensor.h | 17 +--- > > include/libcamera/ipa/core.mojom | 112 ++++++++++++++++++++- > > include/libcamera/ipa/ipa_interface.h | 2 - > > src/ipa/raspberrypi/raspberrypi.cpp | 1 - > > src/libcamera/camera_sensor.cpp | 111 -------------------- > > 5 files changed, 112 insertions(+), 131 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index 2a5c51a1..0905ebfa 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> > > Hrm, I feel like that deserves it's own 'group' (i.e. a new line above > it) because it's the IPA as a sub component almost ... but equally, it's > in the right place here by alphabetical sorting so ... I suspect it's fine. > > > > > #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: > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > > index 6caaa63e..2a09b233 100644 > > --- a/include/libcamera/ipa/core.mojom > > +++ b/include/libcamera/ipa/core.mojom > > @@ -78,7 +78,117 @@ module libcamera; > > uint32 height; > > }; > > > > -[skipHeader] struct CameraSensorInfo { > > +/** > > + * \struct CameraSensorInfo I wonder if we should rename this to IPACameraSensorInfo to match the other structures. > > + * \brief Report the image sensor characteristics > > I haven't checked yet, but do we need to do anything to make sure > doxygen parses this? > > Hopefully doxygen will pick it up from the generated header file - but > can you check that it builds please? As far as I can tell, mojo doesn't process comments, and doxygen can't parse mojom files. We may need to move this to an otherwise empty .cpp file. > > + * > > + * 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 > > + */ > > +struct CameraSensorInfo { > > string model; > > > > uint32 bitsPerPixel; > > 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" > > - > > Certainly glad to see this go now ;-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > namespace libcamera { > > > > /* > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 52d91db2..87774500 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> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index eb84d9eb..170de827 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
Hi Laurent, On 5/12/21 12:16 AM, Laurent Pinchart wrote: > Hello, > > On Tue, May 11, 2021 at 05:31:38PM +0100, Kieran Bingham wrote: >> On 11/05/2021 12:56, 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. Also, update header paths to include >>> CameraSensorInfo definition from IPA interfaces instead of >>> "libcamera/internal/camera_sensor.h". >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> include/libcamera/internal/camera_sensor.h | 17 +--- >>> include/libcamera/ipa/core.mojom | 112 ++++++++++++++++++++- >>> include/libcamera/ipa/ipa_interface.h | 2 - >>> src/ipa/raspberrypi/raspberrypi.cpp | 1 - >>> src/libcamera/camera_sensor.cpp | 111 -------------------- >>> 5 files changed, 112 insertions(+), 131 deletions(-) >>> >>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h >>> index 2a5c51a1..0905ebfa 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> >> Hrm, I feel like that deserves it's own 'group' (i.e. a new line above >> it) because it's the IPA as a sub component almost ... but equally, it's >> in the right place here by alphabetical sorting so ... I suspect it's fine. >> >>> >>> #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: >>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom >>> index 6caaa63e..2a09b233 100644 >>> --- a/include/libcamera/ipa/core.mojom >>> +++ b/include/libcamera/ipa/core.mojom >>> @@ -78,7 +78,117 @@ module libcamera; >>> uint32 height; >>> }; >>> >>> -[skipHeader] struct CameraSensorInfo { >>> +/** >>> + * \struct CameraSensorInfo > I wonder if we should rename this to IPACameraSensorInfo to match the > other structures. yes, I agree and have applied the changes locally. > >>> + * \brief Report the image sensor characteristics >> I haven't checked yet, but do we need to do anything to make sure >> doxygen parses this? >> >> Hopefully doxygen will pick it up from the generated header file - but >> can you check that it builds please? > As far as I can tell, mojo doesn't process comments, and doxygen can't > parse mojom files. We may need to move this to an otherwise empty .cpp > file. Pinged epaul for that on IRC and see what path we should take. I will follow up there itself >>> + * >>> + * 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 >>> + */ >>> +struct CameraSensorInfo { >>> string model; >>> >>> uint32 bitsPerPixel; >>> 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" >>> - >> Certainly glad to see this go now ;-) >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> namespace libcamera { >>> >>> /* >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >>> index 52d91db2..87774500 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> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >>> index eb84d9eb..170de827 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
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index 2a5c51a1..0905ebfa 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: diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom index 6caaa63e..2a09b233 100644 --- a/include/libcamera/ipa/core.mojom +++ b/include/libcamera/ipa/core.mojom @@ -78,7 +78,117 @@ module libcamera; uint32 height; }; -[skipHeader] struct CameraSensorInfo { +/** + * \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 + */ +struct CameraSensorInfo { string model; uint32 bitsPerPixel; 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/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 52d91db2..87774500 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> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index eb84d9eb..170de827 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
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. Also, update header paths to include CameraSensorInfo definition from IPA interfaces instead of "libcamera/internal/camera_sensor.h". Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/internal/camera_sensor.h | 17 +--- include/libcamera/ipa/core.mojom | 112 ++++++++++++++++++++- include/libcamera/ipa/ipa_interface.h | 2 - src/ipa/raspberrypi/raspberrypi.cpp | 1 - src/libcamera/camera_sensor.cpp | 111 -------------------- 5 files changed, 112 insertions(+), 131 deletions(-)