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

Igor,

>However I'm still unclear what value is used as a stub for
>gray_index when there is no gray component.
>Is it -1, num_components, or else ?

It is documented in gxdevcli.h.  (gxdevcli.h is the top level
document for all information related to the color_info fields.)

 
>Also, what is a stub for dither_grays when there is no dithering ?
>Therefore I'm not sure that the condition is correct.

Once again it is documented in gxdevcli.h


>It's not obvious, right ?
>Please add a comment about that.

Comment added.


>Hmm, now I'm finally lost what this routine does.
>Please add a comment with an explanation of the intention.
>You wrote :
>
>> This routine is trying to determine 
>
>I don't see what it "tries" to determine,
>because there is no "tries" in the new code.
>Instead that, it does determine something.
>I'd like to understand for sure, what is "something".

I have already given a detailed description about this routine.
I see no need to repeat it.  As to the use of the word 'trying',
in the ideal case the routine would return the values of the
original colorants which were used to create the color index
value.  However, as previously stated, the routine does not
have all of the information required to accomplish this ideal.
Thus it only 'trying' but it does not always obtain the ideal
case.


>> The routine is mapping
>> 0x00 to 0x0000, 0x01 to 0x0101, ..., and 0xff to 0xffff..  
>
>Hmm, this statement is true for comp_bits[i] == 8.
>It is not true with comp_bits[i] != 8.

As I stated in my original response, I gave an example which
matches your example case of 8 bits.  Obviously the range of input
values changes as the number of bits change.


>Did anybody try other comp_bits[i] ?

Yes.


>if ncomps==2, comp_bits=={3,3},  comp_shift={0,8}, color == 0x0500, i = 1, get :
>shift = 13,
cv[1] = (5 << 13) | (5 << 10) | (5 << 7) | (5 << 4) | (5 << 1) | (5 >>2).
What does this magic number mean ?

The result is (in binary) 1011011011011011 or 0xb6db or 46811 in decimal.
You will note that the maximum for a 3 bit value is 7.  You will also note
that the maximum for a gx_color_value is 0xffff.  Finally note that
0xffff * (5.0 / 7.0) = 46810.71.  The algorithm is a complicated way of
multiplying 0xffff times the bit value for an individual colorant and
dividing by ((1 << comp_bits[i]) - 1).  The algorithm rounds to the nearest
integer.

As I said in my previous email, I am not recommending using this algorithm
since it is complicated and slow.  This decision was made as the result of
a discussion with Russell after the code was submitted to code review.



>Especially what is (5 >>2) ?

(5 >>2) is 1.  This is the part of the algorithm which is doing the
rounding.



>Maybe you mean that only valid values for comp_bits are 1,2,8 ?
>Is this documented ?

The algorithm is valid for any useful value of comp_bits (i.e. 1 to 16).
Your previous question with comp_bits[i] = 3 gives an example of
the algorithm working with a 3 bit value.


Dan


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


Dan,

> From: "Dan Coby" <[email protected]>
> To: "Igor V. Melichev" <[email protected]>; "Gs-Code-Review" <[email protected]>
> Sent: Thursday, May 20, 2004 3:40 AM
> Subject: RE: [gs-code-review] FW: Revised fix for 687418 WTS does not work withbitcmyk driver (xefitra)


> >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;

Oops, you're right : they are not equivalent.
I did not notice gray/color and assumed same expression.
Sorry for that.

However I'm still unclear what value is used as a stub for
gray_index when there is no gray component.
Is it -1, num_components, or else ?
Also, what is a stub for dither_grays when there is no dithering ?
Therefore I'm not sure that the condition is correct.

> >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.

It's not obvious, right ?
Please add a comment about that.

> >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

Hmm, now I'm finally lost what this routine does.
Please add a comment with an explanation of the intention.
You wrote :

> This routine is trying to determine 

I don't see what it "tries" to determine,
because there is no "tries" in the new code.
Instead that, it does determine something.
I'd like to understand for sure, what is "something".

> The routine is mapping
> 0x00 to 0x0000, 0x01 to 0x0101, ..., and 0xff to 0xffff..  

Hmm, this statement is true for comp_bits[i] == 8.
It is not true with comp_bits[i] != 8.
Did anybody try other comp_bits[i] ?
if ncomps==2, comp_bits=={3,3},  comp_shift={0,8}, color == 0x0500, i = 1, get :

shift = 13,
cv[1] = (5 << 13) | (5 << 10) | (5 << 7) | (5 << 4) | (5 << 1) | (5 >>2).
What does this magic number mean ?
Especially what is (5 >>2) ?

Maybe you mean that only valid values for comp_bits are 1,2,8 ?
Is this documented ?

Igor.