diff options
author | Hans de Goede <hdegoede@redhat.com> | 2024-04-16 11:13:45 +0200 |
---|---|---|
committer | Kieran Bingham <kieran.bingham@ideasonboard.com> | 2024-04-16 13:00:21 +0100 |
commit | 3755d966485d96e76eef7756222baf02dea960a4 (patch) | |
tree | 4cc58115df74a4c0e8d181b25962449fd0f3b428 /src/libcamera/software_isp/TODO | |
parent | 783f92c018afb5ea5e18e75897d3a96577623b64 (diff) |
libcamera: software_isp: Add DebayerCpu class
Add CPU based debayering implementation. This initial implementation
only supports debayering packed 10 bits per pixel bayer data in
the 4 standard bayer orders.
Doxygen documentation by Dennis Bonke.
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Co-developed-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Co-developed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Diffstat (limited to 'src/libcamera/software_isp/TODO')
-rw-r--r-- | src/libcamera/software_isp/TODO | 73 |
1 files changed, 73 insertions, 0 deletions
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 7929e73e..29be5386 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -100,3 +100,76 @@ representation in the implementation of process() before using them ? Possibly something to address later, by storing ISP parameters in per-frame buffers like we do for hardware ISPs. + +--- + +6. Input buffer copying configuration + +> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) +> : stats_(std::move(stats)), gammaCorrection_(1.0) +> { +> enableInputMemcpy_ = true; + +Set this appropriately and/or make it configurable. + +--- + +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 +> >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } +> > +> > This, +> +> Note the statistics pass-through stuff is sort of a necessary evil +> since we want one main loop going over the data line by line and +> doing both debayering as well as stats while the line is still +> hot in the l2 cache. And things like the process2() and process4() +> loops are highly CPU debayering specific so I don't think we should +> move those out of the CpuDebayer code. + +Yes, that I understood from the review. "necessary evil" is indeed the +right term :-) I expect it will take quite some design skills to balance +the need for performances and the need for a maintainable architecture. + +> > plus the fact that this class handles colour gains and gamma, +> > makes me thing we have either a naming issue, or an architecture issue. +> +> I agree that this does a bit more then debayering, although +> the debayering really is the main thing it does. +> +> I guess the calculation of the rgb lookup tables which do the +> color gains and gamma could be moved outside of this class, +> that might even be beneficial for GPU based debayering assuming +> that that is going to use rgb lookup tables too (it could +> implement actual color gains + gamma correction in some different +> way). +> +> I think this falls under the lets wait until we have a GPU +> based SoftISP MVP/POC and then do some refactoring to see which +> bits should go where. |