[libcamera-devel] [PATCH 2/3] ipu3: Change Macro migrated from Chrome OS to its std version accordingly
Hanlin Chen
hanlinchen at chromium.org
Thu Oct 7 09:18:29 CEST 2021
Thanks a lot for the review, Umang and Laurent.
On Thu, Oct 7, 2021 at 3:11 PM Han-lin Chen <hanlinchen at google.com> wrote:
> Hi Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Oct 04, 2021 at 05:48:22PM +0800, Han-Lin Chen 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.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at google.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 @@ 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);
> > + memset(&aeInputParams, 0, sizeof(aeInputParams));
>
> How about going C++ style, and doing
>
> aeInputParams = {};
>
> ?
>
Sounds better ;-) Will change it in the next version.
>
> > + 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));
>
> This whole function feels very fragile, but it's not an issue introduced
> by this patch.
>
> > 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);
> > } 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]);
> > } 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;
>
> --
> Regards,
>
> Laurent Pinchart
>
>
> --
> Cheers.
> Hanlin Chen
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211007/ee9b2199/attachment-0001.htm>
More information about the libcamera-devel
mailing list