| Message ID | 20250724152936.99830-3-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-07-24 16:29:36) > The statistics in software ISP is computed basically over the whole > image area, although only on part of the pixels. It is not necessary to > cover the whole image area, it's sufficient to compute the statistics > let's say over the central area of 2/3 of the image width and height, > which should be both sufficient and faster. The speedup is not that > important with the CPU implementation but it may save CPU work > more noticeably with GPU debayering implementation. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > src/libcamera/software_isp/debayer_egl.cpp | 7 +++++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index e56492848..bea153592 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -526,7 +526,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > window_.width = outputCfg.size.width; > window_.height = outputCfg.size.height; > > - /* Don't pass x,y since process() already adjusts src before passing it */ > + /* Don't pass x,y from window_ since process() already adjusts for it. */ > stats_->setWindow(Rectangle(window_.size())); Shouldn't we do the same on both to have the same behaviour ? Ideally this should (later) be configurable and could even map to the Ae controls for CenterWeighted, Spot or Matrix metering ? > > /* pad with patternSize.Width on both left and right side */ > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 9ec966608..9ad7d6fe7 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -393,8 +393,11 @@ int DebayerEGL::configure(const StreamConfiguration &inputCfg, > window_.width = outputCfg.size.width; > window_.height = outputCfg.size.height; > > - /* Don't pass x,y since process() already adjusts src before passing it */ > - stats_->setWindow(Rectangle(window_.size())); > + /* > + * Don't pass x,y from window_ since process() already adjusts for it. > + * But crop the window to 2/3 of its width and height for speedup. > + */ > + stats_->setWindow(Rectangle(window_.size()).croppedBy(2, 3)); I think this is reasonable for the moment though as an optimisation. It's almost 'CenterWeighted' but not actually weighting ... > LOG(Debayer, Debug) << "Input width " << inputCfg.size.width << " height " << inputCfg.size.height; > LOG(Debayer, Debug) << "Output width " << outputCfg.size.width << " height " << outputCfg.size.height; > -- > 2.50.1 >
Quoting Kieran Bingham (2025-08-06 18:39:06) > Quoting Milan Zamazal (2025-07-24 16:29:36) > > The statistics in software ISP is computed basically over the whole > > image area, although only on part of the pixels. It is not necessary to > > cover the whole image area, it's sufficient to compute the statistics > > let's say over the central area of 2/3 of the image width and height, > > which should be both sufficient and faster. The speedup is not that > > important with the CPU implementation but it may save CPU work > > more noticeably with GPU debayering implementation. > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > > src/libcamera/software_isp/debayer_egl.cpp | 7 +++++-- > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > index e56492848..bea153592 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > @@ -526,7 +526,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > > window_.width = outputCfg.size.width; > > window_.height = outputCfg.size.height; > > > > - /* Don't pass x,y since process() already adjusts src before passing it */ > > + /* Don't pass x,y from window_ since process() already adjusts for it. */ > > stats_->setWindow(Rectangle(window_.size())); > > Shouldn't we do the same on both to have the same behaviour ? > > Ideally this should (later) be configurable and could even map to the Ae > controls for CenterWeighted, Spot or Matrix metering ? > > > > > /* pad with patternSize.Width on both left and right side */ > > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > > index 9ec966608..9ad7d6fe7 100644 > > --- a/src/libcamera/software_isp/debayer_egl.cpp > > +++ b/src/libcamera/software_isp/debayer_egl.cpp > > @@ -393,8 +393,11 @@ int DebayerEGL::configure(const StreamConfiguration &inputCfg, > > window_.width = outputCfg.size.width; > > window_.height = outputCfg.size.height; > > > > - /* Don't pass x,y since process() already adjusts src before passing it */ > > - stats_->setWindow(Rectangle(window_.size())); > > + /* > > + * Don't pass x,y from window_ since process() already adjusts for it. > > + * But crop the window to 2/3 of its width and height for speedup. > > + */ Reading the proposal in the first patch - this does look good: stats_->setWindow((window_.size() * 0.66).centeredTo(window_.center()); or even stats_->setWindow((window_.size() * 2 / 3).centeredTo(window_.center()); and tells me all the information I needed before. I can read instantly that it's centered into the window_.center and it's 0.66 of the current window_.size() which I think is nicer than casting to an intermeddiate Rectangle() just to initialise. > > + stats_->setWindow(Rectangle(window_.size()).croppedBy(2, 3)); > > I think this is reasonable for the moment though as an optimisation. > It's almost 'CenterWeighted' but not actually weighting ... > > > LOG(Debayer, Debug) << "Input width " << inputCfg.size.width << " height " << inputCfg.size.height; > > LOG(Debayer, Debug) << "Output width " << outputCfg.size.width << " height " << outputCfg.size.height; > > -- > > 2.50.1 > >
Hi, thank you all for reviews. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-07-24 16:29:36) >> The statistics in software ISP is computed basically over the whole >> image area, although only on part of the pixels. It is not necessary to > >> cover the whole image area, it's sufficient to compute the statistics >> let's say over the central area of 2/3 of the image width and height, >> which should be both sufficient and faster. The speedup is not that >> important with the CPU implementation but it may save CPU work >> more noticeably with GPU debayering implementation. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >> src/libcamera/software_isp/debayer_egl.cpp | 7 +++++-- >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index e56492848..bea153592 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -526,7 +526,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, >> window_.width = outputCfg.size.width; >> window_.height = outputCfg.size.height; >> >> - /* Don't pass x,y since process() already adjusts src before passing it */ >> + /* Don't pass x,y from window_ since process() already adjusts for it. */ >> stats_->setWindow(Rectangle(window_.size())); > > Shouldn't we do the same on both to have the same behaviour ? Yes, it would be better for consistency. > Ideally this should (later) be configurable and could even map to the Ae > controls for CenterWeighted, Spot or Matrix metering ? Maybe and there should be a common method. Not necessary now, I think. >> > >> > /* pad with patternSize.Width on both left and right side */ >> > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> > index 9ec966608..9ad7d6fe7 100644 >> > --- a/src/libcamera/software_isp/debayer_egl.cpp >> > +++ b/src/libcamera/software_isp/debayer_egl.cpp >> > @@ -393,8 +393,11 @@ int DebayerEGL::configure(const StreamConfiguration &inputCfg, >> > window_.width = outputCfg.size.width; >> > window_.height = outputCfg.size.height; >> > >> > - /* Don't pass x,y since process() already adjusts src before passing it */ >> > - stats_->setWindow(Rectangle(window_.size())); >> > + /* >> > + * Don't pass x,y from window_ since process() already adjusts for it. >> > + * But crop the window to 2/3 of its width and height for speedup. >> > + */ > > Reading the proposal in the first patch - this does look good: > > stats_->setWindow((window_.size() * 0.66).centeredTo(window_.center()); > > or even > > stats_->setWindow((window_.size() * 2 / 3).centeredTo(window_.center()); Looks good to me, I use it in v3. > and tells me all the information I needed before. I can read instantly > that it's centered into the window_.center and it's 0.66 of the current > window_.size() which I think is nicer than casting to an intermeddiate > Rectangle() just to initialise. > > > >> > + stats_->setWindow(Rectangle(window_.size()).croppedBy(2, 3)); >> >> I think this is reasonable for the moment though as an optimisation. >> It's almost 'CenterWeighted' but not actually weighting ... >> >> > LOG(Debayer, Debug) << "Input width " << inputCfg.size.width << " height " << >> > inputCfg.size.height; >> > LOG(Debayer, Debug) << "Output width " << outputCfg.size.width << " height " << >> > outputCfg.size.height; >> > -- >> > 2.50.1 >> >
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index e56492848..bea153592 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -526,7 +526,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, window_.width = outputCfg.size.width; window_.height = outputCfg.size.height; - /* Don't pass x,y since process() already adjusts src before passing it */ + /* Don't pass x,y from window_ since process() already adjusts for it. */ stats_->setWindow(Rectangle(window_.size())); /* pad with patternSize.Width on both left and right side */ diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 9ec966608..9ad7d6fe7 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -393,8 +393,11 @@ int DebayerEGL::configure(const StreamConfiguration &inputCfg, window_.width = outputCfg.size.width; window_.height = outputCfg.size.height; - /* Don't pass x,y since process() already adjusts src before passing it */ - stats_->setWindow(Rectangle(window_.size())); + /* + * Don't pass x,y from window_ since process() already adjusts for it. + * But crop the window to 2/3 of its width and height for speedup. + */ + stats_->setWindow(Rectangle(window_.size()).croppedBy(2, 3)); LOG(Debayer, Debug) << "Input width " << inputCfg.size.width << " height " << inputCfg.size.height; LOG(Debayer, Debug) << "Output width " << outputCfg.size.width << " height " << outputCfg.size.height;
The statistics in software ISP is computed basically over the whole image area, although only on part of the pixels. It is not necessary to cover the whole image area, it's sufficient to compute the statistics let's say over the central area of 2/3 of the image width and height, which should be both sufficient and faster. The speedup is not that important with the CPU implementation but it may save CPU work more noticeably with GPU debayering implementation. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/software_isp/debayer_cpu.cpp | 2 +- src/libcamera/software_isp/debayer_egl.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-)