[gs-devel] Need opinions for fixing bug #690834

[Ken Sharp]

At 18:46 27/11/2009 +0900, you wrote:


>This patch works as it fixes bug #690384, but there are issues to be
>discussed.
>
>1) gs_font_cid0.cidata.glyph_data() is called at least twice for each
>CID.  This is inefficient.
>2) CID 0 fallback logic is distributed into three functions,  one in
>psi and two in base.
>3) Current CID 0 fallback logic does not handle errors correctly.
>Some errors should be risen immediately before falling back to CID 0.
>(I didn't do this because I didn't want to distribute complex code
>three places apart in a system.)
>4) Applying this patch makes bug #687832 go back in life.
>
>To tell the truth, I don't know where to start.  Maybe current font
>handing in gs doesn't go along well with CIDFont.

Well CIDFont handling was an addition to Ghostscript, since GS predates 
CIDFonts, so its not entirely surprising that its a slightly klunky fit. I 
would leave the inefficiencies in 1 and 2 for now, but feel free to raise a 
new enhancement bug report for them if you want to make sure they don't get 
forgotten.

IMO Point 3 should be a new 'nice to fix' (ie P4) bug, unless a customer 
raises it as an error report. I'd be wary of converting a .notdef print 
into an error, its entirely possible that Acrobat/CPSI behaves this way and 
we've simply copied it.


>   On the other hand,
>I think I may commit this patch because there is a customer waiting.
>I would like to hear comments from you.  I would appreciate any hints,
>directions or priorities you could give me.

As regards #687832, it 'looks' like we need to set the CTM correctly which 
I think ought to be possible though presumably we need different logic than 
was previously applied.

This issue appears to affect a broad range of InDesign files (ID produces 
many CIDFonts in its PostScript and particularly PDF output now). Unless 
absolutely essential I'd prefer to wait on a fix for #690834 until you can 
fix both problems. I suspect the ID issue is considerably more common than 
this new issue and re-introducing a widespread problem in order to fix an 
uncommon problem doesn't seem like a good idea.

By the way, I noticed from your patch for gschar0.c:

@@ -73,10 +74,11 @@
    if (fdepth == MAX_FONT_STACK)\
      return_error(gs_error_invalidfont);\
    pfont = pdata->FDepVector[pdata->Encoding[fidx]];\
-  if (++fdepth > orig_depth || pfont != pte->fstack.items[fdepth].font ||\
-      orig_index != fidx)\
+  pte->fstack.items[fdepth].index = fidx;\
+  if (++fdepth > orig_depth || pfont != pte->fstack.items[fdepth].font)\
      pte->fstack.items[fdepth].font = pfont, changed = 1;\
-  pte->fstack.items[fdepth].index = fidx
+  else\
+    ;

I don't recall seeing a specific prohibition in the coding standard, but 
I've not seen anywhere else in GS use the multiple statements on single 
line approach in a conditional, and I think it would be better to elide the 
empty else clause altogether, ie:

   if (++fdepth > orig_depth || pfont != pte->fstack.items[fdepth].font) {
      pte->fstack.items[fdepth].font = pfont;
      changed = 1;
     }



My 2p worth,


                 Ken