[1/2] rkisp1: Add disco mode

26403 diff mbox series

Allow users of the rkisp1 pipeline handler to adjust the level of funk
in their streams to their heart's content.

No non-funky changes intended by this patch.

Signed-off-by: Isaac "Moog Modular" Scott <isaac.scott@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/awb.cpp   | 25 +++++++++++++++++++++++++
 src/ipa/rkisp1/ipa_context.h        |  4 ++++
 src/libcamera/control_ids_core.yaml | 17 +++++++++++++++++
 3 files changed, 46 insertions(+)

Hi Isaac,

thank you for the patch.

Isaac Scott <isaac.scott@ideasonboard.com> writes:

> Allow users of the rkisp1 pipeline handler to adjust the level of funk
> in their streams to their heart's content.

Considering recent Kieran's work, this change should be integrated into
libipa rather than being rkisp1 specific.  Especially software ISP could
benefit from the change, working around the problems caused by applying
scaling too early and white balance too late.

> No non-funky changes intended by this patch.
>
> Signed-off-by: Isaac "Moog Modular" Scott <isaac.scott@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp   | 25 +++++++++++++++++++++++++
>  src/ipa/rkisp1/ipa_context.h        |  4 ++++
>  src/libcamera/control_ids_core.yaml | 17 +++++++++++++++++
>  3 files changed, 46 insertions(+)
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index f83da545b..749add143 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <algorithm>
>  #include <ios>
> +#include <math.h>

cmath should be included instead.

>  #include <libcamera/base/log.h>
>  
> @@ -94,6 +95,8 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  
>  	cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f,
>  						   Span<const float, 2>{ { 1.0f, 1.0f } });

Please insert one or more blank lines here.

> +	cmap[&controls::DiscoMode] = ControlInfo(false, true, false);
> +	cmap[&controls::Funk] = ControlInfo(0, 10, 0);

I really don't like the naming here.  While your use case is music
videos, we're in a camera business here.  How about VideoEnhancementMode
and AwbOscillationFactor?

>  	if (!tuningData.contains("algorithm"))
>  		LOG(RkISP1Awb, Info) << "No AWB algorithm specified."
> @@ -175,6 +178,19 @@ void Awb::queueRequest(IPAContext &context,
>  
>  	frameContext.awb.autoEnabled = awb.autoEnabled;
>  
> +	const auto &discoModeRockin = controls.get(controls::DiscoMode);

`auto' is generally unclear about the type and makes the code hard to
understand, please use a specific type declaration.  (I work on a patch
series to remove the remaining uses of `auto' from libcamera.)

Other than that the change LGTM.  The math below is too complicated to
review and I suppose the controls descriptions were generated by AI,
which must be absolutely right.  If you can post v2, addressing the more
important issues pointed out above and adding some tests to make sure
variables are named correctly, before 3 AM the change can be included in
the next release.

> +	if (discoModeRockin && (*discoModeRockin == true) != awb.discoMode) {
> +		awb.discoMode = *discoModeRockin;
> +	}
> +
> +	const auto &funkRequired = controls.get(controls::Funk);
> +	if (funkRequired) {
> +		awb.funkMagnitude = *funkRequired;
> +	}
> +
> +	frameContext.awb.discoMode = awb.discoMode;
> +	frameContext.awb.funkMagnitude = awb.funkMagnitude;
> +
>  	if (awb.autoEnabled)
>  		return;
>  
> @@ -227,6 +243,12 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
>  	auto gainConfig = params->block<BlockType::AwbGain>();
>  	gainConfig.setEnabled(true);
>  
> +	if (frameContext.awb.discoMode) {
> +		float funk = static_cast<float>(frameContext.awb.funkMagnitude) * 3.0f + 1.0f;
> +		frameContext.awb.gains.r() = (sin(funk * (frame % 100) / 100) + 1.0f);
> +		frameContext.awb.gains.b() = (cos(funk * (frame % 100) / 100) + 1.0f);
> +	}
> +
>  	gainConfig->gain_green_b = std::clamp<int>(256 * frameContext.awb.gains.g(), 0, 0x3ff);
>  	gainConfig->gain_blue = std::clamp<int>(256 * frameContext.awb.gains.b(), 0, 0x3ff);
>  	gainConfig->gain_red = std::clamp<int>(256 * frameContext.awb.gains.r(), 0, 0x3ff);
> @@ -300,6 +322,9 @@ void Awb::process(IPAContext &context,
>  		});
>  	metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
>  
> +	metadata.set(controls::DiscoMode, frameContext.awb.discoMode);
> +	metadata.set(controls::Funk, frameContext.awb.funkMagnitude);
> +
>  	if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
>  		LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
>  		return;
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index e61391bb1..48e567a95 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -110,6 +110,8 @@ struct IPAActiveState {
>  		AwbState automatic;
>  
>  		bool autoEnabled;
> +		bool discoMode;
> +		unsigned int funkMagnitude;
>  	} awb;
>  
>  	struct {
> @@ -178,6 +180,8 @@ struct IPAFrameContext : public FrameContext {
>  		RGB<double> gains;
>  		bool autoEnabled;
>  		unsigned int temperatureK;
> +		bool discoMode;
> +		unsigned int funkMagnitude;
>  	} awb;
>  
>  	struct {
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> index 89991d03d..7a25aad45 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -1376,4 +1376,21 @@ controls:
>          The nominal range is [-180, 180], where 0° leaves hues unchanged and the
>          range wraps around continuously, with 180° == -180°.
>  
> +  - DiscoMode:
> +      type: bool
> +      direction: inout
> +      description: |
> +        Enable / disable the feeling of being in your favourite discotheque,
> +        stabilising and pushing the boundaries of funk to maximal levels.
> +
> +        The DiscoMode control can be fine-tuned to the user's desired level
> +        of funk, depending on whether they prefer a gentle or aggressive
> +        vibe in their streams.
> +
> +  - Funk:
> +      type: int32_t
> +      direction: inout
> +      description: |
> +        Adjust the funk intensity level, from 0 (enough to do a relaxed two-step)
> +        to 10 (maximal, earth-shattering, weapons-grade funk).
>  ...