[libcamera-devel] [PATCH v2 1/2] libcamera: camera: Add a mode sensitivity field

[Kieran Bingham]

Hi David,

Quoting David Plowman (2022-01-04 13:30:03)
> Hi Kieran
> 
> Thanks for the reply.
> 
> On Tue, 4 Jan 2022 at 10:58, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Quoting David Plowman (2021-09-22 14:29:14)
> > > The modeSensitivity field is a number that describes how sensitive the
> > > selected sensor mode is compared to other readout modes of the same
> > > sensor. For example, a binned mode might have twice the sensitivity of
> > > the full resolution mode, meaning you would get double the signal
> > > level for the same exposure and gains.
> >
> > Is there a way to define which is the 'base' level?
> 
> I suppose the "base level" would correspond to the mode that gives you
> the lowest signal. But as per the discussion below, it's all relative
> so doesn't matter unless we want to ascribe a particular meaning.
> 
> >
> > Or does it not matter, as it's all relative (but relative to what I
> > guess is my question).
> >
> > I.e. Does it matter, or is it necessary to know that Mode A is 'half' as
> > sensitive as Mode B?
> >
> > In fact, I think that's already a given as whatever the pipeline handler
> > does will be relative. I.e. it would be about comparing the value
> > against other configurations not a standalone value.
> >
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  include/libcamera/camera.h |  2 ++
> > >  src/libcamera/camera.cpp   | 16 +++++++++++++++-
> > >  2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 601ee46e..3314c06b 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -66,6 +66,8 @@ public:
> > >
> > >         Transform transform;
> > >
> > > +       float modeSensitivity;
> > > +
> > >  protected:
> > >         CameraConfiguration();
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 71809bcd..8d795dff 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -156,7 +156,7 @@ LOG_DECLARE_CATEGORY(Camera)
> > >   * \brief Create an empty camera configuration
> > >   */
> > >  CameraConfiguration::CameraConfiguration()
> > > -       : transform(Transform::Identity), config_({})
> > > +       : transform(Transform::Identity), modeSensitivity(1.0), config_({})
> > >  {
> > >  }
> > >
> > > @@ -327,6 +327,20 @@ std::size_t CameraConfiguration::size() const
> > >   * may adjust this field at its discretion if the selection is not supported.
> > >   */
> > >
> > > +/**
> > > + * \var CameraConfiguration::modeSensitivity
> > > + * \brief The relative sensitivity of the chosen sensor mode
> > > + *
> > > + * Some sensors have readout modes with different sensitivities. For example,
> > > + * a binned camera mode might, with the same exposure and gains, produce
> > > + * twice the signal level of the full resolution readout. This would be
> > > + * signalled by the binned mode, when it is chosen, indicating a value here
> > > + * that is twice that of the full resolution mode.
> > > + *
> > > + * This value should only be read, and not set, by the user. It will be
> > > + * valid after the configure method has reteurned successfully.

s/reteurned/returned/

> > > + */
> > > +
> >
> > I bet we've discussed this in the past, but I can't recall. If there are
> > more of these properties, I would suspect they should be in a
> > ControlList somehow - but if there's not expected to be many, an
> > explicit field is fine to me, and I can see the importance of conveying
> > this information.
> 
> Yes, that's a good thought. One thing to note is that we need this
> value back during "configure", so you can calculate the right
> exposure/gain before calling "start". Could the configure method
> return a list of control values, I wonder?

I'm sure a ControlList could be returned somewhere, or it could be a
part of the CameraConfiguration structure itself.

The difficulty is that if we extend the structure here, it's an ABI
break / change, but if we have a ControlList to return these properties
- it can be updated/added to without breaking API/ABI compatibility.

Looking at CameraConfiguration now, I see we have:

 Transform transform;

Which could be considered a configuration property to put in a
ControlList too. And ... maybe there might be some ColourSpace property
to declare on the whole camera, rather than per stream? (Only because I
see a flag ColorSpaceFlag about whether ColorSpaces are per-stream or not)

Would you consider a Transform and a ModeSensitivity to be something
that could be grouped together as CameraConfiguration::properties() ?

Or does that become more complicated?

> > I was also going to say perhaps we need some testing on this in
> > lc-compliance, but given that the default is 1.0 for all camera
> > configurations - I think that's fine. It wouldn't be something we could
> > easily test anyway.
> 
> I have a sensor I'm working on that requires this feature - hopefully
> it won't be too long before others can try it!
> 
> Thanks
> David
> 
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> >
> > >  /**
> > >   * \var CameraConfiguration::config_
> > >   * \brief The vector of stream configurations
> > > --
> > > 2.20.1
> > >