[libcamera-devel] [RFC PATCH 1/2] ipa: rkisp1: awb: add support for RGB means

[Laurent Pinchart]

Hi Quention,

On Mon, Jul 18, 2022 at 10:14:54AM +0200, Quentin Schulz wrote:
> On 7/17/22 23:50, Laurent Pinchart via libcamera-devel wrote:
> > On Thu, May 12, 2022 at 10:42:43AM +0200, Quentin Schulz via libcamera-devel wrote:
> >> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
> >>
> >> RkISP actually supports two modes for color means, RGB and YCbCr. The
> >> variables where the means are stored are identically named regardless of
> >> the color means mode that's been selected.
> >>
> >> Since the gains are computed in RGB mode, a conversion needs to be done
> >> when the mode is YCbCr, which is unnecessary when RGB mode is selected.
> >>
> >> This adds support for RGB means mode too, by checking at runtime which
> >> mode is selected at a given time.
> >>
> >> Cc: Quentin Schulz <foss+libcamera at 0leil.net>
> >> Signed-off-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>
> >> ---
> >>
> >> To be decided if we want to keep supporting both modes or only one?
> >> (Given that at the moment we harcode which mode is used at compile time,
> >>   I'm not sure it's worth keeping dead code around)
> > 
> > I'm finally testing this, and this patch doesn't compile. params has no
> > meas member.
> > 
> 
> Yeah, I think someone reported it on #libcamera a few days after I 
> posted it.
> 
> I got confused by the same variable name not being of the same type and 
> tried to use it the same way.
> 
> I tested it back then by completely swapping YCbCr to RGB means without 
> the check in here against meas_config, just assuming it's RGB (selected 
> in patch 2/2). I clearly didn't compile it before sending the RFC after 
> trying to do "smart" things instead of just sending what I had tested, 
> sorry about that.
> 
> We could create a new variable in the awb algorithm to hold the color 
> mode used to get mean values for the AWB algorithm by the rkisp? Or 
> something like that.

That sounds good. I don't expect the mode to change during a capture
session, or possibly even at all at runtime, so storing it in the Awb
class will work fine.

> The plan for this RFC was more about checking if we 
> still have an issue with RGB AWB not working on rkisp1 (at least rk3399, 
> for some people?) and if so, maybe better document it and offer the 
> ability to support both YCbCr and RGB means for AWB (e.g. have PX30 
> rkisp1 v1.2 use RGB and RK3399 rkisp1 v1.0 use YCbCr?). If it works 
> fine, then maybe we can remove the translation from YCbCr to RGB and 
> simplify a bit the logic (and make it more precise too I assume, but 
> considering it's still a bit on the green side, I'm not sure that is a 
> valid argument :) ).

I've finally managed to test RGB mode on RK3399, and it works as well
(or as badly :-)) as YCbCr mode. That is, once fixing a kernel bug. It
turns out that the R, G and B maximum threshold values are not set by
the driver. They're stored in the same register fields as the YCbCr
parameters, so it works more or less by chance, but it's not right. I'll
send patches.

This leads me to believe that the closed-source camera stack from
Rockchip uses YCbCr mode, at least on Chrome OS. I think we should keep
that as an option, as it gives more control over which pixels are
selected by the accumulator. I however haven't been able to figure out
what the Cb and Cr reference values are for. Does anyone know ?

-- 
Regards,

Laurent Pinchart