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

[Jacopo Mondi]

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