[libcamera-devel] [PATCH 2/3] ipu3: Change Macro migrated from Chrome OS to its std version accordingly
Umang Jain
umang.jain at ideasonboard.com
Tue Oct 5 07:26:30 CEST 2021
Hi Han-Lin
Thank you for the patch.
On 10/4/21 3:18 PM, Han-Lin Chen via libcamera-devel wrote:
> From: hanlinchen<hanlinchen at google.com>
>
> Change the Macro STDCOPY, MEMCPY_S and CLEAR to its std version to better
> suit the style. The patch also fix misusage of STDCOPY as memcpy, which
> leads to copying overflown in PA and SA results.
Oh yes, I see that now.
>
> Signed-off-by: Han-Lin Chen<hanlinchen at google.com>
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> aiq/aiq_input_parameters.cpp | 50 +++++++++-----------
> aiq/aiq_results.cpp | 91 +++++++++++++++---------------------
> 2 files changed, 58 insertions(+), 83 deletions(-)
>
> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp
> index 56301f6..56e8513 100644
> --- a/aiq/aiq_input_parameters.cpp
> +++ b/aiq/aiq_input_parameters.cpp
> @@ -14,11 +14,6 @@
>
> #include <libcamera/base/log.h>
>
> -/* Macros used by imported code */
> -#define STDCOPY(dst, src, size)std::copy((src), ((src) + (size)), (dst))
> -#define MEMCPY_S(dest, dmax, src, smax) memcpy((dest), (src),std::min((size_t)(dmax), (size_t)(smax)))
> -#define CLEAR(x) memset(&(x), 0, sizeof(x))
> -
> namespace libcamera {
>
> LOG_DEFINE_CATEGORY(AIQInputParameters)
> @@ -27,26 +22,26 @@ namespaceipa::ipu3::aiq {
>
> voidAiqInputParameters::init()
> {
> - CLEAR(aeInputParams);
> - CLEAR(afParams);
> - CLEAR(afBracketParams);
> - CLEAR(awbParams);
> - CLEAR(gbceParams);
> - CLEAR(paParams);
> - CLEAR(saParams);
> - CLEAR(sensorDescriptor);
> - CLEAR(exposureWindow);
> - CLEAR(exposureCoordinate);
> - CLEAR(aeFeatures);
> - CLEAR(aeManualLimits);
> - CLEAR(manualFocusParams);
> - CLEAR(focusRect);
> - CLEAR(manualCctRange);
> - CLEAR(manualWhiteCoordinate);
> - CLEAR(awbResults);
> - CLEAR(colorGains);
> - CLEAR(exposureParams);
> - CLEAR(sensorFrameParams);
> + memset(&aeInputParams, 0, sizeof(aeInputParams));
> + memset(&afParams, 0, sizeof(afParams));
> + memset(&afBracketParams, 0, sizeof(afBracketParams));
> + memset(&awbParams, 0, sizeof(awbParams));
> + memset(&gbceParams, 0, sizeof(gbceParams));
> + memset(&paParams, 0, sizeof(paParams));
> + memset(&saParams, 0, sizeof(saParams));
> + memset(&sensorDescriptor, 0, sizeof(sensorDescriptor));
> + memset(&exposureWindow, 0, sizeof(exposureWindow));
> + memset(&exposureCoordinate, 0, sizeof(exposureCoordinate));
> + memset(&aeFeatures, 0, sizeof(aeFeatures));
> + memset(&aeManualLimits, 0, sizeof(aeManualLimits));
> + memset(&manualFocusParams, 0, sizeof(manualFocusParams));
> + memset(&focusRect, 0, sizeof(focusRect));
> + memset(&manualCctRange, 0, sizeof(manualCctRange));
> + memset(&manualWhiteCoordinate, 0, sizeof(manualWhiteCoordinate));
> + memset(&awbResults, 0, sizeof(awbResults));
> + memset(&colorGains, 0, sizeof(colorGains));
> + memset(&exposureParams, 0, sizeof(exposureParams));
> + memset(&sensorFrameParams, 0, sizeof(sensorFrameParams));
> aeLock = false;
> awbLock = false;
> blackLevelLock = false;
> @@ -102,10 +97,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe
> if (this == &other)
> return *this;
>
> - MEMCPY_S(this,
> - sizeof(AiqInputParameters),
> - &other,
> - sizeof(AiqInputParameters));
> + memcpy(this, &other, sizeof(AiqInputParameters));
> reset();
>
> /* Exposure coordinate is nullptr in other than SPOT mode. */
> diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp
> index 9dda17c..f727f36 100644
> --- a/aiq/aiq_results.cpp
> +++ b/aiq/aiq_results.cpp
> @@ -14,9 +14,6 @@
>
> #include <libcamera/base/log.h>
>
> -/* Macros used by imported code */
> -#define STDCOPY(dst, src, size)std::copy((src), ((src) + (size)), (dst))
> -
> namespace libcamera {
>
> LOG_DEFINE_CATEGORY(AIQResults)
> @@ -111,17 +108,14 @@ voidAiqResults::setAe(ia_aiq_ae_results *ae)
> ae->weight_grid->height;
> gridElements =std::clamp<unsigned int>(gridElements, 1, MAX_AE_GRID_SIZE);
>
> - STDCOPY(ae_.weight_grid->weights,
> - ae->weight_grid->weights,
> - gridElements * sizeof(char));
> + std::copy_n(ae->weight_grid->weights, gridElements, ae_.weight_grid->weights);
> } else {
> LOG(AIQResults, Error) << "Not copying AE Weight Grids";
> }
>
> // Copy the flash info structure
> if (ae_.flashes && ae->flashes) {
> - STDCOPY((int8_t *)ae_.flashes, (int8_t *)ae->flashes,
> - NUM_FLASH_LEDS * sizeof(ia_aiq_flash_parameters));
> + std::copy_n(ae->flashes, NUM_FLASH_LEDS, ae_.flashes);
> } else {
> LOG(AIQResults, Error) << "Not copying AE Flashes";
> }
> @@ -172,20 +166,16 @@ voidAiqResults::setGbce(ia_aiq_gbce_results *gbce)
>
> gbce_.gamma_lut_size = gbce->gamma_lut_size;
>
> - STDCOPY((int8_t *)gbce_.r_gamma_lut, (int8_t *)gbce->r_gamma_lut,
> - gbce->gamma_lut_size * sizeof(float));
> - STDCOPY((int8_t *)gbce_.b_gamma_lut, (int8_t *)gbce->b_gamma_lut,
> - gbce->gamma_lut_size * sizeof(float));
> - STDCOPY((int8_t *)gbce_.g_gamma_lut, (int8_t *)gbce->g_gamma_lut,
> - gbce->gamma_lut_size * sizeof(float));
> + std::copy_n(gbce->r_gamma_lut, gbce->gamma_lut_size, gbce_.r_gamma_lut);
> + std::copy_n(gbce->b_gamma_lut, gbce->gamma_lut_size, gbce_.b_gamma_lut);
> + std::copy_n(gbce->g_gamma_lut, gbce->gamma_lut_size, gbce_.g_gamma_lut);
> } else {
> LOG(AIQResults, Error) << "Not copying Gamma LUT channels";
> }
>
> if (gbce->tone_map_lut_size > 0) {
> gbce_.tone_map_lut_size = gbce->tone_map_lut_size;
> - STDCOPY((int8_t *)gbce_.tone_map_lut, (int8_t *)gbce->tone_map_lut,
> - gbce->tone_map_lut_size * sizeof(float));
> + std::copy_n(gbce->tone_map_lut, gbce->tone_map_lut_size, gbce_.tone_map_lut);
> } else {
> LOG(AIQResults, Error) << "Not copying Tone Mapping Gain LUT";
> }
> @@ -200,20 +190,20 @@ voidAiqResults::setPa(ia_aiq_pa_results *pa)
> {
> ASSERT(pa);
>
> - STDCOPY(&pa_.color_conversion_matrix[0][0], &pa->color_conversion_matrix[0][0],
> - MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX *
> - sizeof(pa->color_conversion_matrix[0][0]));
> + std::copy_n(&pa->color_conversion_matrix[0][0],
> + MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX,
> + &pa_.color_conversion_matrix[0][0]);
>
> if (pa_.preferred_acm && pa->preferred_acm) {
> pa_.preferred_acm->sector_count = pa->preferred_acm->sector_count;
>
> - STDCOPY(pa_.preferred_acm->hue_of_sectors,
> - pa->preferred_acm->hue_of_sectors,
> - sizeof(*pa->preferred_acm->hue_of_sectors) * pa->preferred_acm->sector_count);
> + std::copy_n(pa->preferred_acm->hue_of_sectors,
> + pa->preferred_acm->sector_count,
> + pa_.preferred_acm->hue_of_sectors);
>
> - STDCOPY(pa_.preferred_acm->advanced_color_conversion_matrices[0][0],
> - pa->preferred_acm->advanced_color_conversion_matrices[0][0],
> - sizeof(*pa->preferred_acm->advanced_color_conversion_matrices) * pa->preferred_acm->sector_count);
> + std::copy_n(pa->preferred_acm->advanced_color_conversion_matrices[0][0],
> + pa->preferred_acm->sector_count,
> + pa_.preferred_acm->advanced_color_conversion_matrices[0][0]);
> } else {
> LOG(AIQResults, Error) << "Not copying PA hue of sectors";
> }
> @@ -222,17 +212,17 @@ voidAiqResults::setPa(ia_aiq_pa_results *pa)
> pa_.ir_weight->height = pa->ir_weight->height;
> pa_.ir_weight->width = pa->ir_weight->width;
>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_R,
> - pa->ir_weight->ir_weight_grid_R,
> - sizeof(*pa->ir_weight->ir_weight_grid_R) * pa->ir_weight->height * pa->ir_weight->width);
> + std::copy_n(pa->ir_weight->ir_weight_grid_R,
> + pa->ir_weight->height * pa->ir_weight->width,
> + pa_.ir_weight->ir_weight_grid_R);
>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_G,
> - pa->ir_weight->ir_weight_grid_G,
> - sizeof(*pa->ir_weight->ir_weight_grid_G) * pa->ir_weight->height * pa->ir_weight->width);
> + std::copy_n(pa->ir_weight->ir_weight_grid_G,
> + pa->ir_weight->height * pa->ir_weight->width,
> + pa_.ir_weight->ir_weight_grid_G);
>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_B,
> - pa->ir_weight->ir_weight_grid_B,
> - sizeof(*pa->ir_weight->ir_weight_grid_B) * pa->ir_weight->height * pa->ir_weight->width);
> + std::copy_n(pa->ir_weight->ir_weight_grid_B,
> + pa->ir_weight->height * pa->ir_weight->width,
> + pa_.ir_weight->ir_weight_grid_B);
> } else {
> LOG(AIQResults, Error) << "Not copying IR weight";
> }
> @@ -253,13 +243,13 @@ voidAiqResults::setSa(ia_aiq_sa_results *sa)
> sa_.height = sa->height;
> sa_.lsc_update = sa->lsc_update;
>
> + uint32_t lscGridSize = sa_.width * sa_.height;
> /* Check against one of the vectors but resize applicable to all. */
> - if (channelGr_.size() < (sa_.width * sa_.height)) {
> - int lscNewSize = sa_.width * sa_.height;
> - channelGr_.resize(lscNewSize);
> - channelGb_.resize(lscNewSize);
> - channelR_.resize(lscNewSize);
> - channelB_.resize(lscNewSize);
> + if (channelGr_.size() < lscGridSize) {
> + channelGr_.resize(lscGridSize);
> + channelGb_.resize(lscGridSize);
> + channelR_.resize(lscGridSize);
> + channelB_.resize(lscGridSize);
>
> /* Update the SA data pointers to new memory locations. */
> sa_.channel_gr = channelGr_.data();
> @@ -269,23 +259,16 @@ voidAiqResults::setSa(ia_aiq_sa_results *sa)
> }
>
> if (sa->lsc_update) {
> - uint32_t memCopySize = sa->width * sa->height * sizeof(float);
> -
> - STDCOPY((int8_t *)sa_.channel_gr, (int8_t *)sa->channel_gb,
> - memCopySize);
> - STDCOPY((int8_t *)sa_.channel_gb, (int8_t *)sa->channel_gr,
> - memCopySize);
> - STDCOPY((int8_t *)sa_.channel_r, (int8_t *)sa->channel_r,
> - memCopySize);
> - STDCOPY((int8_t *)sa_.channel_b, (int8_t *)sa->channel_b,
> - memCopySize);
> + std::copy_n(sa->channel_gr, lscGridSize, sa_.channel_gr);
> + std::copy_n(sa->channel_gb, lscGridSize, sa_.channel_gb);
> + std::copy_n(sa->channel_r, lscGridSize, sa_.channel_r);
> + std::copy_n(sa->channel_b, lscGridSize, sa_.channel_b);
> } else {
> - LOG(AIQResults, Error) << "Not copying LSC tables.";
> + LOG(AIQResults, Debug) << "Not copying LSC tables.";
> }
>
> - STDCOPY(&sa_.light_source[0],
> - &sa->light_source[0],
> - CMC_NUM_LIGHTSOURCES * sizeof(sa->light_source[0]));
> + std::copy_n(&sa->light_source[0], CMC_NUM_LIGHTSOURCES, &sa_.light_source[0]);
> +
> sa_.scene_difficulty = sa->scene_difficulty;
> sa_.num_patches = sa->num_patches;
> sa_.covered_area = sa->covered_area;
> -- 2.33.0.800.g4c38ced690-goog
More information about the libcamera-devel
mailing list