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

[Laurent Pinchart]

Hi Kieran,

On Thu, Nov 26, 2020 at 10:52:43AM +0000, Kieran Bingham wrote:
> On 26/11/2020 10:49, Laurent Pinchart wrote:
> > On Mon, Nov 23, 2020 at 09:17:33AM +0000, 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.
> > 
> > There could still be value in specifying this in the yaml file, to
> > document which controls are meant to be read-only. Instead of writing
> > "This control is read-only" or "This control is meant to be used in
> > metadata only", a read-only property (I'm sure we'll bikeshed the name,
> > maybe "type: metadata" would be better) would allow compile-time and
> > runtime sanity checks. That's not a prerequisite for this patch though.
> 
> I don't actually think this has to be a read-only control though.
> 
> I can imagine this being an enabled control that will allow increasing
> the gain of an image (without affecting colour balance).
> 
> To me - this is the same as AnalogueGain but applied at the ISP rather
> than the sensor right ?

Yes, this doesn't necessarily have to be read-only, my comment was about
general handling of read-only controls, not this specific one.

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