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

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.