| Message ID | 20210907194107.803730-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@jmondi.org> Reviewed-by: Paul Elder <paul.elder@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 >
Hi Jacopo, Thanks for your patch ! 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@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 will let you decide as this is very subjective I guess ? Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 }; > +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); > > /* >
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@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@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); > > > > /*
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); /*
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@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(-)