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

[Kieran Bingham]

Hi Laurent,

On 26/11/2020 10:49, Laurent Pinchart wrote:
> Hello,
> 
> 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 ?

--
Kieran


> 
>>>> 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
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran