[libcamera-devel] [PATCH 1/1] libcamera: controls: Add DigitalGain control

[David Plowman]

Hi Laurent

Thanks for sending me this discussion, I agree it's an important
topic. Perhaps I can add a few rambling thoughts here and there, and
also try to answer your questions. Obviously my answers are rather
influenced by the Broadcom pipeline, and also the logical Raspberry Pi
view that we put on top of it!

On Thu, 26 Nov 2020 at 12:42, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:
> > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:
> > > > Hi Jacopo
> > > >
> > > > You're right, there's a relationship there. ColourGains obviously
> > > > gives you the red and blue gains determined by the AWB usually. You
> > > > might get the values 1.6 and 2.0 (for red and blue)
> > > >
> > > > In our case, if we report a single "global gain" value you can kind of
> > > > imagine it as the green gain, where the colour gains were normalised
> > > > for a green gain of 1. So if the global gain was 1.25, then the actual
> > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0)  = (2.0,
> > > > 1.25, 2.5).
> > >
> > > That's a really helpful explanation for me, thanks
> > >
> > > > In the per-channel case I guess you'd be reporting these 3 numbers
> > > > directly. For me this duplicates information that's already in the
> > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a
> > > > pipeline that lets you set a global gain - you'd have to query the
> > > > current white balance and work out all three numbers and set them. But
> > > > then you've set the white balance as well. Or maybe we do something
> > > > special so that you haven't? You see why it confuses me! So on balance
> > > > I'm with the single value approach, though I could live either way.
> > >
> > > I see. With the above explanation I really see why a single global
> > > value is probably enough.
> > >
> > > My question is now: are the above notions (blue/red gains normalized
> > > on a green value of 1, how global gain is used to obtain per-channel
> > > gains etc) standard knowledge I'm lacking, or:
> > >
> > > 1) It's worth to describe them in the control documentation as they're
> > > "standard" and won't change per-pipeline
> > > 2) They might differe per-pipeline and they need to be
> > > described in the pipeline model documentation ?
> > > 3) It is assumed the reader knows she's dealing with
> >
> > Difficult. I can't really see too many other ways of defining it, but
> > you know, pipelines all have their nuances. You probably can't
> > guarantee that what I described is completely "standard".
>
> This is really what I'd like to address :-)
>
> Have you seen the "libcamera: camera: Document the camera and pipeline
> model" patch (which is now in the master branch) ? It's the very first
> step in standardizing a logical pipeline model that pipeline handlers
> need to confirm to.
>
> In a nutshell, the idea is that libcamera needs to standardize the
> behaviour of cameras as seen from applications in order to make
> applications portable. To do so, we define a logical pipeline model that
> describes the processing operations performed by the camera, from the
> pixel array to the captured image.
>
> Hardware platforms differ in the feature they support and how they
> implement them, so most operations in the pipeline model are optional.
> Furthermore, there's no requirement to have the hardware performing
> those operations in the same order, or more generically in the same way,
> as the logical model mandates, as long as the visible effect from an
> application point of view is the same. Pipeline handlers are responsible
> for exposing the standard logical model and mapping it to device
> features.
>
> I expect that not all devices will be able to expose the same pipeline
> model, so we will need to support multiple options, with a way for the
> pipeline handlers to report which options apply to the cameras they
> handle. This can be reported through properties or through other means.
> We already support this, albeit implicitly: pipeline handlers that don't
> support some of the processing operations simply don't expose the
> corresponding controls. I expect we will also need properties to report
> the order of operations, when different orders lead to different results
> and all need to be supported. This should however remain the exception
> rather than the rule.
>
> I would like to expand the pipeline model documentation to include
> colour processing, and I think digital gain really fits here. We can
> still apply your patch (or the next version of it) before finalizing
> documentation of the colour processing in the pipeline model, but
> controls will then likely change soon after, which would require
> adapting your applications. It may end up causing more pain than gain.
>
> As you have more experience than me in this area, I wonder if you could
> help reviewing an initial proposal ? I envision the following
> sensitivity and colour-related operations, in order.
>
> - Total gain in the sensor, made of analog and/or digital gain. This
>   applies to all images, RAW and processed, and to all channels. If
>   auto-exposure is enabled, the control value is ignored. The actual
>   value is always reported in metadata.

I agree that we obviously need to know the analogue gain applied by
the sensor. Optionally there might be digital gain in the sensor too.
In many respects I'd quite like to see controls like this:

SensorGain - this is the combined gain applied by the sensor, analogue
and digital (if any).
SensorDigitalGain - this is optional and can be used if the sensor
applies any digital gain.

The reason for this is that applications can simply look for the
SensorGain and ignore anything else. Mostly they won't care if the
gain was analogue or digital. I'd certainly not want application code
to be forced to go checking for SensorDigitalGain when it won't
usually be there. Even when a sensor has applied digital gain, I think
that reporting the SensorDigitalGain information, while helpful,
should be optional. (To be honest I've never seen sensor digital gain
used.)

I would just add that, when AGC is enabled, setting the gain still
makes sense. You still need the AGC (well, AEC/AGC) to control the
shutter speed to give you those fixed ISO captures. Similarly, there's
no reason not to let an application fix the shutter speed and let the
AGC vary the gain.

>
>   Questions:
>
>    - Should we mandate that pipeline handlers and IPAs prioritize analog
>      gain before applying digital gain ?

I can't really see any reason to mandate this. It would be normal, I
agree, and it would probably be helpful to see it reported in the
metadata if digital gain has been used, but I don't really see a need
to force any particular behaviour.

>
>    - Can there be use cases for per-channel sensor-side gains ? In most
>      cases I expect the white balance to be applied in the ISP, but will
>      that always be the case ? What if auto-white-balance is disabled,
>      and the user sets manual per-channel gains, could there be a reason
>      to apply them in the sensor ?

Again, I've never seen sensor-side per channel gains myself, though
you'd be brave to assert that no one will ever use them. I don't see
AWB being disabled as a reason to use them. If you want to fix the
colour gains, there's no reason not to do it in the ISP.

However, it seems reasonable to me to leave it up to the pipeline
handler and its IPAs. If someone wants to set fixed colour gains
(effectively stop the AWB), and the pipeline handler wants to do it by
setting them in the sensor, good luck to them!

>
> - Digital gain on the ISP side (a.k.a. "post-RAW sensitivity boost" in
>   Android). This applies to processed images only, and to all channels.
>   If auto-exposure is enabled, the control value is ignored. The actual
>   value is always reported in metadata.
>
>   Questions:
>
>   - Should this be renamed ? "Digital gain" can be confused with the
>     digital gain in the sensor. A name that captures the fact that the
>     gain is applied on processed images only may be better (I don't like
>     the Android name too much, but it handles this issue).

"Post-RAW sensitivity boost" sounds execrable. Just my opinion,
though! I'd be happy with DigitalGain (on its own) being the gain in
the ISP, and SensorDigitalGain (if present) being the digital gain in
the sensor, but I know opinions will vary! IspDigitalGain? IspGain?
PipelineDigitalGain? PipelineGain?

>
> - Per-channel gains. This is currently handled as red and blue gains,
>   with the green gains being hardcoded to 1.0.
>
>   Questions:
>
>   - Should we change this ? I see two potential issues:
>
>     - Not all systems may use a Bayer colour filter array, so other
>       gains may be needed (I'm thinking about RGBW patterns for
>       instance, or any other from
>       https://en.wikipedia.org/wiki/Color_filter_array).

So I think red and blue gains alone are OK, even with something like
an RGBW filter. Finding white balance is a problem of finding a single
2D point in a plane, giving you (for example) red and blue gains. With
an RGBW filter, I'd expect the AWB calibration process would tell you
what to do with the W pixels once you've been told where in this space
you are.

I'm assuming other primaries (e.g. CMYK) or multi-spectral images are
out of scope!

>
>     - Hardcoding the green gains to 1.0 means that we will have an
>       "average" gain different than 1.0 in most cases. This will
>       effectively change the sensitivity of the camera, and should be
>       taken into account to compute the ISO value. Applications could do
>       so, but I wonder if we shouldn't mandate that the "average" gain
>       remains 1.0, with the "sensitivity boost" being handled by the
>       digital gain control only. How to compute the "average" gain is an
>       interesting problem here, as I expect it will need to take a full
>       colour model into account.

My experience has been as follows. The sensor manufacturer will tell
you that an image with analogue gain of 1 corresponds to an ISO of
(for example) 40. The OEM will then want you to white balance this
image properly and then for it to come out of the system with an ISO
of 40. Yet the RGB colour gains you have applied might actually have
been (1.5, 1.0, 2.0). So perhaps the green gain is really the thing
that corresponds to a "global gain", it's certainly the one with most
effect.

Note further that you can *never* apply any gains less than 1. If you
do, any saturated pixels will become unsaturated, so if you applied
RGB gains like (0.8, 1.0, 1.6) all your bright whites would go cyan.
The upshot is that you have to apply (1.0, 1.25, 2.0) instead (scale
everything up by 1 / 0.8 = 1.25). So in this case, what is the "global
gain"? Is it 1.25? Does that mean your ISO (which you might have
thought was 40) is now 1.25*40 = 50? Maybe. For me, this is probably
OK if you don't let people set the global gain.

Buf if you do, I think there are further problems. What happens if
your user says "I don't want any global gain, so I'll set it to 1". Do
you let the whites go cyan? Or force it to be 1.25 instead so that all
values between 1 and 1.25 have no effect? Or maybe you always include
an extra gain of 1 / 0.8 (in this case), so that we can still call the
good values (1.0, 1.25, 2.0) a "global gain of 1"? Tricky questions.

I end up in a place, therefore, where I feel you kind of have to let
pipelines do what they want. I think we can still maintain the
following:

* Handling the two colour gains (normalised to a green gain of 1) seems fine.
* You should be able to query and set the colour gains (effectively,
the white balance).
* You should ideally be able to query the "global gain". It gives you
some idea of the "effective gain" (in some vague sense) applied to all
pixels. You can use it to scale your ISO estimates, for example.
* If a pipeline wants to let you set the global gain, there's a bit of
a question as to what it really means!

>
> - Demosaicing. This shouldn't apply any gain.

Agree.

>
> - RGB-to-RGB colour transformation matrix.
>
>   Questions:
>
>   - Similarly to the per-channel gains, should we require that this
>     matrix applies no gain (i.e. the sum of elements in a line should be
>     1.0) ?

Agree. Even if a pipeline actually mixes its gains and colour matrix
together, there's no reason not to present them as separate things
externally.

>
> - RGB-to-YUV colour encoding matrix.
>
>   Questions:
>
>   - Similarly to the RGB-to-RGB matrix, should we forbid gains ?

I think so.

>
> Is there any other colour processing operation I have missed ?

Some pipelines have relatively sophisticated lookup tables for
fine-tuning colours, but I don't think those concern anything we've
discussed here.

I think that's everything. Sorry if it's a bit long in places!

David

>
> > A slightly more qualitative definition seems OK to me - the amount of
> > linear gain applied by the pipeline to all pixels (in addition to the
> > colour gains). Particular pipelines might feel they need to document
> > it more carefully if there are any complications in how they deal with
> > it.
> >
> > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:
> > > > > > Hi everyone
> > > > > >
> > > > > > Sounds like we're happy enough from the point of view of this thing
> > > > > > being read-only (for Raspberry Pi at least). Would anyone want any
> > > > > > changes to the wording? Perhaps the final sentence/paragraph might now
> > > > > > be better as
> > > > > >
> > > > > > "This control is present in a request's ControlList only if the
> > > > > > pipeline supports setting the value. Even when it cannot be set by an
> > > > > > application, the pipeline may still report the actual value used in
> > > > > > the metadata returned with completed requests."
> > > > >
> > > > > I don't think it it necessary. It is implied that if a pipeline
> > > > > handler does not support changing the digital gain it should not
> > > > > expose it a one of the Camera's controls.
> > > > >
> > > > > Likewise, if it is something that applications should be informed of,
> > > > > it will be reported via metadata.
> > > > >
> > > > > I think we're good to go, except for the point that we've left
> > > > > floating about having this a single value or a per-channel value.
> > > > >
> > > > > I'm trying to get a feeling how this would be reported by your ISP. I
> > > > > see in example you have two per-channel values for the ColourGains
> > > > > control. Is this anyway related ?
> > > > >
> > > > > > Any other thoughts?
> > > > > >
> > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham wrote:
> > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:
> > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:
> > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:
> > > > > > > >>> This control reports the global digital gain applied by the pipeline
> > > > > > > >>> as a whole, after capturing a raw image from the sensor.
> > > > > > > >>>
> > > > > > > >>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > > >>> ---
> > > > > > > >>>  src/libcamera/control_ids.yaml | 11 +++++++++++
> > > > > > > >>>  1 file changed, 11 insertions(+)
> > > > > > > >>>
> > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > > >>> index c8874fa9..e6362c74 100644
> > > > > > > >>> --- a/src/libcamera/control_ids.yaml
> > > > > > > >>> +++ b/src/libcamera/control_ids.yaml
> > > > > > > >>> @@ -530,4 +530,15 @@ controls:
> > > > > > > >>>          This control is only present when the pipeline supports scaling. Its
> > > > > > > >>>          maximum valid value is given by the properties::ScalerCropMaximum
> > > > > > > >>>          property, and the two can be used to implement digital zoom.
> > > > > > > >>> +
> > > > > > > >>> +  - DigitalGain:
> > > > > > > >>> +      type: float
> > > > > > > >>> +      description: |
> > > > > > > >>> +        Global digital gain value applied to the image during all the
> > > > > > > >>> +        processing steps after capturing the image from the sensor. Any raw
> > > > > > > >>> +        images, therefore, will not include this gain, but the final images
> > > > > > > >>> +        output by the imaging pipeline as a whole will include it.
> > > > > > > >>> +
> > > > > > > >>> +        This control is intended to report the value used by the image
> > > > > > > >>> +        processing pipeline.
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to
> > > > > > > >> pipeline handlers to set this to the appropriate value for each stream
> > > > > > > >> when it completes. The fact that this value would not be applicable to a
> > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata
> > > > > > > >> style value.
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> I'd hope this could be handled by a common helper in that instance so it
> > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,
> > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that
> > > > > > > >> will be a core issue anyway.
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we
> > > > > > > >> determine that someone needs to set it.
> > > > > > > >>
> > > > > > > >> We could introduce a control property between type: and description:
> > > > > > > >>   read-only: true
> > > > > > > >
> > > > > > > > Isn't a read-only control just a metadata ?
> > > > > > > >
> > > > > > > > Wouldn't it be enough for a pipeline that does not support changing
> > > > > > > > the control value from applications not reporting it in the list of
> > > > > > > > supported Camera's controls, but only report it as part of a completed
> > > > > > > > request's metadata ?
> > > > > > >
> > > > > > > Ah, yes of course - because if the control is not listed as supported it
> > > > > > > won't be there to set in the first place! I forgot about that.
> > > > > > >
> > > > > > > So - indeed, no requirement to mark anything as read-only. That will be
> > > > > > > implicit.
> > > > > > >
> > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on
> > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but
> > > > > > > >> that brings it's own questions )
> > > > > > > >>
> > > > > > > >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > > > > >>
> > > > > > > >>>  ...
>
> --
> Regards,
>
> Laurent Pinchart