All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steve Longerbeam <slongerbeam@gmail.com>
To: Philipp Zabel <p.zabel@pengutronix.de>, linux-media@vger.kernel.org
Cc: Tim Harvey <tharvey@gateworks.com>,
	"open list:DRM DRIVERS FOR FREESCALE IMX" 
	<dri-devel@lists.freedesktop.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices
Date: Thu, 14 Feb 2019 10:54:19 -0800	[thread overview]
Message-ID: <58bf1e34-8b9d-7fec-6ba5-5db1d5c5457d@gmail.com> (raw)
In-Reply-To: <1550054109.3937.1.camel@pengutronix.de>



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


  reply	other threads:[~2019-02-14 18:54 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-09  1:47 [PATCH v4 0/4] media: imx: Add support for BT.709 encoding Steve Longerbeam
2019-02-09  1:47 ` [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices Steve Longerbeam
2019-02-09  1:47   ` Steve Longerbeam
2019-02-11  9:58   ` Philipp Zabel
2019-02-11  9:58     ` Philipp Zabel
2019-02-11 18:24     ` Steve Longerbeam
2019-02-11 18:24       ` Steve Longerbeam
2019-02-12 10:17       ` Philipp Zabel
2019-02-12 10:17         ` Philipp Zabel
2019-02-12 17:42         ` Steve Longerbeam
2019-02-12 17:42           ` Steve Longerbeam
2019-02-13 10:35           ` Philipp Zabel
2019-02-13 10:35             ` Philipp Zabel
2019-02-14 18:54             ` Steve Longerbeam [this message]
2019-02-14 18:54               ` Steve Longerbeam
2019-02-11 23:33     ` Steve Longerbeam
2019-02-11 23:33       ` Steve Longerbeam
2019-02-09  1:47 ` [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix Steve Longerbeam
2019-02-09  1:47   ` Steve Longerbeam
2019-02-11 10:00   ` Philipp Zabel
2019-02-11 10:00     ` Philipp Zabel
2019-02-09  1:47 ` [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding Steve Longerbeam
2019-02-09  1:47   ` Steve Longerbeam
2019-02-09  1:47   ` Steve Longerbeam
2019-02-11 10:12   ` Philipp Zabel
2019-02-11 10:12     ` Philipp Zabel
2019-02-11 10:12     ` Philipp Zabel
2019-02-12  1:20     ` Steve Longerbeam
2019-02-12  1:20       ` Steve Longerbeam
2019-02-12  1:20       ` Steve Longerbeam
2019-02-12 11:34       ` Philipp Zabel
2019-02-12 11:34         ` Philipp Zabel
2019-02-12 11:34         ` Philipp Zabel
2019-02-12 17:50         ` Steve Longerbeam
2019-02-12 17:50           ` Steve Longerbeam
2019-02-12 17:50           ` Steve Longerbeam
2019-02-09  1:47 ` [PATCH v4 4/4] media: imx: Allow BT.709 encoding for IC routes Steve Longerbeam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=58bf1e34-8b9d-7fec-6ba5-5db1d5c5457d@gmail.com \
    --to=slongerbeam@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=tharvey@gateworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.