[gs-code-review] RE: stefan's bittagging patch

[Dan Coby]

Henry,

>>Since your purpose is bring the GS and PCL branches together, I
>>have no objection to making this commit.  Committing this stuff
>>is probably the quickest and easiest path toward a single tree.
>>
>>There are several things that I do not like.  I think that they
>>can be corrected after you commit this stuff.
>
>There are some other limitations listed in the README file in the 
>tools/GOT directory.  Also there are scripts there to process the output.

Thanks for the information about the bit tagging device.  It looks like
it will only be useful for a PCL type RGB device.  Is this is what 
is desired?


>>2)  Someone decided to define gs_bitrgbtags_device with a giant
>>structure initialization instead of using the various device
>>macros.  I do not know why.
>>
>...
>
>I think the gs macro-tower compatibility layer really does more harm 
>than good.  I would prefer you leave this as it is since Stefan or I 
>will probably have to maintain it.  In our system if somebody changes a 
>field in the device api I would like the compiler to trigger an error, 
>and not depend on the nimbleness of the whoever is changing the macro 
>puzzle you call the device api to do the right thing for all of the 
>devices.  I really feel just the opposite about fragility.  Your way is 
>more likely to pass on a bug to a customer at runtime.

I agree that the device macros are far from perfect.  There is almost no
documentation about them.  They are passed obscure parameters with no
explanation about what the parameters mean or do.  However by using
both paradigms we are creating the worst of both worlds.

Since the device macros get used for every other device.  It is very
unlikely that will be shipped with flaws.

See my next comment.


>>  It makes for very fragile code since
>>if someone adds a field to the gs_device or gs_prn_device structures,
>>then the initialization will fail.  (Fortunately I think that it
>>will be a compiler failure instead of silently putting the wrong
>>data into device fields.)  I realize the device macros are
>>pretty unobvious to use.  However this structure initialization is
>>even worse since there are not even comments indicating what fields
>>are being initialized.  Instead there is over one hundred lines
>>of obscure numbers without any explanation.  This is particularly bad
>>since this is supposed to be an example of how to do device level
>>object tagging.  
>
>Breakpoint on the device open and print the structure.  That's the way 
>your going to have to deal with it effectively anyway.  But I will 
>certainly concede to doing comments if we can avoid the macro hell.

Consider the following situation and see if I understand you correctly.
A customer has copied the bittagging device and made modifications to
create a custom device for his application.  We then send him an update.
He finds that his device no longer compiles and he gets a very obscure
set of warning messages.  He has to build a version of our code without
his device.  Then he has to use a debugger to look at the device structure
and then try to guess how to modify his code to work with the revised device
structure.  That is not easy if he does not even have any indication
about what the current values mean.  At the least the customer is going
to want a set of comments about the current value sin the structure
initialization.  The use of the device macros would eliminate the
problem entirely.

I think that part of the reason for our difference of opinion is that
you are concerned about the maintenance of only the bit tag device.
I am concerned about the maintenance of the entire suite of devices
that is supported by Ghostscript.  Having one device be an exception to
all of the other devices creates problems.


>>3.  I am not really thrilled about some of the hacks for detecting
>>text versus vectors in the object tagging logic.  Igor has already
>>commented on this issue.  I am enclosing a diff for the relevant
>>changes that I made for custom color processing and switchable CRDs
>>(two other places that also want to make color processing decisions
>>based upon the object tag.
>
>Never intended to work on anything other than an rgb contone printer, 
>which obviously does not worry about not using the cache or alpha.  See 
>the readme.

See my next comment.


>>Note:  I used the term 'object type' in my code instead of Stefan's
>>term 'object tag'.  Stefan's term is probably better since 'object type'
>>is too generic.  I will change my nomenclature to match his.
>>
>>Basically I set the object tag before setting the device color
>>when processing an object.  Thus it is being done a little higher
>>in the processing chain than Stefan's version.  I believe that this
>>eliminates the guess work required about text versus vectors.
>
>Sure it does but then we have to make changes to all the languages.  We 
>agreed to avoid that.

Please give me more information.  Obviously I do not want the break the
operation of PCL.  However I do need to understand the problems.  I would
like to have a common means for determining object tags for the bittagging
device, the custom color callback processing, and for the switchable CRD
logic.

The best path may to be to stick with Stefan's hacks for now and worry
about a more general solution later. 


Dan



-----Original Message-----
From: Henry Stiles [mailto:henry.stiles at artifex.com]
Sent: Tuesday, August 29, 2006 8:43 PM
To: Dan Coby
Cc: Stefan Kemper; Ralph Giles; gs-code-review at ghostscript.com
Subject: Re: stefan's bittagging patch (xefitra)


Dan Coby wrote:

>Ralph,
>
>Since your purpose is bring the GS and PCL branches together, I
>have no objection to making this commit.  Committing this stuff
>is probably the quickest and easiest path toward a single tree.
>
>There are several things that I do not like.  I think that they
>can be corrected after you commit this stuff.
>
>  
>

There are some other limitations listed in the README file in the 
tools/GOT directory.  Also there are scripts there to process the output.

>
>2)  Someone decided to define gs_bitrgbtags_device with a giant
>structure initialization instead of using the various device
>macros.  I do not know why.
>
...

I think the gs macro-tower compatibility layer really does more harm 
than good.  I would prefer you leave this as it is since Stefan or I 
will probably have to maintain it.  In our system if somebody changes a 
field in the device api I would like the compiler to trigger an error, 
and not depend on the nimbleness of the whoever is changing the macro 
puzzle you call the device api to do the right thing for all of the 
devices.  I really feel just the opposite about fragility.  Your way is 
more likely to pass on a bug to a customer at runtime.

>  It makes for very fragile code since
>if someone adds a field to the gs_device or gs_prn_device structures,
>then the initialization will fail.  (Fortunately I think that it
>will be a compiler failure instead of silently putting the wrong
>data into device fields.)  I realize the device macros are
>pretty unobvious to use.  However this structure initialization is
>even worse since there are not even comments indicating what fields
>are being initialized.  Instead there is over one hundred lines
>of obscure numbers without any explanation.  This is particularly bad
>since this is supposed to be an example of how to do device level
>object tagging.
>
>  
>
Breakpoint on the device open and print the structure.  That's the way 
your going to have to deal with it effectively anyway.  But I will 
certainly concede to doing comments if we can avoid the macro hell.

>3.  I am not really thrilled about some of the hacks for detecting
>text versus vectors in the object tagging logic.  Igor has already
>commented on this issue.  I am enclosing a diff for the relevant
>changes that I made for custom color processing and switchable CRDs
>(two other places that also want to make color processing decisions
>based upon the object tag.
>
>  
>
Never intended to work on anything other than an rgb contone printer, 
which obviously does not worry about not using the cache or alpha.  See 
the readme.

>Note:  I used the term 'object type' in my code instead of Stefan's
>term 'object tag'.  Stefan's term is probably better since 'object type'
>is too generic.  I will change my nomenclature to match his.
>
>Basically I set the object tag before setting the device color
>when processing an object.  Thus it is being done a little higher
>in the processing chain than Stefan's version.  I believe that this
>eliminates the guess work required about text versus vectors.
>

Sure it does but then we have to make changes to all the languages.  We 
agreed to avoid that.

>  It
>works with all of the test files that I have tried.  It even found
>some strange cases in the altona test suites.  (The custom color
>processing code simplifies testing since one of its demos changes
>all text to red, all images to green, and all vectors/fills to blue.
>Thus you can look at the object type on the display.)
>
>
>I suggest that we proceed as follows:
>a)  You commit your patch for Stefan's object tagging.
>b)  I modify my custom color processing logic to use Stefan's
>nomenclature and also to include his BITTAG global value.
>c)  I commit my changes which pulls Stefan's version of setting the
>object tag and replaces it with mine.
>d)  I straighten out 688638 and commit this fix.
>
>I am hopeful that I only have to do my commits to a single tree (gs head).
>
>
>Dan
>
>
>-----Original Message-----
>From: Ralph Giles [mailto:ralph.giles at artifex.com]
>Sent: Monday, August 28, 2006 11:11 PM
>To: dan.coby at artifex.com
>Cc: gs-code-review at ghostscript.com
>Subject: stefan's bittagging patch
>
>
>Dan,
>
>This is (I hope!) the complete patch for stefan's bittagging work 
>from the GhostPCL branch. I've not tested it or resolved the conflict 
>with your commit in r6702 as a fix for 688638.
>
>This patch is also missing makefile dependency updates for the new 
>headers.
>
>If you want to do that and apply to gs trunk, that would be great. If 
>you don't have time let me know and I'll do it.
>
> -r
>
>  
>