[libcamera-devel] [PATCH 1/1] libcamera: controls: Add DigitalGain control
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 26 11:57:14 CET 2020
Hi Jacopo, David,
On 26/11/2020 10:25, Jacopo Mondi wrote:
> 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,
That's called colour balance isn't it ?
To me - DigitalGain is analogous to AnalogueGain - and would be expected
to not affect the colour balance.
If you want to control colour balance, that's manipulating the channels
independently, which we have a control for already.
In my opinion, DigitalGain could potentially be something that an
application might set, or even that an IPA might set indicating - "I
want to increase the gain/brightness of the whole image (either beyond
the capability of the AnalogueGain, or if the AnalogueGain can not be
changed)".
And it would be an effect that is /after/ colour balance has occurred.
Perhaps it might be important to declare specifically that the
DigitalGain is a factor applied after any balance gains...
--
Kieran
> 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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list