| Message ID | 20211116162615.27777-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for the patch ! On 16/11/2021 17:26, Laurent Pinchart wrote: > The relative luminance is calculated using an iterative process to > account for saturation in the sensor, as multiplying pixels by a gain > doesn't increase the relative luminance by the same factor if some > regions are saturated. Relative luminance estimation doesn't apply a > saturation, which produces a value that doesn't match what the sensor > will output, and defeats the point of the iterative process. Fix it. > > Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 71398fdd96a6..46aa1b14a100 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > * \param[in] gain The analogue gain to apply to the frame > * \return The relative luminance > * > - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color > - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a > - * theoretical perfect reflector of 100% reference white. We use the Rec. 601 > - * luma here. > + * This function estimates the average relative luminance of the frame that > + * would be output by the sensor if an additional analogue \a gain was applied. s/additional analogue \a gain/additional \a gain/ > + * > + * The estimation is based on the AWB statistics for the current frame. Red, > + * green and blue averages for all cells are first multiplied by the gain, and > + * then saturated to approximate the sensor behaviour at high brightness > + * values. The approximation is quitte rough, as it doesn't take into account > + * non-linearities when approaching saturation. > + * > + * The relative luminance (Y) is computed from the linear RGB components using > + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range, > + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference > + * white. > * > * More detailed information can be found in: > * https://en.wikipedia.org/wiki/Relative_luminance > @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > { > double redSum = 0, greenSum = 0, blueSum = 0; > > + /* Sum the per-channel averages, saturated to 255. */ > for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > uint32_t cellPosition = cellY * stride_ + cellX; > @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > reinterpret_cast<const ipu3_uapi_awb_set_item *>( > &stats->awb_raw_buffer.meta_data[cellPosition] > ); > + const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > > - redSum += cell->R_avg * gain; > - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain; > - blueSum += cell->B_avg * gain; > + redSum += std::min(cell->R_avg * gain, 255.0); > + greenSum += std::min(G_avg * gain, 255.0); > + blueSum += std::min(cell->B_avg * gain, 255.0); > } > } > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Quoting Jean-Michel Hautbois (2021-11-17 10:26:59) > Hi Laurent, > > Thanks for the patch ! > > On 16/11/2021 17:26, Laurent Pinchart wrote: > > The relative luminance is calculated using an iterative process to > > account for saturation in the sensor, as multiplying pixels by a gain > > doesn't increase the relative luminance by the same factor if some > > regions are saturated. Relative luminance estimation doesn't apply a > > saturation, which produces a value that doesn't match what the sensor > > will output, and defeats the point of the iterative process. Fix it. > > > > Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++------- > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > index 71398fdd96a6..46aa1b14a100 100644 > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > > * \param[in] gain The analogue gain to apply to the frame > > * \return The relative luminance > > * > > - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color > > - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a > > - * theoretical perfect reflector of 100% reference white. We use the Rec. 601 > > - * luma here. > > + * This function estimates the average relative luminance of the frame that > > + * would be output by the sensor if an additional analogue \a gain was applied. > > s/additional analogue \a gain/additional \a gain/ > > > + * > > + * The estimation is based on the AWB statistics for the current frame. Red, > > + * green and blue averages for all cells are first multiplied by the gain, and > > + * then saturated to approximate the sensor behaviour at high brightness > > + * values. The approximation is quitte rough, as it doesn't take into account /quitte/quite/ > > + * non-linearities when approaching saturation. > > + * > > + * The relative luminance (Y) is computed from the linear RGB components using > > + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range, /values is/values are/ > > + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference > > + * white. > > * > > * More detailed information can be found in: > > * https://en.wikipedia.org/wiki/Relative_luminance > > @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > { > > double redSum = 0, greenSum = 0, blueSum = 0; > > > > + /* Sum the per-channel averages, saturated to 255. */ > > for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > > for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > > uint32_t cellPosition = cellY * stride_ + cellX; > > @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > reinterpret_cast<const ipu3_uapi_awb_set_item *>( > > &stats->awb_raw_buffer.meta_data[cellPosition] > > ); > > + const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > > > > - redSum += cell->R_avg * gain; > > - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain; > > - blueSum += cell->B_avg * gain; > > + redSum += std::min(cell->R_avg * gain, 255.0); > > + greenSum += std::min(G_avg * gain, 255.0); > > + blueSum += std::min(cell->B_avg * gain, 255.0); > > } > > } > > > > > Still works well in my harsh backlight office conditions too. Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Quoting Kieran Bingham (2021-11-18 09:53:22) > Quoting Jean-Michel Hautbois (2021-11-17 10:26:59) > > Hi Laurent, > > > > Thanks for the patch ! > > > > On 16/11/2021 17:26, Laurent Pinchart wrote: > > > The relative luminance is calculated using an iterative process to > > > account for saturation in the sensor, as multiplying pixels by a gain > > > doesn't increase the relative luminance by the same factor if some > > > regions are saturated. Relative luminance estimation doesn't apply a > > > saturation, which produces a value that doesn't match what the sensor > > > will output, and defeats the point of the iterative process. Fix it. > > > > > > Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation") > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++------- > > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > > index 71398fdd96a6..46aa1b14a100 100644 > > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > > @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > > > * \param[in] gain The analogue gain to apply to the frame > > > * \return The relative luminance > > > * > > > - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color > > > - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a > > > - * theoretical perfect reflector of 100% reference white. We use the Rec. 601 > > > - * luma here. > > > + * This function estimates the average relative luminance of the frame that > > > + * would be output by the sensor if an additional analogue \a gain was applied. > > > > s/additional analogue \a gain/additional \a gain/ > > > > > + * > > > + * The estimation is based on the AWB statistics for the current frame. Red, > > > + * green and blue averages for all cells are first multiplied by the gain, and > > > + * then saturated to approximate the sensor behaviour at high brightness > > > + * values. The approximation is quitte rough, as it doesn't take into account > > /quitte/quite/ > > > > + * non-linearities when approaching saturation. > > > + * > > > + * The relative luminance (Y) is computed from the linear RGB components using > > > + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range, > > /values is/values are/ > > > > + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference > > > + * white. > > > * > > > * More detailed information can be found in: > > > * https://en.wikipedia.org/wiki/Relative_luminance > > > @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > > { > > > double redSum = 0, greenSum = 0, blueSum = 0; > > > > > > + /* Sum the per-channel averages, saturated to 255. */ > > > for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > > > for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > > > uint32_t cellPosition = cellY * stride_ + cellX; > > > @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > > reinterpret_cast<const ipu3_uapi_awb_set_item *>( > > > &stats->awb_raw_buffer.meta_data[cellPosition] > > > ); > > > + const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > > > > > > - redSum += cell->R_avg * gain; > > > - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain; > > > - blueSum += cell->B_avg * gain; > > > + redSum += std::min(cell->R_avg * gain, 255.0); > > > + greenSum += std::min(G_avg * gain, 255.0); > > > + blueSum += std::min(cell->B_avg * gain, 255.0); Seeing this constant in the new RKISP series again, wouldn't it make sense to keep the kMaximumLuminance constant that you remove at the beginning of the series and use it here so that it's self-documented that the min operation is clamping to the maximum luminance value? Or is 255 really not a luminance value here? > > > } > > > } > > > > > > > > > > Still works well in my harsh backlight office conditions too. > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
On Fri, Nov 19, 2021 at 02:35:44PM +0000, Kieran Bingham wrote: > Quoting Kieran Bingham (2021-11-18 09:53:22) > > Quoting Jean-Michel Hautbois (2021-11-17 10:26:59) > > > Hi Laurent, > > > > > > Thanks for the patch ! > > > > > > On 16/11/2021 17:26, Laurent Pinchart wrote: > > > > The relative luminance is calculated using an iterative process to > > > > account for saturation in the sensor, as multiplying pixels by a gain > > > > doesn't increase the relative luminance by the same factor if some > > > > regions are saturated. Relative luminance estimation doesn't apply a > > > > saturation, which produces a value that doesn't match what the sensor > > > > will output, and defeats the point of the iterative process. Fix it. > > > > > > > > Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation") > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++------- > > > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > > > index 71398fdd96a6..46aa1b14a100 100644 > > > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > > > @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > > > > * \param[in] gain The analogue gain to apply to the frame > > > > * \return The relative luminance > > > > * > > > > - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color > > > > - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a > > > > - * theoretical perfect reflector of 100% reference white. We use the Rec. 601 > > > > - * luma here. > > > > + * This function estimates the average relative luminance of the frame that > > > > + * would be output by the sensor if an additional analogue \a gain was applied. > > > > > > s/additional analogue \a gain/additional \a gain/ > > > > > > > + * > > > > + * The estimation is based on the AWB statistics for the current frame. Red, > > > > + * green and blue averages for all cells are first multiplied by the gain, and > > > > + * then saturated to approximate the sensor behaviour at high brightness > > > > + * values. The approximation is quitte rough, as it doesn't take into account > > > > /quitte/quite/ > > > > > > + * non-linearities when approaching saturation. > > > > + * > > > > + * The relative luminance (Y) is computed from the linear RGB components using > > > > + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range, > > > > /values is/values are/ > > > > > > + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference > > > > + * white. > > > > * > > > > * More detailed information can be found in: > > > > * https://en.wikipedia.org/wiki/Relative_luminance > > > > @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > > > { > > > > double redSum = 0, greenSum = 0, blueSum = 0; > > > > > > > > + /* Sum the per-channel averages, saturated to 255. */ > > > > for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > > > > for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > > > > uint32_t cellPosition = cellY * stride_ + cellX; > > > > @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, > > > > reinterpret_cast<const ipu3_uapi_awb_set_item *>( > > > > &stats->awb_raw_buffer.meta_data[cellPosition] > > > > ); > > > > + const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > > > > > > > > - redSum += cell->R_avg * gain; > > > > - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain; > > > > - blueSum += cell->B_avg * gain; > > > > + redSum += std::min(cell->R_avg * gain, 255.0); > > > > + greenSum += std::min(G_avg * gain, 255.0); > > > > + blueSum += std::min(cell->B_avg * gain, 255.0); > > Seeing this constant in the new RKISP series again, wouldn't it make > sense to keep the kMaximumLuminance constant that you remove at the > beginning of the series and use it here so that it's self-documented > that the min operation is clamping to the maximum luminance value? > > Or is 255 really not a luminance value here? 255 here is UINT8_MAX, as the red, green and blue averages are stored as 8-bit values. The maximum values for the sums are then UINT8_MAX * number of cells, and the maximum value for the luminance is the same given that the sum of the three coefficients is 1.0. > > > > } > > > > } > > > > > > > > > > > > Still works well in my harsh backlight office conditions too. > > > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 71398fdd96a6..46aa1b14a100 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, * \param[in] gain The analogue gain to apply to the frame * \return The relative luminance * - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a - * theoretical perfect reflector of 100% reference white. We use the Rec. 601 - * luma here. + * This function estimates the average relative luminance of the frame that + * would be output by the sensor if an additional analogue \a gain was applied. + * + * The estimation is based on the AWB statistics for the current frame. Red, + * green and blue averages for all cells are first multiplied by the gain, and + * then saturated to approximate the sensor behaviour at high brightness + * values. The approximation is quitte rough, as it doesn't take into account + * non-linearities when approaching saturation. + * + * The relative luminance (Y) is computed from the linear RGB components using + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range, + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference + * white. * * More detailed information can be found in: * https://en.wikipedia.org/wiki/Relative_luminance @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, { double redSum = 0, greenSum = 0, blueSum = 0; + /* Sum the per-channel averages, saturated to 255. */ for (unsigned int cellY = 0; cellY < grid.height; cellY++) { for (unsigned int cellX = 0; cellX < grid.width; cellX++) { uint32_t cellPosition = cellY * stride_ + cellX; @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext, reinterpret_cast<const ipu3_uapi_awb_set_item *>( &stats->awb_raw_buffer.meta_data[cellPosition] ); + const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; - redSum += cell->R_avg * gain; - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain; - blueSum += cell->B_avg * gain; + redSum += std::min(cell->R_avg * gain, 255.0); + greenSum += std::min(G_avg * gain, 255.0); + blueSum += std::min(cell->B_avg * gain, 255.0); } }
The relative luminance is calculated using an iterative process to account for saturation in the sensor, as multiplying pixels by a gain doesn't increase the relative luminance by the same factor if some regions are saturated. Relative luminance estimation doesn't apply a saturation, which produces a value that doesn't match what the sensor will output, and defeats the point of the iterative process. Fix it. Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)