<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 21 Apr 2022 at 14:58, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.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, David,<br>
<br>
Quoting Naushir Patuck (2022-04-21 14:13:45)<br>
> Hi Kieran,<br>
> <br>
> On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
> <br>
> ><br>
> ><br>
> > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <<br>
> > <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> ><br>
> >> Quoting Naushir Patuck (2022-04-13 11:44:14)<br>
> >> > Hi Kieran,<br>
> >> ><br>
> >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <<br>
> >> > <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> >> ><br>
> >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)<br>
> >> > > > Hi Kieran,<br>
> >> > > ><br>
> >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <<br>
> >> > > > <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> >> > > ><br>
> >> > > > > Hi David, Naush,<br>
> >> > > > ><br>
> >> > > > > Quoting David Plowman (2021-09-22 14:29:15)<br>
> >> > > > > > These changes retrieve the correct value for sensitivity of the<br>
> >> mode<br>
> >> > > > > > selected for the sensor. This value is known to the CamHelper<br>
> >> which<br>
> >> > > > > > passes it across to the pipeline handler so that it can be set<br>
> >> > > > > > correctly in the CameraConfiguration.<br>
> >> > > > > ><br>
> >> > > > > > Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> >> > > > > > ---<br>
> >> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-<br>
> >> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--<br>
> >> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10<br>
> >> +++++++---<br>
> >> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)<br>
> >> > > > > ><br>
> >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom<br>
> >> > > > > b/include/libcamera/ipa/raspberrypi.mojom<br>
> >> > > > > > index e453d46c..a92a76f8 100644<br>
> >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {<br>
> >> > > > > > libcamera.FileDescriptor lsTableHandle;<br>
> >> > > > > > };<br>
> >> > > > > ><br>
> >> > > > > > +struct IPAConfigResult {<br>
> >> > > > > > + float modeSensitivity;<br>
> >> > > > > > +};<br>
> >> > > > > > +<br>
> >> > > > > > struct StartConfig {<br>
> >> > > > > > libcamera.ControlList controls;<br>
> >> > > > > > int32 dropFrameCount;<br>
> >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {<br>
> >> > > > > > * \param[in] entityControls Controls provided by the<br>
> >> > > pipeline<br>
> >> > > > > entities<br>
> >> > > > > > * \param[in] ipaConfig Pipeline-handler-specific<br>
> >> > > configuration<br>
> >> > > > > data<br>
> >> > > > > > * \param[out] controls Controls to apply by the<br>
> >> pipeline<br>
> >> > > entity<br>
> >> > > > > > + * \param[out] result Other results that the pipeline<br>
> >> handler<br>
> >> > > > > may require<br>
> >> > > > > > *<br>
> >> > > > > > * This function shall be called when the camera is<br>
> >> > > configured<br>
> >> > > > > to inform<br>
> >> > > > > > * the IPA of the camera's streams and the sensor<br>
> >> settings.<br>
> >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {<br>
> >> > > > > > map<uint32, libcamera.IPAStream> streamConfig,<br>
> >> > > > > > map<uint32, libcamera.ControlInfoMap><br>
> >> > > entityControls,<br>
> >> > > > > > IPAConfig ipaConfig)<br>
> >> > > > > > - => (int32 ret, libcamera.ControlList controls);<br>
> >> > > > > > + => (int32 ret, libcamera.ControlList controls,<br>
> >> > > > > IPAConfigResult result);<br>
> >> > > > > ><br>
> >> > > > > > /**<br>
> >> > > > > > * \fn mapBuffers()<br>
> >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > index 047123ab..796e6d15 100644<br>
> >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > @@ -97,7 +97,7 @@ public:<br>
> >> > > > > > const std::map<unsigned int, IPAStream><br>
> >> > > > > &streamConfig,<br>
> >> > > > > > const std::map<unsigned int,<br>
> >> ControlInfoMap><br>
> >> > > > > &entityControls,<br>
> >> > > > > > const ipa::RPi::IPAConfig &data,<br>
> >> > > > > > - ControlList *controls) override;<br>
> >> > > > > > + ControlList *controls,<br>
> >> > > ipa::RPi::IPAConfigResult<br>
> >> > > > > *result) override;<br>
> >> > > > > > void mapBuffers(const std::vector<IPABuffer> &buffers)<br>
> >> > > override;<br>
> >> > > > > > void unmapBuffers(const std::vector<unsigned int> &ids)<br>
> >> > > override;<br>
> >> > > > > > void signalStatReady(const uint32_t bufferId) override;<br>
> >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const<br>
> >> IPACameraSensorInfo<br>
> >> > > > > &sensorInfo,<br>
> >> > > > > > [[maybe_unused]] const std::map<unsigned<br>
> >> int,<br>
> >> > > > > IPAStream> &streamConfig,<br>
> >> > > > > > const std::map<unsigned int,<br>
> >> ControlInfoMap><br>
> >> > > > > &entityControls,<br>
> >> > > > > > const ipa::RPi::IPAConfig &ipaConfig,<br>
> >> > > > > > - ControlList *controls)<br>
> >> > > > > > + ControlList *controls,<br>
> >> > > ipa::RPi::IPAConfigResult<br>
> >> > > > > *result)<br>
> >> > > > > > {<br>
> >> > > > > > if (entityControls.size() != 2) {<br>
> >> > > > > > LOG(IPARPI, Error) << "No ISP or sensor controls<br>
> >> > > found.";<br>
> >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const<br>
> >> IPACameraSensorInfo<br>
> >> > > > > &sensorInfo,<br>
> >> > > > > > /* Pass the camera mode to the CamHelper to setup<br>
> >> > > algorithms. */<br>
> >> > > > > > helper_->SetCameraMode(mode_);<br>
> >> > > > > ><br>
> >> > > > > > + /* The pipeline handler passes out the mode's<br>
> >> sensitivity. */<br>
> >> > > > > > + result->modeSensitivity = mode_.sensitivity;<br>
> >> > > > > > +<br>
> >> > > > > > if (firstStart_) {<br>
> >> > > > > > /* Supply initial values for frame durations. */<br>
> >> > > > > > applyFrameDurations(defaultMinFrameDuration,<br>
> >> > > > > defaultMaxFrameDuration);<br>
> >> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > index 0bdfa727..caf0030e 100644<br>
> >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > > > > > @@ -147,7 +147,7 @@ public:<br>
> >> > > > > > void frameStarted(uint32_t sequence);<br>
> >> > > > > ><br>
> >> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);<br>
> >> > > > > > - int configureIPA(const CameraConfiguration *config);<br>
> >> > > > > > + int configureIPA(CameraConfiguration *config);<br>
> >> > > > > ><br>
> >> > > > > > void statsMetadataComplete(uint32_t bufferId, const<br>
> >> > > ControlList<br>
> >> > > > > &controls);<br>
> >> > > > > > void runIsp(uint32_t bufferId);<br>
> >> > > > > > @@ -1250,7 +1250,7 @@ int<br>
> >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig<br>
> >> > > > > *sensorConfig)<br>
> >> > > > > > return ipa_->init(settings, sensorConfig);<br>
> >> > > > > > }<br>
> >> > > > > ><br>
> >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration<br>
> >> *config)<br>
> >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)<br>
> >> > > > > > {<br>
> >> > > > > > /* We know config must be an RPiCameraConfiguration. */<br>
> >> > > > > > const RPiCameraConfiguration *rpiConfig =<br>
> >> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const<br>
> >> > > > > CameraConfiguration *config)<br>
> >> > > > > ><br>
> >> > > > > > /* Ready the IPA - it must know about the sensor<br>
> >> resolution.<br>
> >> > > */<br>
> >> > > > > > ControlList controls;<br>
> >> > > > > > + ipa::RPi::IPAConfigResult result;<br>
> >> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,<br>
> >> > > entityControls,<br>
> >> > > > > ipaConfig,<br>
> >> > > > > > - &controls);<br>
> >> > > > > > + &controls, &result);<br>
> >> > > > ><br>
> >> > > > > I've just rebased this series to master to facilitate merging,<br>
> >> and with<br>
> >> > > > > fresh eyes I can't help but wonder if this value shouldn't be<br>
> >> returned<br>
> >> > > > > in the validate() phase. (Not sure if this has been asked before /<br>
> >> > > yet).<br>
> >> > > > ><br>
> >> > > > > Is there anything that prevents us adding a validate() to the IPA<br>
> >> > > > > interface to allow validating the configuration and at that point,<br>
> >> > > > > setting the mode sensitivity? Or can this value /only/ be<br>
> >> determined<br>
> >> > > > > when configuring?<br>
> >> > > > ><br>
> >> > > ><br>
> >> > > > I can't think of any reason that this couldn't be done in<br>
> >> validate(), but<br>
> >> > > > David<br>
> >> > > > might have some reasons. However, is an application required to<br>
> >> call<br>
> >> > > > CameraConfiguration::validate() directly? I can see that it does<br>
> >> get<br>
> >> > > called<br>
> >> > > > in Camera::configure(), just before the call to<br>
> >> > > > PipelineHandler::configure().<br>
> >> > > > If there is no requirement to call CameraConfiguration::validate(),<br>
> >> > > would it<br>
> >> > > > matter where the result gets set?<br>
> >> > ><br>
> >> > > There isn't a 'requirement' to do so as such, but it's recommended. If<br>
> >> > > the call to configure can't be satisfied 'precisely' then the<br>
> >> configure<br>
> >> > > call should fail. It should not make any changes or adjustments to the<br>
> >> > > configuration.<br>
> >> > ><br>
> >> > > Validate will tell you if the pipeline handler made any changes, and<br>
> >> > > more than that - the validate before configure /must/ report that no<br>
> >> > > changes were made.<br>
> >> > ><br>
> >> ><br>
> >> > I think this is the part that may be troublesome. Assume an application<br>
> >> > did not call CameraConfiguration::validate() directly, but it does get<br>
> >> > called<br>
> >> > from PipelineHandler::configure(). The validate() will then adjust the<br>
> >> > sensitivity value and return a Status::Adjusted. This will then fail<br>
> >> the<br>
> >> > PipelineHandler::configure() call. Perhaps<br>
> >> CameraConfiguration::validate()<br>
> >> > should be a mandatory call from the application? Or if we adjust the<br>
> >> > sensitivity in CameraConfiguration::validate(), we don't return<br>
> >> > Status::Adjusted?<br>
> >> ><br>
> >><br>
> >> I'm sorry - I was wrong to say there isn't a requirement to do so.<br>
> >><br>
> >><br>
> >> <a href="https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details" rel="noreferrer" target="_blank">https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details</a><br>
> >><br>
> >> documents that applications 'shall' be validated by a call to<br>
> >> validate().<br>
> >><br>
> ><br>
> > Ah, that makes things less ambiguous then. I'll let David comment further<br>
> > when he is back, but I see no reason not to switch this to validate() now.<br>
> ><br>
> <br>
> After having prototyped an ipa::validate() to pass the sensitivity back to<br>
> the<br>
> PH, I've changed my mind on the above statement :-)<br>
> <br>
> The reason for this is that things just feel a bit more awkward in the<br>
> validate<br>
> call.For instance, we have not yet set up a sensor mode in the IPA to use<br>
> for<br>
> deducing the mode sensitivity. If we move the mode setup into<br>
> ipa::validate(),<br>
> there are further things associated with the mode that need to happen there<br>
> (e.g.<br>
> helper->set_mode())that do not feel right to me.<br>
> <br>
> Instead of doing things this way, I wonder if we should look to pass the<br>
> sensitivity out via the Camera::properties_ ControlList. My initial<br>
> reservation<br>
> suggesting this was the assumption that properties were static through the<br>
> lifetime of the camera.However, this is not the case, as we have<br>
> properties::ScalerCropMaximum that changes on every call to configure().<br>
> What if<br>
> we added a properties::SensorSensitivity that also gets updated in the same<br>
> way?<br>
> This will remove the need for CameraConfiguration::modeSensitivityfield<br>
> entirely.<br>
<br>
That sounds ok to me too. I wonder if 'ModeSensitivity' is still the<br>
right name in that instance ...<br></blockquote><div><br></div><div>We could use SensorSensitivity?</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>
I think that would mean that it's only readable after the camera is<br>
configured though - is that ok?<br></blockquote><div><br></div><div>I think that is unavoidable in this case, but equivalent to how we would</div><div>deal with properties::ScalerCropMaximum.</div><div><br></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>
--<br>
Kieran<br>
<br>
> <br>
> Thanks,<br>
> Naush<br>
> <br>
> <br>
> ><br>
> > Regards,<br>
> > Naush<br>
> ><br>
> ><br>
> >><br>
> >> """<br>
> >> The CameraConfiguration holds an ordered list of stream configurations.<br>
> >> It supports iterators and operates as a vector of StreamConfiguration<br>
> >> instances. The stream configurations are inserted by addConfiguration(),<br>
> >> and the at() function or operator[] return a reference to the<br>
> >> StreamConfiguration based on its insertion index. Accessing a stream<br>
> >> configuration with an invalid index results in undefined behaviour.<br>
> >><br>
> >> CameraConfiguration instances are retrieved from the camera with<br>
> >> Camera::generateConfiguration(). Applications may then inspect the<br>
> >> configuration, modify it, and possibly add new stream configuration<br>
> >> entries with addConfiguration(). Once the camera configuration satisfies<br>
> >> the application, it shall be validated by a call to validate(). The<br>
> >> validation implements "try" semantics: it adjusts invalid configurations<br>
> >> to the closest achievable parameters instead of rejecting them<br>
> >> completely. Applications then decide whether to accept the modified<br>
> >> configuration, or try again with a different set of parameters. Once the<br>
> >> configuration is valid, it is passed to Camera::configure().<br>
> >><br>
> >> """<br>
> >><br>
> >> --<br>
> >> Kieran<br>
> >><br>
> >><br>
> >> > Naush<br>
> >> ><br>
> >> ><br>
> >> > ><br>
> >> > > When an application calls validate() - the pipeline handler can make<br>
> >> > > changes to the Configuration (such as filling in the ModeSensitivity<br>
> >> > > here) and if a change was made - it can return<br>
> >> 'ConfigurationAdjusted' -<br>
> >> > > which then means the application should check to see what was<br>
> >> different<br>
> >> > > from what it requested - to what the pipeline handler will deliver.<br>
> >> > ><br>
> >> > > I would like there to be a way to highlight 'what' changes were made<br>
> >> if<br>
> >> > > the validate returns 'Adjusted' - but I'm not sure how we could easily<br>
> >> > > implement that yet.<br>
> >> > ><br>
> >> > > --<br>
> >> > > Kieran<br>
> >> > ><br>
> >> > ><br>
> >> > > > Regards,<br>
> >> > > > Naush<br>
> >> > > ><br>
> >> > > ><br>
> >> > > > ><br>
> >> > > > > Then the validate phase should be able to return the mode<br>
> >> sensitivity<br>
> >> > > of<br>
> >> > > > > the configuration that is being validated.<br>
> >> > > > ><br>
> >> > > > > Doing that we could keep the configure calls using a const<br>
> >> parameter<br>
> >> > > for<br>
> >> > > > > their config structures.<br>
> >> > > > ><br>
> >> > > > > > if (ret < 0) {<br>
> >> > > > > > LOG(RPI, Error) << "IPA configuration failed!";<br>
> >> > > > > > return -EPIPE;<br>
> >> > > > > > }<br>
> >> > > > > ><br>
> >> > > > > > + /* Store the mode sensitivity for the application. */<br>
> >> > > > > > + config->modeSensitivity = result.modeSensitivity;<br>
> >> > > > > > +<br>
> >> > > > > > /*<br>
> >> > > > > > * Configure the H/V flip controls based on the<br>
> >> combination<br>
> >> > > of<br>
> >> > > > > > * the sensor and user transform.<br>
> >> > > > > > --<br>
> >> > > > > > 2.20.1<br>
> >> > > > > ><br>
> >> > > > ><br>
> >> > ><br>
> >><br>
> ><br>
</blockquote></div></div>