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

Jacopo Mondi jacopo at jmondi.org
Tue Nov 24 12:28:08 CET 2020 

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

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.
> > > > >>
> > > > >>
> > > > >> 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