| Message ID | 20211221051023.2628625-3-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Quoting Paul Elder (2021-12-21 05:10:22) > Remove the NoiseReductionMode values that the raspberrypi IPA does not > support. The ControlInfo values that the IPA reports will be used for > capability detection, so values that it does not support shall be > removed. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > No change in v2: > - constexpr did not work > --- > include/libcamera/ipa/raspberrypi.h | 8 +++++++- > src/ipa/raspberrypi/raspberrypi.cpp | 2 -- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > index 548bfba0..593139c5 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -18,6 +18,12 @@ namespace libcamera { > > namespace RPi { > > +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = { > + static_cast<int32_t>(controls::NoiseReductionModeOff), > + static_cast<int32_t>(controls::NoiseReductionModeFast), > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > +}; > + > /* > * List of controls handled by the Raspberry Pi IPA > * > @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({ > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > - { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } > + { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) } Aha, I didn't realise we could liimit the exposed values from controls enums like this. I'll have to keep an eye on how things check to ensure the control value is supported, but it makes sense that it should only report the modes it supports. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > }, controls::controls); > > } /* namespace RPi */ > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 3f497be1..e0685c69 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { > { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, > { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, > { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, > - { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff }, > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > }; > > void IPARPi::queueRequest(const ControlList &controls) > -- > 2.27.0 >
Hi Paul, Kieran Thank you for this patch! On Mon, 3 Jan 2022 at 23:21, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Paul Elder (2021-12-21 05:10:22) > > Remove the NoiseReductionMode values that the raspberrypi IPA does not > > support. The ControlInfo values that the IPA reports will be used for > > capability detection, so values that it does not support shall be > > removed. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > No change in v2: > > - constexpr did not work > > --- > > include/libcamera/ipa/raspberrypi.h | 8 +++++++- > > src/ipa/raspberrypi/raspberrypi.cpp | 2 -- > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > > index 548bfba0..593139c5 100644 > > --- a/include/libcamera/ipa/raspberrypi.h > > +++ b/include/libcamera/ipa/raspberrypi.h > > @@ -18,6 +18,12 @@ namespace libcamera { > > > > namespace RPi { > > > > +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = { > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > +}; Just one thing, didn't we support "NoiseReductionModeMinimal" and "NoiseReductionMode ZSL" too? (Though the former is more important to us.) So I'm thinking those need to be in the list here as well. Best regards David > > + > > /* > > * List of controls handled by the Raspberry Pi IPA > > * > > @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({ > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > > - { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } > > + { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) } > > Aha, I didn't realise we could liimit the exposed values from controls > enums like this. > > I'll have to keep an eye on how things check to ensure the control value > is supported, but it makes sense that it should only report the modes it > supports. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > }, controls::controls); > > > > } /* namespace RPi */ > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 3f497be1..e0685c69 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { > > { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, > > { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, > > { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, > > - { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff }, > > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > > }; > > > > void IPARPi::queueRequest(const ControlList &controls) > > -- > > 2.27.0 > >
Quoting David Plowman (2022-01-04 08:27:49) > Hi Paul, Kieran > > Thank you for this patch! > > On Mon, 3 Jan 2022 at 23:21, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Paul Elder (2021-12-21 05:10:22) > > > Remove the NoiseReductionMode values that the raspberrypi IPA does not > > > support. The ControlInfo values that the IPA reports will be used for > > > capability detection, so values that it does not support shall be > > > removed. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > No change in v2: > > > - constexpr did not work > > > --- > > > include/libcamera/ipa/raspberrypi.h | 8 +++++++- > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 -- > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > > > index 548bfba0..593139c5 100644 > > > --- a/include/libcamera/ipa/raspberrypi.h > > > +++ b/include/libcamera/ipa/raspberrypi.h > > > @@ -18,6 +18,12 @@ namespace libcamera { > > > > > > namespace RPi { > > > > > > +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = { > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > > +}; > > Just one thing, didn't we support "NoiseReductionModeMinimal" and Is 'Minimal' different to 'Fast' somehow? > "NoiseReductionMode ZSL" too? (Though the former is more important to > us.) So I'm thinking those need to be in the list here as well. In this series, ZSL is defined as being used when reprocessing, so it will only apply NoiseReduction on low resolution streams (view finders) assuming that a RAW stream will also be provided which will be de-noised on only later processed frames. As I understand it, this patch is directly removing the listing that ZSL is available, and listing direct matches of what is exposed by the RPi IPA modes? > > Best regards > David > > > > + > > > /* > > > * List of controls handled by the Raspberry Pi IPA > > > * > > > @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({ > > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > > > - { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } > > > + { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) } > > > > Aha, I didn't realise we could liimit the exposed values from controls > > enums like this. > > > > I'll have to keep an eye on how things check to ensure the control value > > is supported, but it makes sense that it should only report the modes it > > supports. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > }, controls::controls); > > > > > > } /* namespace RPi */ > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 3f497be1..e0685c69 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { > > > { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, > > > { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, > > > { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, > > > - { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff }, > > > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > > > }; > > > > > > void IPARPi::queueRequest(const ControlList &controls) > > > -- > > > 2.27.0 > > >
HI Kieran Thanks for the reply. On Tue, 4 Jan 2022 at 10:33, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman (2022-01-04 08:27:49) > > Hi Paul, Kieran > > > > Thank you for this patch! > > > > On Mon, 3 Jan 2022 at 23:21, Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Paul Elder (2021-12-21 05:10:22) > > > > Remove the NoiseReductionMode values that the raspberrypi IPA does not > > > > support. The ControlInfo values that the IPA reports will be used for > > > > capability detection, so values that it does not support shall be > > > > removed. > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > --- > > > > No change in v2: > > > > - constexpr did not work > > > > --- > > > > include/libcamera/ipa/raspberrypi.h | 8 +++++++- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 -- > > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > > > > index 548bfba0..593139c5 100644 > > > > --- a/include/libcamera/ipa/raspberrypi.h > > > > +++ b/include/libcamera/ipa/raspberrypi.h > > > > @@ -18,6 +18,12 @@ namespace libcamera { > > > > > > > > namespace RPi { > > > > > > > > +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = { > > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > > > +}; > > > > Just one thing, didn't we support "NoiseReductionModeMinimal" and > > Is 'Minimal' different to 'Fast' somehow? Yes, in Raspberry PI world "fast" means to use the "fast software colour denoise algorithm" whereas "minimal" means "no software colour denoise at all". (There's also a "high quality" but slower version of software colour denoise.) I think we need to keep "minimal" as an option, but as you say, ZSL doesn't bother us for the moment. Thanks David > > > "NoiseReductionMode ZSL" too? (Though the former is more important to > > us.) So I'm thinking those need to be in the list here as well. > > In this series, ZSL is defined as being used when reprocessing, so it > will only apply NoiseReduction on low resolution streams (view finders) > assuming that a RAW stream will also be provided which will be de-noised > on only later processed frames. > > As I understand it, this patch is directly removing the listing that ZSL > is available, and listing direct matches of what is exposed by the RPi > IPA modes? > > > > > > Best regards > > David > > > > > > + > > > > /* > > > > * List of controls handled by the Raspberry Pi IPA > > > > * > > > > @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({ > > > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > > > > - { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } > > > > + { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) } > > > > > > Aha, I didn't realise we could liimit the exposed values from controls > > > enums like this. > > > > > > I'll have to keep an eye on how things check to ensure the control value > > > is supported, but it makes sense that it should only report the modes it > > > supports. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > }, controls::controls); > > > > > > > > } /* namespace RPi */ > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 3f497be1..e0685c69 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { > > > > { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, > > > > { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, > > > > { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, > > > > - { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff }, > > > > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > > > > }; > > > > > > > > void IPARPi::queueRequest(const ControlList &controls) > > > > -- > > > > 2.27.0 > > > >
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h index 548bfba0..593139c5 100644 --- a/include/libcamera/ipa/raspberrypi.h +++ b/include/libcamera/ipa/raspberrypi.h @@ -18,6 +18,12 @@ namespace libcamera { namespace RPi { +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = { + static_cast<int32_t>(controls::NoiseReductionModeOff), + static_cast<int32_t>(controls::NoiseReductionModeFast), + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), +}; + /* * List of controls handled by the Raspberry Pi IPA * @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({ { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, - { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } + { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) } }, controls::controls); } /* namespace RPi */ diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 3f497be1..e0685c69 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, - { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff }, - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, }; void IPARPi::queueRequest(const ControlList &controls)
Remove the NoiseReductionMode values that the raspberrypi IPA does not support. The ControlInfo values that the IPA reports will be used for capability detection, so values that it does not support shall be removed. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- No change in v2: - constexpr did not work --- include/libcamera/ipa/raspberrypi.h | 8 +++++++- src/ipa/raspberrypi/raspberrypi.cpp | 2 -- 2 files changed, 7 insertions(+), 3 deletions(-)