| Message ID | 20240322131451.3092931-1-dan.scally@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, On Fri, Mar 22, 2024 at 01:14:41PM +0000, Daniel Scally wrote: > Hello all > > The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely), > but the implementations are wholly separate. This introduces the potential for > divergence and also makes both maintenance and improvement more difficult. This > series unifies them as derivations of a new MeanLuminanceAgc class within > libipa. The algorithm itself is done through functions of the base class > with the IPA's derived classes providing a function through which the mean > luminance input to the algorithm can be calculated from their specific stats > formats. > > The old implementations effectively hard coded values for the AeConstraintMode > and AeExposureMode controls; they adhered to a single lower-bound constraint of > 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode > expected shutter time to be driven to its maximum before touching gain at all. > The base class additionally adds infrastructure to discover the supported values > for AeConstraintMode and AeExposureMode controls from a camera sensor tuning > file and uses the values defined in those files with the algorithm instead, > though as no tuning files are modified in this series the behaviour currently > remains as it was before. > > The series **does** alter the calculated shutter time and gain for both the > IPU3 and RkISP1 compared to their bespoke implementations, for two reasons: > > 1. A bug in the way they were implemented meant that an over-exposed image was > corrected more slowly than an under-exposed one. This is fixed and will > improve both IPAs' response to a too-bright image. > 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches > the value from the IPU3 implementation. In the RkISP1 implementation that > value was set to 0.4 with a \todo to see why they were different. My belief > is that they were different because they were implemented in different > lighting conditions. In my very imperfect testing setup the 0.16 target > produced reasonable images on both. Just a wild guess (and the numbers fit nicely). This might be dependent on whether gamma is included in the calculation or not. A value of 0.16 corresponds to 0.4 for gamma=2. Cheers, Stefan > > Without those two changes, the shutter time and gain calculated after this > series matches those calculated by their independent implementations. > > Thanks > Dan > > Daniel Scally (9): > ipa: libipa: Allow creation of empty Histogram > libcamera: controls: Generate enum value-name maps > ipa: libipa: Add MeanLuminanceAgc base class > ipa: ipu3: Parse and store Agc stats > ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc > ipa: ipu3: Remove bespoke AGC functions from IPU3 > ipa: rkisp1: Add parseStatistics() to Agc > ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc > ipa: rkisp1: Remove bespoke Agc functions > > Paul Elder (1): > ipa: libipa: Add ExposureModeHelper > > include/libcamera/control_ids.h.in | 2 + > include/libcamera/property_ids.h.in | 2 + > src/ipa/ipu3/algorithms/agc.cpp | 306 ++++---------- > src/ipa/ipu3/algorithms/agc.h | 27 +- > src/ipa/ipu3/ipa_context.cpp | 3 + > src/ipa/ipu3/ipa_context.h | 5 + > src/ipa/ipu3/ipu3.cpp | 2 +- > src/ipa/libipa/agc.cpp | 526 ++++++++++++++++++++++++ > src/ipa/libipa/agc.h | 82 ++++ > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++ > src/ipa/libipa/exposure_mode_helper.h | 61 +++ > src/ipa/libipa/histogram.h | 1 + > src/ipa/libipa/meson.build | 4 + > src/ipa/rkisp1/algorithms/agc.cpp | 303 ++++---------- > src/ipa/rkisp1/algorithms/agc.h | 19 +- > src/ipa/rkisp1/ipa_context.h | 5 + > src/ipa/rkisp1/rkisp1.cpp | 2 +- > utils/gen-controls.py | 19 + > 18 files changed, 1219 insertions(+), 457 deletions(-) > create mode 100644 src/ipa/libipa/agc.cpp > create mode 100644 src/ipa/libipa/agc.h > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp > create mode 100644 src/ipa/libipa/exposure_mode_helper.h > > -- > 2.34.1 >
On 22/03/2024 13:14, Daniel Scally wrote: > Hello all > > The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely), > but the implementations are wholly separate. This introduces the potential for > divergence and also makes both maintenance and improvement more difficult. This > series unifies them as derivations of a new MeanLuminanceAgc class within > libipa. The algorithm itself is done through functions of the base class > with the IPA's derived classes providing a function through which the mean > luminance input to the algorithm can be calculated from their specific stats > formats. > > The old implementations effectively hard coded values for the AeConstraintMode > and AeExposureMode controls; they adhered to a single lower-bound constraint of > 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode > expected shutter time to be driven to its maximum before touching gain at all. > The base class additionally adds infrastructure to discover the supported values > for AeConstraintMode and AeExposureMode controls from a camera sensor tuning > file and uses the values defined in those files with the algorithm instead, > though as no tuning files are modified in this series the behaviour currently > remains as it was before. > > The series **does** alter the calculated shutter time and gain for both the > IPU3 and RkISP1 compared to their bespoke implementations, for two reasons: > > 1. A bug in the way they were implemented meant that an over-exposed image was > corrected more slowly than an under-exposed one. This is fixed and will > improve both IPAs' response to a too-bright image. > 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches > the value from the IPU3 implementation. In the RkISP1 implementation that > value was set to 0.4 with a \todo to see why they were different. My belief > is that they were different because they were implemented in different > lighting conditions. In my very imperfect testing setup the 0.16 target > produced reasonable images on both. Actually the more I think about this, the more I think we'll need a value for each IPA anyway because it's a figure that depends on how they calculate things. RkISP1 takes an average of an array of values which are the themselves the average Luminance in a zone, where Y=(R+G+B) * 0.332. IPU3 takes sums the average R/G/B value in each zone of red/blue/green pixels and adjusts them by the BT.601 values. Rpi I **think** gets R/G/B sums across an image and adjusts them by BT.601. All of those methods seem bound to produce different estimations of Y even with completely identical scenes, so I'm leaning towards the view that we probably need to define the default kRelativeLuminanceTarget in each IPA's implementation of Agc separately. > > Without those two changes, the shutter time and gain calculated after this > series matches those calculated by their independent implementations. > > Thanks > Dan > > Daniel Scally (9): > ipa: libipa: Allow creation of empty Histogram > libcamera: controls: Generate enum value-name maps > ipa: libipa: Add MeanLuminanceAgc base class > ipa: ipu3: Parse and store Agc stats > ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc > ipa: ipu3: Remove bespoke AGC functions from IPU3 > ipa: rkisp1: Add parseStatistics() to Agc > ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc > ipa: rkisp1: Remove bespoke Agc functions > > Paul Elder (1): > ipa: libipa: Add ExposureModeHelper > > include/libcamera/control_ids.h.in | 2 + > include/libcamera/property_ids.h.in | 2 + > src/ipa/ipu3/algorithms/agc.cpp | 306 ++++---------- > src/ipa/ipu3/algorithms/agc.h | 27 +- > src/ipa/ipu3/ipa_context.cpp | 3 + > src/ipa/ipu3/ipa_context.h | 5 + > src/ipa/ipu3/ipu3.cpp | 2 +- > src/ipa/libipa/agc.cpp | 526 ++++++++++++++++++++++++ > src/ipa/libipa/agc.h | 82 ++++ > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++ > src/ipa/libipa/exposure_mode_helper.h | 61 +++ > src/ipa/libipa/histogram.h | 1 + > src/ipa/libipa/meson.build | 4 + > src/ipa/rkisp1/algorithms/agc.cpp | 303 ++++---------- > src/ipa/rkisp1/algorithms/agc.h | 19 +- > src/ipa/rkisp1/ipa_context.h | 5 + > src/ipa/rkisp1/rkisp1.cpp | 2 +- > utils/gen-controls.py | 19 + > 18 files changed, 1219 insertions(+), 457 deletions(-) > create mode 100644 src/ipa/libipa/agc.cpp > create mode 100644 src/ipa/libipa/agc.h > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp > create mode 100644 src/ipa/libipa/exposure_mode_helper.h >
Hi Dan, On Fri, Mar 22, 2024 at 02:48:06PM +0000, Daniel Scally wrote: > On 22/03/2024 13:14, Daniel Scally wrote: > > Hello all > > > > The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely), > > but the implementations are wholly separate. This introduces the potential for > > divergence and also makes both maintenance and improvement more difficult. This > > series unifies them as derivations of a new MeanLuminanceAgc class within > > libipa. The algorithm itself is done through functions of the base class > > with the IPA's derived classes providing a function through which the mean > > luminance input to the algorithm can be calculated from their specific stats > > formats. > > > > The old implementations effectively hard coded values for the AeConstraintMode > > and AeExposureMode controls; they adhered to a single lower-bound constraint of > > 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode > > expected shutter time to be driven to its maximum before touching gain at all. > > The base class additionally adds infrastructure to discover the supported values > > for AeConstraintMode and AeExposureMode controls from a camera sensor tuning > > file and uses the values defined in those files with the algorithm instead, > > though as no tuning files are modified in this series the behaviour currently > > remains as it was before. > > > > The series **does** alter the calculated shutter time and gain for both the > > IPU3 and RkISP1 compared to their bespoke implementations, for two reasons: > > > > 1. A bug in the way they were implemented meant that an over-exposed image was > > corrected more slowly than an under-exposed one. This is fixed and will > > improve both IPAs' response to a too-bright image. > > 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches > > the value from the IPU3 implementation. In the RkISP1 implementation that > > value was set to 0.4 with a \todo to see why they were different. My belief > > is that they were different because they were implemented in different > > lighting conditions. Then we clearly have something to fix, as the code shouldn't contain compile-time constants that depend on the lighting conditions :-) > > In my very imperfect testing setup the 0.16 target > > produced reasonable images on both. > > Actually the more I think about this, the more I think we'll need a > value for each IPA anyway because it's a figure that depends on how > they calculate things. RkISP1 takes an average of an array of values > which are the themselves the average Luminance in a zone, where > Y=(R+G+B) * 0.332. IPU3 takes sums the average R/G/B value in each > zone of red/blue/green pixels and adjusts them by the BT.601 values. > Rpi I **think** gets R/G/B sums across an image and adjusts them by > BT.601. All of those methods seem bound to produce different > estimations of Y even with completely identical scenes, so I'm leaning > towards the view that we probably need to define the default > kRelativeLuminanceTarget in each IPA's implementation of Agc > separately. Wouldn't it be better to define how the algorithm expects the luminance to be estimated, and require each platform-specific specialization to compute the luminance estimate accordingly ? Injecting ill-defined luminance estimates with ill-defined parameters to tweak the behaviour of the calculations doesn't seem very scientific. > > Without those two changes, the shutter time and gain calculated after this > > series matches those calculated by their independent implementations. > > > > Thanks > > Dan > > > > Daniel Scally (9): > > ipa: libipa: Allow creation of empty Histogram > > libcamera: controls: Generate enum value-name maps > > ipa: libipa: Add MeanLuminanceAgc base class > > ipa: ipu3: Parse and store Agc stats > > ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc > > ipa: ipu3: Remove bespoke AGC functions from IPU3 > > ipa: rkisp1: Add parseStatistics() to Agc > > ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc > > ipa: rkisp1: Remove bespoke Agc functions > > > > Paul Elder (1): > > ipa: libipa: Add ExposureModeHelper > > > > include/libcamera/control_ids.h.in | 2 + > > include/libcamera/property_ids.h.in | 2 + > > src/ipa/ipu3/algorithms/agc.cpp | 306 ++++---------- > > src/ipa/ipu3/algorithms/agc.h | 27 +- > > src/ipa/ipu3/ipa_context.cpp | 3 + > > src/ipa/ipu3/ipa_context.h | 5 + > > src/ipa/ipu3/ipu3.cpp | 2 +- > > src/ipa/libipa/agc.cpp | 526 ++++++++++++++++++++++++ > > src/ipa/libipa/agc.h | 82 ++++ > > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++ > > src/ipa/libipa/exposure_mode_helper.h | 61 +++ > > src/ipa/libipa/histogram.h | 1 + > > src/ipa/libipa/meson.build | 4 + > > src/ipa/rkisp1/algorithms/agc.cpp | 303 ++++---------- > > src/ipa/rkisp1/algorithms/agc.h | 19 +- > > src/ipa/rkisp1/ipa_context.h | 5 + > > src/ipa/rkisp1/rkisp1.cpp | 2 +- > > utils/gen-controls.py | 19 + > > 18 files changed, 1219 insertions(+), 457 deletions(-) > > create mode 100644 src/ipa/libipa/agc.cpp > > create mode 100644 src/ipa/libipa/agc.h > > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp > > create mode 100644 src/ipa/libipa/exposure_mode_helper.h
On Fri, Mar 22, 2024 at 03:33:18PM +0100, Stefan Klug wrote: > Hi Daniel, > > On Fri, Mar 22, 2024 at 01:14:41PM +0000, Daniel Scally wrote: > > Hello all > > > > The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely), > > but the implementations are wholly separate. This introduces the potential for > > divergence and also makes both maintenance and improvement more difficult. This > > series unifies them as derivations of a new MeanLuminanceAgc class within > > libipa. The algorithm itself is done through functions of the base class > > with the IPA's derived classes providing a function through which the mean > > luminance input to the algorithm can be calculated from their specific stats > > formats. > > > > The old implementations effectively hard coded values for the AeConstraintMode > > and AeExposureMode controls; they adhered to a single lower-bound constraint of > > 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode > > expected shutter time to be driven to its maximum before touching gain at all. > > The base class additionally adds infrastructure to discover the supported values > > for AeConstraintMode and AeExposureMode controls from a camera sensor tuning > > file and uses the values defined in those files with the algorithm instead, > > though as no tuning files are modified in this series the behaviour currently > > remains as it was before. > > > > The series **does** alter the calculated shutter time and gain for both the > > IPU3 and RkISP1 compared to their bespoke implementations, for two reasons: > > > > 1. A bug in the way they were implemented meant that an over-exposed image was > > corrected more slowly than an under-exposed one. This is fixed and will > > improve both IPAs' response to a too-bright image. > > 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches > > the value from the IPU3 implementation. In the RkISP1 implementation that > > value was set to 0.4 with a \todo to see why they were different. My belief > > is that they were different because they were implemented in different > > lighting conditions. In my very imperfect testing setup the 0.16 target > > produced reasonable images on both. > > Just a wild guess (and the numbers fit nicely). This might be dependent on > whether gamma is included in the calculation or not. A value of 0.16 > corresponds to 0.4 for gamma=2. Both the IPU3 and RkISP1 are supposed to produce stats computed on linear-space data. We don't linearize the sensor output at the moment, but it should be mostly linear to start with. > > Without those two changes, the shutter time and gain calculated after this > > series matches those calculated by their independent implementations. > > > > Thanks > > Dan > > > > Daniel Scally (9): > > ipa: libipa: Allow creation of empty Histogram > > libcamera: controls: Generate enum value-name maps > > ipa: libipa: Add MeanLuminanceAgc base class > > ipa: ipu3: Parse and store Agc stats > > ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc > > ipa: ipu3: Remove bespoke AGC functions from IPU3 > > ipa: rkisp1: Add parseStatistics() to Agc > > ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc > > ipa: rkisp1: Remove bespoke Agc functions > > > > Paul Elder (1): > > ipa: libipa: Add ExposureModeHelper > > > > include/libcamera/control_ids.h.in | 2 + > > include/libcamera/property_ids.h.in | 2 + > > src/ipa/ipu3/algorithms/agc.cpp | 306 ++++---------- > > src/ipa/ipu3/algorithms/agc.h | 27 +- > > src/ipa/ipu3/ipa_context.cpp | 3 + > > src/ipa/ipu3/ipa_context.h | 5 + > > src/ipa/ipu3/ipu3.cpp | 2 +- > > src/ipa/libipa/agc.cpp | 526 ++++++++++++++++++++++++ > > src/ipa/libipa/agc.h | 82 ++++ > > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++ > > src/ipa/libipa/exposure_mode_helper.h | 61 +++ > > src/ipa/libipa/histogram.h | 1 + > > src/ipa/libipa/meson.build | 4 + > > src/ipa/rkisp1/algorithms/agc.cpp | 303 ++++---------- > > src/ipa/rkisp1/algorithms/agc.h | 19 +- > > src/ipa/rkisp1/ipa_context.h | 5 + > > src/ipa/rkisp1/rkisp1.cpp | 2 +- > > utils/gen-controls.py | 19 + > > 18 files changed, 1219 insertions(+), 457 deletions(-) > > create mode 100644 src/ipa/libipa/agc.cpp > > create mode 100644 src/ipa/libipa/agc.h > > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp > > create mode 100644 src/ipa/libipa/exposure_mode_helper.h
Hi Laurent On 05/04/2024 22:19, Laurent Pinchart wrote: > Hi Dan, > > On Fri, Mar 22, 2024 at 02:48:06PM +0000, Daniel Scally wrote: >> On 22/03/2024 13:14, Daniel Scally wrote: >>> Hello all >>> >>> The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely), >>> but the implementations are wholly separate. This introduces the potential for >>> divergence and also makes both maintenance and improvement more difficult. This >>> series unifies them as derivations of a new MeanLuminanceAgc class within >>> libipa. The algorithm itself is done through functions of the base class >>> with the IPA's derived classes providing a function through which the mean >>> luminance input to the algorithm can be calculated from their specific stats >>> formats. >>> >>> The old implementations effectively hard coded values for the AeConstraintMode >>> and AeExposureMode controls; they adhered to a single lower-bound constraint of >>> 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode >>> expected shutter time to be driven to its maximum before touching gain at all. >>> The base class additionally adds infrastructure to discover the supported values >>> for AeConstraintMode and AeExposureMode controls from a camera sensor tuning >>> file and uses the values defined in those files with the algorithm instead, >>> though as no tuning files are modified in this series the behaviour currently >>> remains as it was before. >>> >>> The series **does** alter the calculated shutter time and gain for both the >>> IPU3 and RkISP1 compared to their bespoke implementations, for two reasons: >>> >>> 1. A bug in the way they were implemented meant that an over-exposed image was >>> corrected more slowly than an under-exposed one. This is fixed and will >>> improve both IPAs' response to a too-bright image. >>> 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches >>> the value from the IPU3 implementation. In the RkISP1 implementation that >>> value was set to 0.4 with a \todo to see why they were different. My belief >>> is that they were different because they were implemented in different >>> lighting conditions. > Then we clearly have something to fix, as the code shouldn't contain > compile-time constants that depend on the lighting conditions :-) > >>> In my very imperfect testing setup the 0.16 target >>> produced reasonable images on both. >> Actually the more I think about this, the more I think we'll need a >> value for each IPA anyway because it's a figure that depends on how >> they calculate things. RkISP1 takes an average of an array of values >> which are the themselves the average Luminance in a zone, where >> Y=(R+G+B) * 0.332. IPU3 takes sums the average R/G/B value in each >> zone of red/blue/green pixels and adjusts them by the BT.601 values. >> Rpi I **think** gets R/G/B sums across an image and adjusts them by >> BT.601. All of those methods seem bound to produce different >> estimations of Y even with completely identical scenes, so I'm leaning >> towards the view that we probably need to define the default >> kRelativeLuminanceTarget in each IPA's implementation of Agc >> separately. > Wouldn't it be better to define how the algorithm expects the luminance > to be estimated, and require each platform-specific specialization to > compute the luminance estimate accordingly ? Injecting ill-defined > luminance estimates with ill-defined parameters to tweak the behaviour > of the calculations doesn't seem very scientific. That approach also has problems though; at minimum it would force the IPU3 IPA to calculate a histogram using the formula that's internally used by the RkISP1 (since we couldn't calculate a histogram using the BT.601 values for RkISP1) and forcing one IPA to adhere to the hardware constraints of another doesn't sound right. I **think** (but am less sure about) that the statistics formats are too different to make a truly equivalent calculation anyway; meaning for the same input data from a sensor I don't think you'd be able to arrive at precisely the same luminance estimation across each ISP. But is that a problem? I think as long as we understand that and make it clear that that's what's happening it ought to be fine. @Stefan, @Jacopo we talked about this on the call that we had together...but I'm afraid I left it too long to revisit the series and now can't remember what we concluded about this difference in luminance targets between ISPs and the likely explanation for it. > >>> Without those two changes, the shutter time and gain calculated after this >>> series matches those calculated by their independent implementations. >>> >>> Thanks >>> Dan >>> >>> Daniel Scally (9): >>> ipa: libipa: Allow creation of empty Histogram >>> libcamera: controls: Generate enum value-name maps >>> ipa: libipa: Add MeanLuminanceAgc base class >>> ipa: ipu3: Parse and store Agc stats >>> ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc >>> ipa: ipu3: Remove bespoke AGC functions from IPU3 >>> ipa: rkisp1: Add parseStatistics() to Agc >>> ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc >>> ipa: rkisp1: Remove bespoke Agc functions >>> >>> Paul Elder (1): >>> ipa: libipa: Add ExposureModeHelper >>> >>> include/libcamera/control_ids.h.in | 2 + >>> include/libcamera/property_ids.h.in | 2 + >>> src/ipa/ipu3/algorithms/agc.cpp | 306 ++++---------- >>> src/ipa/ipu3/algorithms/agc.h | 27 +- >>> src/ipa/ipu3/ipa_context.cpp | 3 + >>> src/ipa/ipu3/ipa_context.h | 5 + >>> src/ipa/ipu3/ipu3.cpp | 2 +- >>> src/ipa/libipa/agc.cpp | 526 ++++++++++++++++++++++++ >>> src/ipa/libipa/agc.h | 82 ++++ >>> src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++ >>> src/ipa/libipa/exposure_mode_helper.h | 61 +++ >>> src/ipa/libipa/histogram.h | 1 + >>> src/ipa/libipa/meson.build | 4 + >>> src/ipa/rkisp1/algorithms/agc.cpp | 303 ++++---------- >>> src/ipa/rkisp1/algorithms/agc.h | 19 +- >>> src/ipa/rkisp1/ipa_context.h | 5 + >>> src/ipa/rkisp1/rkisp1.cpp | 2 +- >>> utils/gen-controls.py | 19 + >>> 18 files changed, 1219 insertions(+), 457 deletions(-) >>> create mode 100644 src/ipa/libipa/agc.cpp >>> create mode 100644 src/ipa/libipa/agc.h >>> create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp >>> create mode 100644 src/ipa/libipa/exposure_mode_helper.h