| Message ID | 20250624083612.27230-11-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta: > Software ISP performs performance measurement on certain part of initial > frames. Let's make this range configurable. > > For this purpose, this patch introduces new configuration options > pipelines.simple.measure.skip and pipelines.simple.measure.number. > Setting the latter one to 0 disables the measurement. > > Instead of the last frame, the class member and its configuration > specify the number of frames to measure. This is easier to use for > users and doesn't require to adjust two configuration parameters when > the number of the initially skipped frames is changed. > > The patch also changes the names of the class members to make them more > accurate. > > Completes software ISP TODO #7. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > Documentation/runtime_configuration.rst | 6 ++++++ > src/libcamera/software_isp/TODO | 25 ---------------------- > src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++-------- > src/libcamera/software_isp/debayer_cpu.h | 7 +++--- > 4 files changed, 25 insertions(+), 38 deletions(-) > > diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst > index f4e86ae24..de74fa732 100644 > --- a/Documentation/runtime_configuration.rst > +++ b/Documentation/runtime_configuration.rst > @@ -52,6 +52,9 @@ file structure: > ... > simple: > copy_input_buffer: # true/false > + measure: > + skip: # non-negative integer, frames to skip initially > + number: # non-negative integer, frames to measure > supported_devices: > - driver: # driver name, e.g. `mxc-isi` > software_isp: # true/false > @@ -87,6 +90,9 @@ Configuration file example > min_total_unicam_buffers: 2 > simple: > copy_input_buffer: false > + measure: > + skip: 50 > + number: 30 > supported_devices: > - driver: mxc-isi > software_isp: true > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO > index 2c919f442..f19e15ae2 100644 > --- a/src/libcamera/software_isp/TODO > +++ b/src/libcamera/software_isp/TODO > @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs. > > --- > > -7. Performance measurement configuration > - > -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > -> /* Measure before emitting signals */ > -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { > -> timespec frameEndTime = {}; > -> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > -> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); > -> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { > -> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - > -> DebayerCpu::kFramesToSkip; > -> LOG(Debayer, Info) > -> << "Processed " << measuredFrames > -> << " frames in " << frameProcessTime_ / 1000 << "us, " > -> << frameProcessTime_ / (1000 * measuredFrames) > -> << " us/frame"; > -> } > -> } > - > -I wonder if there would be a way to control at runtime when/how to > -perform those measurements. Maybe that's a bit overkill. > - > ---- > - > 8. DebayerCpu cleanups > > > >> class DebayerCpu : public Debayer, public Object > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 85e31e98e..27113c6a5 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat > enableInputMemcpy_ = > configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true); > > + skipBeforeMeasure_ = configuration.option<unsigned int>( > + "pipelines/simple/measure/skip") > + .value_or(skipBeforeMeasure_); > + framesToMeasure_ = configuration.option<unsigned int>( > + "pipelines/simple/measure/number") > + .value_or(framesToMeasure_); > + Regarding this and the previous patch, I think the `DebayerCpu` class should not access the configuration like this. A different approach could be creating e.g. struct DebayerCpu::Params { bool inputMemcpy = true; struct { unsigned int skip = 0, frames = 0; } measure; }; and passing that as argument to the constructor, and letting the caller extract the settings from wherever it wants. Alternatively, and I think this is generally useful is other contexts, I would add a `Configuration` type that contains a single `const YamlObject&`, essentially representing a subtree of the configuration. And then this type would have the option(), listOption, envOption(), etc. functions. In addition it would have a `Configuration operator[](path)` or `Configuration child(path)` that returns a `Configuration` object pointing to a child. This would make it possible to not have to consider the entire structure of the configuration from the root. Everything could deal with just its own dictionary, etc. This could then be passed to the constructors of various objects (pipeline handler, ipa, etc.) Thoughts? I am not proposing these changes be made just now, but I think something like this could be useful in the future. Regards, Barnabás Pőcze > /* Initialize color lookup tables */ > for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { > red_[i] = green_[i] = blue_[i] = i; > @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > lineBuffers_[i].resize(lineBufferLength_); > } > > - measuredFrames_ = 0; > + encounteredFrames_ = 0; > frameProcessTime_ = 0; > > return 0; > @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > { > timespec frameStartTime; > > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { > + bool measure = framesToMeasure_ > 0 && > + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ && > + ++encounteredFrames_ > skipBeforeMeasure_; > + if (measure) { > frameStartTime = {}; > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > } > @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > dmaSyncers.clear(); > > /* Measure before emitting signals */ > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > - ++measuredFrames_ > DebayerCpu::kFramesToSkip) { > + if (measure) { > timespec frameEndTime = {}; > clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); > - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { > - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - > - DebayerCpu::kFramesToSkip; > + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) { > LOG(Debayer, Info) > - << "Processed " << measuredFrames > + << "Processed " << framesToMeasure_ > << " frames in " << frameProcessTime_ / 1000 << "us, " > - << frameProcessTime_ / (1000 * measuredFrames) > + << frameProcessTime_ / (1000 * framesToMeasure_) > << " us/frame"; > } > } > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 2f35aa18b..9d343e464 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -161,11 +161,10 @@ private: > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > bool enableInputMemcpy_; > bool swapRedBlueGains_; > - unsigned int measuredFrames_; > + unsigned int encounteredFrames_; > int64_t frameProcessTime_; > - /* Skip 30 frames for things to stabilize then measure 30 frames */ > - static constexpr unsigned int kFramesToSkip = 30; > - static constexpr unsigned int kLastFrameToMeasure = 60; > + unsigned int skipBeforeMeasure_ = 30; > + unsigned int framesToMeasure_ = 30; > }; > > } /* namespace libcamera */
Hi Barnabás, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Hi > > 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta: >> Software ISP performs performance measurement on certain part of initial >> frames. Let's make this range configurable. >> For this purpose, this patch introduces new configuration options >> pipelines.simple.measure.skip and pipelines.simple.measure.number. >> Setting the latter one to 0 disables the measurement. >> Instead of the last frame, the class member and its configuration >> specify the number of frames to measure. This is easier to use for >> users and doesn't require to adjust two configuration parameters when >> the number of the initially skipped frames is changed. >> The patch also changes the names of the class members to make them more >> accurate. >> Completes software ISP TODO #7. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> Documentation/runtime_configuration.rst | 6 ++++++ >> src/libcamera/software_isp/TODO | 25 ---------------------- >> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++-------- >> src/libcamera/software_isp/debayer_cpu.h | 7 +++--- >> 4 files changed, 25 insertions(+), 38 deletions(-) >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst >> index f4e86ae24..de74fa732 100644 >> --- a/Documentation/runtime_configuration.rst >> +++ b/Documentation/runtime_configuration.rst >> @@ -52,6 +52,9 @@ file structure: >> ... >> simple: >> copy_input_buffer: # true/false >> + measure: >> + skip: # non-negative integer, frames to skip initially >> + number: # non-negative integer, frames to measure >> supported_devices: >> - driver: # driver name, e.g. `mxc-isi` >> software_isp: # true/false >> @@ -87,6 +90,9 @@ Configuration file example >> min_total_unicam_buffers: 2 >> simple: >> copy_input_buffer: false >> + measure: >> + skip: 50 >> + number: 30 >> supported_devices: >> - driver: mxc-isi >> software_isp: true >> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO >> index 2c919f442..f19e15ae2 100644 >> --- a/src/libcamera/software_isp/TODO >> +++ b/src/libcamera/software_isp/TODO >> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs. >> --- >> -7. Performance measurement configuration >> - >> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> -> /* Measure before emitting signals */ >> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >> -> timespec frameEndTime = {}; >> -> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >> -> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >> -> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >> -> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >> -> DebayerCpu::kFramesToSkip; >> -> LOG(Debayer, Info) >> -> << "Processed " << measuredFrames >> -> << " frames in " << frameProcessTime_ / 1000 << "us, " >> -> << frameProcessTime_ / (1000 * measuredFrames) >> -> << " us/frame"; >> -> } >> -> } >> - >> -I wonder if there would be a way to control at runtime when/how to >> -perform those measurements. Maybe that's a bit overkill. >> - >> ---- >> - >> 8. DebayerCpu cleanups >> > >> class DebayerCpu : public Debayer, public Object >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 85e31e98e..27113c6a5 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat >> enableInputMemcpy_ = >> configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true); >> + skipBeforeMeasure_ = configuration.option<unsigned int>( >> + "pipelines/simple/measure/skip") >> + .value_or(skipBeforeMeasure_); >> + framesToMeasure_ = configuration.option<unsigned int>( >> + "pipelines/simple/measure/number") >> + .value_or(framesToMeasure_); >> + > > Regarding this and the previous patch, I think the `DebayerCpu` class should > not access the configuration like this. > > A different approach could be creating e.g. > > struct DebayerCpu::Params { > bool inputMemcpy = true; > struct { > unsigned int skip = 0, frames = 0; > } measure; > }; > > and passing that as argument to the constructor, and letting the caller extract the > settings from wherever it wants. OK, makes sense. > Alternatively, and I think this is generally useful is other contexts, I would > add a `Configuration` type that contains a single `const YamlObject&`, essentially > representing a subtree of the configuration. And then this type would have the > option(), listOption, envOption(), etc. functions. In addition it would have a > `Configuration operator[](path)` or `Configuration child(path)` that returns a > `Configuration` object pointing to a child. This would make it possible to not > have to consider the entire structure of the configuration from the root. > Everything could deal with just its own dictionary, etc. This could then be > passed to the constructors of various objects (pipeline handler, ipa, etc.) > > Thoughts? What would be the primary motivation? I can think about: - Not exposing the whole configuration to proprietary code. - Making the configuration lookup relative to not having update it on contingent restructuring (but since such a thing is an annoyance to users above all and it still requires changes somewhere at callers, this doesn't look like any important). Against: - Perhaps just unnecessary complication. - It obfuscates in the code where's the given configuration option in the file. > I am not proposing these changes be made just now, but I think > something like this could be useful in the future. > > > Regards, > Barnabás Pőcze > > >> /* Initialize color lookup tables */ >> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { >> red_[i] = green_[i] = blue_[i] = i; >> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, >> lineBuffers_[i].resize(lineBufferLength_); >> } >> - measuredFrames_ = 0; >> + encounteredFrames_ = 0; >> frameProcessTime_ = 0; >> return 0; >> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> { >> timespec frameStartTime; >> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { >> + bool measure = framesToMeasure_ > 0 && >> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ && >> + ++encounteredFrames_ > skipBeforeMeasure_; >> + if (measure) { >> frameStartTime = {}; >> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >> } >> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> dmaSyncers.clear(); >> /* Measure before emitting signals */ >> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >> + if (measure) { >> timespec frameEndTime = {}; >> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >> - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >> - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >> - DebayerCpu::kFramesToSkip; >> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) { >> LOG(Debayer, Info) >> - << "Processed " << measuredFrames >> + << "Processed " << framesToMeasure_ >> << " frames in " << frameProcessTime_ / 1000 << "us, " >> - << frameProcessTime_ / (1000 * measuredFrames) >> + << frameProcessTime_ / (1000 * framesToMeasure_) >> << " us/frame"; >> } >> } >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index 2f35aa18b..9d343e464 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -161,11 +161,10 @@ private: >> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >> bool enableInputMemcpy_; >> bool swapRedBlueGains_; >> - unsigned int measuredFrames_; >> + unsigned int encounteredFrames_; >> int64_t frameProcessTime_; >> - /* Skip 30 frames for things to stabilize then measure 30 frames */ >> - static constexpr unsigned int kFramesToSkip = 30; >> - static constexpr unsigned int kLastFrameToMeasure = 60; >> + unsigned int skipBeforeMeasure_ = 30; >> + unsigned int framesToMeasure_ = 30; >> }; >> } /* namespace libcamera */
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Barnabás, > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> Hi >> >> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta: >>> Software ISP performs performance measurement on certain part of initial >>> frames. Let's make this range configurable. >>> For this purpose, this patch introduces new configuration options >>> pipelines.simple.measure.skip and pipelines.simple.measure.number. >>> Setting the latter one to 0 disables the measurement. >>> Instead of the last frame, the class member and its configuration >>> specify the number of frames to measure. This is easier to use for >>> users and doesn't require to adjust two configuration parameters when >>> the number of the initially skipped frames is changed. >>> The patch also changes the names of the class members to make them more >>> accurate. >>> Completes software ISP TODO #7. >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> Documentation/runtime_configuration.rst | 6 ++++++ >>> src/libcamera/software_isp/TODO | 25 ---------------------- >>> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++-------- >>> src/libcamera/software_isp/debayer_cpu.h | 7 +++--- >>> 4 files changed, 25 insertions(+), 38 deletions(-) >>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst >>> index f4e86ae24..de74fa732 100644 >>> --- a/Documentation/runtime_configuration.rst >>> +++ b/Documentation/runtime_configuration.rst >>> @@ -52,6 +52,9 @@ file structure: >>> ... >>> simple: >>> copy_input_buffer: # true/false >>> + measure: >>> + skip: # non-negative integer, frames to skip initially >>> + number: # non-negative integer, frames to measure >>> supported_devices: >>> - driver: # driver name, e.g. `mxc-isi` >>> software_isp: # true/false >>> @@ -87,6 +90,9 @@ Configuration file example >>> min_total_unicam_buffers: 2 >>> simple: >>> copy_input_buffer: false >>> + measure: >>> + skip: 50 >>> + number: 30 >>> supported_devices: >>> - driver: mxc-isi >>> software_isp: true >>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO >>> index 2c919f442..f19e15ae2 100644 >>> --- a/src/libcamera/software_isp/TODO >>> +++ b/src/libcamera/software_isp/TODO >>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs. >>> --- >>> -7. Performance measurement configuration >>> - >>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >>> -> /* Measure before emitting signals */ >>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >>> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >>> -> timespec frameEndTime = {}; >>> -> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >>> -> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >>> -> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >>> -> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >>> -> DebayerCpu::kFramesToSkip; >>> -> LOG(Debayer, Info) >>> -> << "Processed " << measuredFrames >>> -> << " frames in " << frameProcessTime_ / 1000 << "us, " >>> -> << frameProcessTime_ / (1000 * measuredFrames) >>> -> << " us/frame"; >>> -> } >>> -> } >>> - >>> -I wonder if there would be a way to control at runtime when/how to >>> -perform those measurements. Maybe that's a bit overkill. >>> - >>> ---- >>> - >>> 8. DebayerCpu cleanups >>> > >> class DebayerCpu : public Debayer, public Object >>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >>> index 85e31e98e..27113c6a5 100644 >>> --- a/src/libcamera/software_isp/debayer_cpu.cpp >>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat >>> enableInputMemcpy_ = >>> configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true); >>> + skipBeforeMeasure_ = configuration.option<unsigned int>( >>> + "pipelines/simple/measure/skip") >>> + .value_or(skipBeforeMeasure_); >>> + framesToMeasure_ = configuration.option<unsigned int>( >>> + "pipelines/simple/measure/number") >>> + .value_or(framesToMeasure_); >>> + >> >> Regarding this and the previous patch, I think the `DebayerCpu` class should >> not access the configuration like this. >> >> A different approach could be creating e.g. >> >> struct DebayerCpu::Params { >> bool inputMemcpy = true; >> struct { >> unsigned int skip = 0, frames = 0; >> } measure; >> }; >> >> and passing that as argument to the constructor, and letting the caller extract the >> settings from wherever it wants. > > OK, makes sense. On the second thought, it doesn't. Why should the caller decide what the given debayering class needs from the configuration rather than the class itself? Note there will be different debayering classes (CPU, GPU, maybe others in future), possibly with their own specific configuration options. >> Alternatively, and I think this is generally useful is other contexts, I would >> add a `Configuration` type that contains a single `const YamlObject&`, essentially >> representing a subtree of the configuration. And then this type would have the >> option(), listOption, envOption(), etc. functions. In addition it would have a >> `Configuration operator[](path)` or `Configuration child(path)` that returns a >> `Configuration` object pointing to a child. This would make it possible to not >> have to consider the entire structure of the configuration from the root. >> Everything could deal with just its own dictionary, etc. This could then be >> passed to the constructors of various objects (pipeline handler, ipa, etc.) >> >> Thoughts? > > What would be the primary motivation? I can think about: > > - Not exposing the whole configuration to proprietary code. > > - Making the configuration lookup relative to not having update it on > contingent restructuring (but since such a thing is an annoyance to > users above all and it still requires changes somewhere at callers, > this doesn't look like any important). > > Against: > > - Perhaps just unnecessary complication. > > - It obfuscates in the code where's the given configuration option in > the file. > >> I am not proposing these changes be made just now, but I think >> something like this could be useful in the future. >> >> >> Regards, >> Barnabás Pőcze >> >> >>> /* Initialize color lookup tables */ >>> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { >>> red_[i] = green_[i] = blue_[i] = i; >>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, >>> lineBuffers_[i].resize(lineBufferLength_); >>> } >>> - measuredFrames_ = 0; >>> + encounteredFrames_ = 0; >>> frameProcessTime_ = 0; >>> return 0; >>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>> { >>> timespec frameStartTime; >>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { >>> + bool measure = framesToMeasure_ > 0 && >>> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ && >>> + ++encounteredFrames_ > skipBeforeMeasure_; >>> + if (measure) { >>> frameStartTime = {}; >>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >>> } >>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>> dmaSyncers.clear(); >>> /* Measure before emitting signals */ >>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >>> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >>> + if (measure) { >>> timespec frameEndTime = {}; >>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >>> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >>> - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >>> - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >>> - DebayerCpu::kFramesToSkip; >>> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) { >>> LOG(Debayer, Info) >>> - << "Processed " << measuredFrames >>> + << "Processed " << framesToMeasure_ >>> << " frames in " << frameProcessTime_ / 1000 << "us, " >>> - << frameProcessTime_ / (1000 * measuredFrames) >>> + << frameProcessTime_ / (1000 * framesToMeasure_) >>> << " us/frame"; >>> } >>> } >>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >>> index 2f35aa18b..9d343e464 100644 >>> --- a/src/libcamera/software_isp/debayer_cpu.h >>> +++ b/src/libcamera/software_isp/debayer_cpu.h >>> @@ -161,11 +161,10 @@ private: >>> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >>> bool enableInputMemcpy_; >>> bool swapRedBlueGains_; >>> - unsigned int measuredFrames_; >>> + unsigned int encounteredFrames_; >>> int64_t frameProcessTime_; >>> - /* Skip 30 frames for things to stabilize then measure 30 frames */ >>> - static constexpr unsigned int kFramesToSkip = 30; >>> - static constexpr unsigned int kLastFrameToMeasure = 60; >>> + unsigned int skipBeforeMeasure_ = 30; >>> + unsigned int framesToMeasure_ = 30; >>> }; >>> } /* namespace libcamera */
Hi 2025. 07. 01. 10:37 keltezéssel, Milan Zamazal írta: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Hi Barnabás, >> >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> Hi >>> >>> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta: >>>> Software ISP performs performance measurement on certain part of initial >>>> frames. Let's make this range configurable. >>>> For this purpose, this patch introduces new configuration options >>>> pipelines.simple.measure.skip and pipelines.simple.measure.number. >>>> Setting the latter one to 0 disables the measurement. >>>> Instead of the last frame, the class member and its configuration >>>> specify the number of frames to measure. This is easier to use for >>>> users and doesn't require to adjust two configuration parameters when >>>> the number of the initially skipped frames is changed. >>>> The patch also changes the names of the class members to make them more >>>> accurate. >>>> Completes software ISP TODO #7. >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> Documentation/runtime_configuration.rst | 6 ++++++ >>>> src/libcamera/software_isp/TODO | 25 ---------------------- >>>> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++-------- >>>> src/libcamera/software_isp/debayer_cpu.h | 7 +++--- >>>> 4 files changed, 25 insertions(+), 38 deletions(-) >>>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst >>>> index f4e86ae24..de74fa732 100644 >>>> --- a/Documentation/runtime_configuration.rst >>>> +++ b/Documentation/runtime_configuration.rst >>>> @@ -52,6 +52,9 @@ file structure: >>>> ... >>>> simple: >>>> copy_input_buffer: # true/false >>>> + measure: >>>> + skip: # non-negative integer, frames to skip initially >>>> + number: # non-negative integer, frames to measure >>>> supported_devices: >>>> - driver: # driver name, e.g. `mxc-isi` >>>> software_isp: # true/false >>>> @@ -87,6 +90,9 @@ Configuration file example >>>> min_total_unicam_buffers: 2 >>>> simple: >>>> copy_input_buffer: false >>>> + measure: >>>> + skip: 50 >>>> + number: 30 >>>> supported_devices: >>>> - driver: mxc-isi >>>> software_isp: true >>>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO >>>> index 2c919f442..f19e15ae2 100644 >>>> --- a/src/libcamera/software_isp/TODO >>>> +++ b/src/libcamera/software_isp/TODO >>>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs. >>>> --- >>>> -7. Performance measurement configuration >>>> - >>>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >>>> -> /* Measure before emitting signals */ >>>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >>>> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >>>> -> timespec frameEndTime = {}; >>>> -> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >>>> -> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >>>> -> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >>>> -> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >>>> -> DebayerCpu::kFramesToSkip; >>>> -> LOG(Debayer, Info) >>>> -> << "Processed " << measuredFrames >>>> -> << " frames in " << frameProcessTime_ / 1000 << "us, " >>>> -> << frameProcessTime_ / (1000 * measuredFrames) >>>> -> << " us/frame"; >>>> -> } >>>> -> } >>>> - >>>> -I wonder if there would be a way to control at runtime when/how to >>>> -perform those measurements. Maybe that's a bit overkill. >>>> - >>>> ---- >>>> - >>>> 8. DebayerCpu cleanups >>>> > >> class DebayerCpu : public Debayer, public Object >>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >>>> index 85e31e98e..27113c6a5 100644 >>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp >>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >>>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat >>>> enableInputMemcpy_ = >>>> configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true); >>>> + skipBeforeMeasure_ = configuration.option<unsigned int>( >>>> + "pipelines/simple/measure/skip") >>>> + .value_or(skipBeforeMeasure_); >>>> + framesToMeasure_ = configuration.option<unsigned int>( >>>> + "pipelines/simple/measure/number") >>>> + .value_or(framesToMeasure_); >>>> + >>> >>> Regarding this and the previous patch, I think the `DebayerCpu` class should >>> not access the configuration like this. >>> >>> A different approach could be creating e.g. >>> >>> struct DebayerCpu::Params { >>> bool inputMemcpy = true; >>> struct { >>> unsigned int skip = 0, frames = 0; >>> } measure; >>> }; >>> >>> and passing that as argument to the constructor, and letting the caller extract the >>> settings from wherever it wants. >> >> OK, makes sense. > > On the second thought, it doesn't. Why should the caller decide what > the given debayering class needs from the configuration rather than the > class itself? Note there will be different debayering classes (CPU, > GPU, maybe others in future), possibly with their own specific > configuration options. It's still the class deciding, no? Just in a different, one might argue, more explicit way. In any case, I think the point is that you have to draw a line somewhere because you can't have everything reach into a magic object for parameters if you want reusable components. When I wrote this, I felt that maybe DebayerCpu is on the other side of the line and it shouldn't access the configuration directly. In any case, I believe the this version is fine, so don't change it unless you want to. > >>> Alternatively, and I think this is generally useful is other contexts, I would >>> add a `Configuration` type that contains a single `const YamlObject&`, essentially >>> representing a subtree of the configuration. And then this type would have the >>> option(), listOption, envOption(), etc. functions. In addition it would have a >>> `Configuration operator[](path)` or `Configuration child(path)` that returns a >>> `Configuration` object pointing to a child. This would make it possible to not >>> have to consider the entire structure of the configuration from the root. >>> Everything could deal with just its own dictionary, etc. This could then be >>> passed to the constructors of various objects (pipeline handler, ipa, etc.) >>> >>> Thoughts? >> >> What would be the primary motivation? I can think about: >> >> - Not exposing the whole configuration to proprietary code. >> >> - Making the configuration lookup relative to not having update it on >> contingent restructuring (but since such a thing is an annoyance to >> users above all and it still requires changes somewhere at callers, >> this doesn't look like any important). This is the primary motivation for me. Having relative paths makes reusing things simpler, e.g. if you want to configure multiple instances in different parts of the configuration file. Admittedly, I don't see a use-case for this currently. >> >> Against: >> >> - Perhaps just unnecessary complication. >> >> - It obfuscates in the code where's the given configuration option in >> the file. That's probably the strongest argument against in my opinion. But if you want to reuse the same component in different parts, then you have to give up some if that clarity either way in my opinion. Regards, Barnabás Pőcze >> >>> I am not proposing these changes be made just now, but I think >>> something like this could be useful in the future. >>> >>> >>> Regards, >>> Barnabás Pőcze >>> >>> >>>> /* Initialize color lookup tables */ >>>> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { >>>> red_[i] = green_[i] = blue_[i] = i; >>>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, >>>> lineBuffers_[i].resize(lineBufferLength_); >>>> } >>>> - measuredFrames_ = 0; >>>> + encounteredFrames_ = 0; >>>> frameProcessTime_ = 0; >>>> return 0; >>>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>>> { >>>> timespec frameStartTime; >>>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { >>>> + bool measure = framesToMeasure_ > 0 && >>>> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ && >>>> + ++encounteredFrames_ > skipBeforeMeasure_; >>>> + if (measure) { >>>> frameStartTime = {}; >>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >>>> } >>>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >>>> dmaSyncers.clear(); >>>> /* Measure before emitting signals */ >>>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >>>> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >>>> + if (measure) { >>>> timespec frameEndTime = {}; >>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >>>> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); >>>> - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { >>>> - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - >>>> - DebayerCpu::kFramesToSkip; >>>> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) { >>>> LOG(Debayer, Info) >>>> - << "Processed " << measuredFrames >>>> + << "Processed " << framesToMeasure_ >>>> << " frames in " << frameProcessTime_ / 1000 << "us, " >>>> - << frameProcessTime_ / (1000 * measuredFrames) >>>> + << frameProcessTime_ / (1000 * framesToMeasure_) >>>> << " us/frame"; >>>> } >>>> } >>>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >>>> index 2f35aa18b..9d343e464 100644 >>>> --- a/src/libcamera/software_isp/debayer_cpu.h >>>> +++ b/src/libcamera/software_isp/debayer_cpu.h >>>> @@ -161,11 +161,10 @@ private: >>>> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >>>> bool enableInputMemcpy_; >>>> bool swapRedBlueGains_; >>>> - unsigned int measuredFrames_; >>>> + unsigned int encounteredFrames_; >>>> int64_t frameProcessTime_; >>>> - /* Skip 30 frames for things to stabilize then measure 30 frames */ >>>> - static constexpr unsigned int kFramesToSkip = 30; >>>> - static constexpr unsigned int kLastFrameToMeasure = 60; >>>> + unsigned int skipBeforeMeasure_ = 30; >>>> + unsigned int framesToMeasure_ = 30; >>>> }; >>>> } /* namespace libcamera */ >
diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst index f4e86ae24..de74fa732 100644 --- a/Documentation/runtime_configuration.rst +++ b/Documentation/runtime_configuration.rst @@ -52,6 +52,9 @@ file structure: ... simple: copy_input_buffer: # true/false + measure: + skip: # non-negative integer, frames to skip initially + number: # non-negative integer, frames to measure supported_devices: - driver: # driver name, e.g. `mxc-isi` software_isp: # true/false @@ -87,6 +90,9 @@ Configuration file example min_total_unicam_buffers: 2 simple: copy_input_buffer: false + measure: + skip: 50 + number: 30 supported_devices: - driver: mxc-isi software_isp: true diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 2c919f442..f19e15ae2 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs. --- -7. Performance measurement configuration - -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) -> /* Measure before emitting signals */ -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { -> timespec frameEndTime = {}; -> clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); -> frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); -> if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { -> const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - -> DebayerCpu::kFramesToSkip; -> LOG(Debayer, Info) -> << "Processed " << measuredFrames -> << " frames in " << frameProcessTime_ / 1000 << "us, " -> << frameProcessTime_ / (1000 * measuredFrames) -> << " us/frame"; -> } -> } - -I wonder if there would be a way to control at runtime when/how to -perform those measurements. Maybe that's a bit overkill. - ---- - 8. DebayerCpu cleanups > >> class DebayerCpu : public Debayer, public Object diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 85e31e98e..27113c6a5 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat enableInputMemcpy_ = configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true); + skipBeforeMeasure_ = configuration.option<unsigned int>( + "pipelines/simple/measure/skip") + .value_or(skipBeforeMeasure_); + framesToMeasure_ = configuration.option<unsigned int>( + "pipelines/simple/measure/number") + .value_or(framesToMeasure_); + /* Initialize color lookup tables */ for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { red_[i] = green_[i] = blue_[i] = i; @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, lineBuffers_[i].resize(lineBufferLength_); } - measuredFrames_ = 0; + encounteredFrames_ = 0; frameProcessTime_ = 0; return 0; @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output { timespec frameStartTime; - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { + bool measure = framesToMeasure_ > 0 && + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ && + ++encounteredFrames_ > skipBeforeMeasure_; + if (measure) { frameStartTime = {}; clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); } @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output dmaSyncers.clear(); /* Measure before emitting signals */ - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && - ++measuredFrames_ > DebayerCpu::kFramesToSkip) { + if (measure) { timespec frameEndTime = {}; clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); frameProcessTime_ += timeDiff(frameEndTime, frameStartTime); - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) { - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure - - DebayerCpu::kFramesToSkip; + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) { LOG(Debayer, Info) - << "Processed " << measuredFrames + << "Processed " << framesToMeasure_ << " frames in " << frameProcessTime_ / 1000 << "us, " - << frameProcessTime_ / (1000 * measuredFrames) + << frameProcessTime_ / (1000 * framesToMeasure_) << " us/frame"; } } diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 2f35aa18b..9d343e464 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -161,11 +161,10 @@ private: unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ bool enableInputMemcpy_; bool swapRedBlueGains_; - unsigned int measuredFrames_; + unsigned int encounteredFrames_; int64_t frameProcessTime_; - /* Skip 30 frames for things to stabilize then measure 30 frames */ - static constexpr unsigned int kFramesToSkip = 30; - static constexpr unsigned int kLastFrameToMeasure = 60; + unsigned int skipBeforeMeasure_ = 30; + unsigned int framesToMeasure_ = 30; }; } /* namespace libcamera */
Software ISP performs performance measurement on certain part of initial frames. Let's make this range configurable. For this purpose, this patch introduces new configuration options pipelines.simple.measure.skip and pipelines.simple.measure.number. Setting the latter one to 0 disables the measurement. Instead of the last frame, the class member and its configuration specify the number of frames to measure. This is easier to use for users and doesn't require to adjust two configuration parameters when the number of the initially skipped frames is changed. The patch also changes the names of the class members to make them more accurate. Completes software ISP TODO #7. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- Documentation/runtime_configuration.rst | 6 ++++++ src/libcamera/software_isp/TODO | 25 ---------------------- src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++-------- src/libcamera/software_isp/debayer_cpu.h | 7 +++--- 4 files changed, 25 insertions(+), 38 deletions(-)