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