[{"id":20664,"web_url":"https://patchwork.libcamera.org/comment/20664/","msgid":"<163586023983.275423.10812486312647451672@Monstersaurus>","date":"2021-11-02T13:37:19","subject":"Re: [libcamera-devel] [PATCH v2 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-10-29 12:59:57)\n> The AIQ algorithm expects the statstistics comes with the effective AiqResults\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\nExcellent, I'm pleased to see this development.\n\nThis patch does a few things which should be explictily mentioned (or\nbroken in to independent patches)\n\n - Make the parameters to setXXX() functions const.\n - Implement copy constructors\n - Implement a RingBuffer to maintain AiqResults History\n\n> Signed-off-by: Han-Lin Chen \n> ---\n> aiq/aiq_results.cpp | 85 ++++++++++++++++++++++++++++++++++++++++-----\n> aiq/aiq_results.h | 38 +++++++++++++++-----\n> 2 files changed, 107 insertions(+), 16 deletions(-)\n> \n> diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp\n> index f727f36..deda4be 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> * 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,48 @@ 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::Push(const AiqResults& result)\n> +{\n> + operator[](end_) = result;\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::searchBackward(\n> + const std::function pred, AiqResults& def)\n> +{\n> + if (size_ == 0)\n> + return def;\n> +\n> + unsigned int i = (end_ - 1) % bufferSize;\n> + while (i != start_) {\n> + if (pred(operator[](i))) {\n> + return operator[](i);\n> + }\n> + i = --i % 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..7005a47 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,8 @@ class AiqResults\n> {\n> public:\n> AiqResults();\n> + AiqResults(const AiqResults &other);\n> + AiqResults &operator=(const AiqResults &other);\n\nShould the move constructor/assingment operators be deleted or\nimplemented?\n\n\n> const ia_aiq_ae_results *ae() { return &ae_; }\n> ia_aiq_af_results *af() { return &af_; }\n> @@ -46,14 +50,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 +113,24 @@ 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 Push(const AiqResults& result);\n> + unsigned int size() { return size_; }\n> + AiqResults& searchBackward(const std::function pred,\n> + AiqResults& def);\n> +\n> +private:\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.33.1.1089.g2158813163f-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 60CC5BDB1C\n\tfor ;\n\tTue, 2 Nov 2021 13:37:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B33B6600C2;\n\tTue, 2 Nov 2021 14:37:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0E26600B8\n\tfor ;\n\tTue, 2 Nov 2021 14:37:22 +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 8D6093E5;\n\tTue, 2 Nov 2021 14:37:22 +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=\"Au0P9YjT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635860242;\n\tbh=5usKhjovYXR0ulYCp+6cq0yzsc0fQDGZ2eUWyUe8wEE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Au0P9YjTxQJTHgQL5+3PH8YCpP79hmo7Jz+qyArlCj3eGFdu3nmjOzhBy2By4K/8+\n\tSGXz0nau7pi4VhSBrIH9ebOVDqhagUJ/BwiD+QKh+yQXe6M7K1uPBldYXA+EEozKN3\n\t9T3fGj+kRrKvfgREaCsdrP2+wuyJdTVCVUhX3EAE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211029120001.2469018-2-hanlinchen@chromium.org>","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-2-hanlinchen@chromium.org>","From":"Kieran Bingham ","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 02 Nov 2021 13:37:19 +0000","Message-ID":"<163586023983.275423.10812486312647451672@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}},{"id":20803,"web_url":"https://patchwork.libcamera.org/comment/20803/","msgid":"","date":"2021-11-10T13:02:17","subject":"Re: [libcamera-devel] [PATCH v2 2/6] ipu3: Add a class\n\tAiqResultsRingBuffer to reserve AiqResults history","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"Hi Kieran,\nI'm sorry for the late reply.\n\nOn Tue, Nov 2, 2021 at 9:37 PM Kieran Bingham\n wrote:\n>\n> Quoting Han-Lin Chen (2021-10-29 12:59:57)\n> > The AIQ algorithm expects the statstistics comes with the effective AiqResults\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> Excellent, I'm pleased to see this development.\n>\n> This patch does a few things which should be explictily mentioned (or\n> broken in to independent patches)\n>\n> - Make the parameters to setXXX() functions const.\n> - Implement copy constructors\n> - Implement a RingBuffer to maintain AiqResults History\nSounds great to me.\n>\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > aiq/aiq_results.cpp | 85 ++++++++++++++++++++++++++++++++++++++++-----\n> > aiq/aiq_results.h | 38 +++++++++++++++-----\n> > 2 files changed, 107 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp\n> > index f727f36..deda4be 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> > * 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,48 @@ 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::Push(const AiqResults& result)\n> > +{\n> > + operator[](end_) = result;\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::searchBackward(\n> > + const std::function pred, AiqResults& def)\n> > +{\n> > + if (size_ == 0)\n> > + return def;\n> > +\n> > + unsigned int i = (end_ - 1) % bufferSize;\n> > + while (i != start_) {\n> > + if (pred(operator[](i))) {\n> > + return operator[](i);\n> > + }\n> > + i = --i % 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..7005a47 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,8 @@ class AiqResults\n> > {\n> > public:\n> > AiqResults();\n> > + AiqResults(const AiqResults &other);\n> > + AiqResults &operator=(const AiqResults &other);\n>\n> Should the move constructor/assingment operators be deleted or\n> implemented?\nI think AiqResult doesn't have a \"move\" semantics ;-).\nThe move constructor/assignment would be suppressed automatically when\nthe copy constructor is defined, but it's good to make it more\nexplicit :/\n>\n>\n> > const ia_aiq_ae_results *ae() { return &ae_; }\n> > ia_aiq_af_results *af() { return &af_; }\n> > @@ -46,14 +50,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 +113,24 @@ 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 Push(const AiqResults& result);\n> > + unsigned int size() { return size_; }\n> > + AiqResults& searchBackward(const std::function pred,\n> > + AiqResults& def);\n> > +\n> > +private:\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.33.1.1089.g2158813163f-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 C0FC8BF415\n\tfor ;\n\tWed, 10 Nov 2021 13:02:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7118D6035F;\n\tWed, 10 Nov 2021 14:02:30 +0100 (CET)","from mail-ot1-x334.google.com (mail-ot1-x334.google.com\n\t[IPv6:2607:f8b0:4864:20::334])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 697556033C\n\tfor ;\n\tWed, 10 Nov 2021 14:02:29 +0100 (CET)","by mail-ot1-x334.google.com with SMTP id\n\tr10-20020a056830080a00b0055c8fd2cebdso3766460ots.6\n\tfor ;\n\tWed, 10 Nov 2021 05:02:29 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BlXUWf6G\"; 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\t:cc; bh=rsmua2eaqBQLPUf92fXDs3s12Kb3W0fN02dySDmKQ/Q=;\n\tb=BlXUWf6GxKX5b9RvuoOUMsIpcBi9QKROwLD0lFWNkyZ6BErBa1KxEzVnPtfdf0/c4p\n\tfwwKIt5nGbBSo2wPKa38fhA8PU0XcklK7bIPdBp6xVcT0tWEuydSnG75wuaprS+I+Ryv\n\t0OrSLZs1chB4SIeUWL8v1c3MM4aybfD8zqFL4=","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:cc;\n\tbh=rsmua2eaqBQLPUf92fXDs3s12Kb3W0fN02dySDmKQ/Q=;\n\tb=VwPZwtF76iICULF/95cg+WyBe00LWQ1UJ4sdS3op+0IWQ2BI1sHTfIN/HFzEk2QrOF\n\t7ytQoUSDl4k9mFZiSYeQm1FCu7eKmoPSECk0GFrJFAB9Of8mzv0QklIcaVwQXnSVb3Gb\n\t9NX1UbqF/eyRjkfcrnR5Y9OgejIDspJi6giz6Qbv+q1hmd6Xz/lgEvgPp/CsC0wA1hto\n\tUrc+0mTqtCK9Y+cKOR7cdAoeckMFGxEmD/37bUHDMVk0+oSuEoQJf5w4hqfgb9Qx/yER\n\t5nJPlnGBow3GaE0Dlqf+BiivxQGZUtLJrIwaBjMGx99tVe1Dbbm8HAR8W5V4mUZhKaZ8\n\tYLAw==","X-Gm-Message-State":"AOAM530jYdhOK1UbDiGrhITxQ2S7Jnhqua9XVIZaR5BQ0rkd+xa3RR2o\n\tRfg9407XJE5xyHJyZc1DPtXMxThY9k0NMO4aHvRb+Z3kQR7H+g==","X-Google-Smtp-Source":"ABdhPJzQUlJtvVMZpEfY5juc2ZJVmelsgWRUnmN9q6F9OJRu/ncCdWoJobqLa68ts9/rhp1VAeDAVUT68mWcto8afuA=","X-Received":"by 2002:a9d:53c4:: with SMTP id\n\ti4mr12250387oth.176.1636549348208; \n\tWed, 10 Nov 2021 05:02:28 -0800 (PST)","MIME-Version":"1.0","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-2-hanlinchen@chromium.org>\n\t<163586023983.275423.10812486312647451672@Monstersaurus>","In-Reply-To":"<163586023983.275423.10812486312647451672@Monstersaurus>","From":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 21:02:17 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20812,"web_url":"https://patchwork.libcamera.org/comment/20812/","msgid":"<163655321158.1946932.17529594889979243576@Monstersaurus>","date":"2021-11-10T14:06:51","subject":"Re: [libcamera-devel] [PATCH v2 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 Hanlin Chen (2021-11-10 13:02:17)\n> Hi Kieran,\n> I'm sorry for the late reply.\n\nDon't worry - it's all asynchronous ;-)\n\n> \n> On Tue, Nov 2, 2021 at 9:37 PM Kieran Bingham\n> wrote:\n> >\n> > Quoting Han-Lin Chen (2021-10-29 12:59:57)\n> > > The AIQ algorithm expects the statstistics comes with the effective AiqResults\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> > Excellent, I'm pleased to see this development.\n> >\n> > This patch does a few things which should be explictily mentioned (or\n> > broken in to independent patches)\n> >\n> > - Make the parameters to setXXX() functions const.\n> > - Implement copy constructors\n> > - Implement a RingBuffer to maintain AiqResults History\n> Sounds great to me.\n> >\n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > aiq/aiq_results.cpp | 85 ++++++++++++++++++++++++++++++++++++++++-----\n> > > aiq/aiq_results.h | 38 +++++++++++++++-----\n> > > 2 files changed, 107 insertions(+), 16 deletions(-)\n> > >\n> > > diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp\n> > > index f727f36..deda4be 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> > > * 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,48 @@ 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::Push(const AiqResults& result)\n> > > +{\n> > > + operator[](end_) = result;\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::searchBackward(\n> > > + const std::function pred, AiqResults& def)\n> > > +{\n> > > + if (size_ == 0)\n> > > + return def;\n> > > +\n> > > + unsigned int i = (end_ - 1) % bufferSize;\n> > > + while (i != start_) {\n> > > + if (pred(operator[](i))) {\n> > > + return operator[](i);\n> > > + }\n> > > + i = --i % 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..7005a47 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,8 @@ class AiqResults\n> > > {\n> > > public:\n> > > AiqResults();\n> > > + AiqResults(const AiqResults &other);\n> > > + AiqResults &operator=(const AiqResults &other);\n> >\n> > Should the move constructor/assingment operators be deleted or\n> > implemented?\n> I think AiqResult doesn't have a \"move\" semantics ;-).\n> The move constructor/assignment would be suppressed automatically when\n> the copy constructor is defined, but it's good to make it more\n> explicit :/\n\nAh yes I recall there are implicit rules that delete things. I can never\nremember them ;-) All I have in my head is the rule of 3/5/x where if\nyou define one you should define or delete all of them ...\n\n\n\n> >\n> >\n> > > const ia_aiq_ae_results *ae() { return &ae_; }\n> > > ia_aiq_af_results *af() { return &af_; }\n> > > @@ -46,14 +50,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 +113,24 @@ 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 Push(const AiqResults& result);\n> > > + unsigned int size() { return size_; }\n> > > + AiqResults& searchBackward(const std::function pred,\n> > > + AiqResults& def);\n> > > +\n> > > +private:\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.33.1.1089.g2158813163f-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 28ED7BF415\n\tfor ;\n\tWed, 10 Nov 2021 14:06:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4E1A6035D;\n\tWed, 10 Nov 2021 15:06:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5A626033C\n\tfor ;\n\tWed, 10 Nov 2021 15:06:54 +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 333D6501;\n\tWed, 10 Nov 2021 15:06:54 +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=\"OGN5Lef/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636553214;\n\tbh=zrupI6w9DbivvzSHt8B/NXZbZIW4fS6sHjP3eQNMWo8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OGN5Lef/iaCqzOl08tX08XSkDBIewtnkJZ3Lk+iA3OJnPFnhOu1mIGovXVWyAw/do\n\tsU+hp454FjdrzcXWXgDeKccaxSdRgwoAXQ7WN2gL4zhnwUaVe1stYtpqCBWcHFUGAZ\n\t5kXW+fGcYCjXD6XtqpXV0xuVQglElwpxvlSOlTt4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-2-hanlinchen@chromium.org>\n\t<163586023983.275423.10812486312647451672@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 14:06:51 +0000","Message-ID":"<163655321158.1946932.17529594889979243576@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]