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