[{"id":20142,"web_url":"https://patchwork.libcamera.org/comment/20142/","msgid":"<163403976410.2943484.8418225803690797783@Monstersaurus>","date":"2021-10-12T11:56:04","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipu3: Change Macro migrated\n\tfrom Chrome OS to its std version accordingly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Han-Lin Chen (2021-10-07 08:21:46)\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> Signed-off-by: Han-Lin Chen \n> Reviewed-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..8a53849 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\nI'm very happy to see these cleaned up.\n\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(AIQInputParameters)\n> @@ -27,26 +22,26 @@ namespace ipa::ipu3::aiq {\n> \n> void AiqInputParameters::init()\n> {\n> - CLEAR(aeInputParams);\n> - CLEAR(afParams);\n> - CLEAR(afBracketParams);\n> - CLEAR(awbParams);\n> - CLEAR(gbceParams);\n> - CLEAR(paParams);\n> - CLEAR(saParams);\n> - CLEAR(sensorDescriptor);\n> - CLEAR(exposureWindow);\n> - CLEAR(exposureCoordinate);\n> - CLEAR(aeFeatures);\n> - CLEAR(aeManualLimits);\n> - CLEAR(manualFocusParams);\n> - CLEAR(focusRect);\n> - CLEAR(manualCctRange);\n> - CLEAR(manualWhiteCoordinate);\n> - CLEAR(awbResults);\n> - CLEAR(colorGains);\n> - CLEAR(exposureParams);\n> - CLEAR(sensorFrameParams);\n> + aeInputParams = {};\n> + afParams = {};\n> + afBracketParams = {};\n> + awbParams = {};\n> + gbceParams = {};\n> + paParams = {};\n> + saParams = {};\n> + sensorDescriptor = {};\n> + exposureWindow = {};\n> + exposureCoordinate = {};\n> + aeFeatures = {};\n> + aeManualLimits = {};\n> + manualFocusParams = {};\n> + focusRect = {};\n> + manualCctRange = {};\n> + manualWhiteCoordinate = {};\n> + awbResults = {};\n> + colorGains = {};\n> + exposureParams = {};\n> + sensorFrameParams = {};\n> aeLock = false;\n> awbLock = false;\n> blackLevelLock = false;\n> @@ -102,10 +97,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n> if (this == &other)\n> return *this;\n> \n> - MEMCPY_S(this,\n> - sizeof(AiqInputParameters),\n> - &other,\n> - sizeof(AiqInputParameters));\n> + memcpy(this, &other, sizeof(AiqInputParameters));\n> reset();\n> \n> /* 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 @@ void AiqResults::setAe(ia_aiq_ae_results *ae)\n> ae->weight_grid->height;\n> gridElements = std::clamp(gridElements, 1, MAX_AE_GRID_SIZE);\n> \n> - STDCOPY(ae_.weight_grid->weights,\n> - ae->weight_grid->weights,\n> - gridElements * sizeof(char));\n> + std::copy_n(ae->weight_grid->weights, gridElements, ae_.weight_grid->weights);\n> } else {\n> LOG(AIQResults, Error) << \"Not copying AE Weight Grids\";\n> }\n> \n> // Copy the flash info structure\n> if (ae_.flashes && ae->flashes) {\n> - STDCOPY((int8_t *)ae_.flashes, (int8_t *)ae->flashes,\n> - NUM_FLASH_LEDS * sizeof(ia_aiq_flash_parameters));\n> + std::copy_n(ae->flashes, NUM_FLASH_LEDS, ae_.flashes);\n> } else {\n> LOG(AIQResults, Error) << \"Not copying AE Flashes\";\n> }\n> @@ -172,20 +166,16 @@ void AiqResults::setGbce(ia_aiq_gbce_results *gbce)\n> \n> gbce_.gamma_lut_size = gbce->gamma_lut_size;\n> \n> - STDCOPY((int8_t *)gbce_.r_gamma_lut, (int8_t *)gbce->r_gamma_lut,\n> - gbce->gamma_lut_size * sizeof(float));\n> - STDCOPY((int8_t *)gbce_.b_gamma_lut, (int8_t *)gbce->b_gamma_lut,\n> - gbce->gamma_lut_size * sizeof(float));\n> - STDCOPY((int8_t *)gbce_.g_gamma_lut, (int8_t *)gbce->g_gamma_lut,\n> - gbce->gamma_lut_size * sizeof(float));\n> + std::copy_n(gbce->r_gamma_lut, gbce->gamma_lut_size, gbce_.r_gamma_lut);\n> + std::copy_n(gbce->b_gamma_lut, gbce->gamma_lut_size, gbce_.b_gamma_lut);\n> + std::copy_n(gbce->g_gamma_lut, gbce->gamma_lut_size, gbce_.g_gamma_lut);\n\nThese look so much better ;-)\n\nOdd that they are ordered R B G rather than R G B ... but I don't think\nthat matters too much.\n\n\n\n> } else {\n> LOG(AIQResults, Error) << \"Not copying Gamma LUT channels\";\n> }\n> \n> if (gbce->tone_map_lut_size > 0) {\n> gbce_.tone_map_lut_size = gbce->tone_map_lut_size;\n> - STDCOPY((int8_t *)gbce_.tone_map_lut, (int8_t *)gbce->tone_map_lut,\n> - gbce->tone_map_lut_size * sizeof(float));\n> + std::copy_n(gbce->tone_map_lut, gbce->tone_map_lut_size, gbce_.tone_map_lut);\n> } else {\n> LOG(AIQResults, Error) << \"Not copying Tone Mapping Gain LUT\";\n> }\n> @@ -200,20 +190,20 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)\n> {\n> ASSERT(pa);\n> \n> - STDCOPY(&pa_.color_conversion_matrix[0][0], &pa->color_conversion_matrix[0][0],\n> - MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX *\n> - sizeof(pa->color_conversion_matrix[0][0]));\n> + std::copy_n(&pa->color_conversion_matrix[0][0],\n> + MAX_COLOR_CONVERSION_MATRIX * MAX_COLOR_CONVERSION_MATRIX,\n> + &pa_.color_conversion_matrix[0][0]);\n> \n> if (pa_.preferred_acm && pa->preferred_acm) {\n> pa_.preferred_acm->sector_count = pa->preferred_acm->sector_count;\n> \n> - STDCOPY(pa_.preferred_acm->hue_of_sectors,\n> - pa->preferred_acm->hue_of_sectors,\n> - sizeof(*pa->preferred_acm->hue_of_sectors) * pa->preferred_acm->sector_count);\n> + std::copy_n(pa->preferred_acm->hue_of_sectors,\n> + pa->preferred_acm->sector_count,\n> + pa_.preferred_acm->hue_of_sectors);\n> \n> - STDCOPY(pa_.preferred_acm->advanced_color_conversion_matrices[0][0],\n> - pa->preferred_acm->advanced_color_conversion_matrices[0][0],\n> - sizeof(*pa->preferred_acm->advanced_color_conversion_matrices) * pa->preferred_acm->sector_count);\n> + std::copy_n(pa->preferred_acm->advanced_color_conversion_matrices[0][0],\n> + pa->preferred_acm->sector_count,\n> + pa_.preferred_acm->advanced_color_conversion_matrices[0][0]);\n\nIndentation seems weird here again, but perhaps we should define a\n.clang-format file and go through the formatting in one big fix.\n\nSo I'm not overly worried about the code style right now.\n\n\nAll the changes I've examined seem to be correct. I hope I didn't miss\nanything critical, but as the testing shows this fixes things I'm happy.\n\nReviewed-by: Kieran Bingham \n\n> } else {\n> LOG(AIQResults, Error) << \"Not copying PA hue of sectors\";\n> }\n> @@ -222,17 +212,17 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)\n> pa_.ir_weight->height = pa->ir_weight->height;\n> pa_.ir_weight->width = pa->ir_weight->width;\n> \n> - STDCOPY(pa_.ir_weight->ir_weight_grid_R,\n> - pa->ir_weight->ir_weight_grid_R,\n> - sizeof(*pa->ir_weight->ir_weight_grid_R) * pa->ir_weight->height * pa->ir_weight->width);\n> + std::copy_n(pa->ir_weight->ir_weight_grid_R,\n> + pa->ir_weight->height * pa->ir_weight->width,\n> + pa_.ir_weight->ir_weight_grid_R);\n> \n> - STDCOPY(pa_.ir_weight->ir_weight_grid_G,\n> - pa->ir_weight->ir_weight_grid_G,\n> - sizeof(*pa->ir_weight->ir_weight_grid_G) * pa->ir_weight->height * pa->ir_weight->width);\n> + std::copy_n(pa->ir_weight->ir_weight_grid_G,\n> + pa->ir_weight->height * pa->ir_weight->width,\n> + pa_.ir_weight->ir_weight_grid_G);\n> \n> - STDCOPY(pa_.ir_weight->ir_weight_grid_B,\n> - pa->ir_weight->ir_weight_grid_B,\n> - sizeof(*pa->ir_weight->ir_weight_grid_B) * pa->ir_weight->height * pa->ir_weight->width);\n> + std::copy_n(pa->ir_weight->ir_weight_grid_B,\n> + pa->ir_weight->height * pa->ir_weight->width,\n> + pa_.ir_weight->ir_weight_grid_B);\n> } else {\n> LOG(AIQResults, Error) << \"Not copying IR weight\";\n> }\n> @@ -253,13 +243,13 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)\n> sa_.height = sa->height;\n> sa_.lsc_update = sa->lsc_update;\n> \n> + uint32_t lscGridSize = sa_.width * sa_.height;\n> /* Check against one of the vectors but resize applicable to all. */\n> - if (channelGr_.size() < (sa_.width * sa_.height)) {\n> - int lscNewSize = sa_.width * sa_.height;\n> - channelGr_.resize(lscNewSize);\n> - channelGb_.resize(lscNewSize);\n> - channelR_.resize(lscNewSize);\n> - channelB_.resize(lscNewSize);\n> + if (channelGr_.size() < lscGridSize) {\n> + channelGr_.resize(lscGridSize);\n> + channelGb_.resize(lscGridSize);\n> + channelR_.resize(lscGridSize);\n> + channelB_.resize(lscGridSize);\n> \n> /* Update the SA data pointers to new memory locations. */\n> sa_.channel_gr = channelGr_.data();\n> @@ -269,23 +259,16 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)\n> }\n> \n> if (sa->lsc_update) {\n> - uint32_t memCopySize = sa->width * sa->height * sizeof(float);\n> -\n> - STDCOPY((int8_t *)sa_.channel_gr, (int8_t *)sa->channel_gb,\n> - memCopySize);\n> - STDCOPY((int8_t *)sa_.channel_gb, (int8_t *)sa->channel_gr,\n> - memCopySize);\n> - STDCOPY((int8_t *)sa_.channel_r, (int8_t *)sa->channel_r,\n> - memCopySize);\n> - STDCOPY((int8_t *)sa_.channel_b, (int8_t *)sa->channel_b,\n> - memCopySize);\n> + std::copy_n(sa->channel_gr, lscGridSize, sa_.channel_gr);\n> + std::copy_n(sa->channel_gb, lscGridSize, sa_.channel_gb);\n> + std::copy_n(sa->channel_r, lscGridSize, sa_.channel_r);\n> + std::copy_n(sa->channel_b, lscGridSize, sa_.channel_b);\n> } else {\n> - LOG(AIQResults, Error) << \"Not copying LSC tables.\";\n> + LOG(AIQResults, Debug) << \"Not copying LSC tables.\";\n> }\n> \n> - STDCOPY(&sa_.light_source[0],\n> - &sa->light_source[0],\n> - CMC_NUM_LIGHTSOURCES * sizeof(sa->light_source[0]));\n> + std::copy_n(&sa->light_source[0], CMC_NUM_LIGHTSOURCES, &sa_.light_source[0]);\n> +\n> sa_.scene_difficulty = sa->scene_difficulty;\n> sa_.num_patches = sa->num_patches;\n> sa_.covered_area = sa->covered_area;\n> -- \n> 2.33.0.882.g93a45727a2-goog\n>","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 651C5C323E\n\tfor ;\n\tTue, 12 Oct 2021 11:56:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF61968F4F;\n\tTue, 12 Oct 2021 13:56:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12FBF68F4C\n\tfor ;\n\tTue, 12 Oct 2021 13:56:07 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 911A3F1;\n\tTue, 12 Oct 2021 13:56:06 +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=\"JDNniVEt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634039766;\n\tbh=/iqraeXxbiDJVhUiPsfiyRYiwwf/1x4JaxenN1UVzzo=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=JDNniVEtqXtXyzqn39hRuy3YYnkjAo9oxMPA1EbwhwIgM7CyU9LTVm4sy6QUpZH3X\n\tdc4HmHA2UWNTgyP+ULVGbOt1q15v5XkKhMI0KKnliJ8jUVJCrRWFSTlcPUG/GERjcp\n\tLFEoCY6w5pBp7J4nyhXSFJJFQabXSVvdPMoHstJk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211007072147.1289490-2-hanlinchen@chromium.org>","References":"<20211007072147.1289490-1-hanlinchen@chromium.org>\n\t<20211007072147.1289490-2-hanlinchen@chromium.org>","From":"Kieran Bingham ","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 12 Oct 2021 12:56:04 +0100","Message-ID":"<163403976410.2943484.8418225803690797783@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipu3: Change Macro migrated\n\tfrom Chrome 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\" "}}]