| Message ID | 20210913102007.2303225-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Mon, Sep 13, 2021 at 07:20:06PM +0900, Paul Elder wrote: > 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> > --- > 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 e0dc6f5e..6e97ef53 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 = { Can this be constexpr ? > + static_cast<int32_t>(controls::NoiseReductionModeOff), > + static_cast<int32_t>(controls::NoiseReductionModeFast), > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > +}; I'm wondering, now that we have the ability to pass control info from the IPA to the pipeline handler, could we move the control info map to the IPA before adding more static data ? Naush, is this something you have looked at by any chance ? > + > /* > * List of controls handled by the Raspberry Pi IPA > * > @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -608,8 +608,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::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff }, > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > }; > > void IPARPi::queueRequest(const ControlList &controls)
Hi Laurent, On Wed, Sep 15, 2021 at 04:48:39AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Mon, Sep 13, 2021 at 07:20:06PM +0900, Paul Elder wrote: > > 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> > > --- > > 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 e0dc6f5e..6e97ef53 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 = { > > Can this be constexpr ? Yeah. > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > +}; > > I'm wondering, now that we have the ability to pass control info from > the IPA to the pipeline handler, could we move the control info map to > the IPA before adding more static data ? That's probably a better idea. I think it should be done before this then. Paul > > Naush, is this something you have looked at by any chance ? > > > + > > /* > > * List of controls handled by the Raspberry Pi IPA > > * > > @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -608,8 +608,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::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff }, > > - { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, > > }; > > > > void IPARPi::queueRequest(const ControlList &controls)
Hi Laurent, On Wed, 15 Sept 2021 at 02:49, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Paul, > > Thank you for the patch. > > On Mon, Sep 13, 2021 at 07:20:06PM +0900, Paul Elder wrote: > > 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> > > --- > > 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 e0dc6f5e..6e97ef53 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 = { > > Can this be constexpr ? > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > +}; > > I'm wondering, now that we have the ability to pass control info from > the IPA to the pipeline handler, could we move the control info map to > the IPA before adding more static data ? > > Naush, is this something you have looked at by any chance ? > I haven't looked into this, but it makes sense to do it this way. It would be almost trivial to pass the control info from ipa::init(). Thanks, Naush > > > + > > /* > > * List of controls handled by the Raspberry Pi IPA > > * > > @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -608,8 +608,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::NoiseReductionModeMinimal, > RPiController::DenoiseMode::ColourOff }, > > - { controls::NoiseReductionModeZSL, > RPiController::DenoiseMode::ColourHighQuality }, > > }; > > > > void IPARPi::queueRequest(const ControlList &controls) > > -- > Regards, > > Laurent Pinchart >
On Wed, Sep 15, 2021 at 08:53:50AM +0100, Naushir Patuck wrote: > On Wed, 15 Sept 2021 at 02:49, Laurent Pinchart wrote: > > On Mon, Sep 13, 2021 at 07:20:06PM +0900, Paul Elder wrote: > > > 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> > > > --- > > > 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 e0dc6f5e..6e97ef53 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 = { > > > > Can this be constexpr ? > > > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > > +}; > > > > I'm wondering, now that we have the ability to pass control info from > > the IPA to the pipeline handler, could we move the control info map to > > the IPA before adding more static data ? > > > > Naush, is this something you have looked at by any chance ? > > I haven't looked into this, but it makes sense to do it this way. > It would be almost trivial to pass the control info from ipa::init(). Sounds good to me. Let's avoid Paul and you implementing this in parallel, is this something you plan to work on ? > > > + > > > /* > > > * List of controls handled by the Raspberry Pi IPA > > > * > > > @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -608,8 +608,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::NoiseReductionModeMinimal, > > RPiController::DenoiseMode::ColourOff }, > > > - { controls::NoiseReductionModeZSL, > > RPiController::DenoiseMode::ColourHighQuality }, > > > }; > > > > > > void IPARPi::queueRequest(const ControlList &controls)
On Wed, 15 Sept 2021 at 09:29, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Wed, Sep 15, 2021 at 08:53:50AM +0100, Naushir Patuck wrote: > > On Wed, 15 Sept 2021 at 02:49, Laurent Pinchart wrote: > > > On Mon, Sep 13, 2021 at 07:20:06PM +0900, Paul Elder wrote: > > > > 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> > > > > --- > > > > 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 e0dc6f5e..6e97ef53 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 > = { > > > > > > Can this be constexpr ? > > > > > > > + static_cast<int32_t>(controls::NoiseReductionModeOff), > > > > + static_cast<int32_t>(controls::NoiseReductionModeFast), > > > > + static_cast<int32_t>(controls::NoiseReductionModeHighQuality), > > > > +}; > > > > > > I'm wondering, now that we have the ability to pass control info from > > > the IPA to the pipeline handler, could we move the control info map to > > > the IPA before adding more static data ? > > > > > > Naush, is this something you have looked at by any chance ? > > > > I haven't looked into this, but it makes sense to do it this way. > > It would be almost trivial to pass the control info from ipa::init(). > > Sounds good to me. Let's avoid Paul and you implementing this in > parallel, is this something you plan to work on ? > I could do the change (hopefully) by the end of the week. Regards, Naush > > > > > + > > > > /* > > > > * List of controls handled by the Raspberry Pi IPA > > > > * > > > > @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -608,8 +608,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::NoiseReductionModeMinimal, > > > RPiController::DenoiseMode::ColourOff }, > > > > - { controls::NoiseReductionModeZSL, > > > RPiController::DenoiseMode::ColourHighQuality }, > > > > }; > > > > > > > > void IPARPi::queueRequest(const ControlList &controls) > > -- > Regards, > > Laurent Pinchart >
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h index e0dc6f5e..6e97ef53 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 * @@ -45,7 +51,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 8d44ab0a..daef1c2d 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -608,8 +608,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::NoiseReductionModeMinimal, 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> --- include/libcamera/ipa/raspberrypi.h | 8 +++++++- src/ipa/raspberrypi/raspberrypi.cpp | 2 -- 2 files changed, 7 insertions(+), 3 deletions(-)