[libcamera-devel,v2,2/3] ipu3: Change Macro migrated from Chrome OS to its std version accordingly

14068 diff mbox series

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.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 aiq/aiq_input_parameters.cpp | 50 +++++++++-----------
 aiq/aiq_results.cpp          | 91 +++++++++++++++---------------------
 2 files changed, 58 insertions(+), 83 deletions(-)

Quoting Han-Lin Chen (2021-10-07 08:21:46)
> 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.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Reviewed-by: Umang Jain <umang.jain@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..8a53849 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))
> -

I'm very happy to see these cleaned up.

>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(AIQInputParameters)
> @@ -27,26 +22,26 @@ namespace ipa::ipu3::aiq {
>  
>  void AiqInputParameters::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);
> +       aeInputParams = {};
> +       afParams = {};
> +       afBracketParams = {};
> +       awbParams = {};
> +       gbceParams = {};
> +       paParams = {};
> +       saParams = {};
> +       sensorDescriptor = {};
> +       exposureWindow = {};
> +       exposureCoordinate = {};
> +       aeFeatures = {};
> +       aeManualLimits = {};
> +       manualFocusParams = {};
> +       focusRect = {};
> +       manualCctRange = {};
> +       manualWhiteCoordinate = {};
> +       awbResults = {};
> +       colorGains = {};
> +       exposureParams = {};
> +       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 @@ void AiqResults::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 @@ void AiqResults::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);

These look so much better ;-)

Odd that they are ordered R B G rather than R G B ... but I don't think
that matters too much.



>         } 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 @@ void AiqResults::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]);

Indentation seems weird here again, but perhaps we should define a
.clang-format file and go through the formatting in one big fix.

So I'm not overly worried about the code style right now.


All the changes I've examined seem to be correct. I hope I didn't miss
anything critical, but as the testing shows this fixes things I'm happy.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         } else {
>                 LOG(AIQResults, Error) << "Not copying PA hue of sectors";
>         }
> @@ -222,17 +212,17 @@ void AiqResults::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 @@ void AiqResults::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 @@ void AiqResults::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.882.g93a45727a2-goog
>