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.