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

Jacopo Mondi jacopo at jmondi.org
Thu Nov 26 11:25:22 CET 2020 

Hi David,

On Thu, Nov 26, 2020 at 09:50:05AM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the update on this.
>
> On Wed, 25 Nov 2020 at 21:02, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi David,
> >     I tried proposing a new version based on the outcome of our
> > discussion
> >
> > Please see below
> >
> > On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:
> > > Hi Jacopo
> > >
> > > Thanks for the reply. More discussion below...
> > >
> > > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > Hi David,
> > > >
> > > > 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".
> > >
> > > 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.
> > >
> > > Best regards
> > > David
> > >
> > > >
> > > > Thanks
> > > >    j
> > > >
> > > > >
> > > > > Thanks!
> > > > > David
> > > > >
> > > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > 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?
> > > > > > >
> > > > > > > Thanks
> > > > > > > David
> > > > > > >
> > > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham
> > > > > > > <kieran.bingham at ideasonboard.com> wrote:
> > > > > > > >
> > > > > > > > Hi Jacopo,
> > > > > > > >
> > > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:
> > > > > > > > > Hi Kieran,
> > > > > > > > >
> > > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:
> > > > > > > > >> Hi David,
> > > > > > > > >>
> > > > > > > > >> 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.
> >
> >   - DigitalGain:
> >       type: float
> >       description: |
> >         Digital gain value applied during the processing steps applied
> >         to the image as captured from the sensor.
> >
> >         The global digital gain factor is applied to all the colour channels
> >         of the RAW image. Different pipeline models are free to
> >         specify how the global gain factor applies to each separate
> >         channel.
> >
> >         If an imaging pipeline applies digital gain in distinct
> >         processing steps, this value indicates their total sum.
> >         Pipelines are free to decide how to adjust each processing
> >         step to respect the received gain factor and shall report
> >         their total value in the request metadata.
> >
> > However this deferring to each pipeline model documentation makes me
> > wonder if expressing per-channel values would make things more
> > explicit...
> >
> > Feel free to take in what you consider appropriate.
>
> Yes, this is all so difficult, we just need to make a decision,
> really. Per-channel values work for me, but only because we're not
> going to let people set them. As a read-only thing, it seems a bit
> easier to understand/explain. We could ignore the question of setting
> a global digital gain until someone wants to do it (maybe no one ever
> will...).
>
> What do you think?

I think that once we have a pipeline that supports per-channel
digital gains we will introduce 'DigitalGains' as an array control,
deprectate this one, and keep supporting it in pipelines that exposed
it (through an internal helper maybe) not to break applications that
made use of it.

It's hard for me to have a firm idea without a real use case, but one
thing that presses me is not to mention this control is read only in
its definition. If that's the case for RPi just report it in metadata
only, but let's not restrict other platforms.

Do you think in the next iteration you will include reporting this
control from the pipeline too ? if I'm not mistaken this version
contains the control definition only.

Thanks
  j


>
> Thanks!
> David
>
> >
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> 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.
> > > > > > > >
> > > > > > > > --
> > > > > > > > Kieran
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >   j
> > > > > > > > >
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> 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
> > > > > > > > >> --
> > > > > > > > >> Kieran
> > > > > > > > >> _______________________________________________
> > > > > > > > >> libcamera-devel mailing list
> > > > > > > > >> libcamera-devel at lists.libcamera.org
> > > > > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel
> > > > > > > >
> > > > > > > > --
> > > > > > > > Regards
> > > > > > > > --
> > > > > > > > Kieran

More information about the libcamera-devel mailing list