[{"id":20986,"web_url":"https://patchwork.libcamera.org/comment/20986/","msgid":"<163717087713.420308.18227064485907267499@Monstersaurus>","date":"2021-11-17T17:41:17","subject":"Re: [libcamera-devel] [IPU3-IPA PATCH v3 2/6] ipu3: Add a class\n\tAiqResultsRingBuffer to reserve AiqResults history","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-11-11 10:49:04)\n> The AIQ algorithm expects the statstistics comes with the effective AiqResults\n\ns/statstistics/statistics/\n\nCould be fixed while applying if there's nothing else.\n\n> applied on the sensor, which may not always be the latest AiqResults,\n> since pipeline handler may delay setting the controls based on SOF.\n> Add a class to reserve the history of the AiqResults generated for previous\n> frames, so IPA can have a chance to look for the suitable one backwards.\n> \n> In details, the patch adds following to AiqResult Class.\n> - Make the parameters to setXXX() functions const.\n> - Implement copy constructors\n> - Implement a RingBuffer to maintain AiqResults History\n\nI wish those were separate patches, but we can keep it as is.\n\nOnly a question below, but I think I can say:\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Han-Lin Chen \n> ---\n> aiq/aiq_results.cpp | 90 +++++++++++++++++++++++++++++++++++++++++----\n> aiq/aiq_results.h | 43 ++++++++++++++++++----\n> 2 files changed, 117 insertions(+), 16 deletions(-)\n> \n> diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp\n> index f727f36..befcf7a 100644\n> --- a/aiq/aiq_results.cpp\n> +++ b/aiq/aiq_results.cpp\n> @@ -63,7 +63,34 @@ AiqResults::AiqResults()\n> sa_.channel_b = channelB_.data();\n> }\n> \n> -void AiqResults::setAe(ia_aiq_ae_results *ae)\n> +AiqResults::AiqResults(const AiqResults &other)\n> + :AiqResults()\n> +{\n> + setAe(&other.ae_);\n> + setAf(&other.af_);\n> + setAfBracket(&other.afBracket_);\n> + setAwb(&other.awb_);\n> + setGbce(&other.gbce_);\n> + setDetectedSceneMode(other.detectedSceneMode_);\n> + setPa(&other.pa_);\n> + setSa(&other.sa_);\n> +}\n> +\n> +AiqResults& AiqResults::operator=(const AiqResults &other)\n> +{\n> + setAe(&other.ae_);\n> + setAf(&other.af_);\n> + setAfBracket(&other.afBracket_);\n> + setAwb(&other.awb_);\n> + setGbce(&other.gbce_);\n> + setDetectedSceneMode(other.detectedSceneMode_);\n> + setPa(&other.pa_);\n> + setSa(&other.sa_);\n> +\n> + return *this;\n> +}\n> +\n> +void AiqResults::setAe(const ia_aiq_ae_results *ae)\n> {\n> /* Todo: Potentially Requires copying\n\nHave you checked through the set*'s to make sure they can be copied\nsuccessfully? I know this was an area that found lots of magic pointers\nthat were somewhere inside the AIQ library space...\n\n> * ia_aiq_aperture_control *aperture_control;\n> @@ -121,7 +148,7 @@ void AiqResults::setAe(ia_aiq_ae_results *ae)\n> }\n> }\n> \n> -void AiqResults::setAf(ia_aiq_af_results *af)\n> +void AiqResults::setAf(const ia_aiq_af_results *af)\n> {\n> ASSERT(af);\n> \n> @@ -133,7 +160,7 @@ void AiqResults::setAf(ia_aiq_af_results *af)\n> af_.final_lens_position_reached = af->final_lens_position_reached;\n> }\n> \n> -void AiqResults::setAfBracket(ia_aiq_af_bracket_results *afBracket)\n> +void AiqResults::setAfBracket(const ia_aiq_af_bracket_results *afBracket)\n> {\n> ASSERT(afBracket);\n> \n> @@ -145,7 +172,7 @@ void AiqResults::setAfBracket(ia_aiq_af_bracket_results *afBracket)\n> afBracket_.lens_positions_bracketing = afBracket->lens_positions_bracketing;\n> }\n> \n> -void AiqResults::setAwb(ia_aiq_awb_results *awb)\n> +void AiqResults::setAwb(const ia_aiq_awb_results *awb)\n> {\n> ASSERT(awb);\n> \n> @@ -157,7 +184,7 @@ void AiqResults::setAwb(ia_aiq_awb_results *awb)\n> awb_.distance_from_convergence = awb->distance_from_convergence;\n> }\n> \n> -void AiqResults::setGbce(ia_aiq_gbce_results *gbce)\n> +void AiqResults::setGbce(const ia_aiq_gbce_results *gbce)\n> {\n> ASSERT(gbce);\n> \n> @@ -181,12 +208,12 @@ void AiqResults::setGbce(ia_aiq_gbce_results *gbce)\n> }\n> }\n> \n> -void AiqResults::setDetectedSceneMode(ia_aiq_scene_mode dsm)\n> +void AiqResults::setDetectedSceneMode(const ia_aiq_scene_mode dsm)\n> {\n> detectedSceneMode_ = dsm;\n> }\n> \n> -void AiqResults::setPa(ia_aiq_pa_results *pa)\n> +void AiqResults::setPa(const ia_aiq_pa_results *pa)\n> {\n> ASSERT(pa);\n> \n> @@ -234,7 +261,7 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)\n> pa_.brightness_level = pa->brightness_level;\n> }\n> \n> -void AiqResults::setSa(ia_aiq_sa_results *sa)\n> +void AiqResults::setSa(const ia_aiq_sa_results *sa)\n> {\n> ASSERT(sa && sa->channel_r && sa->channel_gr &&\n> sa->channel_gb && sa->channel_b);\n> @@ -275,6 +302,53 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)\n> sa_.frame_params = sa->frame_params;\n> }\n> \n> +AiqResults& AiqResultsRingBuffer::operator[](unsigned int index)\n> +{\n> + return std::array::operator[](index % bufferSize);\n> +}\n> +\n> +const AiqResults& AiqResultsRingBuffer::operator[](unsigned int index) const\n> +{\n> + return std::array::operator[](index % bufferSize);\n> +}\n> +\n> +void AiqResultsRingBuffer::reset()\n> +{\n> + start_ = end_ = size_ = 0;\n> +}\n> +\n> +void AiqResultsRingBuffer::extendOne()\n> +{\n> + end_ = (end_ + 1) % bufferSize;\n> + size_ = std::min(size_ + 1, bufferSize);\n> + if (size_ == bufferSize) {\n> + start_ = end_;\n> + }\n> +}\n> +\n> +AiqResults& AiqResultsRingBuffer::latest()\n> +{\n> + unsigned int i = (end_ + bufferSize - 1) % bufferSize;\n> + return operator[](i);\n> +}\n> +\n> +AiqResults& AiqResultsRingBuffer::searchBackward(\n> + const std::function pred, AiqResults& def)\n> +{\n> + if (size_ == 0)\n> + return def;\n> +\n> + unsigned int i = (end_ + bufferSize - 1) % bufferSize;\n> + while (i != start_) {\n> + if (pred(operator[](i))) {\n> + return operator[](i);\n> + }\n> + i = (i + bufferSize - 1) % bufferSize;\n> + }\n> +\n> + return def;\n> +}\n> +\n> } /* namespace ipa::ipu3::aiq */\n> \n> } /* namespace libcamera */\n> diff --git a/aiq/aiq_results.h b/aiq/aiq_results.h\n> index ae19a6c..8c95852 100644\n> --- a/aiq/aiq_results.h\n> +++ b/aiq/aiq_results.h\n> @@ -8,6 +8,8 @@\n> * of the aiq result structures.\n> */\n> \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -37,6 +39,10 @@ class AiqResults\n> {\n> public:\n> AiqResults();\n> + AiqResults(const AiqResults &other);\n> + AiqResults &operator=(const AiqResults &other);\n> + AiqResults(const AiqResults &&other) = delete;\n> + AiqResults &operator=(const AiqResults &&other) = delete;\n> \n> const ia_aiq_ae_results *ae() { return &ae_; }\n> ia_aiq_af_results *af() { return &af_; }\n> @@ -46,14 +52,14 @@ public:\n> const ia_aiq_pa_results *pa() { return &pa_; }\n> const ia_aiq_sa_results *sa() { return &sa_; }\n> \n> - void setAe(ia_aiq_ae_results *ae);\n> - void setAf(ia_aiq_af_results *af);\n> - void setAfBracket(ia_aiq_af_bracket_results *afBracket);\n> - void setAwb(ia_aiq_awb_results *awb);\n> - void setGbce(ia_aiq_gbce_results *gbce);\n> - void setDetectedSceneMode(ia_aiq_scene_mode dsm);\n> - void setPa(ia_aiq_pa_results *pa);\n> - void setSa(ia_aiq_sa_results *sa);\n> + void setAe(const ia_aiq_ae_results *ae);\n> + void setAf(const ia_aiq_af_results *af);\n> + void setAfBracket(const ia_aiq_af_bracket_results *afBracket);\n> + void setAwb(const ia_aiq_awb_results *awb);\n> + void setGbce(const ia_aiq_gbce_results *gbce);\n> + void setDetectedSceneMode(const ia_aiq_scene_mode dsm);\n> + void setPa(const ia_aiq_pa_results *pa);\n> + void setSa(const ia_aiq_sa_results *sa);\n> \n> private:\n> ia_aiq_ae_results ae_;\n> @@ -109,6 +115,27 @@ private:\n> std::vector channelGb_;\n> };\n> \n> +static constexpr unsigned int bufferSize = 16;\n> +class AiqResultsRingBuffer: public std::array\n> +{\n> +public:\n> + AiqResults &operator[](unsigned int index);\n> + const AiqResults &operator[](unsigned int index) const;\n> + void reset();\n> + void extendOne();\n> + AiqResults& latest();\n> + unsigned int size() { return size_; }\n> + AiqResults& searchBackward(const std::function pred,\n> + AiqResults& def);\n> +\n> +private:\n> + void incrementSize();\n> +\n> + unsigned int start_ = 0;\n> + unsigned int end_ = 0;\n> + unsigned int size_ = 0;\n> +};\n> +\n> } /* namespace libcamera::ipa::ipu3::aiq */\n> \n> #endif /* IPA_IPU3_AIQ_RESULTS_H */\n> -- \n> 2.34.0.rc1.387.gb447b232ab-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 E1E2CBF415\n\tfor ;\n\tWed, 17 Nov 2021 17:41:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7D8B60376;\n\tWed, 17 Nov 2021 18:41:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A26B60121\n\tfor ;\n\tWed, 17 Nov 2021 18:41:20 +0100 (CET)","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 98E1DE7;\n\tWed, 17 Nov 2021 18:41:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qbbeisVG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637170879;\n\tbh=gWDdHkojzAqu3Og0h2Pvs30MMu1hB513f60e2gfF4/g=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=qbbeisVGPkfpJKDZCWu3FFuJf8anDN9RVsCN8MO4gUa/lkFMO62/4XyrhJQx+KrIT\n\tjskWoJg0rCru06sFRejTFBEB1JGdY0BLp4tMZKbu3Wh1AWq8y1t2yKy7JX7vm/Ydft\n\tL+Q9RB84u3+IIX2vvL1hh67g0st8MmD/0WpaktEo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211111104908.295992-3-hanlinchen@chromium.org>","References":"<20211111104908.295992-1-hanlinchen@chromium.org>\n\t<20211111104908.295992-3-hanlinchen@chromium.org>","From":"Kieran Bingham ","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 17 Nov 2021 17:41:17 +0000","Message-ID":"<163717087713.420308.18227064485907267499@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [IPU3-IPA PATCH v3 2/6] ipu3: Add a class\n\tAiqResultsRingBuffer to reserve AiqResults history","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\" "}}]