[{"id":20047,"web_url":"https://patchwork.libcamera.org/comment/20047/","msgid":"<17daf114-be4d-5813-c632-540a5517bcfb@ideasonboard.com>","date":"2021-10-05T05:26:30","subject":"Re: [libcamera-devel] [PATCH 2/3] ipu3: Change Macro migrated from\n\tChrome OS to its std version accordingly","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Han-Lin\n\nThank you for the patch.\n\nOn 10/4/21 3:18 PM, Han-Lin Chen via libcamera-devel wrote:\n> From: hanlinchen\n>\n> Change the Macro STDCOPY, MEMCPY_S and CLEAR to its std version to better\n> suit the style. The patch also fix misusage of STDCOPY as memcpy, which\n> leads to copying overflown in PA and SA results.\n\n\nOh yes, I see that now.\n\n>\n> Signed-off-by: Han-Lin Chen\n\n\nReviewed-by: Umang Jain \n> ---\n> aiq/aiq_input_parameters.cpp | 50 +++++++++-----------\n> aiq/aiq_results.cpp | 91 +++++++++++++++---------------------\n> 2 files changed, 58 insertions(+), 83 deletions(-)\n>\n> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> index 56301f6..56e8513 100644\n> --- a/aiq/aiq_input_parameters.cpp\n> +++ b/aiq/aiq_input_parameters.cpp\n> @@ -14,11 +14,6 @@\n> \n> #include \n> \n> -/* Macros used by imported code */\n> -#define STDCOPY(dst, src, size)std::copy((src), ((src) + (size)), (dst))\n> -#define MEMCPY_S(dest, dmax, src, smax) memcpy((dest), (src),std::min((size_t)(dmax), (size_t)(smax)))\n> -#define CLEAR(x) memset(&(x), 0, sizeof(x))\n> -\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(AIQInputParameters)\n> @@ -27,26 +22,26 @@ namespaceipa::ipu3::aiq {\n> \n> voidAiqInputParameters::init()\n> {\n> -\tCLEAR(aeInputParams);\n> -\tCLEAR(afParams);\n> -\tCLEAR(afBracketParams);\n> -\tCLEAR(awbParams);\n> -\tCLEAR(gbceParams);\n> -\tCLEAR(paParams);\n> -\tCLEAR(saParams);\n> -\tCLEAR(sensorDescriptor);\n> -\tCLEAR(exposureWindow);\n> -\tCLEAR(exposureCoordinate);\n> -\tCLEAR(aeFeatures);\n> -\tCLEAR(aeManualLimits);\n> -\tCLEAR(manualFocusParams);\n> -\tCLEAR(focusRect);\n> -\tCLEAR(manualCctRange);\n> -\tCLEAR(manualWhiteCoordinate);\n> -\tCLEAR(awbResults);\n> -\tCLEAR(colorGains);\n> -\tCLEAR(exposureParams);\n> -\tCLEAR(sensorFrameParams);\n> +\tmemset(&aeInputParams, 0, sizeof(aeInputParams));\n> +\tmemset(&afParams, 0, sizeof(afParams));\n> +\tmemset(&afBracketParams, 0, sizeof(afBracketParams));\n> +\tmemset(&awbParams, 0, sizeof(awbParams));\n> +\tmemset(&gbceParams, 0, sizeof(gbceParams));\n> +\tmemset(&paParams, 0, sizeof(paParams));\n> +\tmemset(&saParams, 0, sizeof(saParams));\n> +\tmemset(&sensorDescriptor, 0, sizeof(sensorDescriptor));\n> +\tmemset(&exposureWindow, 0, sizeof(exposureWindow));\n> +\tmemset(&exposureCoordinate, 0, sizeof(exposureCoordinate));\n> +\tmemset(&aeFeatures, 0, sizeof(aeFeatures));\n> +\tmemset(&aeManualLimits, 0, sizeof(aeManualLimits));\n> +\tmemset(&manualFocusParams, 0, sizeof(manualFocusParams));\n> +\tmemset(&focusRect, 0, sizeof(focusRect));\n> +\tmemset(&manualCctRange, 0, sizeof(manualCctRange));\n> +\tmemset(&manualWhiteCoordinate, 0, sizeof(manualWhiteCoordinate));\n> +\tmemset(&awbResults, 0, sizeof(awbResults));\n> +\tmemset(&colorGains, 0, sizeof(colorGains));\n> +\tmemset(&exposureParams, 0, sizeof(exposureParams));\n> +\tmemset(&sensorFrameParams, 0, sizeof(sensorFrameParams));\n> \taeLock = false;\n> \tawbLock = false;\n> \tblackLevelLock = false;\n> @@ -102,10 +97,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n> \tif (this == &other)\n> \t\treturn *this;\n> \n> -\tMEMCPY_S(this,\n> -\t\t sizeof(AiqInputParameters),\n> -\t\t &other,\n> -\t\t sizeof(AiqInputParameters));\n> +\tmemcpy(this, &other, sizeof(AiqInputParameters));\n> \treset();\n> \n> \t/* Exposure coordinate is nullptr in other than SPOT mode. */\n> diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp\n> index 9dda17c..f727f36 100644\n> --- a/aiq/aiq_results.cpp\n> +++ b/aiq/aiq_results.cpp\n> @@ -14,9 +14,6 @@\n> \n> #include \n> \n> -/* Macros used by imported code */\n> -#define STDCOPY(dst, src, size)std::copy((src), ((src) + (size)), (dst))\n> -\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(AIQResults)\n> @@ -111,17 +108,14 @@ voidAiqResults::setAe(ia_aiq_ae_results *ae)\n> \t\t\t\t\t ae->weight_grid->height;\n> \t\tgridElements =std::clamp(gridElements, 1, MAX_AE_GRID_SIZE);\n> \n> -\t\tSTDCOPY(ae_.weight_grid->weights,\n> -\t\t\tae->weight_grid->weights,\n> -\t\t\tgridElements * sizeof(char));\n> +\t\tstd::copy_n(ae->weight_grid->weights, gridElements, ae_.weight_grid->weights);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying AE Weight Grids\";\n> \t}\n> \n> \t// Copy the flash info structure\n> \tif (ae_.flashes && ae->flashes) {\n> -\t\tSTDCOPY((int8_t *)ae_.flashes, (int8_t *)ae->flashes,\n> -\t\t\tNUM_FLASH_LEDS * sizeof(ia_aiq_flash_parameters));\n> +\t\tstd::copy_n(ae->flashes, NUM_FLASH_LEDS, ae_.flashes);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying AE Flashes\";\n> \t}\n> @@ -172,20 +166,16 @@ voidAiqResults::setGbce(ia_aiq_gbce_results *gbce)\n> \n> \t\tgbce_.gamma_lut_size = gbce->gamma_lut_size;\n> \n> -\t\tSTDCOPY((int8_t *)gbce_.r_gamma_lut, (int8_t *)gbce->r_gamma_lut,\n> -\t\t\tgbce->gamma_lut_size * sizeof(float));\n> -\t\tSTDCOPY((int8_t *)gbce_.b_gamma_lut, (int8_t *)gbce->b_gamma_lut,\n> -\t\t\tgbce->gamma_lut_size * sizeof(float));\n> -\t\tSTDCOPY((int8_t *)gbce_.g_gamma_lut, (int8_t *)gbce->g_gamma_lut,\n> -\t\t\tgbce->gamma_lut_size * sizeof(float));\n> +\t\tstd::copy_n(gbce->r_gamma_lut, gbce->gamma_lut_size, gbce_.r_gamma_lut);\n> +\t\tstd::copy_n(gbce->b_gamma_lut, gbce->gamma_lut_size, gbce_.b_gamma_lut);\n> +\t\tstd::copy_n(gbce->g_gamma_lut, gbce->gamma_lut_size, gbce_.g_gamma_lut);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying Gamma LUT channels\";\n> \t}\n> \n> \tif (gbce->tone_map_lut_size > 0) {\n> \t\tgbce_.tone_map_lut_size = gbce->tone_map_lut_size;\n> -\t\tSTDCOPY((int8_t *)gbce_.tone_map_lut, (int8_t *)gbce->tone_map_lut,\n> -\t\t\tgbce->tone_map_lut_size * sizeof(float));\n> +\t\tstd::copy_n(gbce->tone_map_lut, gbce->tone_map_lut_size, gbce_.tone_map_lut);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying Tone Mapping Gain LUT\";\n> \t}\n> @@ -200,20 +190,20 @@ voidAiqResults::setPa(ia_aiq_pa_results *pa)\n> {\n> \tASSERT(pa);\n> \n> -\tSTDCOPY(&pa_.color_conversion_matrix[0][0], &pa->color_conversion_matrix[0][0],\n> -\t\tMAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX *\n> -\t\t\tsizeof(pa->color_conversion_matrix[0][0]));\n> +\tstd::copy_n(&pa->color_conversion_matrix[0][0],\n> +\t\t\tMAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX,\n> +\t\t\t&pa_.color_conversion_matrix[0][0]);\n> \n> \tif (pa_.preferred_acm && pa->preferred_acm) {\n> \t\tpa_.preferred_acm->sector_count = pa->preferred_acm->sector_count;\n> \n> -\t\tSTDCOPY(pa_.preferred_acm->hue_of_sectors,\n> -\t\t\tpa->preferred_acm->hue_of_sectors,\n> -\t\t\tsizeof(*pa->preferred_acm->hue_of_sectors) * pa->preferred_acm->sector_count);\n> +\t\tstd::copy_n(pa->preferred_acm->hue_of_sectors,\n> +\t\t\tpa->preferred_acm->sector_count,\n> +\t\t\tpa_.preferred_acm->hue_of_sectors);\n> \n> -\t\tSTDCOPY(pa_.preferred_acm->advanced_color_conversion_matrices[0][0],\n> -\t\t\tpa->preferred_acm->advanced_color_conversion_matrices[0][0],\n> -\t\t\tsizeof(*pa->preferred_acm->advanced_color_conversion_matrices) * pa->preferred_acm->sector_count);\n> +\t\tstd::copy_n(pa->preferred_acm->advanced_color_conversion_matrices[0][0],\n> +\t\t\tpa->preferred_acm->sector_count,\n> +\t\t\tpa_.preferred_acm->advanced_color_conversion_matrices[0][0]);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying PA hue of sectors\";\n> \t}\n> @@ -222,17 +212,17 @@ voidAiqResults::setPa(ia_aiq_pa_results *pa)\n> \t\tpa_.ir_weight->height = pa->ir_weight->height;\n> \t\tpa_.ir_weight->width = pa->ir_weight->width;\n> \n> -\t\tSTDCOPY(pa_.ir_weight->ir_weight_grid_R,\n> -\t\t\tpa->ir_weight->ir_weight_grid_R,\n> -\t\t\tsizeof(*pa->ir_weight->ir_weight_grid_R) * pa->ir_weight->height * pa->ir_weight->width);\n> +\t\tstd::copy_n(pa->ir_weight->ir_weight_grid_R,\n> +\t\t\tpa->ir_weight->height * pa->ir_weight->width,\n> +\t\t\tpa_.ir_weight->ir_weight_grid_R);\n> \n> -\t\tSTDCOPY(pa_.ir_weight->ir_weight_grid_G,\n> -\t\t\tpa->ir_weight->ir_weight_grid_G,\n> -\t\t\tsizeof(*pa->ir_weight->ir_weight_grid_G) * pa->ir_weight->height * pa->ir_weight->width);\n> +\t\tstd::copy_n(pa->ir_weight->ir_weight_grid_G,\n> +\t\t\tpa->ir_weight->height * pa->ir_weight->width,\n> +\t\t\tpa_.ir_weight->ir_weight_grid_G);\n> \n> -\t\tSTDCOPY(pa_.ir_weight->ir_weight_grid_B,\n> -\t\t\tpa->ir_weight->ir_weight_grid_B,\n> -\t\t\tsizeof(*pa->ir_weight->ir_weight_grid_B) * pa->ir_weight->height * pa->ir_weight->width);\n> +\t\tstd::copy_n(pa->ir_weight->ir_weight_grid_B,\n> +\t\t\tpa->ir_weight->height * pa->ir_weight->width,\n> +\t\t\tpa_.ir_weight->ir_weight_grid_B);\n> \t} else {\n> \t\tLOG(AIQResults, Error) << \"Not copying IR weight\";\n> \t}\n> @@ -253,13 +243,13 @@ voidAiqResults::setSa(ia_aiq_sa_results *sa)\n> \tsa_.height = sa->height;\n> \tsa_.lsc_update = sa->lsc_update;\n> \n> +\tuint32_t lscGridSize = sa_.width * sa_.height;\n> \t/* Check against one of the vectors but resize applicable to all. */\n> -\tif (channelGr_.size() < (sa_.width * sa_.height)) {\n> -\t\tint lscNewSize = sa_.width * sa_.height;\n> -\t\tchannelGr_.resize(lscNewSize);\n> -\t\tchannelGb_.resize(lscNewSize);\n> -\t\tchannelR_.resize(lscNewSize);\n> -\t\tchannelB_.resize(lscNewSize);\n> +\tif (channelGr_.size() < lscGridSize) {\n> +\t\tchannelGr_.resize(lscGridSize);\n> +\t\tchannelGb_.resize(lscGridSize);\n> +\t\tchannelR_.resize(lscGridSize);\n> +\t\tchannelB_.resize(lscGridSize);\n> \n> \t\t/* Update the SA data pointers to new memory locations. */\n> \t\tsa_.channel_gr = channelGr_.data();\n> @@ -269,23 +259,16 @@ voidAiqResults::setSa(ia_aiq_sa_results *sa)\n> \t}\n> \n> \tif (sa->lsc_update) {\n> -\t\tuint32_t memCopySize = sa->width * sa->height * sizeof(float);\n> -\n> -\t\tSTDCOPY((int8_t *)sa_.channel_gr, (int8_t *)sa->channel_gb,\n> -\t\t\tmemCopySize);\n> -\t\tSTDCOPY((int8_t *)sa_.channel_gb, (int8_t *)sa->channel_gr,\n> -\t\t\tmemCopySize);\n> -\t\tSTDCOPY((int8_t *)sa_.channel_r, (int8_t *)sa->channel_r,\n> -\t\t\tmemCopySize);\n> -\t\tSTDCOPY((int8_t *)sa_.channel_b, (int8_t *)sa->channel_b,\n> -\t\t\tmemCopySize);\n> +\t\tstd::copy_n(sa->channel_gr, lscGridSize, sa_.channel_gr);\n> +\t\tstd::copy_n(sa->channel_gb, lscGridSize, sa_.channel_gb);\n> +\t\tstd::copy_n(sa->channel_r, lscGridSize, sa_.channel_r);\n> +\t\tstd::copy_n(sa->channel_b, lscGridSize, sa_.channel_b);\n> \t} else {\n> -\t\tLOG(AIQResults, Error) << \"Not copying LSC tables.\";\n> +\t\tLOG(AIQResults, Debug) << \"Not copying LSC tables.\";\n> \t}\n> \n> -\tSTDCOPY(&sa_.light_source[0],\n> -\t\t&sa->light_source[0],\n> -\t\tCMC_NUM_LIGHTSOURCES * sizeof(sa->light_source[0]));\n> +\tstd::copy_n(&sa->light_source[0], CMC_NUM_LIGHTSOURCES, &sa_.light_source[0]);\n> +\n> \tsa_.scene_difficulty = sa->scene_difficulty;\n> \tsa_.num_patches = sa->num_patches;\n> \tsa_.covered_area = sa->covered_area;\n> -- 2.33.0.800.g4c38ced690-goog","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ABB8BBDC71\n\tfor ;\n\tTue, 5 Oct 2021 05:26:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A6496023D;\n\tTue, 5 Oct 2021 07:26:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 494E76023D\n\tfor ;\n\tTue, 5 Oct 2021 07:26:35 +0200 (CEST)","from [192.168.1.106] (unknown [103.238.109.2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E25225B;\n\tTue, 5 Oct 2021 07:26:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NrD2E+kk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633411595;\n\tbh=dm6cs/NOWZE4KNtTpSeiUF9xyAk8C7aDHRSLThFLyT4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=NrD2E+kkTlG8fFAlOXAgFkcMC47+fmSjWh29/BFVLUnprih9LKxR9mktYoS2UNdQM\n\tf++kMCXXRWadFkXKyc3i6GkXT2sTKLgZg7h0YHbTT9/tA0anhrHnGCVmEr4BoLcniq\n\tSg314AtYqqb5CVxG8YGlkymrqST+MwLWVUt4WVu8=","To":"Han-Lin Chen , libcamera-devel@lists.libcamera.org","References":"<20211004094823.260789-1-hanlinchen@google.com>\n\t","From":"Umang Jain ","Message-ID":"<17daf114-be4d-5813-c632-540a5517bcfb@ideasonboard.com>","Date":"Tue, 5 Oct 2021 10:56:30 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/3] ipu3: Change Macro migrated from\n\tChrome OS to its std version accordingly","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]