[libcamera-devel] [PATCH v2 02/17] libcamera: ipu3: Centralize ImgU sizes definition
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 22 12:07:39 CEST 2021
Hello,
On Wed, Sep 08, 2021 at 11:19:37AM +0200, Jean-Michel Hautbois wrote:
> On 07/09/2021 21:40, 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>
> > ---
> > 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 {
>
> We almost always reference it as ImgU, is a fully capitalized namespace
> better ? I can't really say...
>
> namespace ImgU {
> }
> ifHeight -= ImgU::IF_ALIGN_H;
I'd go one step further (or a few steps), by moving the constants to the
ImgUDevice class instead of creating a namespace (we could rename
ImgUDevice to ImgU if the name ends up being too long). I would also
standardize on kCamelCase for constants, but maybe in a separate patch
(not necessarily part of this series).
> I will let you decide as this is very subjective I guess ?
>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>
> > +
> > +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 };
Size has a constexpr constructor.
> > +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);
> >
> > /*
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list