| Message ID | 20260810103846.1075936-29-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Aug 10, 2026 at 12:38:24PM +0200, Barnabás Pőcze wrote: > The type of `frameSize` is `uint64_t`, but it is assigned the result > of a 32-bit multiplication. So make one of the terms a 64-bit integer > so that the result will be actually 64-bit. > > Furthermore, also adjust the calculation to do the multiplication first > just to be as accurate as possible. No wraparound will happen as long > as the frame size is less than about 16.7 TiB. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/libipa/agc.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > index 3c95071259..def60a8570 100644 > --- a/src/ipa/libipa/agc.cpp > +++ b/src/ipa/libipa/agc.cpp > @@ -297,8 +297,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > > std::array<int64_t, 3> frameDurations; > for (unsigned int i = 0; i < frameHeights.size(); ++i) { > - uint64_t frameSize = lineLength * frameHeights[i]; > - frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U); > + uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i]; > + frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate; > } > > /* > -- > 2.55.0 >
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-10 12:38:24) > The type of `frameSize` is `uint64_t`, but it is assigned the result > of a 32-bit multiplication. So make one of the terms a 64-bit integer > so that the result will be actually 64-bit. > > Furthermore, also adjust the calculation to do the multiplication first > just to be as accurate as possible. No wraparound will happen as long > as the frame size is less than about 16.7 TiB. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/libipa/agc.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > index 3c95071259..def60a8570 100644 > --- a/src/ipa/libipa/agc.cpp > +++ b/src/ipa/libipa/agc.cpp > @@ -297,8 +297,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > > std::array<int64_t, 3> frameDurations; > for (unsigned int i = 0; i < frameHeights.size(); ++i) { > - uint64_t frameSize = lineLength * frameHeights[i]; > - frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U); > + uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i]; > + frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate; > } > > /* > -- > 2.55.0 >
Hi 2026. 08. 12. 7:36 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-08-10 12:38:24) >> The type of `frameSize` is `uint64_t`, but it is assigned the result >> of a 32-bit multiplication. So make one of the terms a 64-bit integer >> so that the result will be actually 64-bit. > > > Is there anything missing from above? >> >> Furthermore, also adjust the calculation to do the multiplication first >> just to be as accurate as possible. No wraparound will happen as long >> as the frame size is less than about 16.7 TiB. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > >> --- >> src/ipa/libipa/agc.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp >> index 3c95071259..def60a8570 100644 >> --- a/src/ipa/libipa/agc.cpp >> +++ b/src/ipa/libipa/agc.cpp >> @@ -297,8 +297,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, >> >> std::array<int64_t, 3> frameDurations; >> for (unsigned int i = 0; i < frameHeights.size(); ++i) { >> - uint64_t frameSize = lineLength * frameHeights[i]; >> - frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U); >> + uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i]; >> + frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate; >> } >> >> /* >> -- >> 2.55.0 >>
Quoting Barnabás Pőcze (2026-08-12 09:36:48) > Hi > > 2026. 08. 12. 7:36 keltezéssel, Stefan Klug írta: > > Hi Barnabás, > > > > Quoting Barnabás Pőcze (2026-08-10 12:38:24) > >> The type of `frameSize` is `uint64_t`, but it is assigned the result > >> of a 32-bit multiplication. So make one of the terms a 64-bit integer > >> so that the result will be actually 64-bit. > > > > > > > > Is there anything missing from above? Oh I forgot to remove the empty lines. I thought about asking how you found that :-) For all sensor sizes that I'm aware of, the old calculation shouldn't cause problems. I guess you didn't run into problems with the pixelrate as well. But anyhow, the new code is better. Best regards, Stefan > > > >> > >> Furthermore, also adjust the calculation to do the multiplication first > >> just to be as accurate as possible. No wraparound will happen as long > >> as the frame size is less than about 16.7 TiB. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > >> --- > >> src/ipa/libipa/agc.cpp | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > >> index 3c95071259..def60a8570 100644 > >> --- a/src/ipa/libipa/agc.cpp > >> +++ b/src/ipa/libipa/agc.cpp > >> @@ -297,8 +297,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > >> > >> std::array<int64_t, 3> frameDurations; > >> for (unsigned int i = 0; i < frameHeights.size(); ++i) { > >> - uint64_t frameSize = lineLength * frameHeights[i]; > >> - frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U); > >> + uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i]; > >> + frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate; > >> } > >> > >> /* > >> -- > >> 2.55.0 > >> >
diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp index 3c95071259..def60a8570 100644 --- a/src/ipa/libipa/agc.cpp +++ b/src/ipa/libipa/agc.cpp @@ -297,8 +297,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, std::array<int64_t, 3> frameDurations; for (unsigned int i = 0; i < frameHeights.size(); ++i) { - uint64_t frameSize = lineLength * frameHeights[i]; - frameDurations[i] = frameSize / (config.sensorInfo.pixelRate / 1000000U); + uint64_t frameSize = static_cast<uint64_t>(lineLength) * frameHeights[i]; + frameDurations[i] = frameSize * 1000000U / config.sensorInfo.pixelRate; } /*
The type of `frameSize` is `uint64_t`, but it is assigned the result of a 32-bit multiplication. So make one of the terms a 64-bit integer so that the result will be actually 64-bit. Furthermore, also adjust the calculation to do the multiplication first just to be as accurate as possible. No wraparound will happen as long as the frame size is less than about 16.7 TiB. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/libipa/agc.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)