RE: FW: Revised fix for 687418 WTS does not work withbitcmyk driver

>0.
>> 5.  Added calls to check_device_separable prior to calls to the
>> various 'fill_in_procs' routines (gx_device_fill_in_procs, 
>> gx_forward_fill_in_procs, fill_in_procs). 
>
>Must any call to fill_in_procs to be prepended with 
>a check_device_separable call ?
>Please dcoument that.

No.  Unfortunately all call to the 'fill_in_procs' routines cannot
be preceded by a call to check_device_separable.  If this were the
case then I would simply add a call to that routine at the start
of each 'fill_in_procs' routine.  However three are at least two
cases in which the 'fill_in_procs' routine is called with a device
structure that is only partially initialized.  This caused the
segment faults seen in the earlier version of this code.  I do
not like the partially initialized device structures being used
in this manner however changing their use is likely to crate more
problems.

A comment has been added to the start of each 'fiil_in_procs'
routine.


>1.
>> --- a/src/gdevcdj.c 13 Nov 2002 23:33:05 -0000 1.13
>> +++ b/src/gdevcdj.c 18 May 2004 03:52:14 -0000
>[...]
>> @@ -408,7 +408,7 @@
>[...]
>> -    (bpp > 8 ? 5 : 2), (bpp > 8 ? 5 : bpp > 1 ? 2 : 0),\
>> +    (bpp > 8 ? 256 : 2), (bpp > 8 ? 256 : bpp > 1 ? 2 : 0),\
>
>Are you sure about "bpp > 8" ? Other similar places use ">=".

The 'bpp' refers to bits per pixel not bits per component.  The
cdj devices do include devices which use 8 bits to save what is
really a 3 bit RGB value.  Thus the test is correct.  I merely
corrected a problem which was causing a device to have a max_gray
value of 255 but a dither_grays of 5.  This is inconsistent.


>2. 
>> @@ -3272,18 +3258,18 @@
>[...]
>> -   ci->dither_grays = (bpp > 8 ? 5 : 2);
>> -   ci->dither_colors = (bpp > 8 ? 5 : bpp > 1 ? 2 : 0);
>> +   ci->dither_grays = (bpp > 8 ? 256 : 2);
>> +   ci->dither_colors = (bpp > 8 ? 256 : bpp > 1 ? 2 : 0);
>
>Same as above.

Same as above.


>3.
>+    if (pinfo->gray_index < num_components &&
>+        (!pinfo->dither_grays || pinfo->dither_grays != (pinfo->max_gray + 1)))
>+     return;
>+    if ((num_components > 1 || pinfo->gray_index != 0) &&
>+ (!pinfo->dither_colors || pinfo->dither_colors != (pinfo->max_color + 1)))
>  return;
>
>I'm unclear with this logics. 
>It is equivalent to :
>
>    if (pinfo->gray_index < num_components || num_components > 1 || pinfo->gray_index != 0)
>        if  (!pinfo->dither_grays || pinfo->dither_grays != pinfo->max_gray + 1)
>               return;

Is it your intention to state that this single if statement is equivalent
to both of my original if statements?  If so then you are obviously wrong.
Your statement does not even look at the values of dither_colors and max_colors.


>Are you sure that the 1st 'if' correctly distinguish all necessary cases ?
>For example, with RGB it appears 'true' due to num_components > 1.
>Is this the intention ?

Once again I am confused about what you are saying.  My first if statement is
not always true if num_components is greater than one.


>Also the second 'if' appears true with any pinfo->dither_grays except pinfo->max_gray + 1,
>therefore it is either overcomplicated or wrong.

I am confused about whether you are referring to my version of the code or yours.
My second if statement does not refer to dither_grays and max_gray at all.
If you are referring to yours, then I have already said that I believe it
to be wrong.


>4.
>> @@ -432,9 +440,9 @@
>> -    for (i = 0; i < num_components; i ++) {
>> +    for (i = 0; i < num_components; i++) {
>
>Hmm, not sure that it is consistent with C-style. 
>Definitely it is inconsistent with other parts of same patch.

The use of 'i++' is the 'normal' case and is consistent with most C programming
style.  It was the old version of 'i ++' which is unusual.  A quick search
with grep did not find any examples of 'i ++' in the remainder of the sources.
The old version was caused by a typo which was copied into several places.
I have corrected a couple more copies of this typo that I did not notice
previously.


>5.
>> + for (j = 0; (color_index & 1) == 0 && color_index != 0; j++)
>>       color_index >>= 1;
>
>A more accurate code :
>    
>    if (color_index != 0)
>         for (j = 0; !(color_index & 1); j++)
>               color_index >>= 1;

Your code is not 'more accurate'.  It leaves the value of 'j' at its
previous value instead of zero.  This incorrect value is then stored
in the next line of code:
	comp_bits[i] = j;


>6.
>>@@ -470,6 +481,20 @@
>[...]
>> + int dither = 1 << comp_bits[i];
>> + if (pinfo->dither_grays != 1 && dither == pinfo->dither_grays) {
>
>C-style requires a space line after definitions.

Blank line added.


>7.
>> +++ b/src/gdevpbm.c 18 May 2004 03:52:23 -0000
>> @@ -325,13 +325,39 @@
>[...]
>> + int bpc = dev->color_info.depth / 3;
>> + int drop = sizeof(gx_color_value) * 8 - bpc;
>> +
>> + return ((((r >> drop) << bpc) + (g >> drop)) << bpc) + (b >> drop);
>
>Suggesting 'uint' for more portability :   a >> -1 isn't portable.

Changed.


>8.
>> --- a/src/gxcmap.c 16 Jan 2004 06:36:59 -0000 1.18
>> +++ b/src/gxcmap.c 18 May 2004 03:52:46 -0000
>[...]
>> @@ -61,11 +62,9 @@
>[...]
>> -        ulong   cbits = cv[i] * ((i == i_gray ?
>> -                                 dev->color_info.max_gray :
>> -                                 dev->color_info.max_color ) + 1);
>> + color |= (gx_color_index)(cv[i] >> (gx_color_value_bits - comp_bits[i]))
>> +  << comp_shift[i];
>>
>> -        color |= (cbits / (gx_max_color_value + 1)) << comp_shift[i];
>
>Not sure what is 'cv' when dev->color_info.max_gray +1 != 1 <<comp_bits[i] .
>Maybe both the old and the new code are incorrect.

This routine is only executed if the device is 'separable'.  In this
case, the value in comp_bits[i] should correspond to either max_gray + 1
or max_color + 1 as appropriate.


>9.
>> @@ -86,12 +87,14 @@
>>  #endif
>>  
>>      for (i = 0; i < ncomps; i++) {
>> -        gx_color_index  div = ( i == dev->color_info.gray_index ? 
>> -                                dev->color_info.max_gray : 
>> -                                dev->color_info.max_color ) + 1;
>> -
>> -        cv[i] = (gx_color_value)(((color & comp_mask[i]) >> comp_shift[i]) * 
>> -            (gx_max_color_value + 1) / div);
>> + bits = (color & comp_mask[i]) >> comp_shift[i];
>> + shift = gx_color_value_bits - comp_bits[i];
>> + cv[i] = bits << shift;
>> + while ((shift-=comp_bits[i]) >= 0)
>> +     cv[i] |= bits << shift;
>> + if (shift < 0)
>> +     cv[i] |= bits >> -shift;
>> + 
>>      }
>
>I can't understand the new code.
>Are you sure that 'bits' to be '|'ed to ALL components ?
>For example, if ncomps==2, comp_bits=={8,8},  comp_shift={0,8}, color == 256, i = 1
>get :
>
>shift = 8;
>cv[1] = (1 | (1 << 8)) = 257.
>
>Is this the intention ? Why ?

Yes this is the intention.  This routine is trying to determine the
original colorant values given a gx_color_index value.  In your example
of an 8 bit device, this routine is trying to map the values 0 to 0xff
to 0 to 0xffff.  Obviously the routine has insufficient information to
determine the original colorant values exactly.  The routine is mapping
0x00 to 0x0000, 0x01 to 0x0101, ..., and 0xff to 0xffff..  I consider
this to be reasonable.  I would also consider a routine which maps 0xff
to 0xff00 or 0xff7f or 0xff80 as reasonable.  The mapping which is being
used does have the advantage that 0xff which is a maximum value in the
color index is mapped to the maximum value for a gx_color_value.

The revised routine came from Russell Lang.  In general, I think that
it is an improvement over the old routine.  He also has a version in
which the while loop is not done but a factor is calculated.  This
version produces slightly different results.  I am going to recommend
(in a separate email to Russell) that a table lookup be used to determine
the factor instead of the while loop (shown above) or the calculation.
Then the result should match the calculation shown above.
Russell's alternate version is at
http://www.ghostscript.com/pipermail/gs-code-review/2004-May/004504.html


>10.
>> --- a/src/zcolor.c 11 Nov 2003 11:23:17 -0000 1.17
>> +++ b/src/zcolor.c 18 May 2004 03:53:04 -0000
>
>I guess, this change is debug purpose only.
>Please document that.

Okay, comments will be added to both zcolor.c and Language.htm.


Dan

-----Original Message-----
From: Igor V. Melichev [mailto:[email protected]]
Sent: Wednesday, May 19, 2004 5:15 AM
To: [email protected]; Gs-Code-Review
Subject: Re: [gs-code-review] FW: Revised fix for 687418 WTS does not
work withbitcmyk driver 


Dan,

0.
> 5.  Added calls to check_device_separable prior to calls to the
> various 'fill_in_procs' routines (gx_device_fill_in_procs, 
> gx_forward_fill_in_procs, fill_in_procs). 

Must any call to fill_in_procs to be prepended with 
a check_device_separable call ?
Please dcoument that.

1.
> --- a/src/gdevcdj.c 13 Nov 2002 23:33:05 -0000 1.13
> +++ b/src/gdevcdj.c 18 May 2004 03:52:14 -0000
[...]
> @@ -408,7 +408,7 @@
[...]
> -    (bpp > 8 ? 5 : 2), (bpp > 8 ? 5 : bpp > 1 ? 2 : 0),\
> +    (bpp > 8 ? 256 : 2), (bpp > 8 ? 256 : bpp > 1 ? 2 : 0),\

Are you sure about "bpp > 8" ? Other similar places use ">=".

2. 
> @@ -3272,18 +3258,18 @@
[...]
> -   ci->dither_grays = (bpp > 8 ? 5 : 2);
> -   ci->dither_colors = (bpp > 8 ? 5 : bpp > 1 ? 2 : 0);
> +   ci->dither_grays = (bpp > 8 ? 256 : 2);
> +   ci->dither_colors = (bpp > 8 ? 256 : bpp > 1 ? 2 : 0);

Same as above.

3.
+    if (pinfo->gray_index < num_components &&
+        (!pinfo->dither_grays || pinfo->dither_grays != (pinfo->max_gray + 1)))
+     return;
+    if ((num_components > 1 || pinfo->gray_index != 0) &&
+ (!pinfo->dither_colors || pinfo->dither_colors != (pinfo->max_color + 1)))
  return;

I'm unclear with this logics. 
It is equivalent to :

    if (pinfo->gray_index < num_components || num_components > 1 || pinfo->gray_index != 0)
        if  (!pinfo->dither_grays || pinfo->dither_grays != pinfo->max_gray + 1)
               return;

Are you sure that the 1st 'if' correctly distinguish all necessary cases ?
For example, with RGB it appears 'true' due to num_components > 1.
Is this the intention ?
Also the second 'if' appears true with any pinfo->dither_grays except pinfo->max_gray + 1,
therefore it is either overcomplicated or wrong.

4.
> @@ -432,9 +440,9 @@
> -    for (i = 0; i < num_components; i ++) {
> +    for (i = 0; i < num_components; i++) {

Hmm, not sure that it is consistent with C-style. 
Definitely it is inconsistent with other parts of same patch.

5.
> + for (j = 0; (color_index & 1) == 0 && color_index != 0; j++)
>       color_index >>= 1;

A more accurate code :
    
    if (color_index != 0)
         for (j = 0; !(color_index & 1); j++)
               color_index >>= 1;


6.
>@@ -470,6 +481,20 @@
[...]
> + int dither = 1 << comp_bits[i];
> + if (pinfo->dither_grays != 1 && dither == pinfo->dither_grays) {

C-style requires a space line after definitions.

7.
> +++ b/src/gdevpbm.c 18 May 2004 03:52:23 -0000
> @@ -325,13 +325,39 @@
[...]
> + int bpc = dev->color_info.depth / 3;
> + int drop = sizeof(gx_color_value) * 8 - bpc;
> +
> + return ((((r >> drop) << bpc) + (g >> drop)) << bpc) + (b >> drop);

Suggesting 'uint' for more portability :   a >> -1 isn't portable.

8.
> --- a/src/gxcmap.c 16 Jan 2004 06:36:59 -0000 1.18
> +++ b/src/gxcmap.c 18 May 2004 03:52:46 -0000
[...]
> @@ -61,11 +62,9 @@
[...]
> -        ulong   cbits = cv[i] * ((i == i_gray ?
> -                                 dev->color_info.max_gray :
> -                                 dev->color_info.max_color ) + 1);
> + color |= (gx_color_index)(cv[i] >> (gx_color_value_bits - comp_bits[i]))
> +  << comp_shift[i];
>
> -        color |= (cbits / (gx_max_color_value + 1)) << comp_shift[i];

Not sure what is 'cv' when dev->color_info.max_gray +1 != 1 <<comp_bits[i] .
Maybe both the old and the new code are incorrect.

9.
> @@ -86,12 +87,14 @@
>  #endif
>  
>      for (i = 0; i < ncomps; i++) {
> -        gx_color_index  div = ( i == dev->color_info.gray_index ? 
> -                                dev->color_info.max_gray : 
> -                                dev->color_info.max_color ) + 1;
> -
> -        cv[i] = (gx_color_value)(((color & comp_mask[i]) >> comp_shift[i]) * 
> -            (gx_max_color_value + 1) / div);
> + bits = (color & comp_mask[i]) >> comp_shift[i];
> + shift = gx_color_value_bits - comp_bits[i];
> + cv[i] = bits << shift;
> + while ((shift-=comp_bits[i]) >= 0)
> +     cv[i] |= bits << shift;
> + if (shift < 0)
> +     cv[i] |= bits >> -shift;
> + 
>      }

I can't understand the new code.
Are you sure that 'bits' to be '|'ed to ALL components ?
For example, if ncomps==2, comp_bits=={8,8},  comp_shift={0,8}, color == 256, i = 1
get :

shift = 8;
cv[1] = (1 | (1 << 8)) = 257.

Is this the intention ? Why ?

10.
> --- a/src/zcolor.c 11 Nov 2003 11:23:17 -0000 1.17
> +++ b/src/zcolor.c 18 May 2004 03:53:04 -0000

I guess, this change is debug purpose only.
Please document that.

Igor.