[libcamera-devel,0/2] Raspberrypi: support per-mode camera sensitivities

12442 mbox series

Hi

Here's an implementation of "per-mode camera sensitivities", following
on from a recent discussion email. This is taking the "don't lie, even
a little" approach, which I am inclined to prefer. There are two
patches as follows, though still some outstanding questions.

1. The first patch adds a "sensitivity" field to our camera mode
structure. The main question here is how to set this value correctly.

- For now, I've just used a virtual function in the CamHelper. This is
  dead simple and it works. (Though the sensor that needs this
  function is still unreleased.)

- But should I be using the sensor database? I'd have to add fields
  for "full resolution sensitivity", "2x2 binned sensitivity" as a
  minimum, though why stop there? There could be many different
  binning modes! Also, I'd have to get this through to the IPA. Would
  that mean writing a serialiser/deserialiser? Some clues on how to do
  that would be helpful, I looked at ipa_data_serializer.cpp but it
  was quite intimidating...

- Or would we be better getting the value from the driver? That might
  need a new read-only V4L2 control. And how would we pass that to the
  IPA, in the IPACameraSensorInfo? It already has mode-dependent
  fields...

2. The second patch takes care of the changing sensitivity in the
AEC/AGC, when the SwitchMode method is called.

Finally, I think there's still a problem with this approach. Suppose
you wanted to run preview, then do a capture with the same output
brightness, but selecting the exposure and gain explicitly for
yourself. You simply can't do this without knowing the sensitivity of
the camera modes. So how could we report this? Note that signalling it
in metadata with completed requests is too late - the typical pattern
would be:

Stop camera -> Reconfigure camera -> Recalculate exposure/gain ->
Start camera

Opinions both sought and appreciated!

Best regards
David

David Plowman (2):
  ipa: raspberrypi: Add sensitivity field to camera mode
  ipa: raspberrypi: AGC: handle modes with different sensitivities

 src/ipa/raspberrypi/cam_helper.cpp           |  5 ++++
 src/ipa/raspberrypi/cam_helper.hpp           |  3 +++
 src/ipa/raspberrypi/controller/camera_mode.h |  2 ++
 src/ipa/raspberrypi/controller/rpi/agc.cpp   | 25 ++++++++++++++++----
 src/ipa/raspberrypi/controller/rpi/agc.hpp   |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp          | 12 ++++++++++
 6 files changed, 43 insertions(+), 5 deletions(-)

Replying to myself...

On Thu, 27 May 2021 at 09:45, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi
>
> Here's an implementation of "per-mode camera sensitivities", following
> on from a recent discussion email. This is taking the "don't lie, even
> a little" approach, which I am inclined to prefer. There are two
> patches as follows, though still some outstanding questions.
>
> 1. The first patch adds a "sensitivity" field to our camera mode
> structure. The main question here is how to set this value correctly.
>
> - For now, I've just used a virtual function in the CamHelper. This is
>   dead simple and it works. (Though the sensor that needs this
>   function is still unreleased.)
>
> - But should I be using the sensor database? I'd have to add fields
>   for "full resolution sensitivity", "2x2 binned sensitivity" as a
>   minimum, though why stop there? There could be many different
>   binning modes! Also, I'd have to get this through to the IPA. Would
>   that mean writing a serialiser/deserialiser? Some clues on how to do
>   that would be helpful, I looked at ipa_data_serializer.cpp but it
>   was quite intimidating...

Having stared at this a bit more, I see one could move structs into
the core.mojom file (like IPACameraSensorInfo), where presumably
things get taken care of for you. Or you could add a
serialiser/deserialiser in ipa_data_serializer.cpp. I notice that
DEFINE_POD_SERIALIZER only works with numeric types but presumably
it's only a small tweak to make it work for anything that is "standard
layout"? Would that work?

Though I'm still not liking this solution too much because it's
unclear how many "mode sensitivities" one would need to add, or indeed
if mode sensitivities might differ for reasons other than binning...
(and there's always "way out there" stuff like HDR modes)

David

>
> - Or would we be better getting the value from the driver? That might
>   need a new read-only V4L2 control. And how would we pass that to the
>   IPA, in the IPACameraSensorInfo? It already has mode-dependent
>   fields...
>
> 2. The second patch takes care of the changing sensitivity in the
> AEC/AGC, when the SwitchMode method is called.
>
> Finally, I think there's still a problem with this approach. Suppose
> you wanted to run preview, then do a capture with the same output
> brightness, but selecting the exposure and gain explicitly for
> yourself. You simply can't do this without knowing the sensitivity of
> the camera modes. So how could we report this? Note that signalling it
> in metadata with completed requests is too late - the typical pattern
> would be:
>
> Stop camera -> Reconfigure camera -> Recalculate exposure/gain ->
> Start camera
>
> Opinions both sought and appreciated!
>
> Best regards
> David
>
> David Plowman (2):
>   ipa: raspberrypi: Add sensitivity field to camera mode
>   ipa: raspberrypi: AGC: handle modes with different sensitivities
>
>  src/ipa/raspberrypi/cam_helper.cpp           |  5 ++++
>  src/ipa/raspberrypi/cam_helper.hpp           |  3 +++
>  src/ipa/raspberrypi/controller/camera_mode.h |  2 ++
>  src/ipa/raspberrypi/controller/rpi/agc.cpp   | 25 ++++++++++++++++----
>  src/ipa/raspberrypi/controller/rpi/agc.hpp   |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp          | 12 ++++++++++
>  6 files changed, 43 insertions(+), 5 deletions(-)
>
> --
> 2.20.1
>

Another reply to myself... :)

On Thu, 27 May 2021 at 14:40, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Replying to myself...
>
> On Thu, 27 May 2021 at 09:45, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > Hi
> >
> > Here's an implementation of "per-mode camera sensitivities", following
> > on from a recent discussion email. This is taking the "don't lie, even
> > a little" approach, which I am inclined to prefer. There are two
> > patches as follows, though still some outstanding questions.
> >
> > 1. The first patch adds a "sensitivity" field to our camera mode
> > structure. The main question here is how to set this value correctly.
> >
> > - For now, I've just used a virtual function in the CamHelper. This is
> >   dead simple and it works. (Though the sensor that needs this
> >   function is still unreleased.)
> >
> > - But should I be using the sensor database? I'd have to add fields
> >   for "full resolution sensitivity", "2x2 binned sensitivity" as a
> >   minimum, though why stop there? There could be many different
> >   binning modes! Also, I'd have to get this through to the IPA. Would
> >   that mean writing a serialiser/deserialiser? Some clues on how to do
> >   that would be helpful, I looked at ipa_data_serializer.cpp but it
> >   was quite intimidating...
>
> Having stared at this a bit more, I see one could move structs into
> the core.mojom file (like IPACameraSensorInfo), where presumably
> things get taken care of for you. Or you could add a
> serialiser/deserialiser in ipa_data_serializer.cpp. I notice that
> DEFINE_POD_SERIALIZER only works with numeric types but presumably
> it's only a small tweak to make it work for anything that is "standard
> layout"? Would that work?
>
> Though I'm still not liking this solution too much because it's
> unclear how many "mode sensitivities" one would need to add, or indeed
> if mode sensitivities might differ for reasons other than binning...
> (and there's always "way out there" stuff like HDR modes)
>
> David
>
> >
> > - Or would we be better getting the value from the driver? That might
> >   need a new read-only V4L2 control. And how would we pass that to the
> >   IPA, in the IPACameraSensorInfo? It already has mode-dependent
> >   fields...
> >
> > 2. The second patch takes care of the changing sensitivity in the
> > AEC/AGC, when the SwitchMode method is called.
> >
> > Finally, I think there's still a problem with this approach. Suppose
> > you wanted to run preview, then do a capture with the same output
> > brightness, but selecting the exposure and gain explicitly for
> > yourself. You simply can't do this without knowing the sensitivity of
> > the camera modes. So how could we report this? Note that signalling it
> > in metadata with completed requests is too late - the typical pattern
> > would be:
> >
> > Stop camera -> Reconfigure camera -> Recalculate exposure/gain ->
> > Start camera

I'd also be interested in people's thoughts on this question - how to
tell the application what the mode sensitivity is. Could it be
returned in the camera configuration perhaps?

Thanks
David

> >
> > Opinions both sought and appreciated!
> >
> > Best regards
> > David
> >
> > David Plowman (2):
> >   ipa: raspberrypi: Add sensitivity field to camera mode
> >   ipa: raspberrypi: AGC: handle modes with different sensitivities
> >
> >  src/ipa/raspberrypi/cam_helper.cpp           |  5 ++++
> >  src/ipa/raspberrypi/cam_helper.hpp           |  3 +++
> >  src/ipa/raspberrypi/controller/camera_mode.h |  2 ++
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp   | 25 ++++++++++++++++----
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp   |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp          | 12 ++++++++++
> >  6 files changed, 43 insertions(+), 5 deletions(-)
> >
> > --
> > 2.20.1
> >