<meta http-equiv="Content-Type" content="text/html; charset=GB18030"><div>Hi Laurent,</div><div><br></div><div>Thanks for your comments.</div><div><br></div><div>>This is fine for test purposes, and we can integrate the software ISP</div><div>>with the simple pipeline handler in a subsequen step.</div><div><br></div><div>Yeah, these patches will not be merged, and just implemented for test</div><div>purposes, which can make me familiar with the workflow of the </div><div>pipeline handler. After finishing this, I will think about intergrating the</div><div>software ISP with simple pipeline handler. </div><div><br>>I see that the implementation is multi-threaded, which is good, as the<br>>software processing is heavy. However, the thread should be moved to the<br>>ISP class. Generally speaking, the ISP class should expose an interface<br>>that resembles a hardware ISP, with functions called from the pipeline<br>>handler to queue work to the ISP, and signals being emitted later when<br>>the work completes. The work itself needs to be performed in a separate<br>>thread.<br><br>>Speaking of interfaces, I think we should also create a base abstract<br>>class that defines the software ISP API. This CPU-based implementation<br>>would then inherit from the base class, and a future GPU-based<br>>implementation would also do the same, exposing the same API. This will<br>>allow switching between the two implementations seamlessly, without any<br>>change in the pipeline handler.</div><div><br></div><div>Thanks for your advice. I will move the thread to ISP and move ISP out of </div><div>pipeline to make ISP interface in the next version. The abstract class ISP will</div><div>be considered too.<br><br>>I've also looked at the ISP implementation itself, and I wonder if it<br>>shouldn't be restructure to match what a hardware ISP would do. ISPs<br>>typically compute statistics from the image, and process pixels in<br>>hardware. A CPU-based algorithm (implemented in libcamera as an IPA<br>>module) consumes those statistics and produces parameters for the pixel<br>>processing. Practically speaking, this means that, for instance, the<br>>autoWhiteBalance() function would need to be split in three parts:<br>>statistics calculation, computation of the colour gains from the<br>>statistics, and multiplication of each pixel by the colour gains. One<br>>reason why I think statistics computation should probably be split out<br>>is that the same statistics could probably be reused in different<br>>processing steps. It's also tempting to move the computation of the<br>>colour gains (and other processing parameters) to an IPA module, but<br>>that may be overkill, maybe having a clear split but keeping all the<br>>code in the ISP class would be better. In any case, different options<br>>should be considered, and we should pick the best one and explain the<br>>reasons.</div><div><br></div><div>I think spliting out the statistics parameters is a good idea. However, the</div><div>most important thing right now is to fix the pipeline handler. With this working,</div><div>I will consider the details how to split out isp algorithm later.<br><br>>Finally, a few miscellaneous comments about the implementation of the<br>>ISP class:<br><br>>- Internal functions should be private, to clearly differentiate between<br>> the public API and the internal implementation.</div><div><br>>- Memory management needs to be considered. You currently allocate a<br>> large scratch buffer (rgbData) in ISP::processing(), and I wonder if<br>> it would make more sense to perform processing line by line to only<br>> require line buffers. This would have an effect on cache usage, which<br>> needs to be considered too. It's an internal implementation decision<br>> though, so it won't affect the API.</div><div><br></div><div>I will consider these in the design process of isp class.<br><br>Regards,<br><br>Fan Siyuan</div>