| Message ID | 20251212002937.3118-21-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > We have a need to generate an identity CCM when running the GPUIsp without > a sensor tuning file. > > A preivous patch added a selfInitialising bool to the IPAContext structure. > When that bool is true generate an identity CCM at colour temperature > 6500k. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> The patch looks OK to me now and I could put my Reviewed-By on it. But thinking about it, I'd suggest to split this series (well, after the precursor series is merged, to keep things manageable). I think the patches up to "Make gpuisp default softisp mode" (included) are on a good track and might be merged soon to bring the GPU ISP support in. But the following patches may need re-evaluation. I think my simple IPA cleanup series (https://patchwork.libcamera.org/project/libcamera/list/?series=5597) resolves many problems that the last patches here try to address. Switching from the current CCM handling to a combined matrix, better isolation of all the algorithms, LUT algorithm removal and moving LUT construction to CPU debayering might perhaps replace the stuff here. So my suggestion would be to work towards merging the first part, then to rebase my simple IPA cleanup and work on it. I hope the patches could be reviewed relatively easily once rebased (they are RFC only because they wait for merged GPU ISP). Then we'll see whether something from the rest of this series is still needed. The last "TODO" patch can be added to the patches to be merged, of course. > --- > src/ipa/simple/algorithms/ccm.cpp | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > index 0a98406c1..ac1ce1685 100644 > --- a/src/ipa/simple/algorithms/ccm.cpp > +++ b/src/ipa/simple/algorithms/ccm.cpp > @@ -27,13 +27,23 @@ namespace ipa::soft::algorithms { > > LOG_DEFINE_CATEGORY(IPASoftCcm) > > -int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) > +int Ccm::init([[maybe_unused]] IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) > { > - int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); > - if (ret < 0) { > - LOG(IPASoftCcm, Error) > - << "Failed to parse 'ccm' parameter from tuning file."; > - return ret; > + if (!context.selfInitialising) { > + int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); > + if (ret < 0) { > + LOG(IPASoftCcm, Error) > + << "Failed to parse 'ccm' parameter from tuning file."; > + return ret; > + } > + } else { > + /* Initialize with identity CCM at standard D65 color temperature */ > + Matrix<float, 3, 3> identityMatrix = Matrix<float, 3, 3>::identity(); > + > + std::map<unsigned int, Matrix<float, 3, 3>> ccmData; > + ccmData[6500] = identityMatrix; > + > + ccm_ = Interpolator<Matrix<float, 3, 3>>(std::move(ccmData)); > } > > context.ccmEnabled = true;
On 12/12/2025 21:21, Milan Zamazal wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> We have a need to generate an identity CCM when running the GPUIsp without >> a sensor tuning file. >> >> A preivous patch added a selfInitialising bool to the IPAContext structure. >> When that bool is true generate an identity CCM at colour temperature >> 6500k. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > The patch looks OK to me now and I could put my Reviewed-By on it. But > thinking about it, I'd suggest to split this series (well, after the > precursor series is merged, to keep things manageable). > > I think the patches up to "Make gpuisp default softisp mode" (included) > are on a good track and might be merged soon to bring the GPU ISP > support in. > > But the following patches may need re-evaluation. I think my simple IPA > cleanup series > (https://patchwork.libcamera.org/project/libcamera/list/?series=5597) > resolves many problems that the last patches here try to address. > Switching from the current CCM handling to a combined matrix, better > isolation of all the algorithms, LUT algorithm removal and moving LUT > construction to CPU debayering might perhaps replace the stuff here. Hmm. Could you not rebase your stuff on top of this ? This series is at version 8 with your proposed series at V2 RFC. We were supposed to be aiming to get this series in for 0.6.0 which we missed. Adding another dependency on 13 more patches 8/13 have no RB yet. I'd really rather get this series finished/in instead of establishing even more dependencies. --- bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 12/12/2025 21:21, Milan Zamazal wrote: >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> > >>> We have a need to generate an identity CCM when running the GPUIsp without >>> a sensor tuning file. >>> >>> A preivous patch added a selfInitialising bool to the IPAContext structure. >>> When that bool is true generate an identity CCM at colour temperature >>> 6500k. >>> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> The patch looks OK to me now and I could put my Reviewed-By on it. But >> thinking about it, I'd suggest to split this series (well, after the >> precursor series is merged, to keep things manageable). >> I think the patches up to "Make gpuisp default softisp mode" (included) >> are on a good track and might be merged soon to bring the GPU ISP >> support in. >> But the following patches may need re-evaluation. I think my simple IPA >> cleanup series >> (https://patchwork.libcamera.org/project/libcamera/list/?series=5597) >> resolves many problems that the last patches here try to address. >> Switching from the current CCM handling to a combined matrix, better >> isolation of all the algorithms, LUT algorithm removal and moving LUT >> construction to CPU debayering might perhaps replace the stuff here. > > Hmm. Could you not rebase your stuff on top of this ? I will, but only once the GPU ISP patches are merged. Rebasing is often painful and error prone and there is no need to bother with it in this case while the underlying patches are not merged and may change. > This series is at version 8 with your proposed series at V2 RFC. > > We were supposed to be aiming to get this series in for 0.6.0 which we missed. > > Adding another dependency on 13 more patches 8/13 have no RB yet. > > I'd really rather get this series finished/in instead of establishing even more dependencies. This is not another dependency, this is an independent/followup work that may replace most of the patches #19-25. Up to you whether you prefer working on getting the patches merged now or keep them aside until the IPA cleanup is done. I don't mind either way, I can ack the patches right now and it's easy reverting them later if needed.
Milan Zamazal <mzamazal@redhat.com> writes: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> We have a need to generate an identity CCM when running the GPUIsp without >> a sensor tuning file. >> >> A preivous patch added a selfInitialising bool to the IPAContext structure. >> When that bool is true generate an identity CCM at colour temperature >> 6500k. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > The patch looks OK to me now and I could put my Reviewed-By on it. Let's go on to save reviewing chores: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > But thinking about it, I'd suggest to split this series (well, after > the precursor series is merged, to keep things manageable). > > I think the patches up to "Make gpuisp default softisp mode" (included) > are on a good track and might be merged soon to bring the GPU ISP > support in. > > But the following patches may need re-evaluation. I think my simple IPA > cleanup series > (https://patchwork.libcamera.org/project/libcamera/list/?series=5597) > resolves many problems that the last patches here try to address. > Switching from the current CCM handling to a combined matrix, better > isolation of all the algorithms, LUT algorithm removal and moving LUT > construction to CPU debayering might perhaps replace the stuff here. > > So my suggestion would be to work towards merging the first part, then > to rebase my simple IPA cleanup and work on it. I hope the patches > could be reviewed relatively easily once rebased (they are RFC only > because they wait for merged GPU ISP). Then we'll see whether something > from the rest of this series is still needed. > > The last "TODO" patch can be added to the patches to be merged, of > course. > >> --- >> src/ipa/simple/algorithms/ccm.cpp | 22 ++++++++++++++++------ >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >> index 0a98406c1..ac1ce1685 100644 >> --- a/src/ipa/simple/algorithms/ccm.cpp >> +++ b/src/ipa/simple/algorithms/ccm.cpp >> @@ -27,13 +27,23 @@ namespace ipa::soft::algorithms { >> >> LOG_DEFINE_CATEGORY(IPASoftCcm) >> >> -int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) >> +int Ccm::init([[maybe_unused]] IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) >> { >> - int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); >> - if (ret < 0) { >> - LOG(IPASoftCcm, Error) >> - << "Failed to parse 'ccm' parameter from tuning file."; >> - return ret; >> + if (!context.selfInitialising) { >> + int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); >> + if (ret < 0) { >> + LOG(IPASoftCcm, Error) >> + << "Failed to parse 'ccm' parameter from tuning file."; >> + return ret; >> + } >> + } else { >> + /* Initialize with identity CCM at standard D65 color temperature */ >> + Matrix<float, 3, 3> identityMatrix = Matrix<float, 3, 3>::identity(); >> + >> + std::map<unsigned int, Matrix<float, 3, 3>> ccmData; >> + ccmData[6500] = identityMatrix; >> + >> + ccm_ = Interpolator<Matrix<float, 3, 3>>(std::move(ccmData)); >> } >> >> context.ccmEnabled = true;
Quoting Milan Zamazal (2025-12-15 10:42:10) > Milan Zamazal <mzamazal@redhat.com> writes: > > > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > > > >> We have a need to generate an identity CCM when running the GPUIsp without > >> a sensor tuning file. > >> > >> A preivous patch added a selfInitialising bool to the IPAContext structure. > >> When that bool is true generate an identity CCM at colour temperature > >> 6500k. > >> > >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > > The patch looks OK to me now and I could put my Reviewed-By on it. > > Let's go on to save reviewing chores: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > But thinking about it, I'd suggest to split this series (well, after > > the precursor series is merged, to keep things manageable). > > > > I think the patches up to "Make gpuisp default softisp mode" (included) > > are on a good track and might be merged soon to bring the GPU ISP > > support in. > > > > But the following patches may need re-evaluation. I think my simple IPA > > cleanup series > > (https://patchwork.libcamera.org/project/libcamera/list/?series=5597) > > resolves many problems that the last patches here try to address. > > Switching from the current CCM handling to a combined matrix, better > > isolation of all the algorithms, LUT algorithm removal and moving LUT > > construction to CPU debayering might perhaps replace the stuff here. I think this is sounding spot on - I'm trying to test GPU ISP - and I've enabled CCM with the following: kbingham@charm:~/iob/libcamera$ cat src/ipa/simple/data/ov5675.yaml # SPDX-License-Identifier: CC0-1.0 %YAML 1.1 --- version: 1 algorithms: - BlackLevel: - Awb: # Color correction matrices can be defined here. The CCM algorithm # has a significant performance impact, and should only be enabled # if tuned. - Ccm: ccms: - ct: 6500 ccm: [ 3, 0, 0, 0, 1, 0, 2, 0, 8] - Lut: - Agc: ... But running cam to capture frames with a bit of extra debug enabled to print the CCM - shows where I see flicker when the CCM gets re-configured and seems to be fighting against the AWB or such: [1:05:28.547392018] [39994] INFO IPASoftCcm ccm.cpp:103 ct 0 ccm - Matrix { [ 0, 0, 0 ], [ 0, 0, 0 ], [ 0, 0, 0 ] } [1:05:28.547517375] [39994] INFO IPASoftCcm ccm.cpp:103 ct 0 ccm - Matrix { [ 3, 0, 0 ], [ 0, 1, 0 ], [ 2, 0, 8 ] } [1:05:28.563253799] [39994] INFO IPASoftAwb awb.cpp:95 gain R/B: Vector { 1.26707, 1, 1.42126 }; temperature: 4973 3928.546437 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 20404224 [1:05:28.580331026] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.580398939] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 3, 0, 0 ], [ 0, 1, 0 ], [ 2, 0, 8 ] } 3928.579823 (29.95 fps) cam0-stream0 seq: 000001 bytesused: 20404224 [1:05:28.613874320] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.613949836] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } 3928.613210 (29.95 fps) cam0-stream0 seq: 000002 bytesused: 20404224 [1:05:28.647249343] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.647313141] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } 3928.646596 (29.95 fps) cam0-stream0 seq: 000003 bytesused: 20404224 [1:05:28.680742119] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.680804406] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4973 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.688169316] [39994] INFO IPASoftAwb awb.cpp:95 gain R/B: Vector { 1.26588, 1, 1.41959 }; temperature: 4978 3928.679981 (29.95 fps) cam0-stream0 seq: 000004 bytesused: 20404224 [1:05:28.713811848] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4978 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } [1:05:28.713864396] [39994] INFO IPASoftCcm ccm.cpp:103 ct 4978 ccm - Matrix { [ 1, 0, 0 ], [ 0, 1, 0 ], [ 0, 0, 1 ] } You can see the CCM has been 'reset' (I think by awb, but haven't dug deep enough yet) - and it stays like that until the colour temperature threshold changes enough to re-interpolate a new CCM. > > > > So my suggestion would be to work towards merging the first part, then > > to rebase my simple IPA cleanup and work on it. I hope the patches > > could be reviewed relatively easily once rebased (they are RFC only > > because they wait for merged GPU ISP). Then we'll see whether something > > from the rest of this series is still needed. > > > > The last "TODO" patch can be added to the patches to be merged, of > > course. > > > >> --- > >> src/ipa/simple/algorithms/ccm.cpp | 22 ++++++++++++++++------ > >> 1 file changed, 16 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > >> index 0a98406c1..ac1ce1685 100644 > >> --- a/src/ipa/simple/algorithms/ccm.cpp > >> +++ b/src/ipa/simple/algorithms/ccm.cpp > >> @@ -27,13 +27,23 @@ namespace ipa::soft::algorithms { > >> > >> LOG_DEFINE_CATEGORY(IPASoftCcm) > >> > >> -int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) > >> +int Ccm::init([[maybe_unused]] IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) > >> { > >> - int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); > >> - if (ret < 0) { > >> - LOG(IPASoftCcm, Error) > >> - << "Failed to parse 'ccm' parameter from tuning file."; > >> - return ret; > >> + if (!context.selfInitialising) { > >> + int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); > >> + if (ret < 0) { > >> + LOG(IPASoftCcm, Error) > >> + << "Failed to parse 'ccm' parameter from tuning file."; > >> + return ret; > >> + } > >> + } else { > >> + /* Initialize with identity CCM at standard D65 color temperature */ > >> + Matrix<float, 3, 3> identityMatrix = Matrix<float, 3, 3>::identity(); > >> + > >> + std::map<unsigned int, Matrix<float, 3, 3>> ccmData; > >> + ccmData[6500] = identityMatrix; > >> + > >> + ccm_ = Interpolator<Matrix<float, 3, 3>>(std::move(ccmData)); > >> } > >> > >> context.ccmEnabled = true; >
diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp index 0a98406c1..ac1ce1685 100644 --- a/src/ipa/simple/algorithms/ccm.cpp +++ b/src/ipa/simple/algorithms/ccm.cpp @@ -27,13 +27,23 @@ namespace ipa::soft::algorithms { LOG_DEFINE_CATEGORY(IPASoftCcm) -int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) +int Ccm::init([[maybe_unused]] IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) { - int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); - if (ret < 0) { - LOG(IPASoftCcm, Error) - << "Failed to parse 'ccm' parameter from tuning file."; - return ret; + if (!context.selfInitialising) { + int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm"); + if (ret < 0) { + LOG(IPASoftCcm, Error) + << "Failed to parse 'ccm' parameter from tuning file."; + return ret; + } + } else { + /* Initialize with identity CCM at standard D65 color temperature */ + Matrix<float, 3, 3> identityMatrix = Matrix<float, 3, 3>::identity(); + + std::map<unsigned int, Matrix<float, 3, 3>> ccmData; + ccmData[6500] = identityMatrix; + + ccm_ = Interpolator<Matrix<float, 3, 3>>(std::move(ccmData)); } context.ccmEnabled = true;
We have a need to generate an identity CCM when running the GPUIsp without a sensor tuning file. A preivous patch added a selfInitialising bool to the IPAContext structure. When that bool is true generate an identity CCM at colour temperature 6500k. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/ipa/simple/algorithms/ccm.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)