[libcamera-devel] [PATCH v3 02/16] libcamera: ipu3: Centralize ImgU sizes definition
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 13 02:48:16 CEST 2021
Hi Jacopo,
Thank you for the patch.
On Mon, Oct 11, 2021 at 05:11:40PM +0200, Jacopo Mondi wrote:
> The definition of several constants that describe the ImgU
> characteristics are spread between two files: ipu3.cpp and imgu.cpp.
>
> As the ipu3.cpp uses definitions from the imgu.cpp file, in order to
> remove the usage of magic numbers, it is required to move the
> definitions to a common header file where they are accessible to the
> other .cpp modules.
>
> Move all the definitions of the ImgU sizes and alignments to the
> ImgUDevice class as static constexpr and update their users accordingly.
>
> Cosmetic changes, no functional changes intended.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> src/libcamera/pipeline/ipu3/imgu.cpp | 86 +++++++++++-----------------
> src/libcamera/pipeline/ipu3/imgu.h | 23 ++++++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 47 +++++++--------
> 3 files changed, 79 insertions(+), 77 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 3e1ef645ec93..f326886bf3da 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -34,22 +34,6 @@ namespace {
> * at revision: 243d13446e44 ("Fix some bug for some resolutions")
> */
>
> -static constexpr unsigned int FILTER_W = 4;
> -static constexpr unsigned int FILTER_H = 4;
> -
> -static constexpr unsigned int IF_ALIGN_W = 2;
> -static constexpr unsigned int IF_ALIGN_H = 4;
> -
> -static constexpr unsigned int BDS_ALIGN_W = 2;
> -static constexpr unsigned int BDS_ALIGN_H = 4;
> -
> -static constexpr unsigned int IF_CROP_MAX_W = 40;
> -static constexpr unsigned int IF_CROP_MAX_H = 540;
> -
> -static constexpr float BDS_SF_MAX = 2.5;
> -static constexpr float BDS_SF_MIN = 1.0;
> -static constexpr float BDS_SF_STEP = 0.03125;
> -
> /* BSD scaling factors: min=1, max=2.5, step=1/32 */
> const std::vector<float> bdsScalingFactors = {
> 1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,
> @@ -124,8 +108,8 @@ bool isSameRatio(const Size &in, const Size &out)
> void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> unsigned int bdsWidth, float bdsSF)
> {
> - unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
> - unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> + unsigned int minIFHeight = iif.height - ImgUDevice::kIFMaxCropHeight;
> + unsigned int minBDSHeight = gdc.height + ImgUDevice::kFilterHeight * 2;
> unsigned int ifHeight;
> float bdsHeight;
>
> @@ -135,7 +119,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> static_cast<float>(gdc.width);
> estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
>
> - ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> + ifHeight = utils::alignUp(estIFHeight, ImgUDevice::kAlignHeight);
> while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
> ifHeight / bdsSF >= minBDSHeight) {
>
> @@ -143,17 +127,17 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> if (std::fmod(height, 1.0) == 0) {
> unsigned int bdsIntHeight = static_cast<unsigned int>(height);
>
> - if (!(bdsIntHeight % BDS_ALIGN_H)) {
> + if (!(bdsIntHeight % ImgUDevice::kBDSAlignHeight)) {
> foundIfHeight = ifHeight;
> bdsHeight = height;
> break;
> }
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= ImgUDevice::kAlignHeight;
> }
>
> - ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> + ifHeight = utils::alignUp(estIFHeight, ImgUDevice::kAlignHeight);
> while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
> ifHeight / bdsSF >= minBDSHeight) {
>
> @@ -161,14 +145,14 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> if (std::fmod(height, 1.0) == 0) {
> unsigned int bdsIntHeight = static_cast<unsigned int>(height);
>
> - if (!(bdsIntHeight % BDS_ALIGN_H)) {
> + if (!(bdsIntHeight % ImgUDevice::kBDSAlignHeight)) {
> foundIfHeight = ifHeight;
> bdsHeight = height;
> break;
> }
> }
>
> - ifHeight += IF_ALIGN_H;
> + ifHeight += ImgUDevice::kAlignHeight;
> }
>
> if (foundIfHeight) {
> @@ -179,32 +163,32 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> return;
> }
> } else {
> - ifHeight = utils::alignUp(iif.height, IF_ALIGN_H);
> + ifHeight = utils::alignUp(iif.height, ImgUDevice::kAlignHeight);
> while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
>
> bdsHeight = ifHeight / bdsSF;
> if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
>
> - if (!(ifHeight % IF_ALIGN_H) &&
> - !(bdsIntHeight % BDS_ALIGN_H)) {
> + if (!(ifHeight % ImgUDevice::kAlignHeight) &&
> + !(bdsIntHeight % ImgUDevice::kBDSAlignHeight)) {
> pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
> { bdsWidth, bdsIntHeight }, gdc });
> }
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= ImgUDevice::kAlignHeight;
> }
> }
> }
>
> void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF)
> {
> - unsigned int minBDSWidth = gdc.width + FILTER_W * 2;
> - unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> + unsigned int minBDSWidth = gdc.width + ImgUDevice::kFilterWidth * 2;
> + unsigned int minBDSHeight = gdc.height + ImgUDevice::kFilterHeight * 2;
>
> float sf = bdsSF;
> - while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> + while (sf <= ImgUDevice::kBDSSfMax && sf >= ImgUDevice::kBDSSfMin) {
> float bdsWidth = static_cast<float>(iif.width) / sf;
> float bdsHeight = static_cast<float>(iif.height) / sf;
>
> @@ -212,16 +196,16 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa
> std::fmod(bdsHeight, 1.0) == 0) {
> unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> - if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&
> - !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)
> + if (!(bdsIntWidth % ImgUDevice::kBDSAlignWidth) && bdsWidth >= minBDSWidth &&
> + !(bdsIntHeight % ImgUDevice::kBDSAlignHeight) && bdsHeight >= minBDSHeight)
> calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> }
>
> - sf += BDS_SF_STEP;
> + sf += ImgUDevice::kBDSSfStep;
> }
>
> sf = bdsSF;
> - while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> + while (sf <= ImgUDevice::kBDSSfMax && sf >= ImgUDevice::kBDSSfMin) {
> float bdsWidth = static_cast<float>(iif.width) / sf;
> float bdsHeight = static_cast<float>(iif.height) / sf;
>
> @@ -229,12 +213,12 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa
> std::fmod(bdsHeight, 1.0) == 0) {
> unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> - if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&
> - !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)
> + if (!(bdsIntWidth % ImgUDevice::kBDSAlignWidth) && bdsWidth >= minBDSWidth &&
> + !(bdsIntHeight % ImgUDevice::kBDSAlignHeight) && bdsHeight >= minBDSHeight)
> calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> }
>
> - sf -= BDS_SF_STEP;
> + sf -= ImgUDevice::kBDSSfStep;
> }
> }
>
> @@ -412,7 +396,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> * \todo Filter out all resolutions < IF_CROP_MAX.
> * See https://bugs.libcamera.org/show_bug.cgi?id=32
> */
> - if (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {
> + if (in.width < ImgUDevice::kIFMaxCropWidth || in.height < ImgUDevice::kIFMaxCropHeight) {
> LOG(IPU3, Error) << "Input resolution " << in.toString()
> << " not supported";
> return {};
> @@ -424,25 +408,25 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
>
> /* Populate the configurations vector by scaling width and height. */
> - unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> - unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> - unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> - unsigned int minIfHeight = in.height - IF_CROP_MAX_H;
> + unsigned int ifWidth = utils::alignUp(in.width, ImgUDevice::kAlignWidth);
> + unsigned int ifHeight = utils::alignUp(in.height, ImgUDevice::kAlignHeight);
> + unsigned int minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> + unsigned int minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
> while (ifWidth >= minIfWidth) {
> while (ifHeight >= minIfHeight) {
> Size iif{ ifWidth, ifHeight };
> calculateBDS(pipe, iif, gdc, sf);
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= ImgUDevice::kAlignHeight;
> }
>
> - ifWidth -= IF_ALIGN_W;
> + ifWidth -= ImgUDevice::kAlignWidth;
> }
>
> /* Repeat search by scaling width first. */
> - ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> - ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> - minIfWidth = in.width - IF_CROP_MAX_W;
> - minIfHeight = in.height - IF_CROP_MAX_H;
> + ifWidth = utils::alignUp(in.width, ImgUDevice::kAlignWidth);
> + ifHeight = utils::alignUp(in.height, ImgUDevice::kAlignHeight);
> + minIfWidth = in.width - ImgUDevice::kIFMaxCropWidth;
> + minIfHeight = in.height - ImgUDevice::kIFMaxCropHeight;
> while (ifHeight >= minIfHeight) {
> /*
> * \todo This procedure is probably broken:
> @@ -451,10 +435,10 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> while (ifWidth >= minIfWidth) {
> Size iif{ ifWidth, ifHeight };
> calculateBDS(pipe, iif, gdc, sf);
> - ifWidth -= IF_ALIGN_W;
> + ifWidth -= ImgUDevice::kAlignWidth;
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= ImgUDevice::kAlignHeight;
> }
>
> if (pipeConfigs.size() == 0) {
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 9d4915116087..690a22e67d47 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -23,6 +23,29 @@ struct StreamConfiguration;
> class ImgUDevice
> {
> public:
> + static constexpr unsigned int kFilterWidth = 4;
> + static constexpr unsigned int kFilterHeight = 4;
> +
> + static constexpr unsigned int kAlignWidth = 2;
> + static constexpr unsigned int kAlignHeight = 4;
I'd name these kIFAlignWidth and kIFAlignHeight, they were called
IF_ALIGN_W and IF_ALIGN_H.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> +
> + static constexpr unsigned int kIFMaxCropWidth = 40;
> + static constexpr unsigned int kIFMaxCropHeight = 540;
> +
> + static constexpr unsigned int kBDSAlignWidth = 2;
> + static constexpr unsigned int kBDSAlignHeight = 4;
> +
> + static constexpr float kBDSSfMax = 2.5;
> + static constexpr float kBDSSfMin = 1.0;
> + static constexpr float kBDSSfStep = 0.03125;
> +
> + static constexpr Size kOutputMinSize = { 2, 2 };
> + static constexpr Size kOutputMaxSize = { 4480, 34004 };
> + static constexpr unsigned int kOutputAlignWidth = 64;
> + static constexpr unsigned int kOutputAlignHeight = 4;
> + static constexpr unsigned int kOutputMarginWidth = 64;
> + static constexpr unsigned int kOutputMarginHeight = 32;
> +
> struct PipeConfig {
> float bds_sf;
> Size iif;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5b2ab2ee6825..bb3826eee422 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -41,12 +41,6 @@ LOG_DEFINE_CATEGORY(IPU3)
>
> static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> -static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 };
> -static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 };
> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;
> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
> static constexpr Size IPU3ViewfinderSize(1280, 720);
>
> static const ControlInfoMap::Map IPU3Controls = {
> @@ -287,9 +281,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> * https://bugs.libcamera.org/show_bug.cgi?id=32
> */
> if (rawSize.isNull())
> - rawSize = maxYuvSize.expandedTo({40, 540})
> - .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> - IMGU_OUTPUT_HEIGHT_MARGIN)
> + rawSize = maxYuvSize.expandedTo({ImgUDevice::kIFMaxCropWidth,
> + ImgUDevice::kIFMaxCropHeight})
> + .alignedUpTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight)
> .boundedTo(data_->cio2_.sensor()->resolution());
> cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> if (!cio2Configuration_.pixelFormat.isValid())
> @@ -345,19 +340,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> */
> unsigned int limit;
> limit = utils::alignDown(cio2Configuration_.size.width - 1,
> - IMGU_OUTPUT_WIDTH_MARGIN);
> + ImgUDevice::kOutputMarginWidth);
> cfg->size.width = std::clamp(cfg->size.width,
> - IMGU_OUTPUT_MIN_SIZE.width,
> + ImgUDevice::kOutputMinSize.width,
> limit);
>
> limit = utils::alignDown(cio2Configuration_.size.height - 1,
> - IMGU_OUTPUT_HEIGHT_MARGIN);
> + ImgUDevice::kOutputMarginHeight);
> cfg->size.height = std::clamp(cfg->size.height,
> - IMGU_OUTPUT_MIN_SIZE.height,
> + ImgUDevice::kOutputMinSize.height,
> limit);
>
> - cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> - IMGU_OUTPUT_HEIGHT_ALIGN);
> + cfg->size.alignDownTo(ImgUDevice::kOutputAlignWidth,
> + ImgUDevice::kOutputAlignHeight);
>
> cfg->pixelFormat = formats::NV12;
> cfg->bufferCount = IPU3_BUFFER_COUNT;
> @@ -443,14 +438,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> * \todo Clarify the alignment constraints as explained
> * in validate()
> */
> - size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> + size = sensorResolution.boundedTo(ImgUDevice::kOutputMaxSize);
> size.width = utils::alignDown(size.width - 1,
> - IMGU_OUTPUT_WIDTH_MARGIN);
> + ImgUDevice::kOutputMarginWidth);
> size.height = utils::alignDown(size.height - 1,
> - IMGU_OUTPUT_HEIGHT_MARGIN);
> + ImgUDevice::kOutputMarginHeight);
> pixelFormat = formats::NV12;
> bufferCount = IPU3_BUFFER_COUNT;
> - streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> + streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
>
> break;
>
> @@ -475,11 +470,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> * to the ImgU output constraints.
> */
> size = sensorResolution.boundedTo(IPU3ViewfinderSize)
> - .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> - IMGU_OUTPUT_HEIGHT_ALIGN);
> + .alignedDownTo(ImgUDevice::kOutputAlignWidth,
> + ImgUDevice::kOutputAlignHeight);
> pixelFormat = formats::NV12;
> bufferCount = IPU3_BUFFER_COUNT;
> - streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> + streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
>
> break;
> }
> @@ -1003,8 +998,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> /* The strictly smaller size than the sensor resolution, aligned to margins. */
> Size minSize = Size(sensor->resolution().width - 1,
> sensor->resolution().height - 1)
> - .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> - IMGU_OUTPUT_HEIGHT_MARGIN);
> + .alignedDownTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight);
>
> /*
> * Either the smallest margin-aligned size larger than the viewfinder
> @@ -1012,8 +1007,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> */
> minSize = Size(IPU3ViewfinderSize.width + 1,
> IPU3ViewfinderSize.height + 1)
> - .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> - IMGU_OUTPUT_HEIGHT_MARGIN)
> + .alignedUpTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight)
> .boundedTo(minSize);
>
> /*
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list