[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [PATCH] xrandr: use full range for gamma table generation
From: Stéphane_Marchesin <stephane.marchesin () gmail ! com>
Date: 2014-04-30 23:36:07
Message-ID: CACP_E++OOi10wkSh_HJb_2K1H1hD3M+eFCW3V8ya9NwUEJNmDQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
And pushed.
On Fri, Apr 18, 2014 at 8:19 PM, Stéphane Marchesin <
stephane.marchesin@gmail.com> wrote:
>
>
>
> On Tue, Apr 1, 2014 at 8:02 PM, <dbehr@chromium.org> wrote:
>
>> From: Dominik Behr <dbehr@chromium.org>
>>
>> Calculate gamma table using full [0,65536) range and do not make any
>> assumptions about relation of gamma table size and significant bits.
>>
>> Gamma table size has nothing to do with number of significant bits in
>> hardware.
>> In particular we are dealing now with gamma table that has 17 entries and
>> 8
>> bit precision, there are other GPUs with 10 bit precision and less than
>> 256
>> entries using partial linear approximation. Deriving assumed gamma table
>> significant bits from size of gamma table leads to incorrect calculations
>> and
>> loss of precision. Also XRandR specification never mentions that gamma
>> tables
>> need to be power of 2.
>>
>> Signed-off-by: Dominik Behr <dbehr@chromium.org>
>>
>
> Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
>
> ---
>> xrandr.c | 25 +++++++------------------
>> 1 file changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/xrandr.c b/xrandr.c
>> index c51bee3..7a5fa30 100644
>> --- a/xrandr.c
>> +++ b/xrandr.c
>> @@ -1394,7 +1394,7 @@ set_gamma(void)
>> output_t *output;
>>
>> for (output = all_outputs; output; output = output->next) {
>> - int i, size, shift;
>> + int i, size;
>> crtc_t *crtc;
>> XRRCrtcGamma *crtc_gamma;
>> float gammaRed;
>> @@ -1429,14 +1429,6 @@ set_gamma(void)
>> continue;
>> }
>>
>> - /*
>> - * The hardware color lookup table has a number of significant
>> - * bits equal to ffs(size) - 1; compute all values so that
>> - * they are in the range [0,size) then shift the values so
>> - * that they occupy the MSBs of the 16-bit X Color.
>> - */
>> - shift = 16 - (ffs(size) - 1);
>> -
>> crtc_gamma = XRRAllocGamma(size);
>> if (!crtc_gamma) {
>> fatal("Gamma allocation failed.\n");
>> @@ -1456,28 +1448,25 @@ set_gamma(void)
>>
>> for (i = 0; i < size; i++) {
>> if (gammaRed == 1.0 && output->brightness == 1.0)
>> - crtc_gamma->red[i] = i;
>> + crtc_gamma->red[i] = (double)i / (double)(size - 1) *
>> 65535.0;
>> else
>> crtc_gamma->red[i] = dmin(pow((double)i/(double)(size -
>> 1),
>> gammaRed) *
>> output->brightness,
>> - 1.0) * (double)(size - 1);
>> - crtc_gamma->red[i] <<= shift;
>> + 1.0) * 65535.0;
>>
>> if (gammaGreen == 1.0 && output->brightness == 1.0)
>> - crtc_gamma->green[i] = i;
>> + crtc_gamma->green[i] = (double)i / (double)(size - 1) *
>> 65535.0;
>> else
>> crtc_gamma->green[i] = dmin(pow((double)i/(double)(size -
>> 1),
>> gammaGreen) *
>> output->brightness,
>> - 1.0) * (double)(size - 1);
>> - crtc_gamma->green[i] <<= shift;
>> + 1.0) * 65535.0;
>>
>> if (gammaBlue == 1.0 && output->brightness == 1.0)
>> - crtc_gamma->blue[i] = i;
>> + crtc_gamma->blue[i] = (double)i / (double)(size - 1) *
>> 65535.0;
>> else
>> crtc_gamma->blue[i] = dmin(pow((double)i/(double)(size -
>> 1),
>> gammaBlue) *
>> output->brightness,
>> - 1.0) * (double)(size - 1);
>> - crtc_gamma->blue[i] <<= shift;
>> + 1.0) * 65535.0;
>> }
>>
>> XRRSetCrtcGamma(dpy, crtc->crtc.xid, crtc_gamma);
>> --
>> 1.9.1.423.g4596e3a
>>
>> _______________________________________________
>> xorg-devel@lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>>
>
>
[Attachment #5 (text/html)]
<div dir="ltr">And pushed.</div><div class="gmail_extra"><br><br><div \
class="gmail_quote">On Fri, Apr 18, 2014 at 8:19 PM, Stéphane Marchesin <span \
dir="ltr"><<a href="mailto:stephane.marchesin@gmail.com" \
target="_blank">stephane.marchesin@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div \
class="gmail_quote"><div class="">On Tue, Apr 1, 2014 at 8:02 PM, <span \
dir="ltr"><<a href="mailto:dbehr@chromium.org" \
target="_blank">dbehr@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">From: Dominik Behr <<a href="mailto:dbehr@chromium.org" \
target="_blank">dbehr@chromium.org</a>><br> <br>
Calculate gamma table using full [0,65536) range and do not make any<br>
assumptions about relation of gamma table size and significant bits.<br>
<br>
Gamma table size has nothing to do with number of significant bits in hardware.<br>
In particular we are dealing now with gamma table that has 17 entries and 8<br>
bit precision, there are other GPUs with 10 bit precision and less than 256<br>
entries using partial linear approximation. Deriving assumed gamma table<br>
significant bits from size of gamma table leads to incorrect calculations and<br>
loss of precision. Also XRandR specification never mentions that gamma tables<br>
need to be power of 2.<br>
<br>
Signed-off-by: Dominik Behr <<a href="mailto:dbehr@chromium.org" \
target="_blank">dbehr@chromium.org</a>><br></blockquote><div><br></div></div><div>Reviewed-by: \
Stéphane Marchesin <<a href="mailto:marcheu@chromium.org" \
target="_blank">marcheu@chromium.org</a>> </div>
<div><div class="h5">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">
---<br>
xrandr.c | 25 +++++++------------------<br>
1 file changed, 7 insertions(+), 18 deletions(-)<br>
<br>
diff --git a/xrandr.c b/xrandr.c<br>
index c51bee3..7a5fa30 100644<br>
--- a/xrandr.c<br>
+++ b/xrandr.c<br>
@@ -1394,7 +1394,7 @@ set_gamma(void)<br>
output_t *output;<br>
<br>
for (output = all_outputs; output; output = output->next) {<br>
- int i, size, shift;<br>
+ int i, size;<br>
crtc_t *crtc;<br>
XRRCrtcGamma *crtc_gamma;<br>
float gammaRed;<br>
@@ -1429,14 +1429,6 @@ set_gamma(void)<br>
continue;<br>
}<br>
<br>
- /*<br>
- * The hardware color lookup table has a number of significant<br>
- * bits equal to ffs(size) - 1; compute all values so that<br>
- * they are in the range [0,size) then shift the values so<br>
- * that they occupy the MSBs of the 16-bit X Color.<br>
- */<br>
- shift = 16 - (ffs(size) - 1);<br>
-<br>
crtc_gamma = XRRAllocGamma(size);<br>
if (!crtc_gamma) {<br>
fatal("Gamma allocation failed.\n");<br>
@@ -1456,28 +1448,25 @@ set_gamma(void)<br>
<br>
for (i = 0; i < size; i++) {<br>
if (gammaRed == 1.0 && output->brightness == 1.0)<br>
- crtc_gamma->red[i] = i;<br>
+ crtc_gamma->red[i] = (double)i / (double)(size - 1) * \
65535.0;<br> else<br>
crtc_gamma->red[i] = dmin(pow((double)i/(double)(size - \
1),<br>
gammaRed) * \
output->brightness,<br>
- 1.0) * (double)(size - \
1);<br>
- crtc_gamma->red[i] <<= shift;<br>
+ 1.0) * 65535.0;<br>
<br>
if (gammaGreen == 1.0 && output->brightness == 1.0)<br>
- crtc_gamma->green[i] = i;<br>
+ crtc_gamma->green[i] = (double)i / (double)(size - 1) * \
65535.0;<br> else<br>
crtc_gamma->green[i] = dmin(pow((double)i/(double)(size - \
1),<br>
gammaGreen) * \
output->brightness,<br>
- 1.0) * (double)(size \
- 1);<br>
- crtc_gamma->green[i] <<= shift;<br>
+ 1.0) * 65535.0;<br>
<br>
if (gammaBlue == 1.0 && output->brightness == 1.0)<br>
- crtc_gamma->blue[i] = i;<br>
+ crtc_gamma->blue[i] = (double)i / (double)(size - 1) * \
65535.0;<br> else<br>
crtc_gamma->blue[i] = dmin(pow((double)i/(double)(size - \
1),<br>
gammaBlue) * \
output->brightness,<br>
- 1.0) * (double)(size \
- 1);<br>
- crtc_gamma->blue[i] <<= shift;<br>
+ 1.0) * 65535.0;<br>
}<br>
<br>
XRRSetCrtcGamma(dpy, crtc->crtc.xid, crtc_gamma);<br>
<span><font color="#888888">--<br>
1.9.1.423.g4596e3a<br>
<br>
_______________________________________________<br>
<a href="mailto:xorg-devel@lists.x.org" target="_blank">xorg-devel@lists.x.org</a>: \
X.Org development<br>
Archives: <a href="http://lists.x.org/archives/xorg-devel" \
target="_blank">http://lists.x.org/archives/xorg-devel</a><br>
Info: <a href="http://lists.x.org/mailman/listinfo/xorg-devel" \
target="_blank">http://lists.x.org/mailman/listinfo/xorg-devel</a><br> \
</font></span></blockquote></div></div></div><br></div></div> \
</blockquote></div><br></div>
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic