[{"id":20050,"web_url":"https://patchwork.libcamera.org/comment/20050/","msgid":"<16f369ff-38ce-b11d-0556-ad58570c475e@ideasonboard.com>","date":"2021-10-05T06:12:52","subject":"Re: [libcamera-devel] [PATCH 3/3] ipu3: Apply shading adapter as\n\tpart of AIQ::run2a()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello Han-Lin,\n\nThank you for the patch.\n\nSince it's a relatively simple patch to handle geared for SA only, my \ncomments are mostly around  how it should handling it design-wise (for \nKieran to double-check when he's back). It can be parallel-ly applied by \nme (or Kieran) and we can take care of handling it internally (unless \nyou have an strong urge to work on this)  ;-)\n\nOn 10/4/21 3:18 PM, Han-Lin Chen via libcamera-devel wrote:\n> From: hanlinchen\n>\n> Apply shading adapter to correct lens shading for both camera.\n>\n> Signed-off-by: Han-Lin Chen\n> ---\n> aiq/aiq.cpp | 20 ++++++++++++++++++--\n> aiq/aiq.h | 4 +++-\n> ipu3.cpp | 2 +-\n> 3 files changed, 22 insertions(+), 4 deletions(-)\n>\n> diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp\n> index 708e9d6..0a92eaf 100644\n> --- a/aiq/aiq.cpp\n> +++ b/aiq/aiq.cpp\n> @@ -20,7 +20,8 @@ LOG_DEFINE_CATEGORY(AIQ)\n> namespaceipa::ipu3::aiq {\n> \n> AIQ::AIQ()\n> -\t: aiq_(nullptr)\n> +\t: aiq_(nullptr),\n> +\t\tsensor_frame_params_{}\n> {\n> \tLOG(AIQ, Info) << \"Creating IA AIQ Wrapper\";\n> }\n> @@ -105,10 +106,19 @@ intAIQ::init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd)\n> \treturn 0;\n> }\n> \n> -intAIQ::configure()\n> +intAIQ::configure(const struct IPAConfigInfo &configInfo)\n> {\n> \tLOG(AIQ, Debug) << \"Configure AIQ\";\n> \n> +\tsensor_frame_params_.horizontal_crop_offset = 0;\n> +\tsensor_frame_params_.vertical_crop_offset = 0;\n> +\tsensor_frame_params_.cropped_image_width = configInfo.sensorInfo.analogCrop.width;\n> +\tsensor_frame_params_.cropped_image_height = configInfo.sensorInfo.analogCrop.height;\n> +\tsensor_frame_params_.horizontal_scaling_numerator = 1;\n> +\tsensor_frame_params_.horizontal_scaling_denominator = 1;\n> +\tsensor_frame_params_.vertical_scaling_numerator = 1;\n> +\tsensor_frame_params_.vertical_scaling_denominator = 1;\n> +\n\n\nI would apply sensor information inside AiqInputParameters::configure() \nsince it already has IPAConfigInfo and sensorFrameParams. Currently we \ndon't seem to set sensorFrameParams, so I would set the above change in \nthere.\n\n> \treturn 0;\n> }\n> \n> @@ -154,6 +164,11 @@ intAIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> \tparams.paParams.exposure_params = results.ae()->exposures[0].exposure;\n> \tparameterAdapterRun(params.paParams, results);\n> \n> +\tparams.saParams.frame_use = params.aeInputParams.frame_use;\n> +\tparams.saParams.sensor_frame_params = &sensor_frame_params_;\n\n\nBroadly speaking, for setting input parameters for various algorithms, \nwe have a dedicated class AiqInputParameters. The above two changes \nshould be moved to AiqInputParameters::setAeAwbAfDefaults() (with scope \nof bikeshedding the name of function).\n\n> +\tparams.saParams.awb_results = results.awb();\n\n\n... This can stay as is since it's dependent on results of awb\n\n> +\tshadingAdapterRun(params.saParams, results);\n> +\n> \tafRun(params.afParams, results);\n> \n> \treturn 0;\n> @@ -328,6 +343,7 @@ intAIQ::shadingAdapterRun(ia_aiq_sa_input_params &saParams,\n> {\n> \tia_aiq_sa_results *saResults = nullptr;\n> \tia_err err = ia_aiq_sa_run(aiq_, &saParams, &saResults);\n> +\n> \tif (err) {\n> \t\tLOG(AIQ, Error) << \"Failed to run shading adapter: \"\n> \t\t\t\t<< decodeError(err);\n> diff --git a/aiq/aiq.h b/aiq/aiq.h\n> index fcd02d2..8a68827 100644\n> --- a/aiq/aiq.h\n> +++ b/aiq/aiq.h\n> @@ -35,7 +35,7 @@ public:\n> \t~AIQ();\n> \n> \tint init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd);\n> -\tint configure();\n> +\tint configure(const struct IPAConfigInfo &configInfo);\n> \tint setStatistics(unsigned int frame,\n> \t\t\t int64_t timestamp, AiqResults &results,\n> \t\t\t const ipu3_uapi_stats_3a *stats);\n> @@ -62,6 +62,8 @@ private:\n> \tia_cmc_t *iaCmc_;\n> \tstd::string version_;\n> \n> + ia_aiq_frame_params sensor_frame_params_;\n> +\n> \tIPAIPU3Stats *aiqStats_;\n> };\n> \n> diff --git a/ipu3.cpp b/ipu3.cpp\n> index b60c58c..ed3c516 100644\n> --- a/ipu3.cpp\n> +++ b/ipu3.cpp\n> @@ -232,7 +232,7 @@ intIPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tint ret;\n> \n> -\tret = aiq_.configure();\n> +\tret = aiq_.configure(configInfo);\n\n\nAs mentioned earlier AiqInputParameters::configure() already takes in \nconfigInfo, we should be using that instead to set sensorFrameParams and \nfinally set SA input parameters (in AiqInputParameters::setAeAwbAfDefaults)\n\n> \tif (ret) {\n> \t\tLOG(IPAIPU3, Error) << \"Failed to configure the AIQ\";\n> \t\treturn ret;\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 0BB0BBDC71\n\tfor ;\n\tTue, 5 Oct 2021 06:13:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7949B691B9;\n\tTue, 5 Oct 2021 08:12:59 +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 AE6E26023D\n\tfor ;\n\tTue, 5 Oct 2021 08:12:58 +0200 (CEST)","from [192.168.1.106] (unknown [103.238.109.2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8F70625B;\n\tTue, 5 Oct 2021 08:12:57 +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=\"mGQ4zhXL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633414378;\n\tbh=U6B4QF57NqSjBrOmvMwgBmb3v7yGkE2Ni+LVHOmmKjo=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=mGQ4zhXL3LN4IEfmiTAJ6pcbDRn1BgLaLiETGmHLqpUeLzYMp4c/wthttJjKF2a91\n\tCYba046ZhoehleAnDRR9LPfBk7IJ1Y8DQuM+yxZdlst5dfz2CxUpq1+Uyhb3BEIbl7\n\tkYDhHc7f6ZoZ7VmQy9IDJyQjRHJWYVOEAhzd6S4Y=","To":"Han-Lin Chen , libcamera-devel@lists.libcamera.org","References":"<20211004094823.260789-1-hanlinchen@google.com>\n\t","From":"Umang Jain ","Message-ID":"<16f369ff-38ce-b11d-0556-ad58570c475e@ideasonboard.com>","Date":"Tue, 5 Oct 2021 11:42:52 +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":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipu3: Apply shading adapter as\n\tpart of AIQ::run2a()","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\" "}},{"id":20095,"web_url":"https://patchwork.libcamera.org/comment/20095/","msgid":"","date":"2021-10-07T07:20:49","subject":"Re: [libcamera-devel] [PATCH 3/3] ipu3: Apply shading adapter as\n\tpart of AIQ::run2a()","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"On Thu, Oct 7, 2021 at 3:08 PM Han-lin Chen wrote:\n>\n> Hello Han-Lin,\n>\n> Thank you for the patch.\n>\n> Since it's a relatively simple patch to handle geared for SA only, my\n> comments are mostly around how it should handling it design-wise (for\n> Kieran to double-check when he's back). It can be parallel-ly applied by\n> me (or Kieran) and we can take care of handling it internally (unless\n> you have an strong urge to work on this) ;-)\n>\n> On 10/4/21 3:18 PM, Han-Lin Chen via libcamera-devel wrote:\n> > From: hanlinchen\n> >\n> > Apply shading adapter to correct lens shading for both camera.\n> >\n> > Signed-off-by: Han-Lin Chen\n> > ---\n> > aiq/aiq.cpp | 20 ++++++++++++++++++--\n> > aiq/aiq.h | 4 +++-\n> > ipu3.cpp | 2 +-\n> > 3 files changed, 22 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp\n> > index 708e9d6..0a92eaf 100644\n> > --- a/aiq/aiq.cpp\n> > +++ b/aiq/aiq.cpp\n> > @@ -20,7 +20,8 @@ LOG_DEFINE_CATEGORY(AIQ)\n> > namespaceipa::ipu3::aiq {\n> >\n> > AIQ::AIQ()\n> > - : aiq_(nullptr)\n> > + : aiq_(nullptr),\n> > + sensor_frame_params_{}\n> > {\n> > LOG(AIQ, Info) << \"Creating IA AIQ Wrapper\";\n> > }\n> > @@ -105,10 +106,19 @@ intAIQ::init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd)\n> > return 0;\n> > }\n> >\n> > -intAIQ::configure()\n> > +intAIQ::configure(const struct IPAConfigInfo &configInfo)\n> > {\n> > LOG(AIQ, Debug) << \"Configure AIQ\";\n> >\n> > + sensor_frame_params_.horizontal_crop_offset = 0;\n> > + sensor_frame_params_.vertical_crop_offset = 0;\n> > + sensor_frame_params_.cropped_image_width = configInfo.sensorInfo.analogCrop.width;\n> > + sensor_frame_params_.cropped_image_height = configInfo.sensorInfo.analogCrop.height;\n> > + sensor_frame_params_.horizontal_scaling_numerator = 1;\n> > + sensor_frame_params_.horizontal_scaling_denominator = 1;\n> > + sensor_frame_params_.vertical_scaling_numerator = 1;\n> > + sensor_frame_params_.vertical_scaling_denominator = 1;\n> > +\n>\n>\n> I would apply sensor information inside AiqInputParameters::configure()\n> since it already has IPAConfigInfo and sensorFrameParams. Currently we\n> don't seem to set sensorFrameParams, so I would set the above change in\n> there.\n>\n> > return 0;\n> > }\n> >\n> > @@ -154,6 +164,11 @@ intAIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> > params.paParams.exposure_params = results.ae()->exposures[0].exposure;\n> > parameterAdapterRun(params.paParams, results);\n> >\n> > + params.saParams.frame_use = params.aeInputParams.frame_use;\n> > + params.saParams.sensor_frame_params = &sensor_frame_params_;\n>\n>\n> Broadly speaking, for setting input parameters for various algorithms,\n> we have a dedicated class AiqInputParameters. The above two changes\n> should be moved to AiqInputParameters::setAeAwbAfDefaults() (with scope\n> of bikeshedding the name of function).\n>\n> > + params.saParams.awb_results = results.awb();\n>\n>\n> ... This can stay as is since it's dependent on results of awb\n>\n> > + shadingAdapterRun(params.saParams, results);\n> > +\n> > afRun(params.afParams, results);\n> >\n> > return 0;\n> > @@ -328,6 +343,7 @@ intAIQ::shadingAdapterRun(ia_aiq_sa_input_params &saParams,\n> > {\n> > ia_aiq_sa_results *saResults = nullptr;\n> > ia_err err = ia_aiq_sa_run(aiq_, &saParams, &saResults);\n> > +\n> > if (err) {\n> > LOG(AIQ, Error) << \"Failed to run shading adapter: \"\n> > << decodeError(err);\n> > diff --git a/aiq/aiq.h b/aiq/aiq.h\n> > index fcd02d2..8a68827 100644\n> > --- a/aiq/aiq.h\n> > +++ b/aiq/aiq.h\n> > @@ -35,7 +35,7 @@ public:\n> > ~AIQ();\n> >\n> > int init(BinaryData &aiqb, BinaryData &nvm, BinaryData &aiqd);\n> > - int configure();\n> > + int configure(const struct IPAConfigInfo &configInfo);\n> > int setStatistics(unsigned int frame,\n> > int64_t timestamp, AiqResults &results,\n> > const ipu3_uapi_stats_3a *stats);\n> > @@ -62,6 +62,8 @@ private:\n> > ia_cmc_t *iaCmc_;\n> > std::string version_;\n> >\n> > + ia_aiq_frame_params sensor_frame_params_;\n> > +\n> > IPAIPU3Stats *aiqStats_;\n> > };\n> >\n> > diff --git a/ipu3.cpp b/ipu3.cpp\n> > index b60c58c..ed3c516 100644\n> > --- a/ipu3.cpp\n> > +++ b/ipu3.cpp\n> > @@ -232,7 +232,7 @@ intIPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >\n> > int ret;\n> >\n> > - ret = aiq_.configure();\n> > + ret = aiq_.configure(configInfo);\n>\n>\n> As mentioned earlier AiqInputParameters::configure() already takes in\n> configInfo, we should be using that instead to set sensorFrameParams and\n> finally set SA input parameters (in AiqInputParameters::setAeAwbAfDefaults)\n\n\nThanks. I see it now. it's more reasonable.\n>\n>\n> > if (ret) {\n> > LOG(IPAIPU3, Error) << \"Failed to configure the AIQ\";\n> > return ret;\n> > -- 2.33.0.800.g4c38ced690-goog\n>\n>\n> --\n> Cheers.\n> Hanlin Chen","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 7F0CDC323E\n\tfor ;\n\tThu, 7 Oct 2021 07:21:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4969691B9;\n\tThu, 7 Oct 2021 09:21:02 +0200 (CEST)","from mail-ot1-x332.google.com (mail-ot1-x332.google.com\n\t[IPv6:2607:f8b0:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2EEB69189\n\tfor ;\n\tThu, 7 Oct 2021 09:21:01 +0200 (CEST)","by mail-ot1-x332.google.com with SMTP id\n\tg62-20020a9d2dc4000000b0054752cfbc59so6483929otb.1\n\tfor ;\n\tThu, 07 Oct 2021 00:21:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"iax4iCWW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=tqm7WsJMF0W9bNYdRQhm6oD0csBg7nUNkRMd3GWBY28=;\n\tb=iax4iCWWCStxi1EoTSbhkJnoeagT6FaS8AlOxWkeQqmO0aKjF9f42c2WvbSntbqgVP\n\tVbo7YFqZUz16IaB16rbhXU96CCOzyhFNzY/8U6e2dv/osZdikbBNBckIHX+qCcVfgA9I\n\tfflZam/URa6lrrUnKmhQGl9DIT677noE42saw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=tqm7WsJMF0W9bNYdRQhm6oD0csBg7nUNkRMd3GWBY28=;\n\tb=iWFzp9X++yjUblbNYe9W1owd+4UDQStIPg2dmrYZTODMKJ6sDFIbhYMxhw2Y+KUYEt\n\tmM0CCUECPtdygxt96ZYAl70rKpVGtDrV+bB/xEjYyNtaVt+tsyGsC6Pqu+PJs3uwLk68\n\tskYfeYkAoS5KtT8vQ9SM2Ql9i+cejPoc7+B8uA+AHDslQUgzlDXbJS7mJe8MHPsfbQJo\n\tMN0vJutQVmTcEvmL7Yl+Tbkj2khD7zwnsjwXCufJUSUROgEV6sRjeaiUMjv5oYWjYnKi\n\t+3vwLn51+lNF76aqSZU3LJ/XwssGFMHQYv8J89lvBZWbB9xPwpjdQ4ZoVT4nQ3kYKcyv\n\tV0YQ==","X-Gm-Message-State":"AOAM533dS3je7mJeIj1KlFY9oHcByLqDc7hva5TPcf/EdIBfpwzD6ehO\n\tXhdSLl2ggY0mmvbDneF7Dr+N9qtnjnhhgiqjCeYLyk/9tQGZBE3r+P4=","X-Google-Smtp-Source":"ABdhPJztqVA4sUk+vwRf/LmEa01/Gc0HB8P04TfKZ4Pq1hqVLwruwSqfkY6XgoFMX6odA5r8zVmFeNRgMsaOOMyKTL0=","X-Received":"by 2002:a05:6830:40cb:: with SMTP id\n\th11mr2299200otu.176.1633591260498; \n\tThu, 07 Oct 2021 00:21:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20211004094823.260789-1-hanlinchen@google.com>\n\t\n\t<16f369ff-38ce-b11d-0556-ad58570c475e@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hanlin Chen ","Date":"Thu, 7 Oct 2021 15:20:49 +0800","Message-ID":"","To":"libcamera-devel@lists.libcamera.org, \n\tUmang Jain ,\n\tlaurent.pinchart@ideasonboard.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipu3: Apply shading adapter as\n\tpart of AIQ::run2a()","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\" "}}]