[RFC,v7,4/6] libcamera: shaders: Add LSC support
diff mbox series

Message ID 20260708201816.299983-5-mzamazal@redhat.com
State Superseded
Headers show
Series
  • LSC for SoftISP simple pipeline
Related show

Commit Message

Milan Zamazal July 8, 2026, 8:18 p.m. UTC 
From: Xander Pronk <xander.c.pronk@gmail.com>

Lens shading correction should be applied after black level
subtraction (in order to make the computations with meaningful values)
and before white balance (especially before white balance stats are
computed).

Note that lens shading correction depends on temperature, which is
computed from the preceding, rather than current, frame (this is due to
how white balance is currently computed).

The shaders are compiled on initialisation, while lens shading
correction can be enabled or disabled dynamically, using the
corresponding control.  The whole correction in the shader is wrapped by
a conditional macro to be able to disable it and all the related
overheads if Lsc algorithm is not enabled at all.  The correction
computation itself is conditional based on a flag that can be changed
for each processed image.

Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info>
Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info>
Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/shaders/bayer_1x_packed.frag | 10 ++++++++++
 src/libcamera/shaders/bayer_unpacked.frag  | 10 ++++++++++
 2 files changed, 20 insertions(+)

Comments

Bryan O'Donoghue July 15, 2026, 12:35 a.m. UTC | #1 
On 08/07/2026 21:18, Milan Zamazal wrote:
> +#if defined(APPLY_LSC)
> +        if (lsc_enabled)
> +                rgb = rgb * texture2D(lsc_tex, textureOut).rgb;
> +#endif

You definitely don't need the define and the bool.

I'd stick to the define - the CPU side can decide whether or not to pass 
the define based on the bool.

The additional uniform is redundant.

---
bod
Bryan O'Donoghue July 15, 2026, 12:38 a.m. UTC | #2 
On 15/07/2026 01:35, Bryan O'Donoghue wrote:
> On 08/07/2026 21:18, Milan Zamazal wrote:
>> +#if defined(APPLY_LSC)
>> +        if (lsc_enabled)
>> +                rgb = rgb * texture2D(lsc_tex, textureOut).rgb;
>> +#endif
> 
> You definitely don't need the define and the bool.
> 
> I'd stick to the define - the CPU side can decide whether or not to pass
> the define based on the bool.
> 
> The additional uniform is redundant.
> 
> ---
> bod

Unless there's a real/realistic use-case of having a control switch LSC 
on/off during runtime.

In which case - just the bool should be sufficient - again on the CPU 
side you would just not bother updating or creating the LSC texture if 
the bool is false.

I don't see that both are needed even for opportunistic runtime control 
toggling. The bool on its own should be enough.

---
bod*2
Milan Zamazal July 15, 2026, 8:55 a.m. UTC | #3 
Bryan O'Donoghue <bod.linux@nxsw.ie> writes:

> On 15/07/2026 01:35, Bryan O'Donoghue wrote:
>> On 08/07/2026 21:18, Milan Zamazal wrote:
>>> +#if defined(APPLY_LSC)
>
>>> +        if (lsc_enabled)
>>> +                rgb = rgb * texture2D(lsc_tex, textureOut).rgb;
>>> +#endif
>> You definitely don't need the define and the bool.
>> I'd stick to the define - the CPU side can decide whether or not to pass
>> the define based on the bool.
>> The additional uniform is redundant.
>> ---
>> bod
>
> Unless there's a real/realistic use-case of having a control switch LSC on/off during runtime.

lsc_enabled is a runtime switch; APPLY_LSC depends on whether LSC is
enabled in the tuning file.

> In which case - just the bool should be sufficient - again on the CPU side you would just not bother
> updating or creating the LSC texture if the bool is false.

Avoiding the texture was the idea behind keeping APPLY_LSC.  If it is
possible and safe not to set the texture (it seems to work fine in my
environment) then APPLY_LSC can be removed.

> I don't see that both are needed even for opportunistic runtime control toggling. The bool on its own
> should be enough.
>
> ---
> bod*2
Milan Zamazal July 15, 2026, 3:39 p.m. UTC | #4 
Milan Zamazal <mzamazal@redhat.com> writes:

> Bryan O'Donoghue <bod.linux@nxsw.ie> writes:
>
>> On 15/07/2026 01:35, Bryan O'Donoghue wrote:
>>> On 08/07/2026 21:18, Milan Zamazal wrote:
>>>> +#if defined(APPLY_LSC)
>>
>>>> +        if (lsc_enabled)
>>>> +                rgb = rgb * texture2D(lsc_tex, textureOut).rgb;
>>>> +#endif
>>> You definitely don't need the define and the bool.
>>> I'd stick to the define - the CPU side can decide whether or not to pass
>>> the define based on the bool.
>>> The additional uniform is redundant.
>>> ---
>>> bod
>>
>> Unless there's a real/realistic use-case of having a control switch LSC on/off during runtime.
>
> lsc_enabled is a runtime switch; APPLY_LSC depends on whether LSC is
> enabled in the tuning file.
>
>> In which case - just the bool should be sufficient - again on the CPU side you would just not bother
>> updating or creating the LSC texture if the bool is false.
>
> Avoiding the texture was the idea behind keeping APPLY_LSC.  If it is
> possible and safe not to set the texture (it seems to work fine in my
> environment) then APPLY_LSC can be removed.

For the track, as discussed at the meeting today: An agreement was to
keep APPLY_LSC and to remove lsc_enabled.  The rationale is that if LSC
is enabled in the tuning file, there is hardly any reason to switch it
off in runtime, except for demoing purposes or so.  Then the performance
penalty caused by still performing LSC, with all 1.0 coefficients,
doesn't matter.  Having APPLY_LSC is still useful, to avoid the
performance penalty and all the LSC texture handling if LSC is not
enabled at all.

Out of curiosity, I measured how much overhead I get by the shader
conditional and the LSC computation there in my environment:

- lsc_enabled set to false: ~5% slower than with the computation removed
  completely from the shader.

- lsc_enabled set to true: ~15% slower than with the computation removed
  completely from the shader.

- The condition removed and performing the computation unconditionally:
  additionally ~15% slower than the case with lsc_enabled set to true,
  while the output is the same!  Very weird.  An LLM suggests register
  pressure resulting in fewer concurrent threads, compared to the case
  where the GPU must consider branching; maybe.

Things can be different once rebased on multi-pass GPU, but I'm not
going to do that until libipa rework is merged.

>> I don't see that both are needed even for opportunistic runtime control toggling. The bool on its own
>> should be enough.
>>
>> ---
>> bod*2

Patch
diff mbox series 

diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag
index 0b641a5f3..33f5c1fde 100644
--- a/src/libcamera/shaders/bayer_1x_packed.frag
+++ b/src/libcamera/shaders/bayer_1x_packed.frag
@@ -71,6 +71,11 @@  uniform vec3 blacklevel;
 uniform float gamma;
 uniform float contrastExp;
 
+#if defined(APPLY_LSC)
+uniform bool lsc_enabled;
+uniform sampler2D lsc_tex;
+#endif
+
 float apply_contrast(float value)
 {
 	// Apply simple S-curve
@@ -232,6 +237,11 @@  void main(void)
 	 */
 	rgb = (rgb - blacklevel) / (1.0 - blacklevel);
 
+#if defined(APPLY_LSC)
+        if (lsc_enabled)
+                rgb = rgb * texture2D(lsc_tex, textureOut).rgb;
+#endif
+
 	/* Apply AWB gains, and saturate each channel at sensor range */
 	rgb = clamp(rgb * awb, vec3(0.0), vec3(1.0));
 
diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag
index 10c5e941b..64649777e 100644
--- a/src/libcamera/shaders/bayer_unpacked.frag
+++ b/src/libcamera/shaders/bayer_unpacked.frag
@@ -30,6 +30,11 @@  uniform vec3            blacklevel;
 uniform float           gamma;
 uniform float           contrastExp;
 
+#if defined(APPLY_LSC)
+uniform bool lsc_enabled;
+uniform sampler2D lsc_tex;
+#endif
+
 float apply_contrast(float value)
 {
     // Apply simple S-curve
@@ -135,6 +140,11 @@  void main(void) {
      */
     rgb = (rgb - blacklevel) / (1.0 - blacklevel);
 
+#if defined(APPLY_LSC)
+    if (lsc_enabled)
+        rgb = rgb * texture2D(lsc_tex, center.xy).rgb;
+#endif
+
     /* Apply AWB gains, and saturate each channel at sensor range */
     rgb = clamp(rgb * awb, vec3(0.0), vec3(1.0));