| Message ID | 20201126123203.19105-4-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Thu, Nov 26, 2020 at 12:32:02PM +0000, David Plowman wrote: > AE/AGC "disabled" is now handled better by the algorithm for itself, > so it no longer needs to be "resumed" before setting fixed shutter or > gain values. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 9853a343..e437c626 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls) > case controls::AE_ENABLE: { > RPiController::Algorithm *agc = controller_.GetAlgorithm("agc"); > ASSERT(agc); > + It's customary, when including unrelated minor changes such as this, to mention it in the commit message, to let reviewers know the change didn't get included by mistake. > if (ctrl.second.get<bool>() == false) > agc->Pause(); > else > @@ -512,10 +513,6 @@ void IPARPi::queueRequest(const ControlList &controls) > /* This expects units of micro-seconds. */ > agc->SetFixedShutter(ctrl.second.get<int32_t>()); > > - /* For the manual values to take effect, AGC must be unpaused. */ > - if (agc->IsPaused()) > - agc->Resume(); > - Only AWB now uses the pause mechanism. I wonder if it will survive, or be dropped there too for the same reason as for the AGC :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); > break; > } > @@ -526,10 +523,6 @@ void IPARPi::queueRequest(const ControlList &controls) > ASSERT(agc); > agc->SetFixedAnalogueGain(ctrl.second.get<float>()); > > - /* For the manual values to take effect, AGC must be unpaused. */ > - if (agc->IsPaused()) > - agc->Resume(); > - > libcameraMetadata_.set(controls::AnalogueGain, > ctrl.second.get<float>()); > break;
Hi Laurent Thanks for the review. On Thu, 26 Nov 2020 at 12:58, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Thu, Nov 26, 2020 at 12:32:02PM +0000, David Plowman wrote: > > AE/AGC "disabled" is now handled better by the algorithm for itself, > > so it no longer needs to be "resumed" before setting fixed shutter or > > gain values. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 9853a343..e437c626 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls) > > case controls::AE_ENABLE: { > > RPiController::Algorithm *agc = controller_.GetAlgorithm("agc"); > > ASSERT(agc); > > + > > It's customary, when including unrelated minor changes such as this, to > mention it in the commit message, to let reviewers know the change > didn't get included by mistake. It was a mistake, of course! :) I'll remove it and address those other points (about the override). Thanks David > > > if (ctrl.second.get<bool>() == false) > > agc->Pause(); > > else > > @@ -512,10 +513,6 @@ void IPARPi::queueRequest(const ControlList &controls) > > /* This expects units of micro-seconds. */ > > agc->SetFixedShutter(ctrl.second.get<int32_t>()); > > > > - /* For the manual values to take effect, AGC must be unpaused. */ > > - if (agc->IsPaused()) > > - agc->Resume(); > > - > > Only AWB now uses the pause mechanism. I wonder if it will survive, or > be dropped there too for the same reason as for the AGC :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); > > break; > > } > > @@ -526,10 +523,6 @@ void IPARPi::queueRequest(const ControlList &controls) > > ASSERT(agc); > > agc->SetFixedAnalogueGain(ctrl.second.get<float>()); > > > > - /* For the manual values to take effect, AGC must be unpaused. */ > > - if (agc->IsPaused()) > > - agc->Resume(); > > - > > libcameraMetadata_.set(controls::AnalogueGain, > > ctrl.second.get<float>()); > > break; > > -- > Regards, > > Laurent Pinchart
Hi David, On 26/11/2020 12:32, David Plowman wrote: > AE/AGC "disabled" is now handled better by the algorithm for itself, > so it no longer needs to be "resumed" before setting fixed shutter or > gain values. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 9853a343..e437c626 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls) > case controls::AE_ENABLE: { > RPiController::Algorithm *agc = controller_.GetAlgorithm("agc"); > ASSERT(agc); > + > if (ctrl.second.get<bool>() == false) > agc->Pause(); > else > @@ -512,10 +513,6 @@ void IPARPi::queueRequest(const ControlList &controls) > /* This expects units of micro-seconds. */ > agc->SetFixedShutter(ctrl.second.get<int32_t>()); > > - /* For the manual values to take effect, AGC must be unpaused. */ > - if (agc->IsPaused()) > - agc->Resume(); > - > libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); > break; > } > @@ -526,10 +523,6 @@ void IPARPi::queueRequest(const ControlList &controls) > ASSERT(agc); > agc->SetFixedAnalogueGain(ctrl.second.get<float>()); > > - /* For the manual values to take effect, AGC must be unpaused. */ > - if (agc->IsPaused()) > - agc->Resume(); > - > libcameraMetadata_.set(controls::AnalogueGain, > ctrl.second.get<float>()); > break; >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 9853a343..e437c626 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls) case controls::AE_ENABLE: { RPiController::Algorithm *agc = controller_.GetAlgorithm("agc"); ASSERT(agc); + if (ctrl.second.get<bool>() == false) agc->Pause(); else @@ -512,10 +513,6 @@ void IPARPi::queueRequest(const ControlList &controls) /* This expects units of micro-seconds. */ agc->SetFixedShutter(ctrl.second.get<int32_t>()); - /* For the manual values to take effect, AGC must be unpaused. */ - if (agc->IsPaused()) - agc->Resume(); - libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>()); break; } @@ -526,10 +523,6 @@ void IPARPi::queueRequest(const ControlList &controls) ASSERT(agc); agc->SetFixedAnalogueGain(ctrl.second.get<float>()); - /* For the manual values to take effect, AGC must be unpaused. */ - if (agc->IsPaused()) - agc->Resume(); - libcameraMetadata_.set(controls::AnalogueGain, ctrl.second.get<float>()); break;
AE/AGC "disabled" is now handled better by the algorithm for itself, so it no longer needs to be "resumed" before setting fixed shutter or gain values. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)