| Message ID | 20220512084244.1833554-1-foss+libcamera@0leil.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Quentin, Thank you for the patch. On Thu, May 12, 2022 at 10:42:43AM +0200, Quentin Schulz via libcamera-devel wrote: > From: Quentin Schulz <quentin.schulz@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@0leil.net> > Signed-off-by: Quentin Schulz <quentin.schulz@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'd like to test this on RK3399 and on i.MX8MP (I assume you've tested the PX30). If all tests pass, then I think we can just drop YUV support completely. Jean-Michel, I don't have RK3399 hardware with me, could you give this series a try ? > src/ipa/rkisp1/algorithms/awb.cpp | 57 ++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index be4585c6..df749b9b 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -124,30 +124,39 @@ void Awb::process([[maybe_unused]] IPAContext &context, const rkisp1_stat_buffer > const rkisp1_cif_isp_stat *params = &stats->params; > const rkisp1_cif_isp_awb_stat *awb = ¶ms->awb; > IPAFrameContext &frameContext = context.frameContext; > - > - /* Get the YCbCr mean values */ > - double yMean = awb->awb_mean[0].mean_y_or_g; > - double crMean = awb->awb_mean[0].mean_cr_or_r; > - double cbMean = awb->awb_mean[0].mean_cb_or_b; > - > - /* > - * Convert from YCbCr to RGB. > - * The hardware uses the following formulas: > - * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B > - * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B > - * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B > - * > - * The inverse matrix is thus: > - * [[1,1636, -0,0623, 1,6008] > - * [1,1636, -0,4045, -0,7949] > - * [1,1636, 1,9912, -0,0250]] > - */ > - yMean -= 16; > - cbMean -= 128; > - crMean -= 128; > - double redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > - double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > - double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + double greenMean; > + double redMean; > + double blueMean; > + > + if (params->meas.awb_meas_config.awb_mode == RKISP1_CIF_ISP_AWB_MODE_RGB) { > + greenMean = awb->awb_mean[0].mean_y_or_g; > + redMean = awb->awb_mean[0].mean_cr_or_r; > + blueMean = awb->awb_mean[0].mean_cb_or_b; > + } else { > + /* Get the YCbCr mean values */ > + double yMean = awb->awb_mean[0].mean_y_or_g; > + double crMean = awb->awb_mean[0].mean_cr_or_r; > + double cbMean = awb->awb_mean[0].mean_cb_or_b; > + > + /* > + * Convert from YCbCr to RGB. > + * The hardware uses the following formulas: > + * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B > + * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B > + * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B > + * > + * The inverse matrix is thus: > + * [[1,1636, -0,0623, 1,6008] > + * [1,1636, -0,4045, -0,7949] > + * [1,1636, 1,9912, -0,0250]] > + */ > + yMean -= 16; > + cbMean -= 128; > + crMean -= 128; > + redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > + greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > + blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + } > > /* Estimate the red and blue gains to apply in a grey world. */ > double redGain = greenMean / (redMean + 1);
Hi Quention, On Thu, May 12, 2022 at 10:42:43AM +0200, Quentin Schulz via libcamera-devel wrote: > From: Quentin Schulz <quentin.schulz@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@0leil.net> > Signed-off-by: Quentin Schulz <quentin.schulz@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. > src/ipa/rkisp1/algorithms/awb.cpp | 57 ++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index be4585c6..df749b9b 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -124,30 +124,39 @@ void Awb::process([[maybe_unused]] IPAContext &context, const rkisp1_stat_buffer > const rkisp1_cif_isp_stat *params = &stats->params; > const rkisp1_cif_isp_awb_stat *awb = ¶ms->awb; > IPAFrameContext &frameContext = context.frameContext; > - > - /* Get the YCbCr mean values */ > - double yMean = awb->awb_mean[0].mean_y_or_g; > - double crMean = awb->awb_mean[0].mean_cr_or_r; > - double cbMean = awb->awb_mean[0].mean_cb_or_b; > - > - /* > - * Convert from YCbCr to RGB. > - * The hardware uses the following formulas: > - * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B > - * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B > - * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B > - * > - * The inverse matrix is thus: > - * [[1,1636, -0,0623, 1,6008] > - * [1,1636, -0,4045, -0,7949] > - * [1,1636, 1,9912, -0,0250]] > - */ > - yMean -= 16; > - cbMean -= 128; > - crMean -= 128; > - double redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > - double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > - double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + double greenMean; > + double redMean; > + double blueMean; > + > + if (params->meas.awb_meas_config.awb_mode == RKISP1_CIF_ISP_AWB_MODE_RGB) { > + greenMean = awb->awb_mean[0].mean_y_or_g; > + redMean = awb->awb_mean[0].mean_cr_or_r; > + blueMean = awb->awb_mean[0].mean_cb_or_b; > + } else { > + /* Get the YCbCr mean values */ > + double yMean = awb->awb_mean[0].mean_y_or_g; > + double crMean = awb->awb_mean[0].mean_cr_or_r; > + double cbMean = awb->awb_mean[0].mean_cb_or_b; > + > + /* > + * Convert from YCbCr to RGB. > + * The hardware uses the following formulas: > + * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B > + * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B > + * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B > + * > + * The inverse matrix is thus: > + * [[1,1636, -0,0623, 1,6008] > + * [1,1636, -0,4045, -0,7949] > + * [1,1636, 1,9912, -0,0250]] > + */ > + yMean -= 16; > + cbMean -= 128; > + crMean -= 128; > + redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > + greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > + blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + } > > /* Estimate the red and blue gains to apply in a grey world. */ > double redGain = greenMean / (redMean + 1);
Hi Laurent, On 7/17/22 23:50, Laurent Pinchart via libcamera-devel wrote: > Hi Quention, > > On Thu, May 12, 2022 at 10:42:43AM +0200, Quentin Schulz via libcamera-devel wrote: >> From: Quentin Schulz <quentin.schulz@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@0leil.net> >> Signed-off-by: Quentin Schulz <quentin.schulz@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. 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 :) ). Cheers, Quentin
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@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@0leil.net> > >> Signed-off-by: Quentin Schulz <quentin.schulz@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 ?
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index be4585c6..df749b9b 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -124,30 +124,39 @@ void Awb::process([[maybe_unused]] IPAContext &context, const rkisp1_stat_buffer const rkisp1_cif_isp_stat *params = &stats->params; const rkisp1_cif_isp_awb_stat *awb = ¶ms->awb; IPAFrameContext &frameContext = context.frameContext; - - /* Get the YCbCr mean values */ - double yMean = awb->awb_mean[0].mean_y_or_g; - double crMean = awb->awb_mean[0].mean_cr_or_r; - double cbMean = awb->awb_mean[0].mean_cb_or_b; - - /* - * Convert from YCbCr to RGB. - * The hardware uses the following formulas: - * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B - * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B - * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B - * - * The inverse matrix is thus: - * [[1,1636, -0,0623, 1,6008] - * [1,1636, -0,4045, -0,7949] - * [1,1636, 1,9912, -0,0250]] - */ - yMean -= 16; - cbMean -= 128; - crMean -= 128; - double redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; - double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; - double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; + double greenMean; + double redMean; + double blueMean; + + if (params->meas.awb_meas_config.awb_mode == RKISP1_CIF_ISP_AWB_MODE_RGB) { + greenMean = awb->awb_mean[0].mean_y_or_g; + redMean = awb->awb_mean[0].mean_cr_or_r; + blueMean = awb->awb_mean[0].mean_cb_or_b; + } else { + /* Get the YCbCr mean values */ + double yMean = awb->awb_mean[0].mean_y_or_g; + double crMean = awb->awb_mean[0].mean_cr_or_r; + double cbMean = awb->awb_mean[0].mean_cb_or_b; + + /* + * Convert from YCbCr to RGB. + * The hardware uses the following formulas: + * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B + * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B + * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B + * + * The inverse matrix is thus: + * [[1,1636, -0,0623, 1,6008] + * [1,1636, -0,4045, -0,7949] + * [1,1636, 1,9912, -0,0250]] + */ + yMean -= 16; + cbMean -= 128; + crMean -= 128; + redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; + greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; + blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; + } /* Estimate the red and blue gains to apply in a grey world. */ double redGain = greenMean / (redMean + 1);