| Message ID | 20250923190657.21453-1-hansg@kernel.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hans de Goede <hansg@kernel.org> writes: > Hi All, > > Due to a combination of not having correct control-delay information for > various sensors as well as the generic nature of the simple-pipeline + > software-ISP meaning that any pipeline delays are unknown it is impossible > to get reliable control-delay values. > > Wrong control-delay values can lead to pretty bad oscilation. An example > oscilation which I observed while debugging this went as follows: > > 1. gain is set at 910 of 1000 max > 2. a new frame is processed and is found to not be bright enough, gain gets > changed to 1000 > 3. the gain gets applied 1 frame earlier then expected > 4. a new frame with gain 1000 is processed and is now found to be too bright > (gain overshoot), the AGC code starts with the old gain of 910 due to > the control-delay issue and decreases this to 820, this gets applied > 1 frame earlier then expected > 5. a new frame with gain 820 get processed, this frame is not bright > enough so the gain gets increased from 1000 to 1000 (clipped at max), > this gets applied 1 frame earlier then expected > 6. a new frame with gain 1000 is processed and is now found to be too bright > (gain overshoot), the AGC code starts with the old gain of 820 due to > the control-delay issue and decreases this to 740, this gets applied > 1 frame earlier then expected ... > > This repeats over and over increasing the oscilation (by decreasing the low > gain value) until the gain bounces between gain-min and gain max leading > to pretty bad flickering. > > This series fixes this by remembering the last set gain and exposure values > instead of using the delayed-ctrls values, combined with skipping a fixed > number of frames after changing values. It fixes the issue in my environment, without any very obvious problem. Tested-by: Milan Zamazal <mzamazal@redhat.com> > This effectively reverts commit bb1aa92eb9ee ("libcamera: software_isp: > Initialize exposure+gain before agc calculations"). That commit was added > to fix the initial gain and exposure starting at 0, this series initializes > the cached values with the actual sensor values on the first frame to > avoid re-introducing this problem. > > This deviates from how other ISPs do things which is not ideal, but atm > it is not possible to fix the wrong control-delay values, so this is > the best we can do. > > Regards, > > Hans > > > Hans de Goede (5): > ipa: software_isp: Fix context_.configuration.agc.againMin init > ipa: software_isp: AGC: do not lower gain below default gain value > ipa: software_isp: AGC: Raise exposure or gain not both at the same > time > ipa: software_isp: AGC: Only use integers for exposure calculations > ipa: software_isp: AGC: Stop using delayed control for previous values > > src/ipa/simple/algorithms/agc.cpp | 53 +++++++++++++++++++++---------- > src/ipa/simple/algorithms/agc.h | 3 +- > src/ipa/simple/ipa_context.h | 8 ++++- > src/ipa/simple/soft_simple.cpp | 11 +++++-- > 4 files changed, 53 insertions(+), 22 deletions(-)
Quoting Hans de Goede (2025-09-23 20:06:52) > Hi All, > > Due to a combination of not having correct control-delay information for > various sensors as well as the generic nature of the simple-pipeline + > software-ISP meaning that any pipeline delays are unknown it is impossible > to get reliable control-delay values. > > Wrong control-delay values can lead to pretty bad oscilation. An example > oscilation which I observed while debugging this went as follows: > > 1. gain is set at 910 of 1000 max > 2. a new frame is processed and is found to not be bright enough, gain gets > changed to 1000 > 3. the gain gets applied 1 frame earlier then expected > 4. a new frame with gain 1000 is processed and is now found to be too bright > (gain overshoot), the AGC code starts with the old gain of 910 due to > the control-delay issue and decreases this to 820, this gets applied > 1 frame earlier then expected > 5. a new frame with gain 820 get processed, this frame is not bright > enough so the gain gets increased from 1000 to 1000 (clipped at max), > this gets applied 1 frame earlier then expected > 6. a new frame with gain 1000 is processed and is now found to be too bright > (gain overshoot), the AGC code starts with the old gain of 820 due to > the control-delay issue and decreases this to 740, this gets applied > 1 frame earlier then expected ... > > This repeats over and over increasing the oscilation (by decreasing the low > gain value) until the gain bounces between gain-min and gain max leading > to pretty bad flickering. > > This series fixes this by remembering the last set gain and exposure values > instead of using the delayed-ctrls values, combined with skipping a fixed > number of frames after changing values. > > This effectively reverts commit bb1aa92eb9ee ("libcamera: software_isp: > Initialize exposure+gain before agc calculations"). That commit was added > to fix the initial gain and exposure starting at 0, this series initializes > the cached values with the actual sensor values on the first frame to > avoid re-introducing this problem. > > This deviates from how other ISPs do things which is not ideal, but atm > it is not possible to fix the wrong control-delay values, so this is > the best we can do. I've tested this - and I can say it improves my camera and makes it usable. So to me that's enough to think we should indeed merge this already. The response time for the AGC is /slow/ and I can easily push it into situations where I can over expose my face - but I think that's me purposefully pushing the limits. Given our current state on master I think we should merge this series - and then work on future improvements on top. This will return the SoftISP to working and usable camera for users which is the most critical aspect at the moment. Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Regards, > > Hans > > > Hans de Goede (5): > ipa: software_isp: Fix context_.configuration.agc.againMin init > ipa: software_isp: AGC: do not lower gain below default gain value > ipa: software_isp: AGC: Raise exposure or gain not both at the same > time > ipa: software_isp: AGC: Only use integers for exposure calculations > ipa: software_isp: AGC: Stop using delayed control for previous values > > src/ipa/simple/algorithms/agc.cpp | 53 +++++++++++++++++++++---------- > src/ipa/simple/algorithms/agc.h | 3 +- > src/ipa/simple/ipa_context.h | 8 ++++- > src/ipa/simple/soft_simple.cpp | 11 +++++-- > 4 files changed, 53 insertions(+), 22 deletions(-) > > -- > 2.51.0 >
Hi All, Due to a combination of not having correct control-delay information for various sensors as well as the generic nature of the simple-pipeline + software-ISP meaning that any pipeline delays are unknown it is impossible to get reliable control-delay values. Wrong control-delay values can lead to pretty bad oscilation. An example oscilation which I observed while debugging this went as follows: 1. gain is set at 910 of 1000 max 2. a new frame is processed and is found to not be bright enough, gain gets changed to 1000 3. the gain gets applied 1 frame earlier then expected 4. a new frame with gain 1000 is processed and is now found to be too bright (gain overshoot), the AGC code starts with the old gain of 910 due to the control-delay issue and decreases this to 820, this gets applied 1 frame earlier then expected 5. a new frame with gain 820 get processed, this frame is not bright enough so the gain gets increased from 1000 to 1000 (clipped at max), this gets applied 1 frame earlier then expected 6. a new frame with gain 1000 is processed and is now found to be too bright (gain overshoot), the AGC code starts with the old gain of 820 due to the control-delay issue and decreases this to 740, this gets applied 1 frame earlier then expected ... This repeats over and over increasing the oscilation (by decreasing the low gain value) until the gain bounces between gain-min and gain max leading to pretty bad flickering. This series fixes this by remembering the last set gain and exposure values instead of using the delayed-ctrls values, combined with skipping a fixed number of frames after changing values. This effectively reverts commit bb1aa92eb9ee ("libcamera: software_isp: Initialize exposure+gain before agc calculations"). That commit was added to fix the initial gain and exposure starting at 0, this series initializes the cached values with the actual sensor values on the first frame to avoid re-introducing this problem. This deviates from how other ISPs do things which is not ideal, but atm it is not possible to fix the wrong control-delay values, so this is the best we can do. Regards, Hans Hans de Goede (5): ipa: software_isp: Fix context_.configuration.agc.againMin init ipa: software_isp: AGC: do not lower gain below default gain value ipa: software_isp: AGC: Raise exposure or gain not both at the same time ipa: software_isp: AGC: Only use integers for exposure calculations ipa: software_isp: AGC: Stop using delayed control for previous values src/ipa/simple/algorithms/agc.cpp | 53 +++++++++++++++++++++---------- src/ipa/simple/algorithms/agc.h | 3 +- src/ipa/simple/ipa_context.h | 8 ++++- src/ipa/simple/soft_simple.cpp | 11 +++++-- 4 files changed, 53 insertions(+), 22 deletions(-)