Created attachment 15494 [details] [review] patch The intel driver doesn't account for scaling and offsetting the range of the Y component from 16-235 to 0-255, and similar for the Cb and Cr components for the overlay device. Also, the matrix factors are set up for Rec. BT.601, which is incorrect; it should be Rec. BT.709.
Why we need secondary gamma enabled? and if set, we need other part for its memory and pipe setup? We can set different color conversion mode per user request, but not force it to BT709, which looks doesn't support on older intel chips. @@ -879,9 +882,9 @@ I830SetupImageVideoOverlay(ScreenPtr pScreen) pPriv->textured = FALSE; pPriv->colorKey = pI830->colorKey & ((1 << pScrn->depth) - 1); pPriv->videoStatus = 0; - pPriv->brightness = 0; - pPriv->contrast = 64; - pPriv->saturation = 128; + pPriv->brightness = -19; /* (255/219) * -16 */ + pPriv->contrast = 75; /* 255/219 * 64 */ + pPriv->saturation = 146; /* 128/112 * 128 */ pPriv->current_crtc = NULL; pPriv->desired_crtc = NULL; pPriv->buf = NULL; This is wrong, values used to program overlay color correct registers are in specific format, e.g contrast in 3i.6f fixed point format, so 64(001.000000) is correct default value. You can see from spec definition. So this patch is acceptable in current case.
Why we need secondary gamma enabled? and if set, we need other part for its memory and pipe setup? We can set different color conversion mode per user request, but not force it to BT709, which looks doesn't support on older intel chips. @@ -879,9 +882,9 @@ I830SetupImageVideoOverlay(ScreenPtr pScreen) pPriv->textured = FALSE; pPriv->colorKey = pI830->colorKey & ((1 << pScrn->depth) - 1); pPriv->videoStatus = 0; - pPriv->brightness = 0; - pPriv->contrast = 64; - pPriv->saturation = 128; + pPriv->brightness = -19; /* (255/219) * -16 */ + pPriv->contrast = 75; /* 255/219 * 64 */ + pPriv->saturation = 146; /* 128/112 * 128 */ pPriv->current_crtc = NULL; pPriv->desired_crtc = NULL; pPriv->buf = NULL; This is wrong, values used to program overlay color correct registers are in specific format, e.g contrast in 3i.6f fixed point format, so 64(001.000000) is correct default value. You can see from spec definition. So this patch is not acceptable in current case.
sorry for comment collision, oops.
Er, sorry, ignore setting the GAMMA bit. It is untested. The gain vaules are already in fixed point format, that is why they're multiplied by 64 and 128 respectively. The default gain values should not be 1.0, since this is incorrect. They should be 255/219 for luma and 128/112 for chroma. If BT709 isn't supported on a particular chip, falling back to BT601 conversion will look a little off, but not as bad as getting the gain values wrong.
(In reply to comment #4) > The gain vaules are already in fixed point format, that is why they're > multiplied by 64 and 128 respectively. 255/219*64 = 64 > > The default gain values should not be 1.0, since this is incorrect. well, spec says it should be 1.0 though. > They should be 255/219 for luma and 128/112 for chroma. As I know those contrast, brightness control will be applied in yuv data process, but not directly act on yuv data. > > If BT709 isn't supported on a particular chip, falling back to BT601 conversion > will look a little off, but not as bad as getting the gain values wrong. > Is there any issue you've seen from video playback?
(In reply to comment #5) > 255/219*64 = 64 Um, what? > > The default gain values should not be 1.0, since this is incorrect. > > well, spec says it should be 1.0 though. It says to use 1.0 only if you want bypass the contrast adjustment. We don't want to bypass the contrast adjustment, because the Y values need to be converted from the range [16,235] to [0,255] before being multiplied by the YUV2RGB matrix. > Is there any issue you've seen from video playback? Yes. The converted color values are wrong. It's easy to see this when you examine a video played through Xv vs. one played using RGB XImages. For example, run: gst-launch videotestsrc ! xvimagesink & gst-launch videotestsrc ! ximagesink
oh, so you use default xv adaptor right? which should be textured video adaptor, how about your xvinfo?
No, it's not textured video. If it was, I wouldn't have been able to fix it with that patch, right?
See also: https://bugs.launchpad.net/xserver-xorg-video-intel/+bug/105656
With or without your default brightness, contrast, etc. change, I can't see much difference in gst videotestsrc. Do you test with a TV out? As yuv2rgb convert is handled by hardware in overlay xv port, I don't understand why we should take care of color value clamping but consider hardware won't handle that for us? Driver default set bypass those color change seems ok to me.
I'm pretty sure you'd prefer video playback to *be* correct rather than *seeming* correct. There are standards for these things, and the relevant one to this situation, Rec BT.709, requires offsetting and scaling the Y, Cb, and Cr values before multiplying by a matrix. The hardware defaults to no offset and no scaling, and the hardware spec doesn't explicitly state what values to use to get BT.709 behavior, so this is giant clue that the default behavior doesn't follow BT.709. I have enough experience in this area that I can just look a video being played back and identify incorrect colorspace conversion. The SMPTE color bars (the gstreamer launch line) are a quick way to test, although not conclusive. On the bottom row of blocks, just to the left of the snow, there are three very-dark-grey areas. One is -5% grey ("superblack"), one is 0% grey ("black"), and one is 5% grey ("dark grey"). When correctly converted to RGB, superblack and black are converted to 0,0,0, and dark-grey is converted to 13,13,13. However, the current driver causes superblack and black to be converted to separate black levels -- this is clue #2. (Also, the white bar is obviously not converted to 255,255,255.) Clue #3 is that I got out the color meter and measured the values coming out of my monitor and compared them to what I expected. Indeed, with the patch, the colors coming out are very nearly correct. (Correct == the same as what comes out when you do the correct conversion. The colors still aren't completely correct due to monitor limitations.) If you have any further objections to this change, please explain it with respect to the above analysis.
Committed. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.