[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch correct value for sensor's modeSensitivity
David Plowman
david.plowman at raspberrypi.com
Thu Apr 21 15:32:20 CEST 2022
Yes, I recall that there was discussion that this number would be
better as a property, and wasn't the conclusion that we should move it
(even if not right now)? So doing it like this is certainly fine with
me.
David
On Thu, 21 Apr 2022 at 14:14, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> 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.
>
> 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