[{"id":19526,"web_url":"https://patchwork.libcamera.org/comment/19526/","msgid":"<20210908003940.GB968527@pyrite.rasen.tech>","date":"2021-09-08T00:39:40","subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Sep 07, 2021 at 09:40:52PM +0200, Jacopo Mondi wrote:\n> The definition of several constants that describe the ImgU\n> characteristics are spread between two files: ipu3.cpp and imgu.cpp.\n> \n> As the ipu3.cpp uses definitions from the imgu.cpp one, in order to\n> remove the usage of magic numbers, it is required to move the\n> definitions to a common header file where they are accessible to the\n> other .cpp modules.\n> \n> Move all the definitions of the ImgU sizes and alignment in a dedicated\n> namespace in imgu.h and update their users accordingly.\n> \n> Cosmetic changes, no functional changes intended.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  src/libcamera/pipeline/ipu3/imgu.cpp | 86 +++++++++++-----------------\n>  src/libcamera/pipeline/ipu3/imgu.h   | 27 +++++++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 47 +++++++--------\n>  3 files changed, 83 insertions(+), 77 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 317e482a1498..441ff1b0705c 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -34,22 +34,6 @@ namespace {\n>   * at revision: 243d13446e44 (\"Fix some bug for some resolutions\")\n>   */\n>  \n> -static constexpr unsigned int FILTER_W = 4;\n> -static constexpr unsigned int FILTER_H = 4;\n> -\n> -static constexpr unsigned int IF_ALIGN_W = 2;\n> -static constexpr unsigned int IF_ALIGN_H = 4;\n> -\n> -static constexpr unsigned int BDS_ALIGN_W = 2;\n> -static constexpr unsigned int BDS_ALIGN_H = 4;\n> -\n> -static constexpr unsigned int IF_CROP_MAX_W = 40;\n> -static constexpr unsigned int IF_CROP_MAX_H = 540;\n> -\n> -static constexpr float BDS_SF_MAX = 2.5;\n> -static constexpr float BDS_SF_MIN = 1.0;\n> -static constexpr float BDS_SF_STEP = 0.03125;\n> -\n>  /* BSD scaling factors: min=1, max=2.5, step=1/32 */\n>  const std::vector<float> bdsScalingFactors = {\n>  \t1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,\n> @@ -124,8 +108,8 @@ bool isSameRatio(const Size &in, const Size &out)\n>  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,\n>  \t\t\tunsigned int bdsWidth, float bdsSF)\n>  {\n> -\tunsigned int minIFHeight = iif.height - IF_CROP_MAX_H;\n> -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> +\tunsigned int minIFHeight = iif.height - IMGU::IF_CROP_MAX_H;\n> +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n>  \tunsigned int ifHeight;\n>  \tfloat bdsHeight;\n>  \n> @@ -135,7 +119,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\t\t    static_cast<float>(gdc.width);\n>  \t\testIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);\n>  \n> -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n>  \n> @@ -143,17 +127,17 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\tif (std::fmod(height, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n>  \n> -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tfoundIfHeight = ifHeight;\n>  \t\t\t\t\tbdsHeight = height;\n>  \t\t\t\t\tbreak;\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n> -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n>  \n> @@ -161,14 +145,14 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\tif (std::fmod(height, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n>  \n> -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tfoundIfHeight = ifHeight;\n>  \t\t\t\t\tbdsHeight = height;\n>  \t\t\t\t\tbreak;\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight += IF_ALIGN_H;\n> +\t\t\tifHeight += IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n>  \t\tif (foundIfHeight) {\n> @@ -179,32 +163,32 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\treturn;\n>  \t\t}\n>  \t} else {\n> -\t\tifHeight = utils::alignUp(iif.height, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(iif.height, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n>  \n>  \t\t\tbdsHeight = ifHeight / bdsSF;\n>  \t\t\tif (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n>  \n> -\t\t\t\tif (!(ifHeight % IF_ALIGN_H) &&\n> -\t\t\t\t    !(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(ifHeight % IMGU::IF_ALIGN_H) &&\n> +\t\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tpipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },\n>  \t\t\t\t\t\t\t\t{ bdsWidth, bdsIntHeight }, gdc });\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \t}\n>  }\n>  \n>  void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF)\n>  {\n> -\tunsigned int minBDSWidth = gdc.width + FILTER_W * 2;\n> -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> +\tunsigned int minBDSWidth = gdc.width + IMGU::FILTER_W * 2;\n> +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n>  \n>  \tfloat sf = bdsSF;\n> -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n>  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n>  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n>  \n> @@ -212,16 +196,16 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n>  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n>  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n>  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n>  \t\t}\n>  \n> -\t\tsf += BDS_SF_STEP;\n> +\t\tsf += IMGU::BDS_SF_STEP;\n>  \t}\n>  \n>  \tsf = bdsSF;\n> -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n>  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n>  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n>  \n> @@ -229,12 +213,12 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n>  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n>  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n>  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n>  \t\t}\n>  \n> -\t\tsf -= BDS_SF_STEP;\n> +\t\tsf -= IMGU::BDS_SF_STEP;\n>  \t}\n>  }\n>  \n> @@ -412,7 +396,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \t * \\todo Filter out all resolutions < IF_CROP_MAX.\n>  \t * See https://bugs.libcamera.org/show_bug.cgi?id=32\n>  \t */\n> -\tif (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {\n> +\tif (in.width < IMGU::IF_CROP_MAX_W || in.height < IMGU::IF_CROP_MAX_H) {\n>  \t\tLOG(IPU3, Error) << \"Input resolution \" << in.toString()\n>  \t\t\t\t << \" not supported\";\n>  \t\treturn {};\n> @@ -424,25 +408,25 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n>  \n>  \t/* Populate the configurations vector by scaling width and height. */\n> -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> -\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> +\tunsigned int ifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> +\tunsigned int ifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> +\tunsigned int minIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> +\tunsigned int minIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n>  \twhile (ifWidth >= minIfWidth) {\n>  \t\twhile (ifHeight >= minIfHeight) {\n>  \t\t\tSize iif{ ifWidth, ifHeight };\n>  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n> -\t\tifWidth -= IF_ALIGN_W;\n> +\t\tifWidth -= IMGU::IF_ALIGN_W;\n>  \t}\n>  \n>  \t/* Repeat search by scaling width first. */\n> -\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> -\tminIfWidth = in.width - IF_CROP_MAX_W;\n> -\tminIfHeight = in.height - IF_CROP_MAX_H;\n> +\tifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> +\tifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> +\tminIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> +\tminIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n>  \twhile (ifHeight >= minIfHeight) {\n>  \t\t/*\n>  \t\t * \\todo This procedure is probably broken:\n> @@ -451,10 +435,10 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \t\twhile (ifWidth >= minIfWidth) {\n>  \t\t\tSize iif{ ifWidth, ifHeight };\n>  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> -\t\t\tifWidth -= IF_ALIGN_W;\n> +\t\t\tifWidth -= IMGU::IF_ALIGN_W;\n>  \t\t}\n>  \n> -\t\tifHeight -= IF_ALIGN_H;\n> +\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t}\n>  \n>  \tif (pipeConfigs.size() == 0) {\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 9d4915116087..df64cbaba5a7 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -15,6 +15,33 @@\n>  \n>  namespace libcamera {\n>  \n> +namespace IMGU {\n> +\n> +static constexpr unsigned int FILTER_W = 4;\n> +static constexpr unsigned int FILTER_H = 4;\n> +\n> +static constexpr unsigned int IF_ALIGN_W = 2;\n> +static constexpr unsigned int IF_ALIGN_H = 4;\n> +\n> +static constexpr unsigned int IF_CROP_MAX_W = 40;\n> +static constexpr unsigned int IF_CROP_MAX_H = 540;\n> +\n> +static constexpr unsigned int BDS_ALIGN_W = 2;\n> +static constexpr unsigned int BDS_ALIGN_H = 4;\n> +\n> +static constexpr float BDS_SF_MAX = 2.5;\n> +static constexpr float BDS_SF_MIN = 1.0;\n> +static constexpr float BDS_SF_STEP = 0.03125;\n> +\n> +static const Size OUTPUT_MIN_SIZE = { 2, 2 };\n> +static const Size OUTPUT_MAX_SIZE = { 4480, 34004 };\n> +static constexpr unsigned int OUTPUT_WIDTH_ALIGN = 64;\n> +static constexpr unsigned int OUTPUT_HEIGHT_ALIGN = 4;\n> +static constexpr unsigned int OUTPUT_WIDTH_MARGIN = 64;\n> +static constexpr unsigned int OUTPUT_HEIGHT_MARGIN = 32;\n> +\n> +} /* namespace IMGU */\n> +\n>  class FrameBuffer;\n>  class MediaDevice;\n>  class Size;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 291338288685..89a05fab69ad 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -41,12 +41,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n>  \n>  static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n>  static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> -static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 };\n> -static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 };\n> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n>  static constexpr Size IPU3ViewfinderSize(1280, 720);\n>  \n>  static const ControlInfoMap::Map IPU3Controls = {\n> @@ -287,9 +281,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t * https://bugs.libcamera.org/show_bug.cgi?id=32\n>  \t */\n>  \tif (rawSize.isNull())\n> -\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> -\t\t\t\t    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> +\t\trawSize = maxYuvSize.alignedUpTo(IMGU::IF_CROP_MAX_W,\n> +\t\t\t\t\t\t IMGU::IF_CROP_MAX_H)\n> +\t\t\t\t    .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN)\n>  \t\t\t\t    .boundedTo(data_->cio2_.sensor()->resolution());\n>  \tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n>  \tif (!cio2Configuration_.pixelFormat.isValid())\n> @@ -345,19 +340,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\t\t */\n>  \t\t\tunsigned int limit;\n>  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\t\t\t\t\t IMGU::OUTPUT_WIDTH_MARGIN);\n>  \t\t\tcfg->size.width = std::clamp(cfg->size.width,\n> -\t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n> +\t\t\t\t\t\t     IMGU::OUTPUT_MIN_SIZE.width,\n>  \t\t\t\t\t\t     limit);\n>  \n>  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \t\t\tcfg->size.height = std::clamp(cfg->size.height,\n> -\t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n> +\t\t\t\t\t\t      IMGU::OUTPUT_MIN_SIZE.height,\n>  \t\t\t\t\t\t      limit);\n>  \n> -\t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> -\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\t\t\tcfg->size.alignDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n>  \n>  \t\t\tcfg->pixelFormat = formats::NV12;\n>  \t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> @@ -443,14 +438,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * \\todo Clarify the alignment constraints as explained\n>  \t\t\t * in validate()\n>  \t\t\t */\n> -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> +\t\t\tsize = sensorResolution.boundedTo(IMGU::OUTPUT_MAX_SIZE);\n>  \t\t\tsize.width = utils::alignDown(size.width - 1,\n> -\t\t\t\t\t\t      IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\t\t\t\t\t      IMGU::OUTPUT_WIDTH_MARGIN);\n>  \t\t\tsize.height = utils::alignDown(size.height - 1,\n> -\t\t\t\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\t\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \t\t\tpixelFormat = formats::NV12;\n>  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n>  \n>  \t\t\tbreak;\n>  \n> @@ -475,11 +470,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * to the ImgU output constraints.\n>  \t\t\t */\n>  \t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> -\t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> -\t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\t\t\t\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n>  \t\t\tpixelFormat = formats::NV12;\n>  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n>  \n>  \t\t\tbreak;\n>  \t\t}\n> @@ -1003,8 +998,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n>  \tSize minSize = Size(sensor->resolution().width - 1,\n>  \t\t\t    sensor->resolution().height - 1)\n> -\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t\t      IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t      IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \n>  \t/*\n>  \t * Either the smallest margin-aligned size larger than the viewfinder\n> @@ -1012,8 +1007,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \t */\n>  \tminSize = Size(IPU3ViewfinderSize.width + 1,\n>  \t\t       IPU3ViewfinderSize.height + 1)\n> -\t\t  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN)\n> +\t\t  .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN)\n>  \t\t  .boundedTo(minSize);\n>  \n>  \t/*\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EE15FBDB1D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  8 Sep 2021 00:39:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 876D760252;\n\tWed,  8 Sep 2021 02:39:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 114F160137\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Sep 2021 02:39:49 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9573A317;\n\tWed,  8 Sep 2021 02:39:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W4asx1Rz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631061588;\n\tbh=lCDvgiNB0OGxMSVwf0sOb+73KZ4ad4YO8f4nioc7wMs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W4asx1Rz8Vp3ObgA3/LfpshxxzmE5DmJfVgkRWrbiwV9dU93aY6WopdE/fLrlB5jo\n\t6waJUu6VwYfvAarFN6XSz3kol0execYAPIZR7CZgm1WYMa/mdyQGin07TBCfSQIOZH\n\t9lZTReIZz3JtCOvK00LFynMM2b6IGo8h5bGo3HlY=","Date":"Wed, 8 Sep 2021 09:39:40 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20210908003940.GB968527@pyrite.rasen.tech>","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210907194107.803730-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19539,"web_url":"https://patchwork.libcamera.org/comment/19539/","msgid":"<05c103aa-c2fd-216a-c8e1-ca82973bc34c@ideasonboard.com>","date":"2021-09-08T09:19:37","subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for your patch !\n\nOn 07/09/2021 21:40, Jacopo Mondi wrote:\n> The definition of several constants that describe the ImgU\n> characteristics are spread between two files: ipu3.cpp and imgu.cpp.\n> \n> As the ipu3.cpp uses definitions from the imgu.cpp one, in order to\n> remove the usage of magic numbers, it is required to move the\n> definitions to a common header file where they are accessible to the\n> other .cpp modules.\n> \n> Move all the definitions of the ImgU sizes and alignment in a dedicated\n> namespace in imgu.h and update their users accordingly.\n> \n> Cosmetic changes, no functional changes intended.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/imgu.cpp | 86 +++++++++++-----------------\n>  src/libcamera/pipeline/ipu3/imgu.h   | 27 +++++++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 47 +++++++--------\n>  3 files changed, 83 insertions(+), 77 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 317e482a1498..441ff1b0705c 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -34,22 +34,6 @@ namespace {\n>   * at revision: 243d13446e44 (\"Fix some bug for some resolutions\")\n>   */\n>  \n> -static constexpr unsigned int FILTER_W = 4;\n> -static constexpr unsigned int FILTER_H = 4;\n> -\n> -static constexpr unsigned int IF_ALIGN_W = 2;\n> -static constexpr unsigned int IF_ALIGN_H = 4;\n> -\n> -static constexpr unsigned int BDS_ALIGN_W = 2;\n> -static constexpr unsigned int BDS_ALIGN_H = 4;\n> -\n> -static constexpr unsigned int IF_CROP_MAX_W = 40;\n> -static constexpr unsigned int IF_CROP_MAX_H = 540;\n> -\n> -static constexpr float BDS_SF_MAX = 2.5;\n> -static constexpr float BDS_SF_MIN = 1.0;\n> -static constexpr float BDS_SF_STEP = 0.03125;\n> -\n>  /* BSD scaling factors: min=1, max=2.5, step=1/32 */\n>  const std::vector<float> bdsScalingFactors = {\n>  \t1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,\n> @@ -124,8 +108,8 @@ bool isSameRatio(const Size &in, const Size &out)\n>  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,\n>  \t\t\tunsigned int bdsWidth, float bdsSF)\n>  {\n> -\tunsigned int minIFHeight = iif.height - IF_CROP_MAX_H;\n> -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> +\tunsigned int minIFHeight = iif.height - IMGU::IF_CROP_MAX_H;\n> +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n>  \tunsigned int ifHeight;\n>  \tfloat bdsHeight;\n>  \n> @@ -135,7 +119,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\t\t    static_cast<float>(gdc.width);\n>  \t\testIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);\n>  \n> -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n>  \n> @@ -143,17 +127,17 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\tif (std::fmod(height, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n>  \n> -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tfoundIfHeight = ifHeight;\n>  \t\t\t\t\tbdsHeight = height;\n>  \t\t\t\t\tbreak;\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n> -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n>  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n>  \n> @@ -161,14 +145,14 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\tif (std::fmod(height, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n>  \n> -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tfoundIfHeight = ifHeight;\n>  \t\t\t\t\tbdsHeight = height;\n>  \t\t\t\t\tbreak;\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight += IF_ALIGN_H;\n> +\t\t\tifHeight += IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n>  \t\tif (foundIfHeight) {\n> @@ -179,32 +163,32 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n>  \t\t\treturn;\n>  \t\t}\n>  \t} else {\n> -\t\tifHeight = utils::alignUp(iif.height, IF_ALIGN_H);\n> +\t\tifHeight = utils::alignUp(iif.height, IMGU::IF_ALIGN_H);\n>  \t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n>  \n>  \t\t\tbdsHeight = ifHeight / bdsSF;\n>  \t\t\tif (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n>  \n> -\t\t\t\tif (!(ifHeight % IF_ALIGN_H) &&\n> -\t\t\t\t    !(bdsIntHeight % BDS_ALIGN_H)) {\n> +\t\t\t\tif (!(ifHeight % IMGU::IF_ALIGN_H) &&\n> +\t\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n>  \t\t\t\t\tpipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },\n>  \t\t\t\t\t\t\t\t{ bdsWidth, bdsIntHeight }, gdc });\n>  \t\t\t\t}\n>  \t\t\t}\n>  \n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \t}\n>  }\n>  \n>  void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF)\n>  {\n> -\tunsigned int minBDSWidth = gdc.width + FILTER_W * 2;\n> -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> +\tunsigned int minBDSWidth = gdc.width + IMGU::FILTER_W * 2;\n> +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n>  \n>  \tfloat sf = bdsSF;\n> -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n>  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n>  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n>  \n> @@ -212,16 +196,16 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n>  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n>  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n>  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n>  \t\t}\n>  \n> -\t\tsf += BDS_SF_STEP;\n> +\t\tsf += IMGU::BDS_SF_STEP;\n>  \t}\n>  \n>  \tsf = bdsSF;\n> -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n>  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n>  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n>  \n> @@ -229,12 +213,12 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n>  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n>  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n>  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n>  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n>  \t\t}\n>  \n> -\t\tsf -= BDS_SF_STEP;\n> +\t\tsf -= IMGU::BDS_SF_STEP;\n>  \t}\n>  }\n>  \n> @@ -412,7 +396,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \t * \\todo Filter out all resolutions < IF_CROP_MAX.\n>  \t * See https://bugs.libcamera.org/show_bug.cgi?id=32\n>  \t */\n> -\tif (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {\n> +\tif (in.width < IMGU::IF_CROP_MAX_W || in.height < IMGU::IF_CROP_MAX_H) {\n>  \t\tLOG(IPU3, Error) << \"Input resolution \" << in.toString()\n>  \t\t\t\t << \" not supported\";\n>  \t\treturn {};\n> @@ -424,25 +408,25 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n>  \n>  \t/* Populate the configurations vector by scaling width and height. */\n> -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> -\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> +\tunsigned int ifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> +\tunsigned int ifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> +\tunsigned int minIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> +\tunsigned int minIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n>  \twhile (ifWidth >= minIfWidth) {\n>  \t\twhile (ifHeight >= minIfHeight) {\n>  \t\t\tSize iif{ ifWidth, ifHeight };\n>  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> -\t\t\tifHeight -= IF_ALIGN_H;\n> +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t\t}\n>  \n> -\t\tifWidth -= IF_ALIGN_W;\n> +\t\tifWidth -= IMGU::IF_ALIGN_W;\n>  \t}\n>  \n>  \t/* Repeat search by scaling width first. */\n> -\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> -\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> -\tminIfWidth = in.width - IF_CROP_MAX_W;\n> -\tminIfHeight = in.height - IF_CROP_MAX_H;\n> +\tifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> +\tifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> +\tminIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> +\tminIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n>  \twhile (ifHeight >= minIfHeight) {\n>  \t\t/*\n>  \t\t * \\todo This procedure is probably broken:\n> @@ -451,10 +435,10 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n>  \t\twhile (ifWidth >= minIfWidth) {\n>  \t\t\tSize iif{ ifWidth, ifHeight };\n>  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> -\t\t\tifWidth -= IF_ALIGN_W;\n> +\t\t\tifWidth -= IMGU::IF_ALIGN_W;\n>  \t\t}\n>  \n> -\t\tifHeight -= IF_ALIGN_H;\n> +\t\tifHeight -= IMGU::IF_ALIGN_H;\n>  \t}\n>  \n>  \tif (pipeConfigs.size() == 0) {\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 9d4915116087..df64cbaba5a7 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -15,6 +15,33 @@\n>  \n>  namespace libcamera {\n>  \n> +namespace IMGU {\n\nWe almost always reference it as ImgU, is a fully capitalized namespace\nbetter ? I can't really say...\n\nnamespace ImgU {\n}\nifHeight -= ImgU::IF_ALIGN_H;\n\nI will let you decide as this is very subjective I guess ?\n\nReviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n\n> +\n> +static constexpr unsigned int FILTER_W = 4;\n> +static constexpr unsigned int FILTER_H = 4;\n> +\n> +static constexpr unsigned int IF_ALIGN_W = 2;\n> +static constexpr unsigned int IF_ALIGN_H = 4;\n> +\n> +static constexpr unsigned int IF_CROP_MAX_W = 40;\n> +static constexpr unsigned int IF_CROP_MAX_H = 540;\n> +\n> +static constexpr unsigned int BDS_ALIGN_W = 2;\n> +static constexpr unsigned int BDS_ALIGN_H = 4;\n> +\n> +static constexpr float BDS_SF_MAX = 2.5;\n> +static constexpr float BDS_SF_MIN = 1.0;\n> +static constexpr float BDS_SF_STEP = 0.03125;\n> +\n> +static const Size OUTPUT_MIN_SIZE = { 2, 2 };\n> +static const Size OUTPUT_MAX_SIZE = { 4480, 34004 };\n> +static constexpr unsigned int OUTPUT_WIDTH_ALIGN = 64;\n> +static constexpr unsigned int OUTPUT_HEIGHT_ALIGN = 4;\n> +static constexpr unsigned int OUTPUT_WIDTH_MARGIN = 64;\n> +static constexpr unsigned int OUTPUT_HEIGHT_MARGIN = 32;\n> +\n> +} /* namespace IMGU */\n> +\n>  class FrameBuffer;\n>  class MediaDevice;\n>  class Size;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 291338288685..89a05fab69ad 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -41,12 +41,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n>  \n>  static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n>  static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> -static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 };\n> -static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 };\n> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> -static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n>  static constexpr Size IPU3ViewfinderSize(1280, 720);\n>  \n>  static const ControlInfoMap::Map IPU3Controls = {\n> @@ -287,9 +281,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t * https://bugs.libcamera.org/show_bug.cgi?id=32\n>  \t */\n>  \tif (rawSize.isNull())\n> -\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> -\t\t\t\t    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> +\t\trawSize = maxYuvSize.alignedUpTo(IMGU::IF_CROP_MAX_W,\n> +\t\t\t\t\t\t IMGU::IF_CROP_MAX_H)\n> +\t\t\t\t    .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN)\n>  \t\t\t\t    .boundedTo(data_->cio2_.sensor()->resolution());\n>  \tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n>  \tif (!cio2Configuration_.pixelFormat.isValid())\n> @@ -345,19 +340,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\t\t */\n>  \t\t\tunsigned int limit;\n>  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\t\t\t\t\t IMGU::OUTPUT_WIDTH_MARGIN);\n>  \t\t\tcfg->size.width = std::clamp(cfg->size.width,\n> -\t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n> +\t\t\t\t\t\t     IMGU::OUTPUT_MIN_SIZE.width,\n>  \t\t\t\t\t\t     limit);\n>  \n>  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \t\t\tcfg->size.height = std::clamp(cfg->size.height,\n> -\t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n> +\t\t\t\t\t\t      IMGU::OUTPUT_MIN_SIZE.height,\n>  \t\t\t\t\t\t      limit);\n>  \n> -\t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> -\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\t\t\tcfg->size.alignDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n>  \n>  \t\t\tcfg->pixelFormat = formats::NV12;\n>  \t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> @@ -443,14 +438,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * \\todo Clarify the alignment constraints as explained\n>  \t\t\t * in validate()\n>  \t\t\t */\n> -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> +\t\t\tsize = sensorResolution.boundedTo(IMGU::OUTPUT_MAX_SIZE);\n>  \t\t\tsize.width = utils::alignDown(size.width - 1,\n> -\t\t\t\t\t\t      IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\t\t\t\t\t      IMGU::OUTPUT_WIDTH_MARGIN);\n>  \t\t\tsize.height = utils::alignDown(size.height - 1,\n> -\t\t\t\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\t\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \t\t\tpixelFormat = formats::NV12;\n>  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n>  \n>  \t\t\tbreak;\n>  \n> @@ -475,11 +470,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * to the ImgU output constraints.\n>  \t\t\t */\n>  \t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> -\t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> -\t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\t\t\t\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n>  \t\t\tpixelFormat = formats::NV12;\n>  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n>  \n>  \t\t\tbreak;\n>  \t\t}\n> @@ -1003,8 +998,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n>  \tSize minSize = Size(sensor->resolution().width - 1,\n>  \t\t\t    sensor->resolution().height - 1)\n> -\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t\t      IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t      IMGU::OUTPUT_HEIGHT_MARGIN);\n>  \n>  \t/*\n>  \t * Either the smallest margin-aligned size larger than the viewfinder\n> @@ -1012,8 +1007,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \t */\n>  \tminSize = Size(IPU3ViewfinderSize.width + 1,\n>  \t\t       IPU3ViewfinderSize.height + 1)\n> -\t\t  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> -\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN)\n> +\t\t  .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> +\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN)\n>  \t\t  .boundedTo(minSize);\n>  \n>  \t/*\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 72000BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  8 Sep 2021 09:19:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B82666916E;\n\tWed,  8 Sep 2021 11:19:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 924186024D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Sep 2021 11:19:40 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:1ff4:3000:cbce:eabb])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A923993;\n\tWed,  8 Sep 2021 11:19:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OTpNUcAy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631092780;\n\tbh=SSQP9ZOEViYNcKPnJempbZ61BVHajbxOMZrRM+1a3jk=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=OTpNUcAytOfDeC2Ts7FCYIi3yWW2foPiaePu/JKhRikcs7V5hcPbJzPL4o49ILAHZ\n\tGWXugH3Y+bCvUha8EjFVLokxQGZvgQ0hQZiecuDWB2NL7s0HUgV81WXJTNiN1MR5qo\n\tiSsm8qXrtHpgoI49yekQrPhlf90Z32YPfd2qlt4Q=","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-3-jacopo@jmondi.org>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<05c103aa-c2fd-216a-c8e1-ca82973bc34c@ideasonboard.com>","Date":"Wed, 8 Sep 2021 11:19:37 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210907194107.803730-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19787,"web_url":"https://patchwork.libcamera.org/comment/19787/","msgid":"<YUsAa3iNMaxQvunM@pendragon.ideasonboard.com>","date":"2021-09-22T10:07:39","subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Sep 08, 2021 at 11:19:37AM +0200, Jean-Michel Hautbois wrote:\n> On 07/09/2021 21:40, Jacopo Mondi wrote:\n> > The definition of several constants that describe the ImgU\n> > characteristics are spread between two files: ipu3.cpp and imgu.cpp.\n> > \n> > As the ipu3.cpp uses definitions from the imgu.cpp one, in order to\n> > remove the usage of magic numbers, it is required to move the\n> > definitions to a common header file where they are accessible to the\n> > other .cpp modules.\n> > \n> > Move all the definitions of the ImgU sizes and alignment in a dedicated\n> > namespace in imgu.h and update their users accordingly.\n> > \n> > Cosmetic changes, no functional changes intended.\n> > \n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/imgu.cpp | 86 +++++++++++-----------------\n> >  src/libcamera/pipeline/ipu3/imgu.h   | 27 +++++++++\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 47 +++++++--------\n> >  3 files changed, 83 insertions(+), 77 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 317e482a1498..441ff1b0705c 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -34,22 +34,6 @@ namespace {\n> >   * at revision: 243d13446e44 (\"Fix some bug for some resolutions\")\n> >   */\n> >  \n> > -static constexpr unsigned int FILTER_W = 4;\n> > -static constexpr unsigned int FILTER_H = 4;\n> > -\n> > -static constexpr unsigned int IF_ALIGN_W = 2;\n> > -static constexpr unsigned int IF_ALIGN_H = 4;\n> > -\n> > -static constexpr unsigned int BDS_ALIGN_W = 2;\n> > -static constexpr unsigned int BDS_ALIGN_H = 4;\n> > -\n> > -static constexpr unsigned int IF_CROP_MAX_W = 40;\n> > -static constexpr unsigned int IF_CROP_MAX_H = 540;\n> > -\n> > -static constexpr float BDS_SF_MAX = 2.5;\n> > -static constexpr float BDS_SF_MIN = 1.0;\n> > -static constexpr float BDS_SF_STEP = 0.03125;\n> > -\n> >  /* BSD scaling factors: min=1, max=2.5, step=1/32 */\n> >  const std::vector<float> bdsScalingFactors = {\n> >  \t1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,\n> > @@ -124,8 +108,8 @@ bool isSameRatio(const Size &in, const Size &out)\n> >  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,\n> >  \t\t\tunsigned int bdsWidth, float bdsSF)\n> >  {\n> > -\tunsigned int minIFHeight = iif.height - IF_CROP_MAX_H;\n> > -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> > +\tunsigned int minIFHeight = iif.height - IMGU::IF_CROP_MAX_H;\n> > +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n> >  \tunsigned int ifHeight;\n> >  \tfloat bdsHeight;\n> >  \n> > @@ -135,7 +119,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> >  \t\t\t\t    static_cast<float>(gdc.width);\n> >  \t\testIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);\n> >  \n> > -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n> >  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> >  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n> >  \n> > @@ -143,17 +127,17 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> >  \t\t\tif (std::fmod(height, 1.0) == 0) {\n> >  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n> >  \n> > -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n> >  \t\t\t\t\tfoundIfHeight = ifHeight;\n> >  \t\t\t\t\tbdsHeight = height;\n> >  \t\t\t\t\tbreak;\n> >  \t\t\t\t}\n> >  \t\t\t}\n> >  \n> > -\t\t\tifHeight -= IF_ALIGN_H;\n> > +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n> >  \t\t}\n> >  \n> > -\t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > +\t\tifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);\n> >  \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> >  \t\t       ifHeight / bdsSF >= minBDSHeight) {\n> >  \n> > @@ -161,14 +145,14 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> >  \t\t\tif (std::fmod(height, 1.0) == 0) {\n> >  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(height);\n> >  \n> > -\t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > +\t\t\t\tif (!(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n> >  \t\t\t\t\tfoundIfHeight = ifHeight;\n> >  \t\t\t\t\tbdsHeight = height;\n> >  \t\t\t\t\tbreak;\n> >  \t\t\t\t}\n> >  \t\t\t}\n> >  \n> > -\t\t\tifHeight += IF_ALIGN_H;\n> > +\t\t\tifHeight += IMGU::IF_ALIGN_H;\n> >  \t\t}\n> >  \n> >  \t\tif (foundIfHeight) {\n> > @@ -179,32 +163,32 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> >  \t\t\treturn;\n> >  \t\t}\n> >  \t} else {\n> > -\t\tifHeight = utils::alignUp(iif.height, IF_ALIGN_H);\n> > +\t\tifHeight = utils::alignUp(iif.height, IMGU::IF_ALIGN_H);\n> >  \t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> >  \n> >  \t\t\tbdsHeight = ifHeight / bdsSF;\n> >  \t\t\tif (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {\n> >  \t\t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> >  \n> > -\t\t\t\tif (!(ifHeight % IF_ALIGN_H) &&\n> > -\t\t\t\t    !(bdsIntHeight % BDS_ALIGN_H)) {\n> > +\t\t\t\tif (!(ifHeight % IMGU::IF_ALIGN_H) &&\n> > +\t\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H)) {\n> >  \t\t\t\t\tpipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },\n> >  \t\t\t\t\t\t\t\t{ bdsWidth, bdsIntHeight }, gdc });\n> >  \t\t\t\t}\n> >  \t\t\t}\n> >  \n> > -\t\t\tifHeight -= IF_ALIGN_H;\n> > +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n> >  \t\t}\n> >  \t}\n> >  }\n> >  \n> >  void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, float bdsSF)\n> >  {\n> > -\tunsigned int minBDSWidth = gdc.width + FILTER_W * 2;\n> > -\tunsigned int minBDSHeight = gdc.height + FILTER_H * 2;\n> > +\tunsigned int minBDSWidth = gdc.width + IMGU::FILTER_W * 2;\n> > +\tunsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;\n> >  \n> >  \tfloat sf = bdsSF;\n> > -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> > +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n> >  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n> >  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n> >  \n> > @@ -212,16 +196,16 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n> >  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n> >  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n> >  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> > -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> > -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> > +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> > +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> >  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n> >  \t\t}\n> >  \n> > -\t\tsf += BDS_SF_STEP;\n> > +\t\tsf += IMGU::BDS_SF_STEP;\n> >  \t}\n> >  \n> >  \tsf = bdsSF;\n> > -\twhile (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {\n> > +\twhile (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {\n> >  \t\tfloat bdsWidth = static_cast<float>(iif.width) / sf;\n> >  \t\tfloat bdsHeight = static_cast<float>(iif.height) / sf;\n> >  \n> > @@ -229,12 +213,12 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa\n> >  \t\t    std::fmod(bdsHeight, 1.0) == 0) {\n> >  \t\t\tunsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);\n> >  \t\t\tunsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);\n> > -\t\t\tif (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> > -\t\t\t    !(bdsIntHeight % BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> > +\t\t\tif (!(bdsIntWidth % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&\n> > +\t\t\t    !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)\n> >  \t\t\t\tcalculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);\n> >  \t\t}\n> >  \n> > -\t\tsf -= BDS_SF_STEP;\n> > +\t\tsf -= IMGU::BDS_SF_STEP;\n> >  \t}\n> >  }\n> >  \n> > @@ -412,7 +396,7 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> >  \t * \\todo Filter out all resolutions < IF_CROP_MAX.\n> >  \t * See https://bugs.libcamera.org/show_bug.cgi?id=32\n> >  \t */\n> > -\tif (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {\n> > +\tif (in.width < IMGU::IF_CROP_MAX_W || in.height < IMGU::IF_CROP_MAX_H) {\n> >  \t\tLOG(IPU3, Error) << \"Input resolution \" << in.toString()\n> >  \t\t\t\t << \" not supported\";\n> >  \t\treturn {};\n> > @@ -424,25 +408,25 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> >  \tfloat sf = findScaleFactor(bdsSF, bdsScalingFactors, true);\n> >  \n> >  \t/* Populate the configurations vector by scaling width and height. */\n> > -\tunsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > -\tunsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > -\tunsigned int minIfWidth = in.width - IF_CROP_MAX_W;\n> > -\tunsigned int minIfHeight = in.height - IF_CROP_MAX_H;\n> > +\tunsigned int ifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> > +\tunsigned int ifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> > +\tunsigned int minIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> > +\tunsigned int minIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n> >  \twhile (ifWidth >= minIfWidth) {\n> >  \t\twhile (ifHeight >= minIfHeight) {\n> >  \t\t\tSize iif{ ifWidth, ifHeight };\n> >  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > -\t\t\tifHeight -= IF_ALIGN_H;\n> > +\t\t\tifHeight -= IMGU::IF_ALIGN_H;\n> >  \t\t}\n> >  \n> > -\t\tifWidth -= IF_ALIGN_W;\n> > +\t\tifWidth -= IMGU::IF_ALIGN_W;\n> >  \t}\n> >  \n> >  \t/* Repeat search by scaling width first. */\n> > -\tifWidth = utils::alignUp(in.width, IF_ALIGN_W);\n> > -\tifHeight = utils::alignUp(in.height, IF_ALIGN_H);\n> > -\tminIfWidth = in.width - IF_CROP_MAX_W;\n> > -\tminIfHeight = in.height - IF_CROP_MAX_H;\n> > +\tifWidth = utils::alignUp(in.width, IMGU::IF_ALIGN_W);\n> > +\tifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);\n> > +\tminIfWidth = in.width - IMGU::IF_CROP_MAX_W;\n> > +\tminIfHeight = in.height - IMGU::IF_CROP_MAX_H;\n> >  \twhile (ifHeight >= minIfHeight) {\n> >  \t\t/*\n> >  \t\t * \\todo This procedure is probably broken:\n> > @@ -451,10 +435,10 @@ ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)\n> >  \t\twhile (ifWidth >= minIfWidth) {\n> >  \t\t\tSize iif{ ifWidth, ifHeight };\n> >  \t\t\tcalculateBDS(pipe, iif, gdc, sf);\n> > -\t\t\tifWidth -= IF_ALIGN_W;\n> > +\t\t\tifWidth -= IMGU::IF_ALIGN_W;\n> >  \t\t}\n> >  \n> > -\t\tifHeight -= IF_ALIGN_H;\n> > +\t\tifHeight -= IMGU::IF_ALIGN_H;\n> >  \t}\n> >  \n> >  \tif (pipeConfigs.size() == 0) {\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> > index 9d4915116087..df64cbaba5a7 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > @@ -15,6 +15,33 @@\n> >  \n> >  namespace libcamera {\n> >  \n> > +namespace IMGU {\n> \n> We almost always reference it as ImgU, is a fully capitalized namespace\n> better ? I can't really say...\n> \n> namespace ImgU {\n> }\n> ifHeight -= ImgU::IF_ALIGN_H;\n\nI'd go one step further (or a few steps), by moving the constants to the\nImgUDevice class instead of creating a namespace (we could rename\nImgUDevice to ImgU if the name ends up being too long). I would also\nstandardize on kCamelCase for constants, but maybe in a separate patch\n(not necessarily part of this series).\n\n> I will let you decide as this is very subjective I guess ?\n> \n> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> \n> > +\n> > +static constexpr unsigned int FILTER_W = 4;\n> > +static constexpr unsigned int FILTER_H = 4;\n> > +\n> > +static constexpr unsigned int IF_ALIGN_W = 2;\n> > +static constexpr unsigned int IF_ALIGN_H = 4;\n> > +\n> > +static constexpr unsigned int IF_CROP_MAX_W = 40;\n> > +static constexpr unsigned int IF_CROP_MAX_H = 540;\n> > +\n> > +static constexpr unsigned int BDS_ALIGN_W = 2;\n> > +static constexpr unsigned int BDS_ALIGN_H = 4;\n> > +\n> > +static constexpr float BDS_SF_MAX = 2.5;\n> > +static constexpr float BDS_SF_MIN = 1.0;\n> > +static constexpr float BDS_SF_STEP = 0.03125;\n> > +\n> > +static const Size OUTPUT_MIN_SIZE = { 2, 2 };\n> > +static const Size OUTPUT_MAX_SIZE = { 4480, 34004 };\n\nSize has a constexpr constructor.\n\n> > +static constexpr unsigned int OUTPUT_WIDTH_ALIGN = 64;\n> > +static constexpr unsigned int OUTPUT_HEIGHT_ALIGN = 4;\n> > +static constexpr unsigned int OUTPUT_WIDTH_MARGIN = 64;\n> > +static constexpr unsigned int OUTPUT_HEIGHT_MARGIN = 32;\n> > +\n> > +} /* namespace IMGU */\n> > +\n> >  class FrameBuffer;\n> >  class MediaDevice;\n> >  class Size;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 291338288685..89a05fab69ad 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -41,12 +41,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >  \n> >  static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >  static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> > -static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 };\n> > -static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 };\n> > -static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> > -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> > -static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> > -static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> >  static constexpr Size IPU3ViewfinderSize(1280, 720);\n> >  \n> >  static const ControlInfoMap::Map IPU3Controls = {\n> > @@ -287,9 +281,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >  \t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> >  \t */\n> >  \tif (rawSize.isNull())\n> > -\t\trawSize = maxYuvSize.alignedUpTo(40, 540)\n> > -\t\t\t\t    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > +\t\trawSize = maxYuvSize.alignedUpTo(IMGU::IF_CROP_MAX_W,\n> > +\t\t\t\t\t\t IMGU::IF_CROP_MAX_H)\n> > +\t\t\t\t    .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN)\n> >  \t\t\t\t    .boundedTo(data_->cio2_.sensor()->resolution());\n> >  \tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> >  \tif (!cio2Configuration_.pixelFormat.isValid())\n> > @@ -345,19 +340,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >  \t\t\t */\n> >  \t\t\tunsigned int limit;\n> >  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> > -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> > +\t\t\t\t\t\t IMGU::OUTPUT_WIDTH_MARGIN);\n> >  \t\t\tcfg->size.width = std::clamp(cfg->size.width,\n> > -\t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n> > +\t\t\t\t\t\t     IMGU::OUTPUT_MIN_SIZE.width,\n> >  \t\t\t\t\t\t     limit);\n> >  \n> >  \t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> > -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > +\t\t\t\t\t\t IMGU::OUTPUT_HEIGHT_MARGIN);\n> >  \t\t\tcfg->size.height = std::clamp(cfg->size.height,\n> > -\t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n> > +\t\t\t\t\t\t      IMGU::OUTPUT_MIN_SIZE.height,\n> >  \t\t\t\t\t\t      limit);\n> >  \n> > -\t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > -\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> > +\t\t\tcfg->size.alignDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> > +\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n> >  \n> >  \t\t\tcfg->pixelFormat = formats::NV12;\n> >  \t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> > @@ -443,14 +438,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >  \t\t\t * \\todo Clarify the alignment constraints as explained\n> >  \t\t\t * in validate()\n> >  \t\t\t */\n> > -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> > +\t\t\tsize = sensorResolution.boundedTo(IMGU::OUTPUT_MAX_SIZE);\n> >  \t\t\tsize.width = utils::alignDown(size.width - 1,\n> > -\t\t\t\t\t\t      IMGU_OUTPUT_WIDTH_MARGIN);\n> > +\t\t\t\t\t\t      IMGU::OUTPUT_WIDTH_MARGIN);\n> >  \t\t\tsize.height = utils::alignDown(size.height - 1,\n> > -\t\t\t\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN);\n> > +\t\t\t\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN);\n> >  \t\t\tpixelFormat = formats::NV12;\n> >  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> > -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> > +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n> >  \n> >  \t\t\tbreak;\n> >  \n> > @@ -475,11 +470,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >  \t\t\t * to the ImgU output constraints.\n> >  \t\t\t */\n> >  \t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> > -\t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > -\t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> > +\t\t\t\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_ALIGN,\n> > +\t\t\t\t\t\t\t      IMGU::OUTPUT_HEIGHT_ALIGN);\n> >  \t\t\tpixelFormat = formats::NV12;\n> >  \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> > -\t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> > +\t\t\tstreamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, size } };\n> >  \n> >  \t\t\tbreak;\n> >  \t\t}\n> > @@ -1003,8 +998,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >  \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n> >  \tSize minSize = Size(sensor->resolution().width - 1,\n> >  \t\t\t    sensor->resolution().height - 1)\n> > -\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > -\t\t\t\t      IMGU_OUTPUT_HEIGHT_MARGIN);\n> > +\t\t       .alignedDownTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t\t      IMGU::OUTPUT_HEIGHT_MARGIN);\n> >  \n> >  \t/*\n> >  \t * Either the smallest margin-aligned size larger than the viewfinder\n> > @@ -1012,8 +1007,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >  \t */\n> >  \tminSize = Size(IPU3ViewfinderSize.width + 1,\n> >  \t\t       IPU3ViewfinderSize.height + 1)\n> > -\t\t  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > -\t\t\t       IMGU_OUTPUT_HEIGHT_MARGIN)\n> > +\t\t  .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t       IMGU::OUTPUT_HEIGHT_MARGIN)\n> >  \t\t  .boundedTo(minSize);\n> >  \n> >  \t/*","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 59B28BF01C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 22 Sep 2021 10:07:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B56C46918C;\n\tWed, 22 Sep 2021 12:07:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E15576917F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Sep 2021 12:07:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95043F1;\n\tWed, 22 Sep 2021 12:07:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"t22uPPew\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632305260;\n\tbh=sngT38aSJM4wQftM809HlvXkKiGvdZq/9kt7m9+p6fo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t22uPPewYTnS12p6KnNYtjMq7+cEW3MjQz0FUnNPj2xkO6AXMsACbSJ3B8O6ifonh\n\tr65X0T81MZn2IoDBExsMZ/jxBrAKHmKKkyepssbpQdL4PefXPqrbOI0ZYrqYogsCeV\n\ttySHAB7n9PLrzl1iZr7MECwB7haR7j+OFkkzaDmw=","Date":"Wed, 22 Sep 2021 13:07:39 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YUsAa3iNMaxQvunM@pendragon.ideasonboard.com>","References":"<20210907194107.803730-1-jacopo@jmondi.org>\n\t<20210907194107.803730-3-jacopo@jmondi.org>\n\t<05c103aa-c2fd-216a-c8e1-ca82973bc34c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<05c103aa-c2fd-216a-c8e1-ca82973bc34c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize\n\tImgU sizes definition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]