| Message ID | 20190209014748.10427-2-slongerbeam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices | expand |
Hi Philipp, On 2/11/19 1:58 AM, Philipp Zabel wrote: > On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: >> The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding >> coefficients, so rename them to indicate that. And add some comments >> to make clear these are BT.601 coefficients encoding between YUV limited >> range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity >> matrix, so rename to ic_csc_identity. No functional changes. >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> Changes in v2: >> - rename ic_csc_rgb2rgb matrix to ic_csc_identity. >> --- >> drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index 594c3cbc8291..3ef61f0b509b 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -183,11 +183,13 @@ struct ic_csc_params { >> }; >> >> /* >> + * BT.601 encoding from RGB full range to YUV limited range: >> + * >> * Y = R * .299 + G * .587 + B * .114; >> * U = R * -.169 + G * -.332 + B * .500 + 128.; >> * V = R * .500 + G * -.419 + B * -.0813 + 128.; > Hm, this is a conversion to full range BT.601. For limited range, the > matrix coefficients > > 0.2990 0.5870 0.1140 > -0.1687 -0.3313 0.5000 > 0.5000 -0.4187 -0.0813 > > should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively: > > Y = R * .2568 + G * .5041 + B * .0979 + 16; > U = R * -.1482 + G * -.2910 + B * .4392 + 128; > V = R * .4392 + G * -.3678 + B * -.0714 + 128; Looking more closely at these coefficients now, I see you are right, they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V range -0.5 to 0.5). Well, not even that -- the coefficients are not being scaled to the limited ranges, but the 0.5 offset (128) _is_ being added to U/V, but no offset for Y. So it is even more messed up. Your corrected coefficients and offsets look correct to me: Y coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to (240 - 16) / 255, and add the offsets for both Y and U/V. But what about this "SAT_MODE" field in the IC task parameter memory? According to the manual the hardware will automatically convert the written coefficients to the correct limited ranges. I see there is a "sat" field defined in the struct but is not being set in the tables. So what should we do, define the full range coefficients, and make use of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and not use SAT_MODE? I'm inclined to do the former. Steve > >> */ >> -static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { >> .coeff = { >> { 77, 150, 29 }, >> { 469, 427, 128 }, >> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> .scale = 1, >> }; >> >> -/* transparent RGB->RGB matrix for graphics combining */ >> -static const struct ic_csc_params ic_csc_rgb2rgb = { >> +/* >> + * identity matrix, used for transparent RGB->RGB graphics >> + * combining. >> + */ >> +static const struct ic_csc_params ic_csc_identity = { >> .coeff = { >> { 128, 0, 0 }, >> { 0, 128, 0 }, >> @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { >> }; >> >> /* >> + * Inverse BT.601 encoding from YUV limited range to RGB full range: >> + * >> * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128)); >> * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128)); >> * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128); >> */ > This looks correct. > >> -static const struct ic_csc_params ic_csc_ycbcr2rgb = { >> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { >> .coeff = { >> { 149, 0, 204 }, >> { 149, 462, 408 }, >> @@ -238,11 +245,11 @@ static int init_csc(struct ipu_ic *ic, >> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); >> >> if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_ycbcr2rgb; >> + params = &ic_csc_ycbcr2rgb_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) >> - params = &ic_csc_rgb2ycbcr; >> + params = &ic_csc_rgb2ycbcr_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_rgb2rgb; >> + params = &ic_csc_identity; >> else { >> dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); >> return -EINVAL; > regards > Philipp
On 2/11/19 1:58 AM, Philipp Zabel wrote: > On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote: >> The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding >> coefficients, so rename them to indicate that. And add some comments >> to make clear these are BT.601 coefficients encoding between YUV limited >> range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity >> matrix, so rename to ic_csc_identity. No functional changes. >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> Changes in v2: >> - rename ic_csc_rgb2rgb matrix to ic_csc_identity. >> --- >> drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c >> index 594c3cbc8291..3ef61f0b509b 100644 >> --- a/drivers/gpu/ipu-v3/ipu-ic.c >> +++ b/drivers/gpu/ipu-v3/ipu-ic.c >> @@ -183,11 +183,13 @@ struct ic_csc_params { >> }; >> >> /* >> + * BT.601 encoding from RGB full range to YUV limited range: >> + * >> * Y = R * .299 + G * .587 + B * .114; >> * U = R * -.169 + G * -.332 + B * .500 + 128.; >> * V = R * .500 + G * -.419 + B * -.0813 + 128.; > Hm, this is a conversion to full range BT.601. For limited range, the > matrix coefficients > > 0.2990 0.5870 0.1140 > -0.1687 -0.3313 0.5000 > 0.5000 -0.4187 -0.0813 > > should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively: > > Y = R * .2568 + G * .5041 + B * .0979 + 16; > U = R * -.1482 + G * -.2910 + B * .4392 + 128; > V = R * .4392 + G * -.3678 + B * -.0714 + 128; > >> */ >> -static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { >> .coeff = { >> { 77, 150, 29 }, >> { 469, 427, 128 }, >> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = { >> .scale = 1, >> }; >> >> -/* transparent RGB->RGB matrix for graphics combining */ >> -static const struct ic_csc_params ic_csc_rgb2rgb = { >> +/* >> + * identity matrix, used for transparent RGB->RGB graphics >> + * combining. >> + */ >> +static const struct ic_csc_params ic_csc_identity = { >> .coeff = { >> { 128, 0, 0 }, >> { 0, 128, 0 }, >> @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { >> }; >> >> /* >> + * Inverse BT.601 encoding from YUV limited range to RGB full range: >> + * >> * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128)); >> * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128)); >> * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128); >> */ > This looks correct. Agreed, the coefficients in the comments are correct, but the actual table values were a bit off. I will correct them for v5 (Green offset should be 272 in the table, not 266). Steve > >> -static const struct ic_csc_params ic_csc_ycbcr2rgb = { >> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { >> .coeff = { >> { 149, 0, 204 }, >> { 149, 462, 408 }, >> @@ -238,11 +245,11 @@ static int init_csc(struct ipu_ic *ic, >> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); >> >> if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_ycbcr2rgb; >> + params = &ic_csc_ycbcr2rgb_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) >> - params = &ic_csc_rgb2ycbcr; >> + params = &ic_csc_rgb2ycbcr_bt601; >> else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) >> - params = &ic_csc_rgb2rgb; >> + params = &ic_csc_identity; >> else { >> dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); >> return -EINVAL; > regards > Philipp
Hi Steve, On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote: [...] > Looking more closely at these coefficients now, I see you are right, > they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V > range -0.5 to 0.5). Well, not even that -- the coefficients are not > being scaled to the limited ranges, but the 0.5 offset (128) _is_ being > added to U/V, but no offset for Y. So it is even more messed up. > > Your corrected coefficients and offsets look correct to me: Y > coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to > (240 - 16) / 255, and add the offsets for both Y and U/V. > > But what about this "SAT_MODE" field in the IC task parameter memory? That just controls the saturation. The result after the matrix multiplication is either saturated to [0..255] or to [16..235]/[16..240] when converting from the internal representation to the 8 bit output. > According to the manual the hardware will automatically convert the > written coefficients to the correct limited ranges. Where did you get that from? "The final calculation result is limited according to the SAT_MODE parameter and rounded to 8 bits." I see no mention of coefficients being modified. > I see there is a "sat" field defined in the struct but is not being > set in the tables. > > So what should we do, define the full range coefficients, and make use > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and > not use SAT_MODE? I'm inclined to do the former. SAT_MODE should be set for conversions to YUV limited range so that the coefficients can be rounded to the closest value. Otherwise we'd have to round towards zero, possibly with a larger error, to make sure the results are inside the valid ranges. regards Philipp
On 2/12/19 2:17 AM, Philipp Zabel wrote: > Hi Steve, > > On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote: > [...] >> Looking more closely at these coefficients now, I see you are right, >> they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V >> range -0.5 to 0.5). Well, not even that -- the coefficients are not >> being scaled to the limited ranges, but the 0.5 offset (128) _is_ being >> added to U/V, but no offset for Y. So it is even more messed up. >> >> Your corrected coefficients and offsets look correct to me: Y >> coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to >> (240 - 16) / 255, and add the offsets for both Y and U/V. >> >> But what about this "SAT_MODE" field in the IC task parameter memory? > That just controls the saturation. The result after the matrix > multiplication is either saturated to [0..255] or to [16..235]/[16..240] > when converting from the internal representation to the 8 bit output. By saturation I think you mean clipped to those ranges? > >> According to the manual the hardware will automatically convert the >> written coefficients to the correct limited ranges. > Where did you get that from? "The final calculation result is limited > according to the SAT_MODE parameter and rounded to 8 bits." I see no > mention of coefficients being modified. Well, as is often the case with this manual, I was interpreting based on poorly written information. By "final calculation result is limited according to the SAT_MODE parameter" I interpreted that to mean the hardware enables scaling from full range to limited range. But I concede that it more likely means it clips the output to those ranges. > >> I see there is a "sat" field defined in the struct but is not being >> set in the tables. >> >> So what should we do, define the full range coefficients, and make use >> of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and >> not use SAT_MODE? I'm inclined to do the former. > SAT_MODE should be set for conversions to YUV limited range so that the > coefficients can be rounded to the closest value. Well, we have already rounded the coefficients to the nearest int in the tables. Do you mean the final result (coeff * color component + offset) is rounded? > Otherwise we'd have to > round towards zero, possibly with a larger error, to make sure the > results are inside the valid ranges. Makes sense, I will turn on that bit for limited range YUV output for v5, so that the final color component result is clipped to limited range and rounded. Steve
On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote: [...] > > > But what about this "SAT_MODE" field in the IC task parameter memory? > > > > That just controls the saturation. The result after the matrix > > multiplication is either saturated to [0..255] or to [16..235]/[16..240] > > when converting from the internal representation to the 8 bit output. > > By saturation I think you mean clipped to those ranges? Yes, thanks. I didn't realize it sounds weird to use saturated this way. See: https://en.wikipedia.org/wiki/Saturation_arithmetic > > > According to the manual the hardware will automatically convert the > > > written coefficients to the correct limited ranges. > > > > Where did you get that from? "The final calculation result is limited > > according to the SAT_MODE parameter and rounded to 8 bits." I see no > > mention of coefficients being modified. > > Well, as is often the case with this manual, I was interpreting based on > poorly written information. By "final calculation result is limited > according to the SAT_MODE parameter" I interpreted that to mean the > hardware enables scaling from full range to limited range. But I concede > that it more likely means it clips the output to those ranges. Ok, with this manual I'm never sure that there aren't some conflicting statements somewhere else that I might have overlooked. Good to see that we are at least basing our interpretations on the same text. > > > I see there is a "sat" field defined in the struct but is not being > > > set in the tables. > > > > > > So what should we do, define the full range coefficients, and make use > > > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and > > > not use SAT_MODE? I'm inclined to do the former. > > > > SAT_MODE should be set for conversions to YUV limited range so that the > > coefficients can be rounded to the closest value. > > Well, we have already rounded the coefficients to the nearest int in the > tables. Do you mean the final result (coeff * color component + offset) > is rounded? The manual says so: "The final calculation result is limited according to the SAT_MODE parameter and rounded to 8 bits", but that's not what I meant. Still, I might have been mistaken. I think due to the fact that the coefficients are multiplied by up to 255 (max pixel value) and then effectively divided by 256 when converting to 8 bit, the only way to overflow limited range is if two coefficients are rounded away from zero in the calculation of a single component. This doesn't seem to happen in practice. A constructed example, conversion to YUV limited range with carefully chosen coefficients. Y = R * .1817 + G * .6153 + B * .0618 + 16; Note that .1817 + .6153 + .0618 < 219/255. With rounded coefficients though: Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136 Now 47 + 158 + 16 > 219, and the result is out of range. regards Philipp
On 2/13/19 2:35 AM, Philipp Zabel wrote: > On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote: > [...] >>>> But what about this "SAT_MODE" field in the IC task parameter memory? >>> That just controls the saturation. The result after the matrix >>> multiplication is either saturated to [0..255] or to [16..235]/[16..240] >>> when converting from the internal representation to the 8 bit output. >> By saturation I think you mean clipped to those ranges? > Yes, thanks. I didn't realize it sounds weird to use saturated this way. > See:https://en.wikipedia.org/wiki/Saturation_arithmetic Ok, saturation can mean the same thing as clipping/clamping. Thanks for the article. I tested a RGB->YUV pipeline with the .sat bit set in the BT.601 rgb2yuv table, with the following pipeline on the SabreSD: 'ov5640 1-003c':0 [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'imx6-mipi-csi2':0 [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'imx6-mipi-csi2':2 [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_csi1':0 [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range crop.bounds:(0,0)/1024x768 crop:(0,0)/1024x768 compose.bounds:(0,0)/1024x768 compose:(0,0)/1024x768] 'ipu1_csi1':1 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prp':0 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prp':1 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prpenc':0 [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] 'ipu1_ic_prpenc':1 [fmt:AYUV8_1X32/800x600@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] /dev/video0:0 Format Video Capture: Width/Height : 800/600 Pixel Format : 'YV12' Field : None Bytes per Line : 800 Size Image : 720000 Colorspace : sRGB Transfer Function : sRGB YCbCr/HSV Encoding: ITU-R 601 Quantization : Limited Range Flags : The result being that the captured image colors are all off (there's a bright pink shade to the images). But I discovered the init_csc() function was not setting the saturation bit at the correct bit offset within the TPMEM. The saturation bit is bit 42, or bit 10 of the second 32-bit word. But the code was writing to bit 9 of the second word. After correcting this, saturation is working fine. I have added another patch that fixes this for v5 series. > >>> SAT_MODE should be set for conversions to YUV limited range so that the >>> coefficients can be rounded to the closest value. >> Well, we have already rounded the coefficients to the nearest int in the >> tables. Do you mean the final result (coeff * color component + offset) >> is rounded? > The manual says so: "The final calculation result is limited according > to the SAT_MODE parameter and rounded to 8 bits", but that's not what I > meant. Still, I might have been mistaken. > > I think due to the fact that the coefficients are multiplied by up to > 255 (max pixel value) and then effectively divided by 256 when > converting to 8 bit, the only way to overflow limited range is if two > coefficients are rounded away from zero in the calculation of a single > component. This doesn't seem to happen in practice. > > A constructed example, conversion to YUV limited range with carefully > chosen coefficients. > > Y = R * .1817 + G * .6153 + B * .0618 + 16; > > Note that .1817 + .6153 + .0618 < 219/255. > With rounded coefficients though: > > Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136 Yes, for a rec.709 conversion and max/worst-case RGB signal = (255,255,255). But the rec.709 coefficients for Y are actually Y = (R * 47 + G * 157 + B * 16 + (16 << 8)) / 256 which for RGB = (255,255,255), Y = 235.14, which doesn't overflow limited range. Steve
diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c index 594c3cbc8291..3ef61f0b509b 100644 --- a/drivers/gpu/ipu-v3/ipu-ic.c +++ b/drivers/gpu/ipu-v3/ipu-ic.c @@ -183,11 +183,13 @@ struct ic_csc_params { }; /* + * BT.601 encoding from RGB full range to YUV limited range: + * * Y = R * .299 + G * .587 + B * .114; * U = R * -.169 + G * -.332 + B * .500 + 128.; * V = R * .500 + G * -.419 + B * -.0813 + 128.; */ -static const struct ic_csc_params ic_csc_rgb2ycbcr = { +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = { .coeff = { { 77, 150, 29 }, { 469, 427, 128 }, @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = { .scale = 1, }; -/* transparent RGB->RGB matrix for graphics combining */ -static const struct ic_csc_params ic_csc_rgb2rgb = { +/* + * identity matrix, used for transparent RGB->RGB graphics + * combining. + */ +static const struct ic_csc_params ic_csc_identity = { .coeff = { { 128, 0, 0 }, { 0, 128, 0 }, @@ -208,11 +213,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = { }; /* + * Inverse BT.601 encoding from YUV limited range to RGB full range: + * * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128)); * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128)); * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128); */ -static const struct ic_csc_params ic_csc_ycbcr2rgb = { +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = { .coeff = { { 149, 0, 204 }, { 149, 462, 408 }, @@ -238,11 +245,11 @@ static int init_csc(struct ipu_ic *ic, (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]); if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) - params = &ic_csc_ycbcr2rgb; + params = &ic_csc_ycbcr2rgb_bt601; else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) - params = &ic_csc_rgb2ycbcr; + params = &ic_csc_rgb2ycbcr_bt601; else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB) - params = &ic_csc_rgb2rgb; + params = &ic_csc_identity; else { dev_err(priv->ipu->dev, "Unsupported color space conversion\n"); return -EINVAL;
The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding coefficients, so rename them to indicate that. And add some comments to make clear these are BT.601 coefficients encoding between YUV limited range and RGB full range. The ic_csc_rgb2rgb matrix is just an identity matrix, so rename to ic_csc_identity. No functional changes. Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- Changes in v2: - rename ic_csc_rgb2rgb matrix to ic_csc_identity. --- drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)