| Message ID | 20201208204441.9356-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Tue, Dec 08, 2020 at 08:44:35PM +0000, David Plowman wrote: > Hi everyone > > Thanks for all the reviews, comments and discussion! I've lost track a > little bit of which comments were in which thread, so perhaps I can > address a few of the remaining issues here. Thanks for the summary :-) > 1. Hide/Mistrust frames at Startup/ModeSwitch > > Yes, I see that some of these are the same now. I'm slightly inclined > to keep all 4 variants for the moment as it seems to me they might be > different, however, if it transpires that new cameras that come along > still don't need them maybe we do some simplification then? Sounds good to me. We plan to move this information to the libcamera core, so at that point we'll propose a rename, if you don't beat us to it. > 2. Should GetConvergenceFrames be in the Algorithm class? > > Again, I'm slightly inclined to avoid it just because there are only 2 > implementations of this, yet there are actually over a dozen > "algorithms". But like all things, we keep an eye on it! Sounds good to me. > 3. "Hiding" vs. "Mistrusting" > > Just to clarify the distinction here, "hiding" means "do not show this > to the user because it will look unexpected in some > way". "Mistrusting" means "do not allow algorithms to process this > frame because it will cause them to wobble or do something bad". I'd like to move away from "hiding". The CamHelper class should report information about the camera sensor, not contain logic that dictates how frames should be handled by the IPA and pipeline handler. It may very well be a semantic difference only, and it doesn't have to be renamed now, we can address that when moving this information to the libcamera core. > "Mistrusting" includes both statistics and metadata from the device. I As statistics are computed from the frame by the ISP, incorrect statistics imply an incorrect frame, right ? In the event that a sensor would produce a first frame containing both bogus data and bogus metadata, and a second frame containing bogus metadata only, do I understand correctly that an algorithm that consumes statistics only could run on the second frame already, while an algorithm that consumes metadata would need to skip the first two frames ? > suppose if the ISP statistics are bad then the frame may be bad too, > so it would feature in the "hide" number already. It is possible that > you could get frames that look OK but where the device is lying about > its metadata, so there is a distinction between the two. > > Yet perhaps this distinction never really happens in practice - so > this should go on this list of things we keep an eye on with a view to > simplification. > > As regards the patches themselves, they are structured exactly as > before. The main differences are: > > 1. No change. > > 2. and 3. GetConvergenceFrames has dropped the parameter (that's > handled back in the IPA now, patch 6). > > 4. Pwl::Inverse is a bit more careful about adding to its existing end > points as it goes. > > 5. No more std::move. > > 6. The adding of the "mistrusted" frames is handled here now. > > And I think that's it, please remind me if anything has slipped > through the cracks! > > David Plowman (6): > src: raspberrypi: Pass the drop frame count in start, not configure > src: ipa: raspberrypi: agc: Add GetConvergenceFrames method to AGC > base class > src: ipa: raspberrypi: awb: Add GetConvergenceFrames method to AWB > base class > src: ipa: raspberrypi: Compute inverse of piecewise linear function > src: ipa: raspberrypi: Estimate the colour temerature if starting with > fixed colour gains > src: ipa: raspberrypi: Move initial frame drop decision to AGC/AWB Minor style issue, we don't normally include the "src: " prefix. It all looks good to me, so I've pushed the patches with the commit subjects updated. > src/ipa/raspberrypi/cam_helper.cpp | 6 +- > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 +++ > .../raspberrypi/controller/agc_algorithm.hpp | 1 + > .../raspberrypi/controller/awb_algorithm.hpp | 1 + > src/ipa/raspberrypi/controller/pwl.cpp | 30 ++++++++ > src/ipa/raspberrypi/controller/pwl.hpp | 3 + > src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++ > src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 + > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +- > src/ipa/raspberrypi/controller/rpi/awb.cpp | 25 +++++++ > src/ipa/raspberrypi/controller/rpi/awb.hpp | 3 + > src/ipa/raspberrypi/raspberrypi.cpp | 73 ++++++++++++++----- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++--- > 13 files changed, 160 insertions(+), 36 deletions(-)
Hi everyone Thanks for all the reviews, comments and discussion! I've lost track a little bit of which comments were in which thread, so perhaps I can address a few of the remaining issues here. 1. Hide/Mistrust frames at Startup/ModeSwitch Yes, I see that some of these are the same now. I'm slightly inclined to keep all 4 variants for the moment as it seems to me they might be different, however, if it transpires that new cameras that come along still don't need them maybe we do some simplification then? 2. Should GetConvergenceFrames be in the Algorithm class? Again, I'm slightly inclined to avoid it just because there are only 2 implementations of this, yet there are actually over a dozen "algorithms". But like all things, we keep an eye on it! 3. "Hiding" vs. "Mistrusting" Just to clarify the distinction here, "hiding" means "do not show this to the user because it will look unexpected in some way". "Mistrusting" means "do not allow algorithms to process this frame because it will cause them to wobble or do something bad". "Mistrusting" includes both statistics and metadata from the device. I suppose if the ISP statistics are bad then the frame may be bad too, so it would feature in the "hide" number already. It is possible that you could get frames that look OK but where the device is lying about its metadata, so there is a distinction between the two. Yet perhaps this distinction never really happens in practice - so this should go on this list of things we keep an eye on with a view to simplification. As regards the patches themselves, they are structured exactly as before. The main differences are: 1. No change. 2. and 3. GetConvergenceFrames has dropped the parameter (that's handled back in the IPA now, patch 6). 4. Pwl::Inverse is a bit more careful about adding to its existing end points as it goes. 5. No more std::move. 6. The adding of the "mistrusted" frames is handled here now. And I think that's it, please remind me if anything has slipped through the cracks! Thanks and best regards David David Plowman (6): src: raspberrypi: Pass the drop frame count in start, not configure src: ipa: raspberrypi: agc: Add GetConvergenceFrames method to AGC base class src: ipa: raspberrypi: awb: Add GetConvergenceFrames method to AWB base class src: ipa: raspberrypi: Compute inverse of piecewise linear function src: ipa: raspberrypi: Estimate the colour temerature if starting with fixed colour gains src: ipa: raspberrypi: Move initial frame drop decision to AGC/AWB src/ipa/raspberrypi/cam_helper.cpp | 6 +- src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 +++ .../raspberrypi/controller/agc_algorithm.hpp | 1 + .../raspberrypi/controller/awb_algorithm.hpp | 1 + src/ipa/raspberrypi/controller/pwl.cpp | 30 ++++++++ src/ipa/raspberrypi/controller/pwl.hpp | 3 + src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++ src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 + src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +- src/ipa/raspberrypi/controller/rpi/awb.cpp | 25 +++++++ src/ipa/raspberrypi/controller/rpi/awb.hpp | 3 + src/ipa/raspberrypi/raspberrypi.cpp | 73 ++++++++++++++----- .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++--- 13 files changed, 160 insertions(+), 36 deletions(-)