| Message ID | 20260326164241.4212-1-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thanks for finding this bug! On Thu, 26 Mar 2026 at 16:42, David Plowman <david.plowman@raspberrypi.com> wrote: > > generateLut was failing to fill in the final slope value, meaning that > fully saturated pixels would full slightly short (the slope of the > final piecewise linear segment would default to zero). > > The loop is slightly reorganised to fix the problem. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/pisp/pisp.cpp | 42 +++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp > index ec7593ff..851f9b30 100644 > --- a/src/ipa/rpi/pisp/pisp.cpp > +++ b/src/ipa/rpi/pisp/pisp.cpp > @@ -80,36 +80,30 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut, std::size_t lutSize, > if (pwl.empty()) > return -EINVAL; > > - int lastY = 0; Strictly, lastY could be initialsed to pwl.eval(0); although it's hard to imagine why anyone would configure a value other than 0 > + int nextY = 0; > for (unsigned int i = 0; i < lutSize; i++) { > - int x, y; > - if (i < 32) > - x = i * 512; > - else if (i < 48) > - x = (i - 32) * 1024 + 16384; > + unsigned int nextI = i + 1; > + > + int nextX; > + if (nextI < 32) > + nextX = nextI * 512; > + else if (nextI < 48) > + nextX = (nextI - 32) * 1024 + 16384; > else > - x = std::min(65535u, (i - 48) * 2048 + 32768); > + nextX = std::min(65535u, (nextI - 48) * 2048 + 32768); I think we can omit the std::min -- the final nextX value would then be 65536. Though depending on how Pwl's extrapolation works numerically, it might then be necessary to clip nextY to 65536.0 to prevent overflow. I note that the hardware's interpolation will always round down. > > - y = pwl.eval(x); > - if (y < 0 || (i && y < lastY)) { > - LOG(IPARPI, Error) > - << "Malformed PWL for Gamma, disabling!"; > - return -1; > - } > + int y = nextY; > + nextY = pwl.eval(nextX); > > - if (i) { > - unsigned int slope = y - lastY; > - if (slope >= (1u << SlopeBits)) { > - slope = (1u << SlopeBits) - 1; > - LOG(IPARPI, Info) > - << ("Maximum Gamma slope exceeded, adjusting!"); > - y = lastY + slope; > - } > - lut[i - 1] |= slope << PosBits; > + unsigned int slope = nextY - y; > + if (slope >= (1u << SlopeBits)) { > + slope = (1u << SlopeBits) - 1; > + LOG(IPARPI, Info) > + << ("Maximum Gamma slope exceeded, adjusting!"); > + nextY = y + slope; > } > > - lut[i] = y; > - lastY = y; > + lut[i] = y | (slope << PosBits); > } > > return 0; > -- > 2.47.3 >
diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp index ec7593ff..851f9b30 100644 --- a/src/ipa/rpi/pisp/pisp.cpp +++ b/src/ipa/rpi/pisp/pisp.cpp @@ -80,36 +80,30 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut, std::size_t lutSize, if (pwl.empty()) return -EINVAL; - int lastY = 0; + int nextY = 0; for (unsigned int i = 0; i < lutSize; i++) { - int x, y; - if (i < 32) - x = i * 512; - else if (i < 48) - x = (i - 32) * 1024 + 16384; + unsigned int nextI = i + 1; + + int nextX; + if (nextI < 32) + nextX = nextI * 512; + else if (nextI < 48) + nextX = (nextI - 32) * 1024 + 16384; else - x = std::min(65535u, (i - 48) * 2048 + 32768); + nextX = std::min(65535u, (nextI - 48) * 2048 + 32768); - y = pwl.eval(x); - if (y < 0 || (i && y < lastY)) { - LOG(IPARPI, Error) - << "Malformed PWL for Gamma, disabling!"; - return -1; - } + int y = nextY; + nextY = pwl.eval(nextX); - if (i) { - unsigned int slope = y - lastY; - if (slope >= (1u << SlopeBits)) { - slope = (1u << SlopeBits) - 1; - LOG(IPARPI, Info) - << ("Maximum Gamma slope exceeded, adjusting!"); - y = lastY + slope; - } - lut[i - 1] |= slope << PosBits; + unsigned int slope = nextY - y; + if (slope >= (1u << SlopeBits)) { + slope = (1u << SlopeBits) - 1; + LOG(IPARPI, Info) + << ("Maximum Gamma slope exceeded, adjusting!"); + nextY = y + slope; } - lut[i] = y; - lastY = y; + lut[i] = y | (slope << PosBits); } return 0;
generateLut was failing to fill in the final slope value, meaning that fully saturated pixels would full slightly short (the slope of the final piecewise linear segment would default to zero). The loop is slightly reorganised to fix the problem. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/pisp/pisp.cpp | 42 +++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 24 deletions(-)