<div dir="ltr"><div dir="ltr">Thanks a lot for the review, Umang and Laurent.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 7, 2021 at 3:11 PM Han-lin Chen <<a href="mailto:hanlinchen@google.com">hanlinchen@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Han-Lin,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Oct 04, 2021 at 05:48:22PM +0800, Han-Lin Chen wrote:<br>
> From: hanlinchen <<a href="mailto:hanlinchen@google.com" target="_blank">hanlinchen@google.com</a>><br>
><br>
> Change the Macro STDCOPY, MEMCPY_S and CLEAR to its std version to better<br>
> suit the style. The patch also fix misusage of STDCOPY as memcpy, which<br>
> leads to copying overflown in PA and SA results.<br>
><br>
> Signed-off-by: Han-Lin Chen <<a href="mailto:hanlinchen@google.com" target="_blank">hanlinchen@google.com</a>><br>
> ---<br>
> aiq/aiq_input_parameters.cpp | 50 +++++++++-----------<br>
> aiq/aiq_results.cpp | 91 +++++++++++++++---------------------<br>
> 2 files changed, 58 insertions(+), 83 deletions(-)<br>
><br>
> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp<br>
> index 56301f6..56e8513 100644<br>
> --- a/aiq/aiq_input_parameters.cpp<br>
> +++ b/aiq/aiq_input_parameters.cpp<br>
> @@ -14,11 +14,6 @@<br>
><br>
> #include <libcamera/base/log.h><br>
><br>
> -/* Macros used by imported code */<br>
> -#define STDCOPY(dst, src, size) std::copy((src), ((src) + (size)), (dst))<br>
> -#define MEMCPY_S(dest, dmax, src, smax) memcpy((dest), (src), std::min((size_t)(dmax), (size_t)(smax)))<br>
> -#define CLEAR(x) memset(&(x), 0, sizeof(x))<br>
> -<br>
> namespace libcamera {<br>
><br>
> LOG_DEFINE_CATEGORY(AIQInputParameters)<br>
> @@ -27,26 +22,26 @@ namespace ipa::ipu3::aiq {<br>
><br>
> void AiqInputParameters::init()<br>
> {<br>
> - CLEAR(aeInputParams);<br>
> - CLEAR(afParams);<br>
> - CLEAR(afBracketParams);<br>
> - CLEAR(awbParams);<br>
> - CLEAR(gbceParams);<br>
> - CLEAR(paParams);<br>
> - CLEAR(saParams);<br>
> - CLEAR(sensorDescriptor);<br>
> - CLEAR(exposureWindow);<br>
> - CLEAR(exposureCoordinate);<br>
> - CLEAR(aeFeatures);<br>
> - CLEAR(aeManualLimits);<br>
> - CLEAR(manualFocusParams);<br>
> - CLEAR(focusRect);<br>
> - CLEAR(manualCctRange);<br>
> - CLEAR(manualWhiteCoordinate);<br>
> - CLEAR(awbResults);<br>
> - CLEAR(colorGains);<br>
> - CLEAR(exposureParams);<br>
> - CLEAR(sensorFrameParams);<br>
> + memset(&aeInputParams, 0, sizeof(aeInputParams));<br>
<br>
How about going C++ style, and doing<br>
<br>
aeInputParams = {};<br>
<br>
?<br></blockquote><div>Sounds better ;-) Will change it in the next version.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + memset(&afParams, 0, sizeof(afParams));<br>
> + memset(&afBracketParams, 0, sizeof(afBracketParams));<br>
> + memset(&awbParams, 0, sizeof(awbParams));<br>
> + memset(&gbceParams, 0, sizeof(gbceParams));<br>
> + memset(&paParams, 0, sizeof(paParams));<br>
> + memset(&saParams, 0, sizeof(saParams));<br>
> + memset(&sensorDescriptor, 0, sizeof(sensorDescriptor));<br>
> + memset(&exposureWindow, 0, sizeof(exposureWindow));<br>
> + memset(&exposureCoordinate, 0, sizeof(exposureCoordinate));<br>
> + memset(&aeFeatures, 0, sizeof(aeFeatures));<br>
> + memset(&aeManualLimits, 0, sizeof(aeManualLimits));<br>
> + memset(&manualFocusParams, 0, sizeof(manualFocusParams));<br>
> + memset(&focusRect, 0, sizeof(focusRect));<br>
> + memset(&manualCctRange, 0, sizeof(manualCctRange));<br>
> + memset(&manualWhiteCoordinate, 0, sizeof(manualWhiteCoordinate));<br>
> + memset(&awbResults, 0, sizeof(awbResults));<br>
> + memset(&colorGains, 0, sizeof(colorGains));<br>
> + memset(&exposureParams, 0, sizeof(exposureParams));<br>
> + memset(&sensorFrameParams, 0, sizeof(sensorFrameParams));<br>
> aeLock = false;<br>
> awbLock = false;<br>
> blackLevelLock = false;<br>
> @@ -102,10 +97,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe<br>
> if (this == &other)<br>
> return *this;<br>
><br>
> - MEMCPY_S(this,<br>
> - sizeof(AiqInputParameters),<br>
> - &other,<br>
> - sizeof(AiqInputParameters));<br>
> + memcpy(this, &other, sizeof(AiqInputParameters));<br>
<br>
This whole function feels very fragile, but it's not an issue introduced<br>
by this patch.<br>
<br>
> reset();<br>
><br>
> /* Exposure coordinate is nullptr in other than SPOT mode. */<br>
> diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp<br>
> index 9dda17c..f727f36 100644<br>
> --- a/aiq/aiq_results.cpp<br>
> +++ b/aiq/aiq_results.cpp<br>
> @@ -14,9 +14,6 @@<br>
><br>
> #include <libcamera/base/log.h><br>
><br>
> -/* Macros used by imported code */<br>
> -#define STDCOPY(dst, src, size) std::copy((src), ((src) + (size)), (dst))<br>
> -<br>
> namespace libcamera {<br>
><br>
> LOG_DEFINE_CATEGORY(AIQResults)<br>
> @@ -111,17 +108,14 @@ void AiqResults::setAe(ia_aiq_ae_results *ae)<br>
> ae->weight_grid->height;<br>
> gridElements = std::clamp<unsigned int>(gridElements, 1, MAX_AE_GRID_SIZE);<br>
><br>
> - STDCOPY(ae_.weight_grid->weights,<br>
> - ae->weight_grid->weights,<br>
> - gridElements * sizeof(char));<br>
> + std::copy_n(ae->weight_grid->weights, gridElements, ae_.weight_grid->weights);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying AE Weight Grids";<br>
> }<br>
><br>
> // Copy the flash info structure<br>
> if (ae_.flashes && ae->flashes) {<br>
> - STDCOPY((int8_t *)ae_.flashes, (int8_t *)ae->flashes,<br>
> - NUM_FLASH_LEDS * sizeof(ia_aiq_flash_parameters));<br>
> + std::copy_n(ae->flashes, NUM_FLASH_LEDS, ae_.flashes);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying AE Flashes";<br>
> }<br>
> @@ -172,20 +166,16 @@ void AiqResults::setGbce(ia_aiq_gbce_results *gbce)<br>
><br>
> gbce_.gamma_lut_size = gbce->gamma_lut_size;<br>
><br>
> - STDCOPY((int8_t *)gbce_.r_gamma_lut, (int8_t *)gbce->r_gamma_lut,<br>
> - gbce->gamma_lut_size * sizeof(float));<br>
> - STDCOPY((int8_t *)gbce_.b_gamma_lut, (int8_t *)gbce->b_gamma_lut,<br>
> - gbce->gamma_lut_size * sizeof(float));<br>
> - STDCOPY((int8_t *)gbce_.g_gamma_lut, (int8_t *)gbce->g_gamma_lut,<br>
> - gbce->gamma_lut_size * sizeof(float));<br>
> + std::copy_n(gbce->r_gamma_lut, gbce->gamma_lut_size, gbce_.r_gamma_lut);<br>
> + std::copy_n(gbce->b_gamma_lut, gbce->gamma_lut_size, gbce_.b_gamma_lut);<br>
> + std::copy_n(gbce->g_gamma_lut, gbce->gamma_lut_size, gbce_.g_gamma_lut);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying Gamma LUT channels";<br>
> }<br>
><br>
> if (gbce->tone_map_lut_size > 0) {<br>
> gbce_.tone_map_lut_size = gbce->tone_map_lut_size;<br>
> - STDCOPY((int8_t *)gbce_.tone_map_lut, (int8_t *)gbce->tone_map_lut,<br>
> - gbce->tone_map_lut_size * sizeof(float));<br>
> + std::copy_n(gbce->tone_map_lut, gbce->tone_map_lut_size, gbce_.tone_map_lut);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying Tone Mapping Gain LUT";<br>
> }<br>
> @@ -200,20 +190,20 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)<br>
> {<br>
> ASSERT(pa);<br>
><br>
> - STDCOPY(&pa_.color_conversion_matrix[0][0], &pa->color_conversion_matrix[0][0],<br>
> - MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX *<br>
> - sizeof(pa->color_conversion_matrix[0][0]));<br>
> + std::copy_n(&pa->color_conversion_matrix[0][0],<br>
> + MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX,<br>
> + &pa_.color_conversion_matrix[0][0]);<br>
><br>
> if (pa_.preferred_acm && pa->preferred_acm) {<br>
> pa_.preferred_acm->sector_count = pa->preferred_acm->sector_count;<br>
><br>
> - STDCOPY(pa_.preferred_acm->hue_of_sectors,<br>
> - pa->preferred_acm->hue_of_sectors,<br>
> - sizeof(*pa->preferred_acm->hue_of_sectors) * pa->preferred_acm->sector_count);<br>
> + std::copy_n(pa->preferred_acm->hue_of_sectors,<br>
> + pa->preferred_acm->sector_count,<br>
> + pa_.preferred_acm->hue_of_sectors);<br>
><br>
> - STDCOPY(pa_.preferred_acm->advanced_color_conversion_matrices[0][0],<br>
> - pa->preferred_acm->advanced_color_conversion_matrices[0][0],<br>
> - sizeof(*pa->preferred_acm->advanced_color_conversion_matrices) * pa->preferred_acm->sector_count);<br>
> + std::copy_n(pa->preferred_acm->advanced_color_conversion_matrices[0][0],<br>
> + pa->preferred_acm->sector_count,<br>
> + pa_.preferred_acm->advanced_color_conversion_matrices[0][0]);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying PA hue of sectors";<br>
> }<br>
> @@ -222,17 +212,17 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)<br>
> pa_.ir_weight->height = pa->ir_weight->height;<br>
> pa_.ir_weight->width = pa->ir_weight->width;<br>
><br>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_R,<br>
> - pa->ir_weight->ir_weight_grid_R,<br>
> - sizeof(*pa->ir_weight->ir_weight_grid_R) * pa->ir_weight->height * pa->ir_weight->width);<br>
> + std::copy_n(pa->ir_weight->ir_weight_grid_R,<br>
> + pa->ir_weight->height * pa->ir_weight->width,<br>
> + pa_.ir_weight->ir_weight_grid_R);<br>
><br>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_G,<br>
> - pa->ir_weight->ir_weight_grid_G,<br>
> - sizeof(*pa->ir_weight->ir_weight_grid_G) * pa->ir_weight->height * pa->ir_weight->width);<br>
> + std::copy_n(pa->ir_weight->ir_weight_grid_G,<br>
> + pa->ir_weight->height * pa->ir_weight->width,<br>
> + pa_.ir_weight->ir_weight_grid_G);<br>
><br>
> - STDCOPY(pa_.ir_weight->ir_weight_grid_B,<br>
> - pa->ir_weight->ir_weight_grid_B,<br>
> - sizeof(*pa->ir_weight->ir_weight_grid_B) * pa->ir_weight->height * pa->ir_weight->width);<br>
> + std::copy_n(pa->ir_weight->ir_weight_grid_B,<br>
> + pa->ir_weight->height * pa->ir_weight->width,<br>
> + pa_.ir_weight->ir_weight_grid_B);<br>
> } else {<br>
> LOG(AIQResults, Error) << "Not copying IR weight";<br>
> }<br>
> @@ -253,13 +243,13 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)<br>
> sa_.height = sa->height;<br>
> sa_.lsc_update = sa->lsc_update;<br>
><br>
> + uint32_t lscGridSize = sa_.width * sa_.height;<br>
> /* Check against one of the vectors but resize applicable to all. */<br>
> - if (channelGr_.size() < (sa_.width * sa_.height)) {<br>
> - int lscNewSize = sa_.width * sa_.height;<br>
> - channelGr_.resize(lscNewSize);<br>
> - channelGb_.resize(lscNewSize);<br>
> - channelR_.resize(lscNewSize);<br>
> - channelB_.resize(lscNewSize);<br>
> + if (channelGr_.size() < lscGridSize) {<br>
> + channelGr_.resize(lscGridSize);<br>
> + channelGb_.resize(lscGridSize);<br>
> + channelR_.resize(lscGridSize);<br>
> + channelB_.resize(lscGridSize);<br>
><br>
> /* Update the SA data pointers to new memory locations. */<br>
> sa_.channel_gr = channelGr_.data();<br>
> @@ -269,23 +259,16 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)<br>
> }<br>
><br>
> if (sa->lsc_update) {<br>
> - uint32_t memCopySize = sa->width * sa->height * sizeof(float);<br>
> -<br>
> - STDCOPY((int8_t *)sa_.channel_gr, (int8_t *)sa->channel_gb,<br>
> - memCopySize);<br>
> - STDCOPY((int8_t *)sa_.channel_gb, (int8_t *)sa->channel_gr,<br>
> - memCopySize);<br>
> - STDCOPY((int8_t *)sa_.channel_r, (int8_t *)sa->channel_r,<br>
> - memCopySize);<br>
> - STDCOPY((int8_t *)sa_.channel_b, (int8_t *)sa->channel_b,<br>
> - memCopySize);<br>
> + std::copy_n(sa->channel_gr, lscGridSize, sa_.channel_gr);<br>
> + std::copy_n(sa->channel_gb, lscGridSize, sa_.channel_gb);<br>
> + std::copy_n(sa->channel_r, lscGridSize, sa_.channel_r);<br>
> + std::copy_n(sa->channel_b, lscGridSize, sa_.channel_b);<br>
> } else {<br>
> - LOG(AIQResults, Error) << "Not copying LSC tables.";<br>
> + LOG(AIQResults, Debug) << "Not copying LSC tables.";<br>
> }<br>
><br>
> - STDCOPY(&sa_.light_source[0],<br>
> - &sa->light_source[0],<br>
> - CMC_NUM_LIGHTSOURCES * sizeof(sa->light_source[0]));<br>
> + std::copy_n(&sa->light_source[0], CMC_NUM_LIGHTSOURCES, &sa_.light_source[0]);<br>
> +<br>
> sa_.scene_difficulty = sa->scene_difficulty;<br>
> sa_.num_patches = sa->num_patches;<br>
> sa_.covered_area = sa->covered_area;<br>
<br>
--<br>
Regards,<br>
<br>
Laurent Pinchart<br>
<br>
<br>
-- <br>
Cheers.<br>
Hanlin Chen<br>
</blockquote></div></div>