[{"id":5209,"web_url":"https://patchwork.libcamera.org/comment/5209/","msgid":"","date":"2020-06-15T15:20:58","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi\n\nI was wondering what folks wanted to do about reviewing patches like\nthis one, when they're very much in the domain of the Raspberry Pi\nIPAs. Do we need a different procedure for them, do you think?\n\nThanks!\n\nDavid\n\nOn Wed, 10 Jun 2020 at 15:24, David Plowman\n wrote:\n>\n> When the sensor is switched into a different mode the line\n> timings may change, yet the V4L2 driver remembers the number of\n> exposure lines, not the total time. So this will change \"under\n> our feet\".\n>\n> These patches extend the IPA handling of the camera mode switch\n> so that correct exposure values are recalculated for the new mode\n> and written to the sensor before it starts streaming again. They\n> also allow the IPA to alter how the total exposure is divided\n> between actual exposure and analogue gain, according to the\n> selected exposure profile in the camera tuning.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/algorithm.cpp | 3 +-\n> src/ipa/raspberrypi/controller/algorithm.hpp | 2 +-\n> src/ipa/raspberrypi/controller/controller.cpp | 4 +--\n> src/ipa/raspberrypi/controller/controller.hpp | 2 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++-\n> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-\n> src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++-\n> src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-\n> .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++-\n> .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++---------\n> 13 files changed, 46 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp\n> index 9bd3df8..55cb201 100644\n> --- a/src/ipa/raspberrypi/controller/algorithm.cpp\n> +++ b/src/ipa/raspberrypi/controller/algorithm.cpp\n> @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms)\n>\n> void Algorithm::Initialise() {}\n>\n> -void Algorithm::SwitchMode(CameraMode const &camera_mode)\n> +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> (void)camera_mode;\n> + (void)metadata;\n> }\n>\n> void Algorithm::Prepare(Metadata *image_metadata)\n> diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp\n> index b82c184..187c50c 100644\n> --- a/src/ipa/raspberrypi/controller/algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/algorithm.hpp\n> @@ -37,7 +37,7 @@ public:\n> virtual void Resume() { paused_ = false; }\n> virtual void Read(boost::property_tree::ptree const ¶ms);\n> virtual void Initialise();\n> - virtual void SwitchMode(CameraMode const &camera_mode);\n> + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> virtual void Prepare(Metadata *image_metadata);\n> virtual void Process(StatisticsPtr &stats, Metadata *image_metadata);\n> Metadata &GetGlobalMetadata() const\n> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> index 20dd4c7..7c4b04f 100644\n> --- a/src/ipa/raspberrypi/controller/controller.cpp\n> +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> @@ -56,11 +56,11 @@ void Controller::Initialise()\n> RPI_LOG(\"Controller finished\");\n> }\n>\n> -void Controller::SwitchMode(CameraMode const &camera_mode)\n> +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> RPI_LOG(\"Controller starting\");\n> for (auto &algo : algorithms_)\n> - algo->SwitchMode(camera_mode);\n> + algo->SwitchMode(camera_mode, metadata);\n> switch_mode_called_ = true;\n> RPI_LOG(\"Controller finished\");\n> }\n> diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp\n> index d685386..6ba9412 100644\n> --- a/src/ipa/raspberrypi/controller/controller.hpp\n> +++ b/src/ipa/raspberrypi/controller/controller.hpp\n> @@ -39,7 +39,7 @@ public:\n> Algorithm *CreateAlgorithm(char const *name);\n> void Read(char const *filename);\n> void Initialise();\n> - void SwitchMode(CameraMode const &camera_mode);\n> + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> void Prepare(Metadata *image_metadata);\n> void Process(StatisticsPtr stats, Metadata *image_metadata);\n> Metadata &GetGlobalMetadata();\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index a474287..c02b5ec 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> constraint_mode_name_ = constraint_mode_name;\n> }\n>\n> +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +{\n> + // On a mode switch, it's possible the exposure profile could change,\n> + // so we run through the dividing up of exposure/gain again and\n> + // write the results into the metadata we've been given.\n> + if (status_.total_exposure_value) {\n> + housekeepConfig();\n> + divvyupExposure();\n> + writeAndFinish(metadata, false);\n> + }\n> +}\n> +\n> void Agc::Prepare(Metadata *image_metadata)\n> {\n> AgcStatus status;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index dbcefba..9a7e89c 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -75,6 +75,7 @@ public:\n> void SetMeteringMode(std::string const &metering_mode_name) override;\n> void SetExposureMode(std::string const &exposure_mode_name) override;\n> void SetConstraintMode(std::string const &contraint_mode_name) override;\n> + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> void Prepare(Metadata *image_metadata) override;\n> void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 821a0ca..76e2f04 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -173,8 +173,10 @@ void Alsc::Initialise()\n> lambda_r_[i] = lambda_b_[i] = 1.0;\n> }\n>\n> -void Alsc::SwitchMode(CameraMode const &camera_mode)\n> +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> + (void)metadata;\n> +\n> // There's a bit of a question what we should do if the \"crop\" of the\n> // camera mode has changed. Any calculation currently in flight would\n> // not be useful to the new mode, so arguably we should abort it, and\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> index c8ed3d2..3806257 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> @@ -50,7 +50,7 @@ public:\n> ~Alsc();\n> char const *Name() const override;\n> void Initialise() override;\n> - void SwitchMode(CameraMode const &camera_mode) override;\n> + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> void Read(boost::property_tree::ptree const ¶ms) override;\n> void Prepare(Metadata *image_metadata) override;\n> void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> index 2209d79..2cafde3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> @@ -27,8 +27,10 @@ char const *Noise::Name() const\n> return NAME;\n> }\n>\n> -void Noise::SwitchMode(CameraMode const &camera_mode)\n> +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> + (void)metadata;\n> +\n> // For example, we would expect a 2x2 binned mode to have a \"noise\n> // factor\" of sqrt(2x2) = 2. (can't be less than one, right?)\n> mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> index 51d46a3..25bf188 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> @@ -18,7 +18,7 @@ class Noise : public Algorithm\n> public:\n> Noise(Controller *controller);\n> char const *Name() const override;\n> - void SwitchMode(CameraMode const &camera_mode) override;\n> + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> void Read(boost::property_tree::ptree const ¶ms) override;\n> void Prepare(Metadata *image_metadata) override;\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> index 1f07bb6..086952f 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> @@ -26,8 +26,10 @@ char const *Sharpen::Name() const\n> return NAME;\n> }\n>\n> -void Sharpen::SwitchMode(CameraMode const &camera_mode)\n> +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> + (void)metadata;\n> +\n> // can't be less than one, right?\n> mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> }\n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> index 3b0d680..f871aa6 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> @@ -18,7 +18,7 @@ class Sharpen : public Algorithm\n> public:\n> Sharpen(Controller *controller);\n> char const *Name() const override;\n> - void SwitchMode(CameraMode const &camera_mode) override;\n> + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> void Read(boost::property_tree::ptree const ¶ms) override;\n> void Prepare(Metadata *image_metadata) override;\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9669f21..42c84b1 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> mistrust_count_ = helper_->MistrustFramesStartup();\n> }\n>\n> + struct AgcStatus agcStatus;\n> + /* These zero values mean not program anything (unless overwritten). */\n> + agcStatus.shutter_time = 0.0;\n> + agcStatus.analogue_gain = 0.0;\n> +\n> if (!controllerInit_) {\n> /* Load the tuning file for this sensor. */\n> controller_.Read(tuningFile_.c_str());\n> controller_.Initialise();\n> controllerInit_ = true;\n>\n> - /* Calculate initial values for gain and exposure. */\n> - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> -\n> - ControlList ctrls(unicam_ctrls_);\n> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> -\n> - IPAOperationData op;\n> - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> - op.controls.push_back(ctrls);\n> - queueFrameAction.emit(0, op);\n> + /* Supply initial values for gain and exposure. */\n> + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> }\n>\n> - controller_.SwitchMode(mode_);\n> + RPi::Metadata metadata;\n> + controller_.SwitchMode(mode_, &metadata);\n> +\n> + /* SwitchMode may supply updated exposure/gain values to use. */\n> + metadata.Get(\"agc.status\", agcStatus);\n> + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> + applyAGC(&agcStatus);\n>\n> lastMode_ = mode_;\n> }\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","Received":["from mail-ot1-x335.google.com (mail-ot1-x335.google.com\n\t[IPv6:2607:f8b0:4864:20::335])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B0C1603D8\n\tfor ;\n\tMon, 15 Jun 2020 17:21:11 +0200 (CEST)","by mail-ot1-x335.google.com with SMTP id n6so13413133otl.0\n\tfor ;\n\tMon, 15 Jun 2020 08:21:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"BJr4a+6m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=XClHXDiNbYZIz6aGeLMsa3YDLZJLV22Wvr44upi4g5Q=;\n\tb=BJr4a+6mhefctIGrGgIz+DomjtJzggNKu8WBrNwCTzExmPr4C2s6/SSq9O7FDs/dOi\n\ttuSrCAiXGDinVofDJzoPlv5bMfHgH7yJmtxA5uXQBsgY+YOsXNkWWdBCkhxRDiOa/DrY\n\tdP+VjMLqs7gwdHYl1mXTIJkbEMfgLTCHghtSdeEzooD1rNH/MpZOiEECLkHqGHMnqMiN\n\t7zOZ+uI/9wsm1ooiqw8DfQ0HShwxOLtKiQ+Z70o61fdN9ea0NEO6BqVfZZVSrxfWwywp\n\t30c9JDRcNXKr7PIcFdj/SdOjABp9bmheFpNLTp7K6EkfhvkSjajFrZE3s1ErAvZmFEAG\n\tCkUw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=XClHXDiNbYZIz6aGeLMsa3YDLZJLV22Wvr44upi4g5Q=;\n\tb=PgyRILqakE2jNcPysJFQPl+wgFzjVsIF5SwIOKOMNr7KwuJ5vH7L6SYK/XpDOYtbOk\n\tK9TrsFP3SYhzds0dy20xEawSaBVAqH7RuNjJl0IlFUA19MfmgAopp+YDKdOdnHPUfZHS\n\tVoWK/6W8j/Q0MZD83HOkdBFezH5c0NZgLFOjqzfENLlU2jnoLtKWNgCo4/3SvEGvlPvO\n\tdL6UcgRByOdLmRZ4WVWTQJ6GrkP140XmvxXnQtwSKRjBUXUD3DowwDSQPYmr6O/P4bOI\n\tRYrfKwa5tQiAZ5DGRVKA2ZHoS/Puypm6d+Ocdx9nVaJwHFCmce8owCpQ56fbdYHZy1Ya\n\trYnw==","X-Gm-Message-State":"AOAM531bZcmc9y1C0VMUoH7mQDVqislu0Ax2S5iw+phpPu7TE2sz9a09\n\tNSMOq3bi+jFYNDV4CJRVsQI3y6hNE1YGkihTo0npdyiJ","X-Google-Smtp-Source":"ABdhPJy0bWvagMZrGrbNZ/jnDvE8OntAC9eietBEZOOeJ3nacejdcWFOfnuuS5NllFqoTvXb6LLR+FPf+ieQKvbaBsM=","X-Received":"by 2002:a9d:4813:: with SMTP id\n\tc19mr22815800otf.317.1592234469085; \n\tMon, 15 Jun 2020 08:21:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142455.4641-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20200610142455.4641-1-david.plowman@raspberrypi.com>","From":"David Plowman ","Date":"Mon, 15 Jun 2020 16:20:58 +0100","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","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","X-List-Received-Date":"Mon, 15 Jun 2020 15:21:11 -0000"}},{"id":5262,"web_url":"https://patchwork.libcamera.org/comment/5262/","msgid":"<20200618021000.GH19884@pendragon.ideasonboard.com>","date":"2020-06-18T02:10:00","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch, and sorry for the late reply.\n\nOn Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote:\n> Hi\n> \n> I was wondering what folks wanted to do about reviewing patches like\n> this one, when they're very much in the domain of the Raspberry Pi\n> IPAs. Do we need a different procedure for them, do you think?\n\nIt's indeed difficult for us to review such patches, as they touch code\nwe're not familiar with. There's however value in upholding at least\npart of the existing process.\n\nSending the patches to the list is necessary for external review, but\nalso serves as a way to notify subscribers of what's going on. There may\nbe people reading the patches to educate themselves on the topic, even\nif they stay silent (and I certainly want to acquire more knowledge\nabout the Raspberry Pi IPA implementation, so I'm in that category).\nPosting all patches to the list also creates a single submission\nmechanism, which I would like to preserve.\n\nWhen it comes to mandatory review, if nobody considers themselves able\nto review a patch, then review should probably not be mandatory. In this\nspecific case, it would however be nice if someone else from Raspberry\nPi could review the code, provided there's at least another developer\nwho has enough expertise on the topic. The reason is simply that it's\ntoo easy for bugs to creep in, another pair of eyeballs at least\nglancing through the code is helpful. If that's not possible, so be it,\nand I'm fine accepting unreviewed code in that case.\n\nThere's one part of the process that needs to be updated though. We\nusually apply patches once we have reviewed them and consider them\nready. If nobody reviews a particular patch, then this will never\nhappen. One option would be for you to send a pull request once you\nconsider that the code is ready for integration, and you don't expect\nany other review. If sending a pull request is too complicated, I'm also\nfine with just a ping by e-mail (I may reconsider that in the future if\nthe volume of patches gets much larger, but for the foreseable future I\ndon't think it will be an issue).\n\nWould this work for you ?\n\nThis being said, please see my review below :-)\n\n> On Wed, 10 Jun 2020 at 15:24, David Plowman wrote:\n> >\n> > When the sensor is switched into a different mode the line\n> > timings may change, yet the V4L2 driver remembers the number of\n> > exposure lines, not the total time. So this will change \"under\n> > our feet\".\n> >\n> > These patches extend the IPA handling of the camera mode switch\n\ns/These patches extend/This patch extends/\n\nunless I've missed a patch :-)\n\n> > so that correct exposure values are recalculated for the new mode\n> > and written to the sensor before it starts streaming again. They\n> > also allow the IPA to alter how the total exposure is divided\n> > between actual exposure and analogue gain, according to the\n> > selected exposure profile in the camera tuning.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +-\n> > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +-\n> > src/ipa/raspberrypi/controller/controller.cpp | 4 +--\n> > src/ipa/raspberrypi/controller/controller.hpp | 2 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++-\n> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-\n> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++-\n> > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-\n> > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++-\n> > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++---------\n> > 13 files changed, 46 insertions(+), 24 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > index 9bd3df8..55cb201 100644\n> > --- a/src/ipa/raspberrypi/controller/algorithm.cpp\n> > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms)\n> >\n> > void Algorithm::Initialise() {}\n> >\n> > -void Algorithm::SwitchMode(CameraMode const &camera_mode)\n> > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > (void)camera_mode;\n> > + (void)metadata;\n\nWe don't enable unused arguments warnings today, so this isn't strictly\nnecessary, but we plan to enable them in the future. We haven't decided\nyet how to do so. One option would be to remove the parameter name from\nthe function definition:\n\nvoid Algorithm::SwitchMode(CameraMode const &, Metadata *)\n{\n}\n\nThere's however a concern that this would make the code less readable,\nat least for inline functions, as the only source of information of\nexpected parameter usage would be lost. We have considered\n\nvoid Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/)\n{\n}\n\nbut this was disliked by most people (I don't mind it much personally,\neven if the result doesn't look extremely pretty). Another option would\nbe to use [[maybe_unused]] once we switch to C++17 (which we very likely\nwill at some point, but we don't know when yet).\n\nConclusion: no need to change your patch, but if you have any feedback\non those options, feel free to voice it.\n\n> > }\n> >\n> > void Algorithm::Prepare(Metadata *image_metadata)\n> > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > index b82c184..187c50c 100644\n> > --- a/src/ipa/raspberrypi/controller/algorithm.hpp\n> > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > @@ -37,7 +37,7 @@ public:\n> > virtual void Resume() { paused_ = false; }\n> > virtual void Read(boost::property_tree::ptree const ¶ms);\n> > virtual void Initialise();\n> > - virtual void SwitchMode(CameraMode const &camera_mode);\n> > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n\nOn a general note, it would be nice to document the Algorithm class API,\nbut that's a separate issue.\n\n> > virtual void Prepare(Metadata *image_metadata);\n> > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata);\n> > Metadata &GetGlobalMetadata() const\n> > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> > index 20dd4c7..7c4b04f 100644\n> > --- a/src/ipa/raspberrypi/controller/controller.cpp\n> > +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> > @@ -56,11 +56,11 @@ void Controller::Initialise()\n> > RPI_LOG(\"Controller finished\");\n> > }\n> >\n> > -void Controller::SwitchMode(CameraMode const &camera_mode)\n> > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > RPI_LOG(\"Controller starting\");\n> > for (auto &algo : algorithms_)\n> > - algo->SwitchMode(camera_mode);\n> > + algo->SwitchMode(camera_mode, metadata);\n> > switch_mode_called_ = true;\n> > RPI_LOG(\"Controller finished\");\n> > }\n> > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp\n> > index d685386..6ba9412 100644\n> > --- a/src/ipa/raspberrypi/controller/controller.hpp\n> > +++ b/src/ipa/raspberrypi/controller/controller.hpp\n> > @@ -39,7 +39,7 @@ public:\n> > Algorithm *CreateAlgorithm(char const *name);\n> > void Read(char const *filename);\n> > void Initialise();\n> > - void SwitchMode(CameraMode const &camera_mode);\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> > void Prepare(Metadata *image_metadata);\n> > void Process(StatisticsPtr stats, Metadata *image_metadata);\n> > Metadata &GetGlobalMetadata();\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index a474287..c02b5ec 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> > constraint_mode_name_ = constraint_mode_name;\n> > }\n> >\n> > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > +{\n> > + // On a mode switch, it's possible the exposure profile could change,\n> > + // so we run through the dividing up of exposure/gain again and\n> > + // write the results into the metadata we've been given.\n> > + if (status_.total_exposure_value) {\n> > + housekeepConfig();\n> > + divvyupExposure();\n\nI had to use a dictionary to understand divvyup :-)\n\n> > + writeAndFinish(metadata, false);\n> > + }\n\nThis part I have trouble reviewing, so I'll just trust you.\n\n> > +}\n\nI think I would have split this patch in two parts, a first patch that\nadds the metadata parameter, and a second patch that uses it here and in\nIPARPi::configure(). There's no need to resubmit the patch for this\nreason, but I think that it would ease review, especially for the IPA\nmodule given our lack of familiarity with the code base.\n\n> > +\n> > void Agc::Prepare(Metadata *image_metadata)\n> > {\n> > AgcStatus status;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index dbcefba..9a7e89c 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -75,6 +75,7 @@ public:\n> > void SetMeteringMode(std::string const &metering_mode_name) override;\n> > void SetExposureMode(std::string const &exposure_mode_name) override;\n> > void SetConstraintMode(std::string const &contraint_mode_name) override;\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > void Prepare(Metadata *image_metadata) override;\n> > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index 821a0ca..76e2f04 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -173,8 +173,10 @@ void Alsc::Initialise()\n> > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > }\n> >\n> > -void Alsc::SwitchMode(CameraMode const &camera_mode)\n> > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > + (void)metadata;\n> > +\n> > // There's a bit of a question what we should do if the \"crop\" of the\n> > // camera mode has changed. Any calculation currently in flight would\n> > // not be useful to the new mode, so arguably we should abort it, and\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > index c8ed3d2..3806257 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > @@ -50,7 +50,7 @@ public:\n> > ~Alsc();\n> > char const *Name() const override;\n> > void Initialise() override;\n> > - void SwitchMode(CameraMode const &camera_mode) override;\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > void Read(boost::property_tree::ptree const ¶ms) override;\n> > void Prepare(Metadata *image_metadata) override;\n> > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > index 2209d79..2cafde3 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > @@ -27,8 +27,10 @@ char const *Noise::Name() const\n> > return NAME;\n> > }\n> >\n> > -void Noise::SwitchMode(CameraMode const &camera_mode)\n> > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > + (void)metadata;\n> > +\n> > // For example, we would expect a 2x2 binned mode to have a \"noise\n> > // factor\" of sqrt(2x2) = 2. (can't be less than one, right?)\n> > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > index 51d46a3..25bf188 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > @@ -18,7 +18,7 @@ class Noise : public Algorithm\n> > public:\n> > Noise(Controller *controller);\n> > char const *Name() const override;\n> > - void SwitchMode(CameraMode const &camera_mode) override;\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > void Read(boost::property_tree::ptree const ¶ms) override;\n> > void Prepare(Metadata *image_metadata) override;\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > index 1f07bb6..086952f 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const\n> > return NAME;\n> > }\n> >\n> > -void Sharpen::SwitchMode(CameraMode const &camera_mode)\n> > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > + (void)metadata;\n> > +\n> > // can't be less than one, right?\n> > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > }\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > index 3b0d680..f871aa6 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm\n> > public:\n> > Sharpen(Controller *controller);\n> > char const *Name() const override;\n> > - void SwitchMode(CameraMode const &camera_mode) override;\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > void Read(boost::property_tree::ptree const ¶ms) override;\n> > void Prepare(Metadata *image_metadata) override;\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9669f21..42c84b1 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > mistrust_count_ = helper_->MistrustFramesStartup();\n> > }\n> >\n> > + struct AgcStatus agcStatus;\n> > + /* These zero values mean not program anything (unless overwritten). */\n> > + agcStatus.shutter_time = 0.0;\n> > + agcStatus.analogue_gain = 0.0;\n> > +\n> > if (!controllerInit_) {\n> > /* Load the tuning file for this sensor. */\n> > controller_.Read(tuningFile_.c_str());\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > - /* Calculate initial values for gain and exposure. */\n> > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> > -\n> > - ControlList ctrls(unicam_ctrls_);\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > -\n> > - IPAOperationData op;\n> > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> > - op.controls.push_back(ctrls);\n> > - queueFrameAction.emit(0, op);\n> > + /* Supply initial values for gain and exposure. */\n> > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > }\n> >\n> > - controller_.SwitchMode(mode_);\n> > + RPi::Metadata metadata;\n> > + controller_.SwitchMode(mode_, &metadata);\n> > +\n> > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > + metadata.Get(\"agc.status\", agcStatus);\n> > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> > + applyAGC(&agcStatus);\n\nThere's something I don't quite get. You mention in the commit message\nthat the issue is caused by the length of the line being changed.\nWouldn't it be enough to move the control setting code outside of the\n!controllerInit_ section ? Or is my understand correct, and the larger\nrework here is related to the last part of the commit message, the\nrecalculation of the split between exposure time and gain ?\n\nWith the small fix to the commit message,\n\nReviewed-by: Laurent Pinchart \n\nPlease let me know if you would like to submit a new version with\nadditional changes based on the comments above, or if you would like me\nto apply this one.\n\n> >\n> > lastMode_ = mode_;\n> > }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DD67603C1\n\tfor ;\n\tThu, 18 Jun 2020 04:10:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD242F9;\n\tThu, 18 Jun 2020 04:10:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tKEdZajD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592446223;\n\tbh=JEZTBv3kDPi6Bq9IqEkYMpOhSIjrFkjWELHDiE+2f9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tKEdZajD8k6W4kGctWacnmoO5xpuFNhucgm/NiomOzEqVMv0OOPJfOcI6vkz4Zc/u\n\t4ioinBJC3bBGeYD2KKpHdvf2YCGd5V5FKtczU2w86oJ3SSyL/t+RzTg9Y6yb1RFGGq\n\tx8GGP4uWR5I4HxxW8IIxs0kgNPeIF8lNzm9hHtT0=","Date":"Thu, 18 Jun 2020 05:10:00 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200618021000.GH19884@pendragon.ideasonboard.com>","References":"<20200610142455.4641-1-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","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","X-List-Received-Date":"Thu, 18 Jun 2020 02:10:24 -0000"}},{"id":5272,"web_url":"https://patchwork.libcamera.org/comment/5272/","msgid":"","date":"2020-06-18T09:46:48","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Thu, 18 Jun 2020 at 03:10, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch, and sorry for the late reply.\n>\n> On Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote:\n> > Hi\n> >\n> > I was wondering what folks wanted to do about reviewing patches like\n> > this one, when they're very much in the domain of the Raspberry Pi\n> > IPAs. Do we need a different procedure for them, do you think?\n>\n> It's indeed difficult for us to review such patches, as they touch code\n> we're not familiar with. There's however value in upholding at least\n> part of the existing process.\n>\n> Sending the patches to the list is necessary for external review, but\n> also serves as a way to notify subscribers of what's going on. There may\n> be people reading the patches to educate themselves on the topic, even\n> if they stay silent (and I certainly want to acquire more knowledge\n> about the Raspberry Pi IPA implementation, so I'm in that category).\n> Posting all patches to the list also creates a single submission\n> mechanism, which I would like to preserve.\n>\n> When it comes to mandatory review, if nobody considers themselves able\n> to review a patch, then review should probably not be mandatory. In this\n> specific case, it would however be nice if someone else from Raspberry\n> Pi could review the code, provided there's at least another developer\n> who has enough expertise on the topic. The reason is simply that it's\n> too easy for bugs to creep in, another pair of eyeballs at least\n> glancing through the code is helpful. If that's not possible, so be it,\n> and I'm fine accepting unreviewed code in that case.\n>\n> There's one part of the process that needs to be updated though. We\n> usually apply patches once we have reviewed them and consider them\n> ready. If nobody reviews a particular patch, then this will never\n> happen. One option would be for you to send a pull request once you\n> consider that the code is ready for integration, and you don't expect\n> any other review. If sending a pull request is too complicated, I'm also\n> fine with just a ping by e-mail (I may reconsider that in the future if\n> the volume of patches gets much larger, but for the foreseable future I\n> don't think it will be an issue).\n>\n> Would this work for you ?\n\nThanks for your thoughts. I think this works for us. It seems\nsensible that commits in this area should be reviewed by at\nleast one other Pi person.\n\n>\n> This being said, please see my review below :-)\n>\n> > On Wed, 10 Jun 2020 at 15:24, David Plowman wrote:\n> > >\n> > > When the sensor is switched into a different mode the line\n> > > timings may change, yet the V4L2 driver remembers the number of\n> > > exposure lines, not the total time. So this will change \"under\n> > > our feet\".\n> > >\n> > > These patches extend the IPA handling of the camera mode switch\n>\n> s/These patches extend/This patch extends/\n>\n> unless I've missed a patch :-)\n>\n> > > so that correct exposure values are recalculated for the new mode\n> > > and written to the sensor before it starts streaming again. They\n> > > also allow the IPA to alter how the total exposure is divided\n> > > between actual exposure and analogue gain, according to the\n> > > selected exposure profile in the camera tuning.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +-\n> > > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +-\n> > > src/ipa/raspberrypi/controller/controller.cpp | 4 +--\n> > > src/ipa/raspberrypi/controller/controller.hpp | 2 +-\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++\n> > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++-\n> > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-\n> > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++-\n> > > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-\n> > > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++-\n> > > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++---------\n> > > 13 files changed, 46 insertions(+), 24 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > index 9bd3df8..55cb201 100644\n> > > --- a/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms)\n> > >\n> > > void Algorithm::Initialise() {}\n> > >\n> > > -void Algorithm::SwitchMode(CameraMode const &camera_mode)\n> > > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > (void)camera_mode;\n> > > + (void)metadata;\n>\n> We don't enable unused arguments warnings today, so this isn't strictly\n> necessary, but we plan to enable them in the future. We haven't decided\n> yet how to do so. One option would be to remove the parameter name from\n> the function definition:\n>\n> void Algorithm::SwitchMode(CameraMode const &, Metadata *)\n> {\n> }\n>\n> There's however a concern that this would make the code less readable,\n> at least for inline functions, as the only source of information of\n> expected parameter usage would be lost. We have considered\n>\n> void Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/)\n> {\n> }\n>\n> but this was disliked by most people (I don't mind it much personally,\n> even if the result doesn't look extremely pretty). Another option would\n> be to use [[maybe_unused]] once we switch to C++17 (which we very likely\n> will at some point, but we don't know when yet).\n>\n> Conclusion: no need to change your patch, but if you have any feedback\n> on those options, feel free to voice it.\n\nTricky one. For now I would agree that \"(void)parameter;\" is no\nworse than anything else!\n\n>\n> > > }\n> > >\n> > > void Algorithm::Prepare(Metadata *image_metadata)\n> > > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > index b82c184..187c50c 100644\n> > > --- a/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > @@ -37,7 +37,7 @@ public:\n> > > virtual void Resume() { paused_ = false; }\n> > > virtual void Read(boost::property_tree::ptree const ¶ms);\n> > > virtual void Initialise();\n> > > - virtual void SwitchMode(CameraMode const &camera_mode);\n> > > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n>\n> On a general note, it would be nice to document the Algorithm class API,\n> but that's a separate issue.\n\nCertainly. There's obviously documentation in the big Pi/libcamera pdf,\nbut you have in mind something more in the code, in the header file\nitself? If you can point me at what you consider a good example to\nfollow, then I'll definitely have a go!\n\n>\n> > > virtual void Prepare(Metadata *image_metadata);\n> > > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata);\n> > > Metadata &GetGlobalMetadata() const\n> > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> > > index 20dd4c7..7c4b04f 100644\n> > > --- a/src/ipa/raspberrypi/controller/controller.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> > > @@ -56,11 +56,11 @@ void Controller::Initialise()\n> > > RPI_LOG(\"Controller finished\");\n> > > }\n> > >\n> > > -void Controller::SwitchMode(CameraMode const &camera_mode)\n> > > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > RPI_LOG(\"Controller starting\");\n> > > for (auto &algo : algorithms_)\n> > > - algo->SwitchMode(camera_mode);\n> > > + algo->SwitchMode(camera_mode, metadata);\n> > > switch_mode_called_ = true;\n> > > RPI_LOG(\"Controller finished\");\n> > > }\n> > > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp\n> > > index d685386..6ba9412 100644\n> > > --- a/src/ipa/raspberrypi/controller/controller.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/controller.hpp\n> > > @@ -39,7 +39,7 @@ public:\n> > > Algorithm *CreateAlgorithm(char const *name);\n> > > void Read(char const *filename);\n> > > void Initialise();\n> > > - void SwitchMode(CameraMode const &camera_mode);\n> > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> > > void Prepare(Metadata *image_metadata);\n> > > void Process(StatisticsPtr stats, Metadata *image_metadata);\n> > > Metadata &GetGlobalMetadata();\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index a474287..c02b5ec 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> > > constraint_mode_name_ = constraint_mode_name;\n> > > }\n> > >\n> > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > +{\n> > > + // On a mode switch, it's possible the exposure profile could change,\n> > > + // so we run through the dividing up of exposure/gain again and\n> > > + // write the results into the metadata we've been given.\n> > > + if (status_.total_exposure_value) {\n> > > + housekeepConfig();\n> > > + divvyupExposure();\n>\n> I had to use a dictionary to understand divvyup :-)\n\nSorry... (and there's a serious point in that using unfamiliar words\nis unhelpful now that this code is public)\n\n>\n> > > + writeAndFinish(metadata, false);\n> > > + }\n>\n> This part I have trouble reviewing, so I'll just trust you.\n>\n> > > +}\n>\n> I think I would have split this patch in two parts, a first patch that\n> adds the metadata parameter, and a second patch that uses it here and in\n> IPARPi::configure(). There's no need to resubmit the patch for this\n> reason, but I think that it would ease review, especially for the IPA\n> module given our lack of familiarity with the code base.\n\nUnderstood, I see that now that you mention it. (Still learning!)\n\n>\n> > > +\n> > > void Agc::Prepare(Metadata *image_metadata)\n> > > {\n> > > AgcStatus status;\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > index dbcefba..9a7e89c 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > @@ -75,6 +75,7 @@ public:\n> > > void SetMeteringMode(std::string const &metering_mode_name) override;\n> > > void SetExposureMode(std::string const &exposure_mode_name) override;\n> > > void SetConstraintMode(std::string const &contraint_mode_name) override;\n> > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > void Prepare(Metadata *image_metadata) override;\n> > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > index 821a0ca..76e2f04 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > @@ -173,8 +173,10 @@ void Alsc::Initialise()\n> > > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > > }\n> > >\n> > > -void Alsc::SwitchMode(CameraMode const &camera_mode)\n> > > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > + (void)metadata;\n> > > +\n> > > // There's a bit of a question what we should do if the \"crop\" of the\n> > > // camera mode has changed. Any calculation currently in flight would\n> > > // not be useful to the new mode, so arguably we should abort it, and\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > index c8ed3d2..3806257 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > @@ -50,7 +50,7 @@ public:\n> > > ~Alsc();\n> > > char const *Name() const override;\n> > > void Initialise() override;\n> > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > void Prepare(Metadata *image_metadata) override;\n> > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > index 2209d79..2cafde3 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > @@ -27,8 +27,10 @@ char const *Noise::Name() const\n> > > return NAME;\n> > > }\n> > >\n> > > -void Noise::SwitchMode(CameraMode const &camera_mode)\n> > > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > + (void)metadata;\n> > > +\n> > > // For example, we would expect a 2x2 binned mode to have a \"noise\n> > > // factor\" of sqrt(2x2) = 2. (can't be less than one, right?)\n> > > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > index 51d46a3..25bf188 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > @@ -18,7 +18,7 @@ class Noise : public Algorithm\n> > > public:\n> > > Noise(Controller *controller);\n> > > char const *Name() const override;\n> > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > void Prepare(Metadata *image_metadata) override;\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > index 1f07bb6..086952f 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const\n> > > return NAME;\n> > > }\n> > >\n> > > -void Sharpen::SwitchMode(CameraMode const &camera_mode)\n> > > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > + (void)metadata;\n> > > +\n> > > // can't be less than one, right?\n> > > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > > }\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > index 3b0d680..f871aa6 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm\n> > > public:\n> > > Sharpen(Controller *controller);\n> > > char const *Name() const override;\n> > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > void Prepare(Metadata *image_metadata) override;\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 9669f21..42c84b1 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > mistrust_count_ = helper_->MistrustFramesStartup();\n> > > }\n> > >\n> > > + struct AgcStatus agcStatus;\n> > > + /* These zero values mean not program anything (unless overwritten). */\n> > > + agcStatus.shutter_time = 0.0;\n> > > + agcStatus.analogue_gain = 0.0;\n> > > +\n> > > if (!controllerInit_) {\n> > > /* Load the tuning file for this sensor. */\n> > > controller_.Read(tuningFile_.c_str());\n> > > controller_.Initialise();\n> > > controllerInit_ = true;\n> > >\n> > > - /* Calculate initial values for gain and exposure. */\n> > > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> > > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> > > -\n> > > - ControlList ctrls(unicam_ctrls_);\n> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > > -\n> > > - IPAOperationData op;\n> > > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> > > - op.controls.push_back(ctrls);\n> > > - queueFrameAction.emit(0, op);\n> > > + /* Supply initial values for gain and exposure. */\n> > > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > > }\n> > >\n> > > - controller_.SwitchMode(mode_);\n> > > + RPi::Metadata metadata;\n> > > + controller_.SwitchMode(mode_, &metadata);\n> > > +\n> > > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > > + metadata.Get(\"agc.status\", agcStatus);\n> > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> > > + applyAGC(&agcStatus);\n>\n> There's something I don't quite get. You mention in the commit message\n> that the issue is caused by the length of the line being changed.\n> Wouldn't it be enough to move the control setting code outside of the\n> !controllerInit_ section ? Or is my understand correct, and the larger\n> rework here is related to the last part of the commit message, the\n> recalculation of the split between exposure time and gain ?\n\nYes, that's right.\n\n>\n> With the small fix to the commit message,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> Please let me know if you would like to submit a new version with\n> additional changes based on the comments above, or if you would like me\n> to apply this one.\n\nI'd quite like to submit a version 2, split into two separate commits as\nyou suggested. I need the practice!!\n\nBest regards\nDavid\n\n>\n> > >\n> > > lastMode_ = mode_;\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-ot1-x341.google.com (mail-ot1-x341.google.com\n\t[IPv6:2607:f8b0:4864:20::341])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7C5061167\n\tfor ;\n\tThu, 18 Jun 2020 11:47:00 +0200 (CEST)","by mail-ot1-x341.google.com with SMTP id n70so4042190ota.5\n\tfor ;\n\tThu, 18 Jun 2020 02:47:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"GctVwJ+L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=iAS7ELqlAun3KE6h1iVMlo9+Pie0kPD1tnQLBweCNmc=;\n\tb=GctVwJ+LeS5KwqN0Qmit0UROh48WYaRrfjseZfn131SuV5xggqgQJXE0QihVzFJmko\n\tV3pEe83xT/7N2Z5LGB6muu6G4mVfmW/wxD0iv8WTnsjmf0I5D84WQ8WTK3W5ugOy//Ai\n\tJQ/D2pA+9PNmniZ19CsNbNmYnjgKdlJnXy8HhiZFPq4WyQt490Vd7S68TULnKRTKOdyI\n\tiu5JYGJlYVMeqPjhJhdCSziQn162uA02Cv66xivVWUWCY9TelcsXrDWux0h6knJ19Ygv\n\t8XFYJvBLkNvSZLWbvamvJcvLivQdZ/OMG4GgqsjwC+iVpp9A4mtpNA8QgUlACjqgzW2+\n\tTL3w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=iAS7ELqlAun3KE6h1iVMlo9+Pie0kPD1tnQLBweCNmc=;\n\tb=sRHMpUMWIe9FpQWkPAKzW172+L1uSBWIi0OUUZvq46urlbRG84UKHkyFAqeinb9FdO\n\trc/WHcq4VRgoei0oU1kDTwa2FoNFTW8zIkpNzBIZwnmSNwnNJe5/xJ2Wk5evDHJn84oH\n\tz2PezKeeFOz9UvG1X+eiRS2q0iSsKvwLhvjFWu3r2yDc0GUeynARa9XCcNkvtCyls5/I\n\tWCWQRT7D9VeoFEj8NICb4cKmYWGRag5s/ZzBwdDkNjbEcZ3TyyVakoP6I7quUiLhluPB\n\tQ2ZdvDEgFAmGwN7aJq5rxatBWedMOai/ItCFLH0ZrnSjuEFLg1EnW0TEm59WHMypoqzg\n\t1dfA==","X-Gm-Message-State":"AOAM532omDM7W84pDqnjcySaqaO66YgEXCiOJ0R1gj1p4xN2WzUWSpp4\n\tKB7iisuNRtIXjuqfXm33ZKSJcsH/a7VeP+kisEqqEydQ0+w=","X-Google-Smtp-Source":"ABdhPJxiTFhKDYhqDzQNnY4Yie5RvFf47ds0gP78p0Zqa/QvRn0MqxmZ0ajQyr+hTm80UjeGaGpZGNqugDrUk3z5KxE=","X-Received":"by 2002:a9d:5604:: with SMTP id e4mr2665863oti.166.1592473619167;\n\tThu, 18 Jun 2020 02:46:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142455.4641-1-david.plowman@raspberrypi.com>\n\t\n\t<20200618021000.GH19884@pendragon.ideasonboard.com>","In-Reply-To":"<20200618021000.GH19884@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Thu, 18 Jun 2020 10:46:48 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","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","X-List-Received-Date":"Thu, 18 Jun 2020 09:47:01 -0000"}},{"id":5313,"web_url":"https://patchwork.libcamera.org/comment/5313/","msgid":"<20200622024043.GH25355@pendragon.ideasonboard.com>","date":"2020-06-22T02:40:43","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Thu, Jun 18, 2020 at 10:46:48AM +0100, David Plowman wrote:\n> On Thu, 18 Jun 2020 at 03:10, Laurent Pinchart wrote:\n> > On Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote:\n> > > Hi\n> > >\n> > > I was wondering what folks wanted to do about reviewing patches like\n> > > this one, when they're very much in the domain of the Raspberry Pi\n> > > IPAs. Do we need a different procedure for them, do you think?\n> >\n> > It's indeed difficult for us to review such patches, as they touch code\n> > we're not familiar with. There's however value in upholding at least\n> > part of the existing process.\n> >\n> > Sending the patches to the list is necessary for external review, but\n> > also serves as a way to notify subscribers of what's going on. There may\n> > be people reading the patches to educate themselves on the topic, even\n> > if they stay silent (and I certainly want to acquire more knowledge\n> > about the Raspberry Pi IPA implementation, so I'm in that category).\n> > Posting all patches to the list also creates a single submission\n> > mechanism, which I would like to preserve.\n> >\n> > When it comes to mandatory review, if nobody considers themselves able\n> > to review a patch, then review should probably not be mandatory. In this\n> > specific case, it would however be nice if someone else from Raspberry\n> > Pi could review the code, provided there's at least another developer\n> > who has enough expertise on the topic. The reason is simply that it's\n> > too easy for bugs to creep in, another pair of eyeballs at least\n> > glancing through the code is helpful. If that's not possible, so be it,\n> > and I'm fine accepting unreviewed code in that case.\n> >\n> > There's one part of the process that needs to be updated though. We\n> > usually apply patches once we have reviewed them and consider them\n> > ready. If nobody reviews a particular patch, then this will never\n> > happen. One option would be for you to send a pull request once you\n> > consider that the code is ready for integration, and you don't expect\n> > any other review. If sending a pull request is too complicated, I'm also\n> > fine with just a ping by e-mail (I may reconsider that in the future if\n> > the volume of patches gets much larger, but for the foreseable future I\n> > don't think it will be an issue).\n> >\n> > Would this work for you ?\n> \n> Thanks for your thoughts. I think this works for us. It seems\n> sensible that commits in this area should be reviewed by at\n> least one other Pi person.\n> \n> > This being said, please see my review below :-)\n> >\n> > > On Wed, 10 Jun 2020 at 15:24, David Plowman wrote:\n> > > >\n> > > > When the sensor is switched into a different mode the line\n> > > > timings may change, yet the V4L2 driver remembers the number of\n> > > > exposure lines, not the total time. So this will change \"under\n> > > > our feet\".\n> > > >\n> > > > These patches extend the IPA handling of the camera mode switch\n> >\n> > s/These patches extend/This patch extends/\n> >\n> > unless I've missed a patch :-)\n> >\n> > > > so that correct exposure values are recalculated for the new mode\n> > > > and written to the sensor before it starts streaming again. They\n> > > > also allow the IPA to alter how the total exposure is divided\n> > > > between actual exposure and analogue gain, according to the\n> > > > selected exposure profile in the camera tuning.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +-\n> > > > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +-\n> > > > src/ipa/raspberrypi/controller/controller.cpp | 4 +--\n> > > > src/ipa/raspberrypi/controller/controller.hpp | 2 +-\n> > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++\n> > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++-\n> > > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-\n> > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++-\n> > > > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-\n> > > > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++-\n> > > > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++---------\n> > > > 13 files changed, 46 insertions(+), 24 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > > index 9bd3df8..55cb201 100644\n> > > > --- a/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp\n> > > > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms)\n> > > >\n> > > > void Algorithm::Initialise() {}\n> > > >\n> > > > -void Algorithm::SwitchMode(CameraMode const &camera_mode)\n> > > > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > (void)camera_mode;\n> > > > + (void)metadata;\n> >\n> > We don't enable unused arguments warnings today, so this isn't strictly\n> > necessary, but we plan to enable them in the future. We haven't decided\n> > yet how to do so. One option would be to remove the parameter name from\n> > the function definition:\n> >\n> > void Algorithm::SwitchMode(CameraMode const &, Metadata *)\n> > {\n> > }\n> >\n> > There's however a concern that this would make the code less readable,\n> > at least for inline functions, as the only source of information of\n> > expected parameter usage would be lost. We have considered\n> >\n> > void Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/)\n> > {\n> > }\n> >\n> > but this was disliked by most people (I don't mind it much personally,\n> > even if the result doesn't look extremely pretty). Another option would\n> > be to use [[maybe_unused]] once we switch to C++17 (which we very likely\n> > will at some point, but we don't know when yet).\n> >\n> > Conclusion: no need to change your patch, but if you have any feedback\n> > on those options, feel free to voice it.\n> \n> Tricky one. For now I would agree that \"(void)parameter;\" is no\n> worse than anything else!\n> \n> > > > }\n> > > >\n> > > > void Algorithm::Prepare(Metadata *image_metadata)\n> > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > > index b82c184..187c50c 100644\n> > > > --- a/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp\n> > > > @@ -37,7 +37,7 @@ public:\n> > > > virtual void Resume() { paused_ = false; }\n> > > > virtual void Read(boost::property_tree::ptree const ¶ms);\n> > > > virtual void Initialise();\n> > > > - virtual void SwitchMode(CameraMode const &camera_mode);\n> > > > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> >\n> > On a general note, it would be nice to document the Algorithm class API,\n> > but that's a separate issue.\n> \n> Certainly. There's obviously documentation in the big Pi/libcamera pdf,\n> but you have in mind something more in the code, in the header file\n> itself? If you can point me at what you consider a good example to\n> follow, then I'll definitely have a go!\n\nI'm thinking about doxygen comments, in the .cpp file, like we do for\nthe libcamera core.\n\n> > > > virtual void Prepare(Metadata *image_metadata);\n> > > > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata);\n> > > > Metadata &GetGlobalMetadata() const\n> > > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> > > > index 20dd4c7..7c4b04f 100644\n> > > > --- a/src/ipa/raspberrypi/controller/controller.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> > > > @@ -56,11 +56,11 @@ void Controller::Initialise()\n> > > > RPI_LOG(\"Controller finished\");\n> > > > }\n> > > >\n> > > > -void Controller::SwitchMode(CameraMode const &camera_mode)\n> > > > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > RPI_LOG(\"Controller starting\");\n> > > > for (auto &algo : algorithms_)\n> > > > - algo->SwitchMode(camera_mode);\n> > > > + algo->SwitchMode(camera_mode, metadata);\n> > > > switch_mode_called_ = true;\n> > > > RPI_LOG(\"Controller finished\");\n> > > > }\n> > > > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp\n> > > > index d685386..6ba9412 100644\n> > > > --- a/src/ipa/raspberrypi/controller/controller.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/controller.hpp\n> > > > @@ -39,7 +39,7 @@ public:\n> > > > Algorithm *CreateAlgorithm(char const *name);\n> > > > void Read(char const *filename);\n> > > > void Initialise();\n> > > > - void SwitchMode(CameraMode const &camera_mode);\n> > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> > > > void Prepare(Metadata *image_metadata);\n> > > > void Process(StatisticsPtr stats, Metadata *image_metadata);\n> > > > Metadata &GetGlobalMetadata();\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > index a474287..c02b5ec 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> > > > constraint_mode_name_ = constraint_mode_name;\n> > > > }\n> > > >\n> > > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > +{\n> > > > + // On a mode switch, it's possible the exposure profile could change,\n> > > > + // so we run through the dividing up of exposure/gain again and\n> > > > + // write the results into the metadata we've been given.\n> > > > + if (status_.total_exposure_value) {\n> > > > + housekeepConfig();\n> > > > + divvyupExposure();\n> >\n> > I had to use a dictionary to understand divvyup :-)\n> \n> Sorry... (and there's a serious point in that using unfamiliar words\n> is unhelpful now that this code is public)\n> \n> > > > + writeAndFinish(metadata, false);\n> > > > + }\n> >\n> > This part I have trouble reviewing, so I'll just trust you.\n> >\n> > > > +}\n> >\n> > I think I would have split this patch in two parts, a first patch that\n> > adds the metadata parameter, and a second patch that uses it here and in\n> > IPARPi::configure(). There's no need to resubmit the patch for this\n> > reason, but I think that it would ease review, especially for the IPA\n> > module given our lack of familiarity with the code base.\n> \n> Understood, I see that now that you mention it. (Still learning!)\n> \n> > > > +\n> > > > void Agc::Prepare(Metadata *image_metadata)\n> > > > {\n> > > > AgcStatus status;\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > index dbcefba..9a7e89c 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > @@ -75,6 +75,7 @@ public:\n> > > > void SetMeteringMode(std::string const &metering_mode_name) override;\n> > > > void SetExposureMode(std::string const &exposure_mode_name) override;\n> > > > void SetConstraintMode(std::string const &contraint_mode_name) override;\n> > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > > void Prepare(Metadata *image_metadata) override;\n> > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > index 821a0ca..76e2f04 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > @@ -173,8 +173,10 @@ void Alsc::Initialise()\n> > > > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > > > }\n> > > >\n> > > > -void Alsc::SwitchMode(CameraMode const &camera_mode)\n> > > > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > + (void)metadata;\n> > > > +\n> > > > // There's a bit of a question what we should do if the \"crop\" of the\n> > > > // camera mode has changed. Any calculation currently in flight would\n> > > > // not be useful to the new mode, so arguably we should abort it, and\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > index c8ed3d2..3806257 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > @@ -50,7 +50,7 @@ public:\n> > > > ~Alsc();\n> > > > char const *Name() const override;\n> > > > void Initialise() override;\n> > > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > > void Prepare(Metadata *image_metadata) override;\n> > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > > index 2209d79..2cafde3 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> > > > @@ -27,8 +27,10 @@ char const *Noise::Name() const\n> > > > return NAME;\n> > > > }\n> > > >\n> > > > -void Noise::SwitchMode(CameraMode const &camera_mode)\n> > > > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > + (void)metadata;\n> > > > +\n> > > > // For example, we would expect a 2x2 binned mode to have a \"noise\n> > > > // factor\" of sqrt(2x2) = 2. (can't be less than one, right?)\n> > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > > index 51d46a3..25bf188 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> > > > @@ -18,7 +18,7 @@ class Noise : public Algorithm\n> > > > public:\n> > > > Noise(Controller *controller);\n> > > > char const *Name() const override;\n> > > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > > void Prepare(Metadata *image_metadata) override;\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > > index 1f07bb6..086952f 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > > > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const\n> > > > return NAME;\n> > > > }\n> > > >\n> > > > -void Sharpen::SwitchMode(CameraMode const &camera_mode)\n> > > > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > + (void)metadata;\n> > > > +\n> > > > // can't be less than one, right?\n> > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor);\n> > > > }\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > > index 3b0d680..f871aa6 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > > > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm\n> > > > public:\n> > > > Sharpen(Controller *controller);\n> > > > char const *Name() const override;\n> > > > - void SwitchMode(CameraMode const &camera_mode) override;\n> > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > > void Prepare(Metadata *image_metadata) override;\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 9669f21..42c84b1 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > mistrust_count_ = helper_->MistrustFramesStartup();\n> > > > }\n> > > >\n> > > > + struct AgcStatus agcStatus;\n> > > > + /* These zero values mean not program anything (unless overwritten). */\n> > > > + agcStatus.shutter_time = 0.0;\n> > > > + agcStatus.analogue_gain = 0.0;\n> > > > +\n> > > > if (!controllerInit_) {\n> > > > /* Load the tuning file for this sensor. */\n> > > > controller_.Read(tuningFile_.c_str());\n> > > > controller_.Initialise();\n> > > > controllerInit_ = true;\n> > > >\n> > > > - /* Calculate initial values for gain and exposure. */\n> > > > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> > > > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> > > > -\n> > > > - ControlList ctrls(unicam_ctrls_);\n> > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > > > -\n> > > > - IPAOperationData op;\n> > > > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> > > > - op.controls.push_back(ctrls);\n> > > > - queueFrameAction.emit(0, op);\n> > > > + /* Supply initial values for gain and exposure. */\n> > > > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > > > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > > > }\n> > > >\n> > > > - controller_.SwitchMode(mode_);\n> > > > + RPi::Metadata metadata;\n> > > > + controller_.SwitchMode(mode_, &metadata);\n> > > > +\n> > > > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > > > + metadata.Get(\"agc.status\", agcStatus);\n> > > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> > > > + applyAGC(&agcStatus);\n> >\n> > There's something I don't quite get. You mention in the commit message\n> > that the issue is caused by the length of the line being changed.\n> > Wouldn't it be enough to move the control setting code outside of the\n> > !controllerInit_ section ? Or is my understand correct, and the larger\n> > rework here is related to the last part of the commit message, the\n> > recalculation of the split between exposure time and gain ?\n> \n> Yes, that's right.\n> \n> > With the small fix to the commit message,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > Please let me know if you would like to submit a new version with\n> > additional changes based on the comments above, or if you would like me\n> > to apply this one.\n> \n> I'd quite like to submit a version 2, split into two separate commits as\n> you suggested. I need the practice!!\n\nThanks :-) I'll review it now.\n\n> > > >\n> > > > lastMode_ = mode_;\n> > > > }","headers":{"Return-Path":"","Received":["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 51A99609A3\n\tfor ;\n\tMon, 22 Jun 2020 04:41:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1E2030D;\n\tMon, 22 Jun 2020 04:41:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nXuLqEdG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592793668;\n\tbh=9CJ2GEXUwL0sdc/AmN+B15O4Bl3rMQMfl2gtbKb/qI8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nXuLqEdG8AwkWzZVJ5MPTq4holY3WxrXBPortubCAG4qV4iKq+Z3tsJv/alWFbQBN\n\tjVhFnZ0ziBaQK+UvlgVJRGzV9ijK5b8lHJo60QjNSY0g/G6bXpWbPIi1WHsaOFPs8r\n\tDvqloYJgHZsvUKR4npfwLZOwew/Rtfnt5lq+km7E=","Date":"Mon, 22 Jun 2020 05:40:43 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200622024043.GH25355@pendragon.ideasonboard.com>","References":"<20200610142455.4641-1-david.plowman@raspberrypi.com>\n\t\n\t<20200618021000.GH19884@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: correct\n\texposure values when camera mode changes","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","X-List-Received-Date":"Mon, 22 Jun 2020 02:41:08 -0000"}}]