[libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize ImgU sizes definition
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Sep 8 02:39:40 CEST 2021
Hi Jacopo,
On Tue, Sep 07, 2021 at 09:40:52PM +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 one, 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 alignment in a dedicated
> namespace in imgu.h 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>
> ---
> src/libcamera/pipeline/ipu3/imgu.cpp | 86 +++++++++++-----------------
> src/libcamera/pipeline/ipu3/imgu.h | 27 +++++++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 47 +++++++--------
> 3 files changed, 83 insertions(+), 77 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 317e482a1498..441ff1b0705c 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 - IMGU::IF_CROP_MAX_H;
> + unsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 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, IMGU::IF_ALIGN_H);
> 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 % IMGU::BDS_ALIGN_H)) {
> foundIfHeight = ifHeight;
> bdsHeight = height;
> break;
> }
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= IMGU::IF_ALIGN_H;
> }
>
> - ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> + ifHeight = utils::alignUp(estIFHeight, IMGU::IF_ALIGN_H);
> 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 % IMGU::BDS_ALIGN_H)) {
> foundIfHeight = ifHeight;
> bdsHeight = height;
> break;
> }
> }
>
> - ifHeight += IF_ALIGN_H;
> + ifHeight += IMGU::IF_ALIGN_H;
> }
>
> 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, IMGU::IF_ALIGN_H);
> 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 % IMGU::IF_ALIGN_H) &&
> + !(bdsIntHeight % IMGU::BDS_ALIGN_H)) {
> pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
> { bdsWidth, bdsIntHeight }, gdc });
> }
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= IMGU::IF_ALIGN_H;
> }
> }
> }
>
> 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 + IMGU::FILTER_W * 2;
> + unsigned int minBDSHeight = gdc.height + IMGU::FILTER_H * 2;
>
> float sf = bdsSF;
> - while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> + while (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {
> 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 % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&
> + !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)
> calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> }
>
> - sf += BDS_SF_STEP;
> + sf += IMGU::BDS_SF_STEP;
> }
>
> sf = bdsSF;
> - while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> + while (sf <= IMGU::BDS_SF_MAX && sf >= IMGU::BDS_SF_MIN) {
> 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 % IMGU::BDS_ALIGN_W) && bdsWidth >= minBDSWidth &&
> + !(bdsIntHeight % IMGU::BDS_ALIGN_H) && bdsHeight >= minBDSHeight)
> calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> }
>
> - sf -= BDS_SF_STEP;
> + sf -= IMGU::BDS_SF_STEP;
> }
> }
>
> @@ -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 < IMGU::IF_CROP_MAX_W || in.height < IMGU::IF_CROP_MAX_H) {
> 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, IMGU::IF_ALIGN_W);
> + unsigned int ifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);
> + unsigned int minIfWidth = in.width - IMGU::IF_CROP_MAX_W;
> + unsigned int minIfHeight = in.height - IMGU::IF_CROP_MAX_H;
> while (ifWidth >= minIfWidth) {
> while (ifHeight >= minIfHeight) {
> Size iif{ ifWidth, ifHeight };
> calculateBDS(pipe, iif, gdc, sf);
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= IMGU::IF_ALIGN_H;
> }
>
> - ifWidth -= IF_ALIGN_W;
> + ifWidth -= IMGU::IF_ALIGN_W;
> }
>
> /* 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, IMGU::IF_ALIGN_W);
> + ifHeight = utils::alignUp(in.height, IMGU::IF_ALIGN_H);
> + minIfWidth = in.width - IMGU::IF_CROP_MAX_W;
> + minIfHeight = in.height - IMGU::IF_CROP_MAX_H;
> 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 -= IMGU::IF_ALIGN_W;
> }
>
> - ifHeight -= IF_ALIGN_H;
> + ifHeight -= IMGU::IF_ALIGN_H;
> }
>
> if (pipeConfigs.size() == 0) {
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 9d4915116087..df64cbaba5a7 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -15,6 +15,33 @@
>
> namespace libcamera {
>
> +namespace IMGU {
> +
> +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 IF_CROP_MAX_W = 40;
> +static constexpr unsigned int IF_CROP_MAX_H = 540;
> +
> +static constexpr unsigned int BDS_ALIGN_W = 2;
> +static constexpr unsigned int BDS_ALIGN_H = 4;
> +
> +static constexpr float BDS_SF_MAX = 2.5;
> +static constexpr float BDS_SF_MIN = 1.0;
> +static constexpr float BDS_SF_STEP = 0.03125;
> +
> +static const Size OUTPUT_MIN_SIZE = { 2, 2 };
> +static const Size OUTPUT_MAX_SIZE = { 4480, 34004 };
> +static constexpr unsigned int OUTPUT_WIDTH_ALIGN = 64;
> +static constexpr unsigned int OUTPUT_HEIGHT_ALIGN = 4;
> +static constexpr unsigned int OUTPUT_WIDTH_MARGIN = 64;
> +static constexpr unsigned int OUTPUT_HEIGHT_MARGIN = 32;
> +
> +} /* namespace IMGU */
> +
> class FrameBuffer;
> class MediaDevice;
> class Size;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 291338288685..89a05fab69ad 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.alignedUpTo(40, 540)
> - .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> - IMGU_OUTPUT_HEIGHT_MARGIN)
> + rawSize = maxYuvSize.alignedUpTo(IMGU::IF_CROP_MAX_W,
> + IMGU::IF_CROP_MAX_H)
> + .alignedUpTo(IMGU::OUTPUT_WIDTH_MARGIN,
> + IMGU::OUTPUT_HEIGHT_MARGIN)
> .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);
> + IMGU::OUTPUT_WIDTH_MARGIN);
> cfg->size.width = std::clamp(cfg->size.width,
> - IMGU_OUTPUT_MIN_SIZE.width,
> + IMGU::OUTPUT_MIN_SIZE.width,
> limit);
>
> limit = utils::alignDown(cio2Configuration_.size.height - 1,
> - IMGU_OUTPUT_HEIGHT_MARGIN);
> + IMGU::OUTPUT_HEIGHT_MARGIN);
> cfg->size.height = std::clamp(cfg->size.height,
> - IMGU_OUTPUT_MIN_SIZE.height,
> + IMGU::OUTPUT_MIN_SIZE.height,
> limit);
>
> - cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> - IMGU_OUTPUT_HEIGHT_ALIGN);
> + cfg->size.alignDownTo(IMGU::OUTPUT_WIDTH_ALIGN,
> + IMGU::OUTPUT_HEIGHT_ALIGN);
>
> 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(IMGU::OUTPUT_MAX_SIZE);
> size.width = utils::alignDown(size.width - 1,
> - IMGU_OUTPUT_WIDTH_MARGIN);
> + IMGU::OUTPUT_WIDTH_MARGIN);
> size.height = utils::alignDown(size.height - 1,
> - IMGU_OUTPUT_HEIGHT_MARGIN);
> + IMGU::OUTPUT_HEIGHT_MARGIN);
> pixelFormat = formats::NV12;
> bufferCount = IPU3_BUFFER_COUNT;
> - streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> + streamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, 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(IMGU::OUTPUT_WIDTH_ALIGN,
> + IMGU::OUTPUT_HEIGHT_ALIGN);
> pixelFormat = formats::NV12;
> bufferCount = IPU3_BUFFER_COUNT;
> - streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> + streamFormats[pixelFormat] = { { IMGU::OUTPUT_MIN_SIZE, 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(IMGU::OUTPUT_WIDTH_MARGIN,
> + IMGU::OUTPUT_HEIGHT_MARGIN);
>
> /*
> * 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(IMGU::OUTPUT_WIDTH_MARGIN,
> + IMGU::OUTPUT_HEIGHT_MARGIN)
> .boundedTo(minSize);
>
> /*
> --
> 2.32.0
>
More information about the libcamera-devel
mailing list