| Message ID | 20210324114415.19866-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. On Wed, 24 Mar 2021 at 11:44, David Plowman <david.plowman@raspberrypi.com> wrote: > If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE > controls, assume it wants to be told the colour gains. We will need to > add these to the list of delayed controls we can apply. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 17 ++++++++++++++--- > .../pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 1c928b72..7437a77e 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1030,13 +1030,24 @@ void IPARPi::processStats(unsigned int bufferId) > RPiController::StatisticsPtr statistics = > std::make_shared<bcm2835_isp_stats>(*stats); > controller_.Process(statistics, &rpiMetadata_); > > + ControlList ctrls(sensorCtrls_); > + > struct AgcStatus agcStatus; > - if (rpiMetadata_.Get("agc.status", agcStatus) == 0) { > - ControlList ctrls(sensorCtrls_); > + if (rpiMetadata_.Get("agc.status", agcStatus) == 0) > applyAGC(&agcStatus, ctrls); > > - setDelayedControls.emit(ctrls); > + struct AwbStatus awbStatus; > + if (rpiMetadata_.Get("awb.status", awbStatus) == 0 && > + sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end() > && > + sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != > sensorCtrls_.end()) { > + ctrls.set(V4L2_CID_RED_BALANCE, > + static_cast<int32_t>(awbStatus.gain_r * 256)); > + ctrls.set(V4L2_CID_BLUE_BALANCE, > + static_cast<int32_t>(awbStatus.gain_b * 256)); > The 256 const bothers me slightly. Is this FP multiplier going to be true for all sensors? Any way we can deduce this (perhaps from the min value)? > } > + > + if (!ctrls.empty()) > + setDelayedControls.emit(ctrls); > } > > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList > &ctrls) > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2cac802c..7bac1503 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1027,6 +1027,15 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } > }, > { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } } > }; > + > + /* If the sensor exposes red/blue balance controls, we will update > them. */ > + const ControlInfoMap &sensorControls = > data->unicam_[Unicam::Image].dev()->controls(); > + if (sensorControls.find(V4L2_CID_RED_BALANCE) != > sensorControls.end() && > + sensorControls.find(V4L2_CID_BLUE_BALANCE) != > sensorControls.end()) { > + params[V4L2_CID_RED_BALANCE] = { sensorConfig.vblankDelay, > false }; > + params[V4L2_CID_BLUE_BALANCE] = { > sensorConfig.vblankDelay, false }; > Should the delay value be 1 here, as the change likely happens on the very next frame? Regards, Naush > + } > + > data->delayedCtrls_ = > std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), > params); > data->sensorMetadata_ = sensorConfig.sensorMetadata; > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush Thanks for the feedback! On Wed, 7 Apr 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for your patch. > > On Wed, 24 Mar 2021 at 11:44, David Plowman <david.plowman@raspberrypi.com> wrote: >> >> If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE >> controls, assume it wants to be told the colour gains. We will need to >> add these to the list of delayed controls we can apply. >> >> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >> --- >> src/ipa/raspberrypi/raspberrypi.cpp | 17 ++++++++++++++--- >> .../pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++ >> 2 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >> index 1c928b72..7437a77e 100644 >> --- a/src/ipa/raspberrypi/raspberrypi.cpp >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp >> @@ -1030,13 +1030,24 @@ void IPARPi::processStats(unsigned int bufferId) >> RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats); >> controller_.Process(statistics, &rpiMetadata_); >> >> + ControlList ctrls(sensorCtrls_); >> + >> struct AgcStatus agcStatus; >> - if (rpiMetadata_.Get("agc.status", agcStatus) == 0) { >> - ControlList ctrls(sensorCtrls_); >> + if (rpiMetadata_.Get("agc.status", agcStatus) == 0) >> applyAGC(&agcStatus, ctrls); >> >> - setDelayedControls.emit(ctrls); >> + struct AwbStatus awbStatus; >> + if (rpiMetadata_.Get("awb.status", awbStatus) == 0 && >> + sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end() && >> + sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != sensorCtrls_.end()) { >> + ctrls.set(V4L2_CID_RED_BALANCE, >> + static_cast<int32_t>(awbStatus.gain_r * 256)); >> + ctrls.set(V4L2_CID_BLUE_BALANCE, >> + static_cast<int32_t>(awbStatus.gain_b * 256)); > > > The 256 const bothers me slightly. Is this FP multiplier going to be true for all sensors? > Any way we can deduce this (perhaps from the min value)? For the sensor I have, there doesn't seem to be a documented min value, so the driver will basically accept anything. Values less than 256 (1.0x) might seem unlikely, but it's perhaps not reasonable to rule them out completely. My other suggestion was to add a CamHelper::ColourGainCode method. What do you make of that? It would also cover the theoretical possibility that the value we give to the driver is not a simple linear gain. (Note also that there's no need for the reverse mapping - code back to gain - as we don't need it.) > >> >> } >> + >> + if (!ctrls.empty()) >> + setDelayedControls.emit(ctrls); >> } >> >> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> index 2cac802c..7bac1503 100644 >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> @@ -1027,6 +1027,15 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) >> { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } }, >> { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } } >> }; >> + >> + /* If the sensor exposes red/blue balance controls, we will update them. */ >> + const ControlInfoMap &sensorControls = data->unicam_[Unicam::Image].dev()->controls(); >> + if (sensorControls.find(V4L2_CID_RED_BALANCE) != sensorControls.end() && >> + sensorControls.find(V4L2_CID_BLUE_BALANCE) != sensorControls.end()) { >> + params[V4L2_CID_RED_BALANCE] = { sensorConfig.vblankDelay, false }; >> + params[V4L2_CID_BLUE_BALANCE] = { sensorConfig.vblankDelay, false }; > > > Should the delay value be 1 here, as the change likely happens on the very next frame? Aha, interesting point! So my thinking was: 1. We never read these values back, so I don't care about when they're reported as having taken effect, and 2. I want the value to be applied immediately, not after several frames of delay. As such, the same delay as "the worst" of the other controls should be optimal. To some extent, perhaps this is revealing a minor abuse of the delayed control system for something we didn't really intend. But I don't think we provide any other way of getting these values back to the PH. As a minimum though, I guess a comment would be helpful. Do you think we need to do something more/different? Thanks! David > > Regards, > Naush > > >> >> + } >> + >> data->delayedCtrls_ = std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), params); >> data->sensorMetadata_ = sensorConfig.sensorMetadata; >> >> -- >> 2.20.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, On Thu, 8 Apr 2021 at 10:20, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the feedback! > > On Wed, 7 Apr 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi David, > > > > Thank you for your patch. > > > > On Wed, 24 Mar 2021 at 11:44, David Plowman < > david.plowman@raspberrypi.com> wrote: > >> > >> If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE > >> controls, assume it wants to be told the colour gains. We will need to > >> add these to the list of delayed controls we can apply. > >> > >> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >> --- > >> src/ipa/raspberrypi/raspberrypi.cpp | 17 ++++++++++++++--- > >> .../pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++ > >> 2 files changed, 23 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > >> index 1c928b72..7437a77e 100644 > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp > >> @@ -1030,13 +1030,24 @@ void IPARPi::processStats(unsigned int bufferId) > >> RPiController::StatisticsPtr statistics = > std::make_shared<bcm2835_isp_stats>(*stats); > >> controller_.Process(statistics, &rpiMetadata_); > >> > >> + ControlList ctrls(sensorCtrls_); > >> + > >> struct AgcStatus agcStatus; > >> - if (rpiMetadata_.Get("agc.status", agcStatus) == 0) { > >> - ControlList ctrls(sensorCtrls_); > >> + if (rpiMetadata_.Get("agc.status", agcStatus) == 0) > >> applyAGC(&agcStatus, ctrls); > >> > >> - setDelayedControls.emit(ctrls); > >> + struct AwbStatus awbStatus; > >> + if (rpiMetadata_.Get("awb.status", awbStatus) == 0 && > >> + sensorCtrls_.find(V4L2_CID_RED_BALANCE) != > sensorCtrls_.end() && > >> + sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != > sensorCtrls_.end()) { > >> + ctrls.set(V4L2_CID_RED_BALANCE, > >> + static_cast<int32_t>(awbStatus.gain_r * 256)); > >> + ctrls.set(V4L2_CID_BLUE_BALANCE, > >> + static_cast<int32_t>(awbStatus.gain_b * 256)); > > > > > > The 256 const bothers me slightly. Is this FP multiplier going to be > true for all sensors? > > Any way we can deduce this (perhaps from the min value)? > > For the sensor I have, there doesn't seem to be a documented min > value, so the driver will basically accept anything. Values less than > 256 (1.0x) might seem unlikely, but it's perhaps not reasonable to > rule them out completely. > > My other suggestion was to add a CamHelper::ColourGainCode method. > What do you make of that? It would also cover the theoretical > possibility that the value we give to the driver is not a simple > linear gain. (Note also that there's no need for the reverse mapping - > code back to gain - as we don't need it.) > This sounds like a good idea to me. > > > > >> > >> } > >> + > >> + if (!ctrls.empty()) > >> + setDelayedControls.emit(ctrls); > >> } > >> > >> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList > &ctrls) > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> index 2cac802c..7bac1503 100644 > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> @@ -1027,6 +1027,15 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > >> { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, > false } }, > >> { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } } > >> }; > >> + > >> + /* If the sensor exposes red/blue balance controls, we will > update them. */ > >> + const ControlInfoMap &sensorControls = > data->unicam_[Unicam::Image].dev()->controls(); > >> + if (sensorControls.find(V4L2_CID_RED_BALANCE) != > sensorControls.end() && > >> + sensorControls.find(V4L2_CID_BLUE_BALANCE) != > sensorControls.end()) { > >> + params[V4L2_CID_RED_BALANCE] = { > sensorConfig.vblankDelay, false }; > >> + params[V4L2_CID_BLUE_BALANCE] = { > sensorConfig.vblankDelay, false }; > > > > > > Should the delay value be 1 here, as the change likely happens on the > very next frame? > > Aha, interesting point! So my thinking was: > > 1. We never read these values back, so I don't care about when they're > reported as having taken effect, and > 2. I want the value to be applied immediately, not after several > frames of delay. As such, the same delay as "the worst" of the other > controls should be optimal. > > To some extent, perhaps this is revealing a minor abuse of the delayed > control system for something we didn't really intend. But I don't > think we provide any other way of getting these values back to the PH. > > As a minimum though, I guess a comment would be helpful. Do you think > we need to do something more/different? > I wonder if we should bypass DelayedControls for this entirely? If we create a new Signal called SetSensorControls in the IPA interface that pushes the ctrl values directly to the sensor, that should be enough. Hopefully not a big change, but may make this bit a bit neater. Regards, Naush > > Thanks! > David > > > > > Regards, > > Naush > > > > > >> > >> + } > >> + > >> data->delayedCtrls_ = > std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), > params); > >> data->sensorMetadata_ = sensorConfig.sensorMetadata; > >> > >> -- > >> 2.20.1 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 1c928b72..7437a77e 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1030,13 +1030,24 @@ void IPARPi::processStats(unsigned int bufferId) RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats); controller_.Process(statistics, &rpiMetadata_); + ControlList ctrls(sensorCtrls_); + struct AgcStatus agcStatus; - if (rpiMetadata_.Get("agc.status", agcStatus) == 0) { - ControlList ctrls(sensorCtrls_); + if (rpiMetadata_.Get("agc.status", agcStatus) == 0) applyAGC(&agcStatus, ctrls); - setDelayedControls.emit(ctrls); + struct AwbStatus awbStatus; + if (rpiMetadata_.Get("awb.status", awbStatus) == 0 && + sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end() && + sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != sensorCtrls_.end()) { + ctrls.set(V4L2_CID_RED_BALANCE, + static_cast<int32_t>(awbStatus.gain_r * 256)); + ctrls.set(V4L2_CID_BLUE_BALANCE, + static_cast<int32_t>(awbStatus.gain_b * 256)); } + + if (!ctrls.empty()) + setDelayedControls.emit(ctrls); } void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 2cac802c..7bac1503 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1027,6 +1027,15 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } }, { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } } }; + + /* If the sensor exposes red/blue balance controls, we will update them. */ + const ControlInfoMap &sensorControls = data->unicam_[Unicam::Image].dev()->controls(); + if (sensorControls.find(V4L2_CID_RED_BALANCE) != sensorControls.end() && + sensorControls.find(V4L2_CID_BLUE_BALANCE) != sensorControls.end()) { + params[V4L2_CID_RED_BALANCE] = { sensorConfig.vblankDelay, false }; + params[V4L2_CID_BLUE_BALANCE] = { sensorConfig.vblankDelay, false }; + } + data->delayedCtrls_ = std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), params); data->sensorMetadata_ = sensorConfig.sensorMetadata;
If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE controls, assume it wants to be told the colour gains. We will need to add these to the list of delayed controls we can apply. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 17 ++++++++++++++--- .../pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)