[libcamera-devel,0/4] ipa: ipu3: af: Temporarily suspend AWB and AE during AF scanning
mbox series

Message ID 20220322024155.6528-1-hpa@redhat.com
Headers show
Series
  • ipa: ipu3: af: Temporarily suspend AWB and AE during AF scanning
Related show

Message

Kate Hsuan March 22, 2022, 2:41 a.m. UTC 
The floating results of AWB and AE significantly impact the AF
performance and the AF scene variance estimation. If the exposure value
and color settings are changed by the algorithm, the AF scene variance will
be affected and the algorithm may find an incorrect local maximum value.
Consequently, a wrong lens position is determined and we could only get
a blurred image.

This patch proposed an AWB and AE suspension mechanism while AF is
searching for the focus. A lock flag is used to identify that AWB and
AE should be suspended or not. AGC and AE algorithm test the flag and
determine that they have to perform the algorithm or suspend itself.

Kate Hsuan (4):
  ipa: ipu3: af: Introduce AWB and AE lock when AF scanning
  ipa: ipu3: af: AE and AWE lock request flow when performing AF
  ipa: ipu3: awb: AWB lock for AF scan
  ipa: ipu3: agc: AE lock for AF scanning

 src/ipa/ipu3/algorithms/af.cpp  | 25 +++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/af.h    |  2 ++
 src/ipa/ipu3/algorithms/agc.cpp | 14 ++++++++++++++
 src/ipa/ipu3/algorithms/agc.h   |  1 +
 src/ipa/ipu3/algorithms/awb.cpp | 13 +++++++++++++
 src/ipa/ipu3/algorithms/awb.h   |  1 +
 src/ipa/ipu3/ipa_context.h      |  1 +
 7 files changed, 57 insertions(+)

Comments

Jean-Michel Hautbois March 22, 2022, 2:01 p.m. UTC | #1 
Hi Kate !

On 22/03/2022 03:41, Kate Hsuan via libcamera-devel wrote:
> The floating results of AWB and AE significantly impact the AF
> performance and the AF scene variance estimation. If the exposure value
> and color settings are changed by the algorithm, the AF scene variance will
> be affected and the algorithm may find an incorrect local maximum value.
> Consequently, a wrong lens position is determined and we could only get
> a blurred image.

I agree with you, at least on one assumption: AWB can have an impact on 
AF. AE is more debatable I think :-).
I think we should consider something a bit different from what you did, 
if you don't mind, as I don't think AWB should know anything out of its 
own scope, same for AGC, same for AF, same for... you get it :-).
This should be taken into account at the IPA level, the controller, 
which could set a state for an algorithm.

I think that, while autofocus is starting, or in coarse search mode, we 
should not change the awb gains. When is gets into the fine search mode, 
then AWB could be started as it should not interfere anymore. So, I 
think AF could have a 'status' field in its frameContext, and the 
algorithms (AWB, AGC, probably AF) should be "pausable". David sent a 
patch lately for RPi [1] and this is something we should consider.

That beeing said, and I hope I am clear in my explanations, before 
calling the process() call for the algorithms, the IPU3IPU could set the 
status of AWB and AGC based on the AF status. I would start AF after AGC 
is stable enough (could be based on a number of frames for a first try) 
and I would enable AWB while AGC is starting.

Once AGC is stable, AF is entering the coarse search, and in this phase 
AWB is paused. Once it enters the fine search, we can resume AWB.

I think it should be enough, as we are not zooming, so illumination is 
not changing a lot with focus change AFAIK.

I don't really have time to test it so if you want/can I would greatly 
appreciate :-).

Thanks !
JM

[1]: https://patchwork.libcamera.org/patch/15343/

> 
> This patch proposed an AWB and AE suspension mechanism while AF is
> searching for the focus. A lock flag is used to identify that AWB and
> AE should be suspended or not. AGC and AE algorithm test the flag and
> determine that they have to perform the algorithm or suspend itself.
> 
> Kate Hsuan (4):
>    ipa: ipu3: af: Introduce AWB and AE lock when AF scanning
>    ipa: ipu3: af: AE and AWE lock request flow when performing AF
>    ipa: ipu3: awb: AWB lock for AF scan
>    ipa: ipu3: agc: AE lock for AF scanning
> 
>   src/ipa/ipu3/algorithms/af.cpp  | 25 +++++++++++++++++++++++++
>   src/ipa/ipu3/algorithms/af.h    |  2 ++
>   src/ipa/ipu3/algorithms/agc.cpp | 14 ++++++++++++++
>   src/ipa/ipu3/algorithms/agc.h   |  1 +
>   src/ipa/ipu3/algorithms/awb.cpp | 13 +++++++++++++
>   src/ipa/ipu3/algorithms/awb.h   |  1 +
>   src/ipa/ipu3/ipa_context.h      |  1 +
>   7 files changed, 57 insertions(+)
>
Hans de Goede March 23, 2022, 1:30 p.m. UTC | #2 
Hi,

On 3/22/22 15:01, Jean-Michel Hautbois via libcamera-devel wrote:
> Hi Kate !
> 
> On 22/03/2022 03:41, Kate Hsuan via libcamera-devel wrote:
>> The floating results of AWB and AE significantly impact the AF
>> performance and the AF scene variance estimation. If the exposure value
>> and color settings are changed by the algorithm, the AF scene variance will
>> be affected and the algorithm may find an incorrect local maximum value.
>> Consequently, a wrong lens position is determined and we could only get
>> a blurred image.
> 
> I agree with you, at least on one assumption: AWB can have an impact on AF. AE is more debatable I think :-).

Interesting I would actually expect the importance to be the otherway
around. If AE is way off, so way overexposed or much to dark, there
is not much on which the focus algorithm can actually focus. Where as
if an image is e.g. to "green" I would not expect that to make a lot
of difference for the contrast of the image.

> I think we should consider something a bit different from what you did, if you don't mind, as I don't think AWB should know anything out of its own scope, same for AGC, same for AF, same for... you get it :-).
> This should be taken into account at the IPA level, the controller, which could set a state for an algorithm.

I agree, but I think we want this on other IPU-s too, so maybe we need to do this
at an even higher level, assuming that is possible (I still need
to familiarize myself with libcamera more) ?

> I think that, while autofocus is starting, or in coarse search mode, we should not change the awb gains. When is gets into the fine search mode, then AWB could be started as it should not interfere anymore. So, I think AF could have a 'status' field in its frameContext, and the algorithms (AWB, AGC, probably AF) should be "pausable". David sent a patch lately for RPi [1] and this is something we should consider.
> 
> That beeing said, and I hope I am clear in my explanations, before calling the process() call for the algorithms, the IPU3IPU could set the status of AWB and AGC based on the AF status. I would start AF after AGC is stable enough (could be based on a number of frames for a first try) and I would enable AWB while AGC is starting.
> 
> Once AGC is stable, AF is entering the coarse search, and in this phase AWB is paused. Once it enters the fine search, we can resume AWB.

Agreed I think we also need some sort of AGC "done" bit because as
said I don't think doing auto-focus before we at least have the
exposure setup somewhat correct makes a lot of sense.

> I think it should be enough, as we are not zooming, so illumination is not changing a lot with focus change AFAIK.

Regards,

Hans



> 
> I don't really have time to test it so if you want/can I would greatly appreciate :-).
> 
> Thanks !
> JM
> 
> [1]: https://patchwork.libcamera.org/patch/15343/
> 
>>
>> This patch proposed an AWB and AE suspension mechanism while AF is
>> searching for the focus. A lock flag is used to identify that AWB and
>> AE should be suspended or not. AGC and AE algorithm test the flag and
>> determine that they have to perform the algorithm or suspend itself.
>>
>> Kate Hsuan (4):
>>    ipa: ipu3: af: Introduce AWB and AE lock when AF scanning
>>    ipa: ipu3: af: AE and AWE lock request flow when performing AF
>>    ipa: ipu3: awb: AWB lock for AF scan
>>    ipa: ipu3: agc: AE lock for AF scanning
>>
>>   src/ipa/ipu3/algorithms/af.cpp  | 25 +++++++++++++++++++++++++
>>   src/ipa/ipu3/algorithms/af.h    |  2 ++
>>   src/ipa/ipu3/algorithms/agc.cpp | 14 ++++++++++++++
>>   src/ipa/ipu3/algorithms/agc.h   |  1 +
>>   src/ipa/ipu3/algorithms/awb.cpp | 13 +++++++++++++
>>   src/ipa/ipu3/algorithms/awb.h   |  1 +
>>   src/ipa/ipu3/ipa_context.h      |  1 +
>>   7 files changed, 57 insertions(+)
>>
>
Jean-Michel Hautbois March 23, 2022, 1:52 p.m. UTC | #3 
Hello Hans,

On 23/03/2022 14:30, Hans de Goede wrote:
> Hi,
> 
> On 3/22/22 15:01, Jean-Michel Hautbois via libcamera-devel wrote:
>> Hi Kate !
>>
>> On 22/03/2022 03:41, Kate Hsuan via libcamera-devel wrote:
>>> The floating results of AWB and AE significantly impact the AF
>>> performance and the AF scene variance estimation. If the exposure value
>>> and color settings are changed by the algorithm, the AF scene variance will
>>> be affected and the algorithm may find an incorrect local maximum value.
>>> Consequently, a wrong lens position is determined and we could only get
>>> a blurred image.
>>
>> I agree with you, at least on one assumption: AWB can have an impact on AF. AE is more debatable I think :-).
> 
> Interesting I would actually expect the importance to be the otherway
> around. If AE is way off, so way overexposed or much to dark, there
> is not much on which the focus algorithm can actually focus. Where as
> if an image is e.g. to "green" I would not expect that to make a lot
> of difference for the contrast of the image.

Well, indeed, if your scene is too dark or too bright, you will have big 
issues. But in case you are flickering around a somehow not that bad 
value, then shouldn't really change the edges. On the contrary if you 
change only the red channel for instance, you may experience weird 
things... I am not sure that I am clear here :-). Maybe should we 
conduct experiments extracting the filters output in various conditions 
for a given scene ?

> 
>> I think we should consider something a bit different from what you did, if you don't mind, as I don't think AWB should know anything out of its own scope, same for AGC, same for AF, same for... you get it :-).
>> This should be taken into account at the IPA level, the controller, which could set a state for an algorithm.
> 
> I agree, but I think we want this on other IPU-s too, so maybe we need to do this
> at an even higher level, assuming that is possible (I still need
> to familiarize myself with libcamera more) ?
> 
>> I think that, while autofocus is starting, or in coarse search mode, we should not change the awb gains. When is gets into the fine search mode, then AWB could be started as it should not interfere anymore. So, I think AF could have a 'status' field in its frameContext, and the algorithms (AWB, AGC, probably AF) should be "pausable". David sent a patch lately for RPi [1] and this is something we should consider.
>>
>> That beeing said, and I hope I am clear in my explanations, before calling the process() call for the algorithms, the IPU3IPU could set the status of AWB and AGC based on the AF status. I would start AF after AGC is stable enough (could be based on a number of frames for a first try) and I would enable AWB while AGC is starting.
>>
>> Once AGC is stable, AF is entering the coarse search, and in this phase AWB is paused. Once it enters the fine search, we can resume AWB.
> 
> Agreed I think we also need some sort of AGC "done" bit because as
> said I don't think doing auto-focus before we at least have the
> exposure setup somewhat correct makes a lot of sense.
> 
>> I think it should be enough, as we are not zooming, so illumination is not changing a lot with focus change AFAIK.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> I don't really have time to test it so if you want/can I would greatly appreciate :-).
>>
>> Thanks !
>> JM
>>
>> [1]: https://patchwork.libcamera.org/patch/15343/
>>
>>>
>>> This patch proposed an AWB and AE suspension mechanism while AF is
>>> searching for the focus. A lock flag is used to identify that AWB and
>>> AE should be suspended or not. AGC and AE algorithm test the flag and
>>> determine that they have to perform the algorithm or suspend itself.
>>>
>>> Kate Hsuan (4):
>>>     ipa: ipu3: af: Introduce AWB and AE lock when AF scanning
>>>     ipa: ipu3: af: AE and AWE lock request flow when performing AF
>>>     ipa: ipu3: awb: AWB lock for AF scan
>>>     ipa: ipu3: agc: AE lock for AF scanning
>>>
>>>    src/ipa/ipu3/algorithms/af.cpp  | 25 +++++++++++++++++++++++++
>>>    src/ipa/ipu3/algorithms/af.h    |  2 ++
>>>    src/ipa/ipu3/algorithms/agc.cpp | 14 ++++++++++++++
>>>    src/ipa/ipu3/algorithms/agc.h   |  1 +
>>>    src/ipa/ipu3/algorithms/awb.cpp | 13 +++++++++++++
>>>    src/ipa/ipu3/algorithms/awb.h   |  1 +
>>>    src/ipa/ipu3/ipa_context.h      |  1 +
>>>    7 files changed, 57 insertions(+)
>>>
>>
>
Kate Hsuan March 24, 2022, 10:36 a.m. UTC | #4 
Hi Jean-Michel and Hans,

On Wed, Mar 23, 2022 at 9:52 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> Hello Hans,
>
> On 23/03/2022 14:30, Hans de Goede wrote:
> > Hi,
> >
> > On 3/22/22 15:01, Jean-Michel Hautbois via libcamera-devel wrote:
> >> Hi Kate !
> >>
> >> On 22/03/2022 03:41, Kate Hsuan via libcamera-devel wrote:
> >>> The floating results of AWB and AE significantly impact the AF
> >>> performance and the AF scene variance estimation. If the exposure value
> >>> and color settings are changed by the algorithm, the AF scene variance will
> >>> be affected and the algorithm may find an incorrect local maximum value.
> >>> Consequently, a wrong lens position is determined and we could only get
> >>> a blurred image.
> >>
> >> I agree with you, at least on one assumption: AWB can have an impact on AF. AE is more debatable I think :-).
> >
> > Interesting I would actually expect the importance to be the otherway
> > around. If AE is way off, so way overexposed or much to dark, there
> > is not much on which the focus algorithm can actually focus. Where as
> > if an image is e.g. to "green" I would not expect that to make a lot
> > of difference for the contrast of the image.
>
> Well, indeed, if your scene is too dark or too bright, you will have big
> issues. But in case you are flickering around a somehow not that bad
> value, then shouldn't really change the edges. On the contrary if you
> change only the red channel for instance, you may experience weird
> things... I am not sure that I am clear here :-). Maybe should we
> conduct experiments extracting the filters output in various conditions
> for a given scene ?

I observed that the AF variance changed dramatically when AE or AWB
condition changed. Even under the AF stable state, if the light was
turned to dark or lighter, the variance was changed and triggered the
AF scanning. I can export some of the filtered output for some
conditions. Could we list the conditions which you are interested in?
Thank you :)

>
> >
> >> I think we should consider something a bit different from what you did, if you don't mind, as I don't think AWB should know anything out of its own scope, same for AGC, same for AF, same for... you get it :-).
> >> This should be taken into account at the IPA level, the controller, which could set a state for an algorithm.
> >
> > I agree, but I think we want this on other IPU-s too, so maybe we need to do this
> > at an even higher level, assuming that is possible (I still need
> > to familiarize myself with libcamera more) ?
> >
> >> I think that, while autofocus is starting, or in coarse search mode, we should not change the awb gains. When is gets into the fine search mode, then AWB could be started as it should not interfere anymore. So, I think AF could have a 'status' field in its frameContext, and the algorithms (AWB, AGC, probably AF) should be "pausable". David sent a patch lately for RPi [1] and this is something we should consider.
> >>
> >> That beeing said, and I hope I am clear in my explanations, before calling the process() call for the algorithms, the IPU3IPU could set the status of AWB and AGC based on the AF status. I would start AF after AGC is stable enough (could be based on a number of frames for a first try) and I would enable AWB while AGC is starting.
> >>
> >> Once AGC is stable, AF is entering the coarse search, and in this phase AWB is paused. Once it enters the fine search, we can resume AWB.
> >
> > Agreed I think we also need some sort of AGC "done" bit because as
> > said I don't think doing auto-focus before we at least have the
> > exposure setup somewhat correct makes a lot of sense.
> >
> >> I think it should be enough, as we are not zooming, so illumination is not changing a lot with focus change AFAIK.
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >>
> >> I don't really have time to test it so if you want/can I would greatly appreciate :-).
> >>
> >> Thanks !
> >> JM
> >>
> >> [1]: https://patchwork.libcamera.org/patch/15343/
> >>
> >>>
> >>> This patch proposed an AWB and AE suspension mechanism while AF is
> >>> searching for the focus. A lock flag is used to identify that AWB and
> >>> AE should be suspended or not. AGC and AE algorithm test the flag and
> >>> determine that they have to perform the algorithm or suspend itself.
> >>>
> >>> Kate Hsuan (4):
> >>>     ipa: ipu3: af: Introduce AWB and AE lock when AF scanning
> >>>     ipa: ipu3: af: AE and AWE lock request flow when performing AF
> >>>     ipa: ipu3: awb: AWB lock for AF scan
> >>>     ipa: ipu3: agc: AE lock for AF scanning
> >>>
> >>>    src/ipa/ipu3/algorithms/af.cpp  | 25 +++++++++++++++++++++++++
> >>>    src/ipa/ipu3/algorithms/af.h    |  2 ++
> >>>    src/ipa/ipu3/algorithms/agc.cpp | 14 ++++++++++++++
> >>>    src/ipa/ipu3/algorithms/agc.h   |  1 +
> >>>    src/ipa/ipu3/algorithms/awb.cpp | 13 +++++++++++++
> >>>    src/ipa/ipu3/algorithms/awb.h   |  1 +
> >>>    src/ipa/ipu3/ipa_context.h      |  1 +
> >>>    7 files changed, 57 insertions(+)
> >>>
> >>
> >
>