| Message ID | 20211013154125.133419-5-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-10-13 16:41:16) > When the zones are used for the grey world algorithm, we need a minimal > number of zones to make it relevant, and we want the green values to be > at least 32 today (with a maximum of 255). > > This value is a bit high, as we now are correcting the black levels, and > a sudden change of colors can be visible for the end user when the gains > are applied. > > Lower the value to 16. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 30693923..b18874a3 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms { > > LOG_DEFINE_CATEGORY(IPU3Awb) > Do you document the meaning of this constant, and what it is used for / effects of changing it in the later documentation series? If not, could you add something please? (Either in this patch, or later) Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > -static constexpr uint32_t kMinGreenLevelInZone = 32; > +static constexpr uint32_t kMinGreenLevelInZone = 16; > > /** > * \struct Accumulator > -- > 2.30.2 >
On Thu, Oct 14, 2021 at 11:50:22AM +0100, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-10-13 16:41:16) > > When the zones are used for the grey world algorithm, we need a minimal > > number of zones to make it relevant, and we want the green values to be > > at least 32 today (with a maximum of 255). > > > > This value is a bit high, as we now are correcting the black levels, and > > a sudden change of colors can be visible for the end user when the gains > > are applied. > > > > Lower the value to 16. If I understand the patch correctly, the most relevant part here is the minimal green value for a zone to be considered. I would then write the above as When zones are used for the grey world algorithm, they are only considered if their average green value is at least 32/255 to exclude zones that are too dark and don't provide relevant colour information (on the opposite side of the spectrum, saturated regions are excluded by the ImgU statistics engine). The algorithm requires a minimal number of zones that meet this criteria in order to run. Now that we correct the black level, the 32/255 minimal value is a bit high and prevents the algorithm for running in low-light conditions. Lower the value to 16/255 to fix it. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/awb.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > > index 30693923..b18874a3 100644 > > --- a/src/ipa/ipu3/algorithms/awb.cpp > > +++ b/src/ipa/ipu3/algorithms/awb.cpp > > @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms { > > > > LOG_DEFINE_CATEGORY(IPU3Awb) > > Do you document the meaning of this constant, and what it is used for / > effects of changing it in the later documentation series? > > If not, could you add something please? > > (Either in this patch, or later) Agreed. It could also make sense to capture the commit message or parts of it in the documentation. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > -static constexpr uint32_t kMinGreenLevelInZone = 32; > > +static constexpr uint32_t kMinGreenLevelInZone = 16; > > > > /** > > * \struct Accumulator
On 15/10/2021 00:55, Laurent Pinchart wrote: > On Thu, Oct 14, 2021 at 11:50:22AM +0100, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-10-13 16:41:16) >>> When the zones are used for the grey world algorithm, we need a minimal >>> number of zones to make it relevant, and we want the green values to be >>> at least 32 today (with a maximum of 255). >>> >>> This value is a bit high, as we now are correcting the black levels, and >>> a sudden change of colors can be visible for the end user when the gains >>> are applied. >>> >>> Lower the value to 16. > > If I understand the patch correctly, the most relevant part here is the > minimal green value for a zone to be considered. I would then write the > above as > > When zones are used for the grey world algorithm, they are only > considered if their average green value is at least 32/255 to exclude > zones that are too dark and don't provide relevant colour information > (on the opposite side of the spectrum, saturated regions are excluded by > the ImgU statistics engine). > > The algorithm requires a minimal number of zones that meet this criteria > in order to run. Now that we correct the black level, the 32/255 minimal > value is a bit high and prevents the algorithm for running in low-light > conditions. Lower the value to 16/255 to fix it. > Thanks, it is better wrote (as always ;-)). >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/algorithms/awb.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>> index 30693923..b18874a3 100644 >>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>> @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms { >>> >>> LOG_DEFINE_CATEGORY(IPU3Awb) >> >> Do you document the meaning of this constant, and what it is used for / >> effects of changing it in the later documentation series? >> >> If not, could you add something please? >> >> (Either in this patch, or later) > > Agreed. > > It could also make sense to capture the commit message or parts of it in > the documentation. > OK, I will add those in the v2 for the "Document IPAIPU3" series. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> -static constexpr uint32_t kMinGreenLevelInZone = 32; >>> +static constexpr uint32_t kMinGreenLevelInZone = 16; >>> >>> /** >>> * \struct Accumulator >
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 30693923..b18874a3 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms { LOG_DEFINE_CATEGORY(IPU3Awb) -static constexpr uint32_t kMinGreenLevelInZone = 32; +static constexpr uint32_t kMinGreenLevelInZone = 16; /** * \struct Accumulator
When the zones are used for the grey world algorithm, we need a minimal number of zones to make it relevant, and we want the green values to be at least 32 today (with a maximum of 255). This value is a bit high, as we now are correcting the black levels, and a sudden change of colors can be visible for the end user when the gains are applied. Lower the value to 16. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)